bug#40335: 27.0.90; elp-not-profilable not up to date

bug#40335: 27.0.90; elp-not-profilable not up to date

[Philipp Stephani]

emacs -Q -eval '(elp-instrument-list (quote (float-time eq)))'

Then something like C-x d leads to infinite recursion.  It looks like
the list `elp-not-profilable' is outdated and should be regenerated.


In GNU Emacs 27.0.90 (build 26, x86_64-pc-linux-gnu, GTK+ Version 3.24.12)
 of 2020-03-30
Repository revision: c6e0981b96eaa12c28b70c949ccd6e426c13df4d
Repository branch: emacs-27
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux rodete

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --enable-gcc-warnings=warn-only
 --enable-gtk-deprecation-warnings --without-pop --with-mailutils
 --enable-checking --enable-check-lisp-object-type --with-modules
 'CFLAGS=-O0 -ggdb3''

Configured features:
XPM JPEG TIFF GIF PNG SOUND DBUS GSETTINGS GLIB NOTIFY INOTIFY
LIBSELINUX GNUTLS FREETYPE HARFBUZZ XFT ZLIB TOOLKIT_SCROLL_BARS GTK3
X11 XDBE XIM MODULES THREADS PDUMPER GMP

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc dired dired-loaddefs
format-spec rfc822 mml easymenu mml-sec epa epg epg-config gnus-util
rmail rmail-loaddefs text-property-search time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils phst skeleton
derived edmacro kmacro pcase ffap thingatpt url-parse auth-source cl-seq
eieio eieio-core cl-macs eieio-loaddefs password-cache json url-vars
subr-x rx map gnutls puny seq byte-opt gv bytecomp byte-compile cconv
dbus xml cl-loaddefs cl-lib tooltip eldoc electric uniquify ediff-hook
vc-hooks lisp-float-type mwheel term/x-win x-win term/common-win x-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode elisp-mode lisp-mode prog-mode register page
tab-bar menu-bar rfn-eshadow isearch timer select scroll-bar mouse
jit-lock font-lock syntax facemenu font-core term/tty-colors frame
minibuffer cl-generic cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite charscript charprop case-table epa-hook jka-cmpr-hook help
simple abbrev obarray cl-preloaded nadvice loaddefs button faces
cus-face macroexp files text-properties overlay sha1 md5 base64 format
env code-pages mule custom widget hashtable-print-readable backquote
threads dbusbind inotify dynamic-setting system-font-setting
font-render-setting move-toolbar gtk x-toolkit x multi-tty
make-network-process emacs)

Memory information:
((conses 16 60087 7983)
 (symbols 48 8215 1)
 (strings 32 21013 1555)
 (string-bytes 1 684132)
 (vectors 16 12144)
 (vector-slots 8 169112 6402)
 (floats 8 25 25)
 (intervals 56 202 0)
 (buffers 1000 12))

-- 
Google Germany GmbH
Erika-Mann-Straße 33
80636 München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

If you received this communication by mistake, please don’t forward it to
anyone else (it may contain confidential or privileged information), please
erase all copies of it, including all attachments, and please let the sender
know it went to the wrong person.  Thanks.

bug#40335: 27.0.90; elp-not-profilable not up to date

[Štěpán Němec]

[-- Attachment #1: Type: text/plain, Size: 1238 bytes --]

On Mon, 30 Mar 2020 23:25:01 +0200
Philipp Stephani wrote:

> emacs -Q -eval '(elp-instrument-list (quote (float-time eq)))'
>
> Then something like C-x d leads to infinite recursion.  It looks like
> the list `elp-not-profilable' is outdated and should be regenerated.

The list (as well as the comment) is definitely out of date, given that
since its last modification elp has been updated to use nadvice.el, also
the `float-time' weren't originally being used, OTOH some of the
original functions listed aren't used by elp any more.

But I can't tell which functions should really be on it. The comment
says that functions directly or indirectly used by the wrapper should be
avoided, but from my anecdotal testing, the only of those that cause
problems are `apply', `error', `current-time' and `float-time'.

All the others can be removed AFAICT, and none of `eq', `get', `aref' or
`time-subtract', also called by the wrapper lambdas
(`elp--make-wrapper'), seemed to cause issues for me.

IIUC advice has no effect for calls from C functions to C functions, but
I don't understand what makes e.g. `float-time' (which breaks) different
from `aref' or `get' (which apparently don't) in that respect.

Here's what seems to work for me:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: elp.diff --]
[-- Type: text/x-patch, Size: 1079 bytes --]

diff --git a/lisp/emacs-lisp/elp.el b/lisp/emacs-lisp/elp.el
index 7dd3cbd1a2..f357783256 100644
--- a/lisp/emacs-lisp/elp.el
+++ b/lisp/emacs-lisp/elp.el
@@ -202,16 +202,9 @@ elp-master
   "Master function symbol.")
 
 (defvar elp-not-profilable
-  ;; First, the functions used inside each instrumented function:
-  '(called-interactively-p
-    ;; Then the functions used by the above functions.  I used
-    ;; (delq nil (mapcar (lambda (x) (and (symbolp x) (fboundp x) x))
-    ;;                   (aref (symbol-function 'elp-wrapper) 2)))
-    ;; to help me find this list.
-    error call-interactively apply current-time
-    ;; Andreas Politz reports problems profiling these (Bug#4233):
-    + byte-code-function-p functionp byte-code subrp
-    indirect-function fboundp)
+  ;; functions used inside each instrumented function cause infinite
+  ;; recursion:
+  '(apply current-time error float-time)
   "List of functions that cannot be profiled.
 Those functions are used internally by the profiling code and profiling
 them would thus lead to infinite recursion.")

bug#40335: 27.0.90; elp-not-profilable not up to date

[Noam Postavsky]

Štěpán Němec <stepnem@gmail.com> writes:

> All the others can be removed AFAICT, and none of `eq', `get', `aref' or
> `time-subtract', also called by the wrapper lambdas
> (`elp--make-wrapper'), seemed to cause issues for me.
>
> IIUC advice has no effect for calls from C functions to C functions, but
> I don't understand what makes e.g. `float-time' (which breaks) different
> from `aref' or `get' (which apparently don't) in that respect.

The obvious difference between `float-time' and `aref' or `get' is that
the latter have byte code ops (so advice doesn't work on byte-compiled
calls to them).  However, that doesn't explain why `subtract-time' is
okay while `float-time' causes problems.

bug#40335: 27.0.90; elp-not-profilable not up to date

[Štěpán Němec]

On Mon, 13 Apr 2020 11:05:49 -0400
Noam Postavsky wrote:

> Štěpán Němec <stepnem@gmail.com> writes:
>
>> All the others can be removed AFAICT, and none of `eq', `get', `aref' or
>> `time-subtract', also called by the wrapper lambdas
>> (`elp--make-wrapper'), seemed to cause issues for me.
>>
>> IIUC advice has no effect for calls from C functions to C functions, but
>> I don't understand what makes e.g. `float-time' (which breaks) different
>> from `aref' or `get' (which apparently don't) in that respect.
>
> The obvious difference between `float-time' and `aref' or `get' is that
> the latter have byte code ops (so advice doesn't work on byte-compiled
> calls to them).

Ah! That makes sense, thank you.

> However, that doesn't explain why `subtract-time' is
> okay while `float-time' causes problems.

Right, because that was just an error on my part: `time-subtract' does
in fact exhibit the problem. But its alias `subtract-time' doesn't, even
when advised explicitly. I guess advices ignore aliases (i.e. pass
through to the real definition)?

-- 
Štěpán

bug#40335: 27.0.90; elp-not-profilable not up to date

[Noam Postavsky]

Štěpán Němec <stepnem@gmail.com> writes:

>> However, that doesn't explain why `subtract-time' is
>> okay while `float-time' causes problems.
>
> Right, because that was just an error on my part: `time-subtract' does
> in fact exhibit the problem. But its alias `subtract-time' doesn't, even
> when advised explicitly. I guess advices ignore aliases (i.e. pass
> through to the real definition)?

Seems to be the opposite: the advice applies only to the alias, so since
elp uses the time-subtract name, advising subtract-time doesn't cause
problems.

bug#40335: 27.0.90; elp-not-profilable not up to date

[Štěpán Němec]

On Mon, 13 Apr 2020 12:05:06 -0400
Noam Postavsky wrote:

>> Right, because that was just an error on my part: `time-subtract' does
>> in fact exhibit the problem. But its alias `subtract-time' doesn't, even
>> when advised explicitly. I guess advices ignore aliases (i.e. pass
>> through to the real definition)?
>
> Seems to be the opposite: the advice applies only to the alias, so since
> elp uses the time-subtract name, advising subtract-time doesn't cause
> problems.

Indeed, thanks :-D

I wonder what the best way forward is here. (info "(elisp) Profiling")
states that elp "is limited to profiling functions written in Lisp, it
cannot profile Emacs primitives". So given that of the problem-makers
only `error' is a Lisp function, the simplest solution would be just
replacing `special-form-p' with `subrp' in `elp-profilable-p', thus
disallowing instrumenting primitives altogether.

If we want to preserve the partial support for primitives, do we want to
support as much as possible, e.g. by runtime-checking if
`elp--make-wrapper' is compiled and determine the set of problem-makers
dynamically, or do we just update the static `elp-not-profilable' list
conservatively (i.e., including _all_ functions called from the
wrappers, to make sure they don't cause problems even when
`elp--make-wrapper' is run interpreted)?

-- 
Štěpán

bug#40335: 27.0.90; elp-not-profilable not up to date

[Lars Ingebrigtsen]

Štěpán Němec <stepnem@gmail.com> writes:

> I wonder what the best way forward is here. (info "(elisp) Profiling")
> states that elp "is limited to profiling functions written in Lisp, it
> cannot profile Emacs primitives".  So given that of the problem-makers
> only `error' is a Lisp function, the simplest solution would be just
> replacing `special-form-p' with `subrp' in `elp-profilable-p', thus
> disallowing instrumenting primitives altogether.

I think it means that it can't profile the innards of C functions -- but
doing `M-x elp-profile-function RET - RET' works fine, as far as I can
tell?

(dotimes (i 1000)
  (- 1 2))

`M-x elp-results RET':

-              1000        0.000410623   4.10623e-07

> If we want to preserve the partial support for primitives, do we want to
> support as much as possible, e.g. by runtime-checking if
> `elp--make-wrapper' is compiled and determine the set of problem-makers
> dynamically, or do we just update the static `elp-not-profilable' list
> conservatively (i.e., including _all_ functions called from the
> wrappers, to make sure they don't cause problems even when
> `elp--make-wrapper' is run interpreted)?

I think we should just update `elp-not-profilable', but we should assume
that `elp--make-wrapper' is byte-compiled.  (Otherwise it'd be less
useful.)

Anybody have any other opinions here?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#40335: 27.0.90; elp-not-profilable not up to date

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I think we should just update `elp-not-profilable', but we should assume
> that `elp--make-wrapper' is byte-compiled.  (Otherwise it'd be less
> useful.)
>
> Anybody have any other opinions here?

Nobody had in a month, so I made this change.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#40335: 27.0.90; elp-not-profilable not up to date

[Philipp Stephani]

Am Mo., 13. Apr. 2020 um 18:54 Uhr schrieb Štěpán Němec <stepnem@gmail.com>:
>
> On Mon, 13 Apr 2020 12:05:06 -0400
> Noam Postavsky wrote:
>
> >> Right, because that was just an error on my part: `time-subtract' does
> >> in fact exhibit the problem. But its alias `subtract-time' doesn't, even
> >> when advised explicitly. I guess advices ignore aliases (i.e. pass
> >> through to the real definition)?
> >
> > Seems to be the opposite: the advice applies only to the alias, so since
> > elp uses the time-subtract name, advising subtract-time doesn't cause
> > problems.
>
> Indeed, thanks :-D
>
> I wonder what the best way forward is here. (info "(elisp) Profiling")
> states that elp "is limited to profiling functions written in Lisp, it
> cannot profile Emacs primitives". So given that of the problem-makers
> only `error' is a Lisp function, the simplest solution would be just
> replacing `special-form-p' with `subrp' in `elp-profilable-p', thus
> disallowing instrumenting primitives altogether.

That seems a bit too drastic. Some primitives are long-running (e.g.
call-process), and instrumenting them is generally useful and also
supported in practice.

>
> If we want to preserve the partial support for primitives, do we want to
> support as much as possible, e.g. by runtime-checking if
> `elp--make-wrapper' is compiled and determine the set of problem-makers
> dynamically, or do we just update the static `elp-not-profilable' list
> conservatively (i.e., including _all_ functions called from the
> wrappers, to make sure they don't cause problems even when
> `elp--make-wrapper' is run interpreted)?

I think the latter makes the most sense. Probably it would also make
sense to exclude primitives and other functions that are so fast that
instrumenting them never makes sense (car, consp, etc.).