bug#27397: [PATCH] New commands for bulk tracing of elisp functions

classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Phil Sainty
trace.el currently only has commands for adding a trace to a single
function, and for untracing either a single function or ALL traced
functions.

This patch adds commands for tracing and untracing functions in bulk,
either by function name prefix (`trace-package' and `untrace-package')
or by regexp (`trace-regexp' and `untrace-regexp').

The use of the term "package" for the prefix-based commands was chosen
for consistency with `elp-instrument-package', from which this code is
partly derived, and from which I have also maintained a warning and
basic protection against attempts to trace too many functions at once
(which can indeed render Emacs unusable).  It it still possible to do
this, but I feel the checks which are currently in place are probably
sufficient to protect against accidents.

`trace-is-traceable-p' is using Stefan's suggestion in his comment on
my initial implementation at https://stackoverflow.com/a/44241058 and
I have changed the completing-read filter predicate in
`trace--read-args' to also use this (instead of `fboundp'), on the
assumption that this makes more sense.

(I note that the `trace-is-traced' function does not follow the usual
naming convention for predicates.  Should this be renamed to
`trace-is-traced-p' ?)

This patch also removes some outdated docstring comments for
`untrace-function' which were clearly remnants of the earlier
implementation using advice.el (whereas the current library is based
on nadvice.el).


-Phil

0001-New-commands-for-bulk-tracing-of-elisp-functions.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Dmitry Gutov
On 6/16/17 4:32 PM, Phil Sainty wrote:

> This patch adds commands for tracing and untracing functions in bulk,
> either by function name prefix (`trace-package' and `untrace-package')
> or by regexp (`trace-regexp' and `untrace-regexp').

Looking good. And I've wanted these commands both times I've had a need
to use trace.el.

This patch could probably use a NEWS entry, though.

> (I note that the `trace-is-traced' function does not follow the usual
> naming convention for predicates.  Should this be renamed to
> `trace-is-traced-p' ?)

I'm not 100% sure about the protocol here, but including both "-is-" and
"-p" in a function name seems too much.

So maybe you should do the opposite and rename trace-is-traceable-p to
trace-is-traceable.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Kaushal Modi
In reply to this post by Phil Sainty
> I note that the `trace-is-traced' function does not follow the usual
> naming convention for predicates.  Should this be renamed to
> `trace-is-traced-p'

Based on current examples[1], it is more common to see predicate *functions* end in "-p". So the "-is-" portion is maybe redundant.

My suggestion would be something like "trace-traced-p" or "trace-fn-traced-p".


--

Kaushal Modi

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Phil Sainty
In reply to this post by Dmitry Gutov
On 17/06/17 02:58, Dmitry Gutov wrote:
> This patch could probably use a NEWS entry, though.

Yes, that's a good point. I'll add one to the next revision.
Something like this?


** Trace

*** New commands 'trace-package' and 'trace-regexp' (and their
counterparts 'untrace-package' and 'untrace-regexp') allow for the
bulk tracing of calls to functions with names matching a specified
prefix or regexp.


As there's no info node for trace.el, that could perhaps be a +++
entry; although a grep shows me that "(tramp) Traces and Profiles"
could be amended to change:

     (require 'trace)
     (dolist (elt (all-completions "tramp-" obarray 'functionp))
       (trace-function-background (intern elt)))

to:

     (trace-package "tramp-")


It would be good to add a node to the elisp manual covering trace.el,
but that's not strictly part of this patch.


-Phil



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Phil Sainty
In reply to this post by Kaushal Modi
> On 6/16/17 4:32 PM, Phil Sainty wrote:
>> (I note that the `trace-is-traced' function does not follow the usual
>> naming convention for predicates.  Should this be renamed to
>> `trace-is-traced-p' ?)


On 17/06/17 02:58, Dmitry Gutov wrote:
> I'm not 100% sure about the protocol here, but including both "-is-"
> and "-p" in a function name seems too much.
>
> So maybe you should do the opposite and rename trace-is-traceable-p to
> trace-is-traceable.


On 17/06/17 03:43, Kaushal Modi wrote:
> Based on current examples[1], it is more common to see predicate
> *functions* end in "-p". So the "-is-" portion is maybe redundant.
>
> My suggestion would be something like "trace-traced-p" or
> "trace-fn-traced-p".
>
> [1]: https://debbugs.gnu.org/cgi/bugreport.cgi?att=0;bug=26564;msg=5


FWIW, after loading more or less all the lisp in trunk, apropos tells me:

2381 matches for ".*-p$"
126 matches for "-is-"
28 matches for "-is-.*-p$"


The -p suffix is certainly what I'm used to seeing, but -is- is entirely
readable to my mind, so I'm happy either way.  Consistency is good, so
some kind of change seemed sensible to me, but I don't especially mind
one way or the other.

If one of the maintainers wants to make a recommendation, I'll update
the code (or leave it as-is) accordingly.


-Phil




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Dmitry Gutov
In reply to this post by Phil Sainty
On 6/17/17 11:43 AM, Phil Sainty wrote:
> On 17/06/17 02:58, Dmitry Gutov wrote:
>> This patch could probably use a NEWS entry, though.
>
> Yes, that's a good point. I'll add one to the next revision.
> Something like this?

Looks good to me, both proposed changes.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Phil Sainty
In reply to this post by Phil Sainty
Thanks to the merge with
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=1343 and
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=6464 I see that there
was a previous submission of a `trace-package' command which (a) used
foreground tracing, and (b) provided `trace-package-background' as a
separate command.

I should note that I elected not to support foreground tracing for
my proposed commands, simply because it seemed to me that foreground
tracing could be problematic in too many cases, when the traces are
being applied en masse on the basis of function name pattern matching.

The current `trace-function-foreground' docstring says:

> This function creates BUFFER if it does not exist.  This buffer will
> popup whenever FUNCTION is called.  Do not use this function to trace
> functions that switch buffers, or do any other display-oriented
> stuff - use ‘trace-function-background’ instead.

My compromise was to indicate the `trace-buffer' value in the echo
area when the commands are invoked, so that the user would know where
the trace output was happening.  Actually popping up that buffer when
the `trace-package' or `trace-regexp' command is used would be another
(perhaps nicer) option?

I'm open to recommendations for the most useful approach.

Do others think that foreground versions of these commands would be
a good idea?

A prefix argument could mean "use foreground tracing", but that's
inconsistent with `trace--read-args' which uses a prefix arg to prompt
the user for the trace buffer and a context expression -- which I now
realise is behaviour that my commands should incorporate as well.


-Phil



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Phil Sainty
On 17/06/17 21:20, Phil Sainty wrote:
> inconsistent with `trace--read-args' which uses a prefix arg to prompt
> the user for the trace buffer and a context expression -- which I now
> realise is behaviour that my commands should incorporate as well.

I've implemented this change, and attached the current WIP patch.

In the process I noticed that the existing behaviour of trace--read-args
was quite unfriendly if you wanted to set a trace buffer but had no need
of a context expression -- typing RET at the context expression prompt
triggered an "End of file during parsing" error, as that input string
is processed by `read-from-string'.

I've changed that code to treat an empty input as "nil" (which is read
to `nil'), and to ignore `nil' context expressions entirely (as opposed
to printing their evaluated value as "[nil]" in the trace buffer).


-Phil

0001-New-commands-for-bulk-tracing-of-elisp-functions.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Dmitry Gutov
On 6/17/17 3:31 PM, Phil Sainty wrote:

> In the process I noticed that the existing behaviour of trace--read-args
> was quite unfriendly if you wanted to set a trace buffer but had no need
> of a context expression -- typing RET at the context expression prompt
> triggered an "End of file during parsing" error, as that input string
> is processed by `read-from-string'.
>
> I've changed that code to treat an empty input as "nil" (which is read
> to `nil'), and to ignore `nil' context expressions entirely (as opposed
> to printing their evaluated value as "[nil]" in the trace buffer).

Sounds good.

Why the rename, though? Those are not arguments for the function we're
going to trace. trace--read-args sounds as appropriate, if not more.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Dmitry Gutov
In reply to this post by Phil Sainty
On 6/17/17 12:20 PM, Phil Sainty wrote:

> Actually popping up that buffer when
> the `trace-package' or `trace-regexp' command is used would be another
> (perhaps nicer) option?

Perhaps.

> Do others think that foreground versions of these commands would be
> a good idea?

I'm fine with having only the background versions, but others are
welcome to weigh in.

> A prefix argument could mean "use foreground tracing"

Nah, if we do want foreground tracing, adding separate commands
shouldn't be a problem.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Phil Sainty
In reply to this post by Dmitry Gutov
On 18/06/17 10:59, Dmitry Gutov wrote:
> Why the rename, though? Those are not arguments for the function we're
> going to trace. trace--read-args sounds as appropriate, if not more.

That was because the behaviour of `trace--read-args' had been quite
specific to the `trace-function*' commands -- its primary purpose was
to prompt for a single function -- and I thought the name should reflect
that.  I agree with you, though -- the new name wasn't ideal either.

I've now refactored this like so:

* `trace--read-args' has been split into `trace--read-function` and
  `trace--read-extra-args'.

* `trace--read-function` reads only a function.

* `The interactive specs for the trace-function*' commands are now
   similar to those of the new bulk trace commands, in explicitly
   calling `trace--read-extra-args':

  (interactive
   (cons (trace--read-function "Trace function: ")
         (and current-prefix-arg (trace--read-extra-args))))

I think this name change makes better sense, and the code is now more
consistent between commands.


-Phil



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Dmitry Gutov
On 6/18/17 4:06 AM, Phil Sainty wrote:

> I've now refactored this like so:
>
> * `trace--read-args' has been split into `trace--read-function` and
>    `trace--read-extra-args'.
>
> * `trace--read-function` reads only a function.

That sounds better, thanks!



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Phil Sainty
In reply to this post by Phil Sainty
Just attaching the current patch.  Thanks for the input Dmitry and
Kaushal.  I'll leave it a while to see whether anyone else wishes to
weigh in on the outstanding items:

* The inconsistent predicate naming of `trace-is-traced' vs
  `trace-is-traceable-p'.
  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27397#19

* Whether foreground-tracing variants of the new commands are wanted.
  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27397#25
  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27397#34

* Whether it's a good idea to display the trace buffer initially
  when one of the background tracing commands is invoked.


(I guess I'll follow up next weekend if no one has anything to add
in the interim.)


-Phil

0001-New-commands-for-bulk-tracing-of-elisp-functions.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Michael Albinus
In reply to this post by Phil Sainty
Phil Sainty <[hidden email]> writes:

Hi Phil,

> As there's no info node for trace.el, that could perhaps be a +++
> entry; although a grep shows me that "(tramp) Traces and Profiles"
> could be amended to change:
>
>      (require 'trace)
>      (dolist (elt (all-completions "tramp-" obarray 'functionp))
>        (trace-function-background (intern elt)))
>
> to:
>
>      (trace-package "tramp-")

There's no package "tramp-". Better would be

        (trace-regexp "^tramp-")

Could be changed in the Tramp manual (modulo backwards
compatibility). One problem I'm always faced with Tramp are autoloaded
functions. The code as given in the Tramp manual instruments only
functions, which are either already loaded, or which are marked as to be
autoloaded. Functions from a Tramp package, which are loaded later on,
are not handled.

Could you add this functionality?

> -Phil

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Phil Sainty
On 19/06/17 19:45, Michael Albinus wrote:
>>      (trace-package "tramp-")
>
> There's no package "tramp-". Better would be
>
>         (trace-regexp "^tramp-")

To be clear, the two are functionally equivalent; but if you'd prefer
it written that way then I'm happy to make that change.


> One problem I'm always faced with Tramp are autoloaded functions.
> The code as given in the Tramp manual instruments only functions
> which are either already loaded, or which are marked as to be
> autoloaded. Functions from a Tramp package, which are loaded later
> on, are not handled.
>
> Could you add this functionality?

I'm not certain what you're asking here.

With respect to the manual entry being discussed here, we could
trivially show code to `require' all of the tramp-* libraries prior
to calling trace-package or trace-regexp. e.g.:

(mapc 'require '(tramp tramp-adb tramp-cache tramp-cmds
                       tramp-compat tramp-ftp tramp-gvfs tramp-sh
                       tramp-smb tramp-uu trampver))

(or else something which automatically locates library names starting
with "tramp-" and loads them all).

Perhaps you're actually be suggesting some kind of `eval-after-load'
tracing behaviour, though?


-Phil



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Michael Albinus
Phil Sainty <[hidden email]> writes:

Hi Phil,

>> There's no package "tramp-". Better would be
>>
>>         (trace-regexp "^tramp-")
>
> To be clear, the two are functionally equivalent; but if you'd prefer
> it written that way then I'm happy to make that change.

Ahh, my error. I thought that `trace-package' takes a package name (or
symbol), and traces only all functions which have been loaded by this
package. (trace-package 'tramp) would then trace the function
`tramp-file-name-handler (defined in tramp.el) , but not the function
`tramp-sh-file-name-handler'. The latter one is defined in tramp-sh.el.

> With respect to the manual entry being discussed here, we could
> trivially show code to `require' all of the tramp-* libraries prior
> to calling trace-package or trace-regexp. e.g.:
>
> (mapc 'require '(tramp tramp-adb tramp-cache tramp-cmds
>                        tramp-compat tramp-ftp tramp-gvfs tramp-sh
>                        tramp-smb tramp-uu trampver))
>
> (or else something which automatically locates library names starting
> with "tramp-" and loads them all).

That's not what I want. Often, I hunt bugs related to the order of
autoloaded functions, and this order shall be kept also when tracing.

> Perhaps you're actually be suggesting some kind of `eval-after-load'
> tracing behaviour, though?

Yes, that's the idea. If `trace-package' uses as argument a package name
as proposed above, the instrumentation shall happen in an
`eval-after-load' form for that package.

`trace-regexp', on the other hand, shall instrument the functions in a
form added to `after-load-functions', additonally to the functions
already loaded.

> -Phil

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Phil Sainty
On 19/06/17 21:56, Michael Albinus wrote:
> Ahh, my error. I thought that `trace-package' takes a package name
> (or symbol), and traces only all functions which have been loaded by
> this package.

I concede that the name may cause some confusion, but I chose it
for consistency with `elp-instrument-package' which uses the same
function-name-prefix meaning, so the name makes sense from that
perspective. Obviously ELP pre-dates package.el. YMMV.

Maybe `elp-instrument-package' and `trace-package' should become
`elp-instrument-prefix' and `trace-prefix' ? (I'm not against that
idea.)


>> some kind of `eval-after-load' tracing behaviour
>
> Yes, that's the idea. If `trace-package' uses as argument a package
> name as proposed above, the instrumentation shall happen in an
> `eval-after-load' form for that package.

I think `trace-library' would be the appropriate name?

(If `trace-package' were about ELPA packages, then we have multi-file
packages to consider, which expands the scope further.)

Of course we can't guarantee that library foo.el adheres to a foo-*
naming scheme for all its functions (or that other libraries don't
define any foo-* functions). Would we just ignore this and trace
everything starting with foo- on the assumption that this is good
enough? Or would we parse the library in order to trace that library's
functions precisely?


> `trace-regexp', on the other hand, shall instrument the functions in
> a form added to `after-load-functions', additonally to the functions
> already loaded.

Which could also apply to (the current meaning of) `trace-package'.

I suppose this could potentially be an additional y-or-n-p prompt for
the extended interactive argument input (after BUFFER and CONTEXT)
when using a prefix arg.

I think that the equivalent `untrace-*' commands would need to remove
such `after-load-functions' entries by default, but perhaps a prefix
argument to those would allow users to choose.

I can see potential for users to end up with unwanted remnant entries
in after-load-functions (e.g. trace-regexp "A\\|B", then untrace-regexp
"A" and "B" separately), so I think there could be some fiddly aspects
to all of this; although one could argue that anyone using these
features in the first place is likely to know what they're doing, and
would be able to cope with such situations easily enough; especially
if `untrace-all' takes care of the after-load cases.


-Phil



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Dmitry Gutov
In reply to this post by Michael Albinus
Hi Michael,

On 6/19/17 12:56 PM, Michael Albinus wrote:
> That's not what I want. Often, I hunt bugs related to the order of
> autoloaded functions, and this order shall be kept also when tracing.

Your case is surely valid, but sounds a bit niche to me. It shouldn't be
too hard to evaluate this, though:

(with-eval-after-load 'tramp
   (trace-package "tramp-"))

And the current patch makes it easier.

>> Perhaps you're actually be suggesting some kind of `eval-after-load'
>> tracing behaviour, though?
>
> Yes, that's the idea. If `trace-package' uses as argument a package name
> as proposed above, the instrumentation shall happen in an
> `eval-after-load' form for that package.

Considering elp-instrument-package does not do that, I think we should
limit the scope of the currently discussed patch, and trace only already
loaded functions.

If we do what you suggest, it should be a new discussion, and it should
improve elp-instrument-package as well.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Michael Albinus
Dmitry Gutov <[hidden email]> writes:

> Hi Michael,

Hi Dmitry,

> Your case is surely valid, but sounds a bit niche to me. It shouldn't
> be too hard to evaluate this, though:
>
> (with-eval-after-load 'tramp
>   (trace-package "tramp-"))

But this would also trace functions from tramp-sh.el, which I don't want.

>> Yes, that's the idea. If `trace-package' uses as argument a package name
>> as proposed above, the instrumentation shall happen in an
>> `eval-after-load' form for that package.
>
> Considering elp-instrument-package does not do that, I think we should
> limit the scope of the currently discussed patch, and trace only
> already loaded functions.
>
> If we do what you suggest, it should be a new discussion, and it
> should improve elp-instrument-package as well.

I don't believe it will be too hard to implement; why not doing it just
now?

Improving elp* could be a second step then.

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27397: [PATCH] New commands for bulk tracing of elisp functions

Dmitry Gutov
On 6/19/17 2:36 PM, Michael Albinus wrote:

>> (with-eval-after-load 'tramp
>>    (trace-package "tramp-"))
>
> But this would also trace functions from tramp-sh.el, which I don't want.

What would be the equivalent of your proposal, then?

>> If we do what you suggest, it should be a new discussion, and it
>> should improve elp-instrument-package as well.
>
> I don't believe it will be too hard to implement; why not doing it just
> now?

Because we risk stopping now for a while and having two inconsistent
functions in Emacs. It also adds a burden to the contributor who might
now have signed up for tackling this particular challenge.

There will also be some nuances to work out, I'm sure.



12
Loading...