GNU bug report logs - #46326
27.1.50; Excessive memory allocations with minibuffer-with-setup-hook

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 46326 in the body.
You can then email your comments to 46326 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: mail <at> daniel-mendler.de
To: bug-gnu-emacs <at> gnu.org
Subject: 27.1.50; Excessive memory allocations with minibuffer-with-setup-hook
Date: Fri, 05 Feb 2021 13:51:41 +0100

Dear Emacs developers,

I have an issue on 27.1.50 with excessive memory allocations when using
minibuffer-with-setup-hook with large closures and :append. This issue 
has been
observed in my Consult package, which is similar to Counsel and which 
provides
commands based on completing-read. Naturally I have to perform some 
setup in the
minibuffer-setup-hook. Most of the memory allocations are due to 
add-hook and in
particular the sort function. The sort functions is called because of 
:append
which sets a hook priority. The reason seems to be that large closures 
are
copied, but I didn't fully investigate the reasons for the issue.

Example profile:

    140,068,687  92%       - consult-buffer
    140,068,687  92%        - consult--buffer
    140,068,687  92%         - consult--multi
    138,933,527  91%          - consult--read
    138,933,527  91%           - apply
    138,933,527  91%            - #<subr consult--read>
     99,916,347  65%             - consult--with-async-1
     99,916,347  65%              - #<compiled 0x3e02774d5b1a3f7>
     99,912,203  65%               - consult--with-preview-1
     80,735,735  53%                - #<compiled -0xbfb98ad3a8adf1c>
     80,735,735  53%                 - completing-read
     80,735,735  53%                  - selectrum-completing-read
     62,909,765  41%                   - selectrum-read
>>>  58,826,659  38%                    + add-hook
      1,077,206   0%                    + 
selectrum--minibuffer-post-command-hook
      ....
>>>  19,176,468  12%                + add-hook
>>>  39,017,180  25%             + add-hook

The issue can be mitigated by using a modified version of
minibuffer-with-setup-hook, where I am creating a symbol and fsetting 
instead of
adding a lambda directly via add-hook. This technique is also used in
set-transient-map in order to avoid problems with letrec-closures and
add-hook/remove-hook.

    (defmacro consult--minibuffer-with-setup-hook (fun &rest body)
      (declare (indent 1) (debug t))
      (let ((hook (make-symbol "hook"))
            (append))
        (when (eq (car-safe fun) :append)
          (setq append '(t) fun (cadr fun)))
        `(let ((,hook (make-symbol "consult--minibuffer-setup")))
           (fset ,hook (lambda ()
                         (remove-hook 'minibuffer-setup-hook ,hook)
                         (funcall ,fun)))
           (unwind-protect
               (progn
                 (add-hook 'minibuffer-setup-hook ,hook ,@append)
                 ,@body)
             (remove-hook 'minibuffer-setup-hook ,hook)))))

See the issue https://github.com/minad/consult/issues/193 for reference.

Maybe there exists an alternative solution to the problem and the 
problem should
be fixed on a different level than by replacing the 
minibuffer-with-setup-hook
macro?

Daniel Mendler

In GNU Emacs 27.1.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, 
cairo version 1.16.0) of 2020-10-24
Repository revision: c847d5998f588dbf3eca5ea1ec573a2d64a97607
Repository branch: emacs-27
Windowing system distributor 'The X.Org Foundation', version 
11.0.12004000
System Description: Debian GNU/Linux 10 (buster)

Message #8 received at 46326 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: mail <at> daniel-mendler.de
Cc: 46326 <at> debbugs.gnu.org
Subject: Re: bug#46326: 27.1.50;
 Excessive memory allocations with minibuffer-with-setup-hook
Date: Fri, 05 Feb 2021 16:09:09 +0200

> Date: Fri, 05 Feb 2021 13:51:41 +0100
> From: mail <at> daniel-mendler.de
> 
> I have an issue on 27.1.50 with excessive memory allocations when using
> minibuffer-with-setup-hook with large closures and :append. This issue 
> has been
> observed in my Consult package, which is similar to Counsel and which 
> provides
> commands based on completing-read. Naturally I have to perform some 
> setup in the
> minibuffer-setup-hook. Most of the memory allocations are due to 
> add-hook and in
> particular the sort function. The sort functions is called because of 
> :append
> which sets a hook priority. The reason seems to be that large closures 
> are
> copied, but I didn't fully investigate the reasons for the issue.
> 
> Example profile:
> 
>      140,068,687  92%       - consult-buffer
>      140,068,687  92%        - consult--buffer

Please show information about the memory consumption.  (The so-called
"memory profile" doesn't actually measure memory consumption at all,
it just uses memory allocation calls as a poor-man's profiling
signal.)

Please show a recipe starting from "emacs -Q" where a lot of memory is
being consumed, and please show how much memory does the recipe
consume.  Also, I presume invoking "M-x garbage-collect RET" doesn't
release that memory?  If so, please show the return value of
garbage-collect.

> See the issue https://github.com/minad/consult/issues/193 for reference.

I don't see any memory information in that discussion.  I guess you
thought the memory profiler somehow shows memory consumption, but it
doesn't.

What exactly caused you to run the profiler? is the code slow or
something?  If so, the problem may be something other than memory
consumption.

Message #11 received at 46326 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: mail <at> daniel-mendler.de
Cc: 46326 <at> debbugs.gnu.org
Subject: Re: bug#46326: 27.1.50; Excessive memory allocations with
 minibuffer-with-setup-hook
Date: Fri, 05 Feb 2021 17:58:27 +0200

> Date: Fri, 05 Feb 2021 16:20:13 +0100
> From: mail <at> daniel-mendler.de
> Cc: 46326 <at> debbugs.gnu.org
> 
> > Please show information about the memory consumption.  (The so-called
> > "memory profile" doesn't actually measure memory consumption at all,
> > it just uses memory allocation calls as a poor-man's profiling
> > signal.)
> 
> Yes, I am aware. I am not saying that live memory is the problem,
> but the excessive amount of allocations is a problem, which creates
> unnecessary garbage collector pauses. Therefore I can also not show
> you a profile of the memory consumption, since it is not relevant.

If you are saying that the Lisp code in question conses too many
objects unnecessarily, then the solution is to modify the code to cons
less objects.  That doesn't necessarily indicates the existence of a
bug in Emacs, certainly not with its memory management.

> > What exactly caused you to run the profiler? is the code slow or
> > something?  If so, the problem may be something other than memory
> > consumption.
> 
> Yes, it is slow. Add-hook performs unnecessary allocations causing
> pauses. After replacing the minibuffer-with-setup-hook default
> implementation with my workaround the memory allocations went away
> and the command is much faster. The add-hook calls do not appear in
> the memory profile anymore. See the comment
> https://github.com/minad/consult/issues/193#issuecomment-770650405.

So if you already have a solution for the problem, what is it that you
want us as Emacs maintainers to investigate and fix here?

Message #14 received at 46326 <at> debbugs.gnu.org (full text, mbox):

From: mail <at> daniel-mendler.de
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 46326 <at> debbugs.gnu.org
Subject: Re: bug#46326: 27.1.50; Excessive memory allocations with
 minibuffer-with-setup-hook
Date: Fri, 05 Feb 2021 16:20:13 +0100

[Message part 1 (text/plain, inline)]

Hello Eli,

thank you for your prompt answer!

> Please show information about the memory consumption.  (The so-called
> "memory profile" doesn't actually measure memory consumption at all,
> it just uses memory allocation calls as a poor-man's profiling
> signal.)

Yes, I am aware. I am not saying that live memory is the problem, but 
the
excessive amount of allocations is a problem, which creates unnecessary 
garbage
collector pauses. Therefore I can also not show you a profile of the 
memory
consumption, since it is not relevant.

> Please show a recipe starting from "emacs -Q" where a lot of memory is
> being consumed, and please show how much memory does the recipe
> consume.  Also, I presume invoking "M-x garbage-collect RET" doesn't
> release that memory?  If so, please show the return value of
> garbage-collect.

I hope it is not asking too much to load a single elisp file 
(consult.el)? The
package file has no further dependencies besides Emacs. Then I can give 
you the
following reproducible. The only difference between the two files is 
that in one
case minibuffer-with-setup-hook is used and in the other case
consult--minibuffer-with-setup-hook is used. I appended the two files. 
The hashes
correspond to the git commit hashes in case you want to pull the files 
from the
git repository.

    ;; excessive allocations - using minibuffer-with-setup-hook
    (load "consult-2a197310923a732033bdb757b20c6f90cfad784a.el")

    ;; no suspicious allocations - using 
consult--minibuffer-with-setup-hook
    ;;(load "consult-27e055e7c75e2148449e7b0be4d464b03c36a1bd.el")

    (profiler-start 'mem)
    (dotimes (count 100)
      (run-at-time 0 nil (lambda ()
        		     (message "%s" count)
        		     (setq unread-command-events '(?\C-g))))
      (condition-case _
          (consult-buffer)
        (quit)))
    (profiler-stop)
    (profiler-report)

>> See the issue https://github.com/minad/consult/issues/193 for 
>> reference.
> 
> I don't see any memory information in that discussion.  I guess you
> thought the memory profiler somehow shows memory consumption, but it
> doesn't.

The profile in the issue shows the memory allocation profile. I think 
you refer
to my comment
https://github.com/minad/consult/issues/193#issuecomment-770416491, 
where I
stated my confusion, since the creator of the issue talked about "memory 
usage".
I was only wondering what the profiler actually measures and I looked 
into the
profiler code to confirm that it indeed measures allocations.

> What exactly caused you to run the profiler? is the code slow or
> something?  If so, the problem may be something other than memory
> consumption.

Yes, it is slow. Add-hook performs unnecessary allocations causing 
pauses. After
replacing the minibuffer-with-setup-hook default implementation with my
workaround the memory allocations went away and the command is much 
faster. The
add-hook calls do not appear in the memory profile anymore. See the 
comment
https://github.com/minad/consult/issues/193#issuecomment-770650405.

     56,846,572  89%       - consult-buffer
     56,846,572  89%        - consult--buffer
     56,846,572  89%         - consult--multi
     53,076,020  83%          - consult--read
     53,076,020  83%           - apply
     53,076,020  83%            - #<compiled -0x1bf0d3486ff553f9>
     53,076,020  83%             - consult--with-async-1
     53,076,020  83%              - #<compiled 0x3e0275e58912bf7>
     53,076,020  83%               - consult--with-preview-1
     53,074,964  83%                - #<compiled -0xbfb98ce1324d29c>
     53,074,964  83%                 - completing-read
     53,074,964  83%                  - selectrum-completing-read
      5,530,436   8%                   + selectrum-read
         70,240   0%          + consult--multi-candidates
          4,360   0%            consult--multi-predicate
          1,056   0%          + consult--multi-preprocess

Daniel Mendler

[consult-2a197310923a732033bdb757b20c6f90cfad784a.el (text/plain, attachment)]

[consult-27e055e7c75e2148449e7b0be4d464b03c36a1bd.el (text/plain, attachment)]

Message #17 received at 46326 <at> debbugs.gnu.org (full text, mbox):

From: mail <at> daniel-mendler.de
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 46326 <at> debbugs.gnu.org
Subject: Re: bug#46326: 27.1.50; Excessive memory allocations with
 minibuffer-with-setup-hook
Date: Fri, 05 Feb 2021 17:10:46 +0100

Hello Eli!

> If you are saying that the Lisp code in question conses too many
> objects unnecessarily, then the solution is to modify the code to cons
> less objects.  That doesn't necessarily indicates the existence of a
> bug in Emacs, certainly not with its memory management.

My code certainly *does not allocate* too much. It is the add-hook 
implementation
or more precisely the minibuffer-with-setup-hook implementation which is
responsible for the excessive allocations. For this reason this is an 
upstream
bug. In my workaround, I am replacing the upstream version of 
minibuffer-with-setup-hook
with my own version to show the difference. This does not mean that I 
consider
the problem to be gone. I rather decided to report the problem instead.

> So if you already have a solution for the problem, what is it that you
> want us as Emacs maintainers to investigate and fix here?

I am actually not sure what the proper upstream fix is. I see the 
following
options.

1. Replace minibuffer-with-setup-hook with my version if you think
   my version is better and an acceptable fix.
2. Investigate the reasons why add-hook with priorities somehow copies
   large closures during sorting. This is unacceptably costly.

To give you a better picture of the overall situation - it looks like 
this:

    (minibuffer-with-setup-hook
        (lambda () ...large closure...)
      (minibuffer-with-setup-hook
          (lambda () ...another large closure...)
        (minibuffer-with-setup-hook
            (:append (lambda () ...and yet another one...))
          (completing-read ...))))

The bug is that the large closures are copied for some unknown reason in 
the
add-hook calls which I am not controlling as a library author. My 
replacement
adds the hooks in another way, by creating a symbol+fset instead of 
putting the
lambda directly in the minibuffer-setup-hook variable.

Daniel

Message #20 received at 46326 <at> debbugs.gnu.org (full text, mbox):

From: jakanakaevangeli <jakanakaevangeli <at> chiru.no>
To: mail <at> daniel-mendler.de
Cc: 46326 <at> debbugs.gnu.org
Subject: RE: 27.1.50; Excessive memory allocations with
 minibuffer-with-setup-hook
Date: Mon, 08 Feb 2021 10:25:33 +0100

I seem to have solved the problem by doing the following simple
substitutions in add-hook and remove-hook:

        (alist-get ... #'equal) -> (alist-get ...)
        equal -> eq
        member -> memq
        delete -> delq

This changes the behaviour of add/remove-hook a little, for example

        (add-hook    (lambda () (test)))
        (remove-hook (lambda () (test)))

will not be reverse operations anymore, since the two lambdas are not eq
as they are created separately. But the performance gain for big
non-compiled closures might be worth considering.

Message #23 received at 46326 <at> debbugs.gnu.org (full text, mbox):

From: mail <at> daniel-mendler.de
To: jakanakaevangeli <jakanakaevangeli <at> chiru.no>
Cc: 46326 <at> debbugs.gnu.org
Subject: Re: 27.1.50; Excessive memory allocations with
 minibuffer-with-setup-hook
Date: Tue, 09 Feb 2021 01:19:05 +0100

On 2021-02-08 10:25, jakanakaevangeli wrote:
> I seem to have solved the problem by doing the following simple
> substitutions in add-hook and remove-hook:
> 
>         (alist-get ... #'equal) -> (alist-get ...)
>         equal -> eq
>         member -> memq
>         delete -> delq
> 
> This changes the behaviour of add/remove-hook a little, for example
> 
>         (add-hook    (lambda () (test)))
>         (remove-hook (lambda () (test)))
> 
> will not be reverse operations anymore, since the two lambdas are not
> eq
> as they are created separately. But the performance gain for big
> non-compiled closures might be worth considering.

Thank you, this also seems like a possible solution. However I fear that
it
is way to intrusive and could break a lot of existing code. As I
mentioned
before set-transient-map avoids adding a letrec-lambda via add-hook
because
it would make problems with equal. This means the "broken nature" of
add-hook
is somehow accepted pervasively over the Emacs codebase. My fix is less
intrusive
in the sense that it would just fix `minibuffer-with-setup-hook'.

Message #26 received at 46326 <at> debbugs.gnu.org (full text, mbox):

From: jakanakaevangeli <jakanakaevangeli <at> chiru.no>
To: mail <at> daniel-mendler.de
Cc: 46326 <at> debbugs.gnu.org
Subject: Re: 27.1.50; Excessive memory allocations with
 minibuffer-with-setup-hook
Date: Tue, 09 Feb 2021 23:13:03 +0100

        (minibuffer-with-setup-hook
            (lambda ())
          (let ((print-circle t))
            (message "%S" (car minibuffer-setup-hook))))

Evaluating this in with lexical-binding enabled, we can see that the
anonymous closure added to the hook indeed contains circular structures.
Since remove-hook uses delete rather than delq, it would probably be a
good idea to have minibuffer-with-setup-hook add a symbol rather than
closure to the hook, just like in set-transient-map.

I believe that set-transient-map was adjusted to use a symbol
(in commit bf1e6ae81df3a2a8f2ba4027c9e96a0097acddc5) for a very similar
reason.

Message #29 received at 46326 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: mail <at> daniel-mendler.de
Cc: 46326 <at> debbugs.gnu.org
Subject: Re: bug#46326: 27.1.50; Excessive memory allocations with
 minibuffer-with-setup-hook
Date: Fri, 23 Apr 2021 14:26:57 -0400

> I have an issue on 27.1.50 with excessive memory allocations when using
> minibuffer-with-setup-hook with large closures and :append.

Indeed, we have a problem there.  I think it's fairly hard to fix it
for good without introducing incompatibilities, because `add-hook` has
been defined to compare its functions with `equal` "for ever" and
changing it to use `eq` or `function-equal` will inevitably break
code out there in subtle ways.

IOW I think the better fix is to change `minibuffer-with-setup-hook` to
use an indirection via a symbol.

As for reducing the impact of the underlying issue, I see we could
reduce the amount of `equal` tests being performed, by using `eq` for
the lookups in `hook--depth-alist`.
So before we install the "real" solution, could you try the patch below
and report how much it helps (if at all)?


        Stefan


diff --git a/lisp/subr.el b/lisp/subr.el
index c2be26a15f5..7b718a48a8d 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -1830,12 +1834,13 @@ add-hook
     (unless (member function hook-value)
       (when (stringp function)          ;FIXME: Why?
 	(setq function (purecopy function)))
+      ;; All those `equal' tests performed between functions can end up being
+      ;; costly since those functions may be large recursive and even cyclic
+      ;; structures, so we index `hook--depth-alist' with `eq'.  (bug#46326)
       (when (or (get hook 'hook--depth-alist) (not (zerop depth)))
         ;; Note: The main purpose of the above `when' test is to avoid running
         ;; this `setf' before `gv' is loaded during bootstrap.
-        (setf (alist-get function (get hook 'hook--depth-alist)
-                         0 'remove #'equal)
-              depth))
+        (push (cons function depth) (get hook 'hook--depth-alist)))
       (setq hook-value
 	    (if (< 0 depth)
 		(append hook-value (list function))
@@ -1845,8 +1850,8 @@ add-hook
           (setq hook-value
                 (sort (if (< 0 depth) hook-value (copy-sequence hook-value))
                       (lambda (f1 f2)
-                        (< (alist-get f1 depth-alist 0 nil #'equal)
-                           (alist-get f2 depth-alist 0 nil #'equal))))))))
+                        (< (alist-get f1 depth-alist 0 nil #'eq)
+                           (alist-get f2 depth-alist 0 nil #'eq))))))))
     ;; Set the actual variable
     (if local
 	(progn
@@ -1907,11 +1912,20 @@ remove-hook
 	       (not (and (consp (symbol-value hook))
 			 (memq t (symbol-value hook)))))
       (setq local t))
-    (let ((hook-value (if local (symbol-value hook) (default-value hook))))
+    (let ((hook-value (if local (symbol-value hook) (default-value hook)))
+          (old-fun nil))
       ;; Remove the function, for both the list and the non-list cases.
       (if (or (not (listp hook-value)) (eq (car hook-value) 'lambda))
-	  (if (equal hook-value function) (setq hook-value nil))
-	(setq hook-value (delete function (copy-sequence hook-value))))
+	  (when (equal hook-value function)
+	    (setq old-fun hook-value)
+	    (setq hook-value nil))
+	(when (setq old-fun (car (member function hook-value)))
+	  (setq hook-value (remq old-fun hook-value))))
+      (when old-fun
+        ;; Remove auxiliary depth info to avoid leaks.
+        (put hook 'hook--depth-alist
+             (delq (assq old-fun (get hook 'hook--depth-alist))
+                   (get hook 'hook--depth-alist))))
       ;; If the function is on the global hook, we need to shadow it locally
       ;;(when (and local (member function (default-value hook))
       ;;	       (not (member (cons 'not function) hook-value)))

Message #32 received at 46326 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>, jakanakaevangeli <at> chiru.no
Cc: 46326 <at> debbugs.gnu.org
Subject: Re: bug#46326: 27.1.50; Excessive memory allocations with
 minibuffer-with-setup-hook
Date: Fri, 23 Apr 2021 21:28:24 +0200

For me it seems to fix the issue. @jakanakaevangeli, can you confirm? I 
would still prefer to see a "proper fix". But given the backward 
compatibility requirements such a fix may not exist.

Perhaps one could introduce some deprecation behavior. If a hook is 
removed and the object is not found via eq but found via equal, then 
print a warning? And then change the add-hook/remove-hook functions to 
eq only in some later version.

Furthermore as a stop-gap measure one may still apply my patched 
symbol-indirection `minibuffer-with-setup-hook`, and revert it once the 
proper fix has been applied.

(Using the symbol indirection seems to have other debuggability 
advantages. Closures are not particularly nice to debug in elisp, I hope 
we will also see some improvements regarding that. It is at least on my 
Elisp wishlist to have better introspection for closures, location info 
etc.)

Note that `set-transient-map` already uses the symbol indirection. It 
may make sense to link to this bug from there such that one can adjust 
this function also at some later point depending on the resolution of 
this issue. The comment in `set-transient-map` reads like a bug to me
"Don't use letrec, because equal (in add/remove-hook) would get trapped 
in a cycle." :)

Daniel

On 4/23/21 8:26 PM, Stefan Monnier wrote:
>> I have an issue on 27.1.50 with excessive memory allocations when using
>> minibuffer-with-setup-hook with large closures and :append.
> 
> Indeed, we have a problem there.  I think it's fairly hard to fix it
> for good without introducing incompatibilities, because `add-hook` has
> been defined to compare its functions with `equal` "for ever" and
> changing it to use `eq` or `function-equal` will inevitably break
> code out there in subtle ways.
> 
> IOW I think the better fix is to change `minibuffer-with-setup-hook` to
> use an indirection via a symbol.
> 
> As for reducing the impact of the underlying issue, I see we could
> reduce the amount of `equal` tests being performed, by using `eq` for
> the lookups in `hook--depth-alist`.
> So before we install the "real" solution, could you try the patch below
> and report how much it helps (if at all)?
> 
> 
>          Stefan
> 
> 
> diff --git a/lisp/subr.el b/lisp/subr.el
> index c2be26a15f5..7b718a48a8d 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -1830,12 +1834,13 @@ add-hook
>       (unless (member function hook-value)
>         (when (stringp function)          ;FIXME: Why?
>   	(setq function (purecopy function)))
> +      ;; All those `equal' tests performed between functions can end up being
> +      ;; costly since those functions may be large recursive and even cyclic
> +      ;; structures, so we index `hook--depth-alist' with `eq'.  (bug#46326)
>         (when (or (get hook 'hook--depth-alist) (not (zerop depth)))
>           ;; Note: The main purpose of the above `when' test is to avoid running
>           ;; this `setf' before `gv' is loaded during bootstrap.
> -        (setf (alist-get function (get hook 'hook--depth-alist)
> -                         0 'remove #'equal)
> -              depth))
> +        (push (cons function depth) (get hook 'hook--depth-alist)))
>         (setq hook-value
>   	    (if (< 0 depth)
>   		(append hook-value (list function))
> @@ -1845,8 +1850,8 @@ add-hook
>             (setq hook-value
>                   (sort (if (< 0 depth) hook-value (copy-sequence hook-value))
>                         (lambda (f1 f2)
> -                        (< (alist-get f1 depth-alist 0 nil #'equal)
> -                           (alist-get f2 depth-alist 0 nil #'equal))))))))
> +                        (< (alist-get f1 depth-alist 0 nil #'eq)
> +                           (alist-get f2 depth-alist 0 nil #'eq))))))))
>       ;; Set the actual variable
>       (if local
>   	(progn
> @@ -1907,11 +1912,20 @@ remove-hook
>   	       (not (and (consp (symbol-value hook))
>   			 (memq t (symbol-value hook)))))
>         (setq local t))
> -    (let ((hook-value (if local (symbol-value hook) (default-value hook))))
> +    (let ((hook-value (if local (symbol-value hook) (default-value hook)))
> +          (old-fun nil))
>         ;; Remove the function, for both the list and the non-list cases.
>         (if (or (not (listp hook-value)) (eq (car hook-value) 'lambda))
> -	  (if (equal hook-value function) (setq hook-value nil))
> -	(setq hook-value (delete function (copy-sequence hook-value))))
> +	  (when (equal hook-value function)
> +	    (setq old-fun hook-value)
> +	    (setq hook-value nil))
> +	(when (setq old-fun (car (member function hook-value)))
> +	  (setq hook-value (remq old-fun hook-value))))
> +      (when old-fun
> +        ;; Remove auxiliary depth info to avoid leaks.
> +        (put hook 'hook--depth-alist
> +             (delq (assq old-fun (get hook 'hook--depth-alist))
> +                   (get hook 'hook--depth-alist))))
>         ;; If the function is on the global hook, we need to shadow it locally
>         ;;(when (and local (member function (default-value hook))
>         ;;	       (not (member (cons 'not function) hook-value)))
> 

Message #35 received at 46326 <at> debbugs.gnu.org (full text, mbox):

From: <jakanakaevangeli <at> chiru.no>
To: Daniel Mendler <mail <at> daniel-mendler.de>, Stefan Monnier
 <monnier <at> iro.umontreal.ca>
Cc: 46326 <at> debbugs.gnu.org
Subject: Re: bug#46326: 27.1.50; Excessive memory allocations with
 minibuffer-with-setup-hook
Date: Fri, 23 Apr 2021 22:34:06 +0200

Daniel Mendler <mail <at> daniel-mendler.de> writes:

> For me it seems to fix the issue. @jakanakaevangeli, can you confirm?

Yup, confirmed. The crucial part this patch does to fix this issue is
removing depth info to avoid leaks, which is also directly related to
bug46414.

Message #38 received at 46326 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: jakanakaevangeli <at> chiru.no, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 46326 <at> debbugs.gnu.org
Subject: Re: bug#46326: 27.1.50; Excessive memory allocations with
 minibuffer-with-setup-hook
Date: Fri, 23 Apr 2021 22:52:07 +0200

On 4/23/21 10:34 PM, jakanakaevangeli <at> chiru.no wrote:
> Yup, confirmed. The crucial part this patch does to fix this issue is
> removing depth info to avoid leaks, which is also directly related to
> bug46414.

Thanks!

Is there still a possibility to trigger a similar memory 
allocation/hanging bug with the patch by Stefan or are the remaining 
equals harmless? Eyeing also towards this worrisome comment in 
`set-transient-map`... cycles and hanging...

Daniel

Message #41 received at 46326 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Daniel Mendler <mail <at> daniel-mendler.de>
Cc: 46326 <at> debbugs.gnu.org, jakanakaevangeli <at> chiru.no
Subject: Re: bug#46326: 27.1.50; Excessive memory allocations with
 minibuffer-with-setup-hook
Date: Fri, 23 Apr 2021 17:27:23 -0400

jakanakaevangeli <at> chiru.no [2021-04-23 22:34:06] wrote:
> Daniel Mendler <mail <at> daniel-mendler.de> writes:
>> For me it seems to fix the issue. @jakanakaevangeli, can you confirm?
> Yup, confirmed.

Thanks, pushed to `master`.  Maybe we should consider it for `emacs-27`
since it's a regression in Emacs-27.

Eli, WDYT?

> The crucial part this patch does to fix this issue is
> removing depth info to avoid leaks, which is also directly related to
> bug46414.

Indeed, thanks.

> For me it seems to fix the issue. @jakanakaevangeli, can you confirm?
> I would still prefer to see a "proper fix". But given the backward
> compatibility requirements such a fix may not exist.
> Perhaps one could introduce some deprecation behavior. If a hook is removed
> and the object is not found via eq but found via equal, then print
> a warning? And then change the add-hook/remove-hook functions to eq only in
> some later version.

I suggest you `M-x report-emacs-bug` and make such a proposal.
We already have 3 places where we bump into this (`set-transient-map`,
`eval-after-load`, and `minibuffer-with-setup-hook`).

> Furthermore as a stop-gap measure one may still apply my patched
> symbol-indirection `minibuffer-with-setup-hook`, and revert it once the
> proper fix has been applied.

Yes, that was indeed the intention.  It's been pushed as well.

> (Using the symbol indirection seems to have other debuggability
> advantages. Closures are not particularly nice to debug in elisp, I hope we
> will also see some improvements regarding that.

We already have had some improvements (if byte-compiled, they should
print with a hyperlink that shows the disassembled bytecode, which is
a lot more palatable than the raw bytecode we used to present).

> It is at least on my Elisp wishlist to have better introspection for
> closures, location info etc.)

There's indeed still a fair bit of progress to be made.

> Note that `set-transient-map` already uses the symbol indirection. It may
> make sense to link to this bug from there such that one can adjust this
> function also at some later point depending on the resolution of this
> issue. The comment in `set-transient-map` reads like a bug to me
> "Don't use letrec, because equal (in add/remove-hook) would get trapped in
> a cycle." :)

Yes, it's (or was) clearly a bug in `equal`.  IIRC our current `equal`
is better in this respect, but I'd be surprised if we can't make it get
stuck in a loop in some cases.


        Stefan

Message #44 received at 46326 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: mail <at> daniel-mendler.de, 46326 <at> debbugs.gnu.org, jakanakaevangeli <at> chiru.no
Subject: Re: bug#46326: 27.1.50;
 Excessive memory allocations with minibuffer-with-setup-hook
Date: Sat, 24 Apr 2021 09:10:23 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Date: Fri, 23 Apr 2021 17:27:23 -0400
> Cc: jakanakaevangeli <at> chiru.no, 46326 <at> debbugs.gnu.org
> 
> jakanakaevangeli <at> chiru.no [2021-04-23 22:34:06] wrote:
> > Daniel Mendler <mail <at> daniel-mendler.de> writes:
> >> For me it seems to fix the issue. @jakanakaevangeli, can you confirm?
> > Yup, confirmed.
> 
> Thanks, pushed to `master`.  Maybe we should consider it for `emacs-27`
> since it's a regression in Emacs-27.
> 
> Eli, WDYT?

It's too late for such changes in Emacs 27, sorry.  And I don't think
there will be Emacs 27.3 anyway.

Message #47 received at 46326 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: mail <at> daniel-mendler.de, 46326 <at> debbugs.gnu.org, jakanakaevangeli <at> chiru.no
Subject: Re: bug#46326: 27.1.50; Excessive memory allocations with
 minibuffer-with-setup-hook
Date: Sat, 24 Apr 2021 09:06:05 -0400

> It's too late for such changes in Emacs 27, sorry.  And I don't think
> there will be Emacs 27.3 anyway.

OK, thanks,


        Stefan

Message #50 received at 46326 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Daniel Mendler <mail <at> daniel-mendler.de>, jakanakaevangeli <at> chiru.no,
 46326 <at> debbugs.gnu.org
Subject: Re: bug#46326: 27.1.50; Excessive memory allocations with
 minibuffer-with-setup-hook
Date: Thu, 16 Jun 2022 15:06:16 +0200

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> Yup, confirmed.
>
> Thanks, pushed to `master`. 

The reported issue here was fixed, and then discussion turned to more
general things.  But I don't think there's more to do in this bug
report, so I'm closing it now.

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.