GNU bug report logs - #51620
add-hook repeatedly adds same function into hook--depth-alist

Previous Next

Package: emacs;

Reported by: Filipp Gunbin <fgunbin <at> fastmail.fm>

Date: Fri, 5 Nov 2021 23:27:02 UTC

Severity: normal

Tags: patch

Fixed in version 29.1

Done: Filipp Gunbin <fgunbin <at> fastmail.fm>

Bug is archived. No further changes may be made.

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

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

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#51620; Package emacs. (Fri, 05 Nov 2021 23:27:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Filipp Gunbin <fgunbin <at> fastmail.fm>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Fri, 05 Nov 2021 23:27:02 GMT) Full text and rfc822 format available.

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

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: bug-gnu-emacs <at> gnu.org
Subject: add-hook repeatedly adds same function into hook--depth-alist
Date: Sat, 06 Nov 2021 02:26:32 +0300
I have the following case: in my "javaimp" package I have code which
scans java files "in the same project":

(defun javaimp--get-directory-classes (dir)
  (when (file-accessible-directory-p dir)
    (seq-mapcat #'javaimp--get-file-classes
                (seq-filter (lambda (file)
                              (not (file-symlink-p file)))
                            (directory-files-recursively dir "\\.java\\'")))))

There're some perfomance problems to fix, so I started with measuring
time via M-x benchmark.  Surpisingly, I saw that with each run (in a
large project) the time increased by 8 seconds or so.  Profiling lead
me to add-hook and the fix made in bug#46326.

In short, javaimp--get-file-classes visits file in temp buffer and
uses syntax-ppss to parse Java code.  On a large project, this is done
many times, and next invocation of javaimp--get-directory-classes does
everything again (this is what I wanted to fix, as well as look at
fewer things during parsing).  So I stumbled into problem which
perhaps goes unnoticed in normal file editing, where you don't process
tens of thousands of files with syntax-ppss (which calls add-hook)
repeatedly.

For reference, syntax.el does this:
		(add-hook 'before-change-functions
			  #'syntax-ppss-flush-cache
                          ;; We should be either the very last function on
                          ;; before-change-functions or the very first on
                          ;; after-change-functions.


This is what I get when I run my test:

(length (get 'before-change-functions 'hook--depth-alist)) => 58063
<call javaimp--get-directory-classes on a large project>
(length (get 'before-change-functions 'hook--depth-alist)) => 65303

All elements are `(syntax-ppss-flush-cache . 99)'.


A simple reproducer:
- $ echo 'print("Hello world!");' > /tmp/hello.py
- emacs -Q
- C-x C-f /tmp/hello.py
- M-: (length (get 'before-change-functions 'hook--depth-alist)), observe number N
- revisit the same file via C-x C-v
- M-: (length (get 'before-change-functions 'hook--depth-alist)), observe number N+1
- on each revisit the number increments


Next, to the code.
There was this change in the patch for 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)))

(Probably the comment and the first test in "or" should have been
removed with the change, but I'm not suggesting that because I'm
suggesting restoring setf)

setf with #'eq test would be a better option than push, because it won't
repeatedly add the same (as in "eq") element, if we reach this code
somehow.

In our case we reach this code for each new buffer, because the check
is:

    (unless (member function hook-value)

and `before-change-functions' is of course buffer-local.  So we keep
pushing elements into (get hook 'hook--depth-alist) for each new buffer.

And, unrelated to this, I fail to understand why copy-sequence is here
in the code further down in add-hook, could someone please explain?

          (setq hook-value
                (sort (if (< 0 depth) hook-value (copy-sequence hook-value))


I suggest this one-liner, which fixes the problem for me, however I
certainly need someone (Stefan M.?) to look at this.

TIA, Filipp

diff --git a/lisp/subr.el b/lisp/subr.el
index 8ff403e113..2b8b6deeb0 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -1868,7 +1868,7 @@ add-hook
       (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.
-        (push (cons function depth) (get hook 'hook--depth-alist)))
+        (setf (alist-get function (get hook 'hook--depth-alist) 0) depth))
       (setq hook-value
 	    (if (< 0 depth)
 		(append hook-value (list function))



In GNU Emacs 28.0.60 (build 4, x86_64-apple-darwin20.6.0, NS appkit-2022.60 Version 11.6 (Build 20G165))
 of 2021-11-05 built on fgunbin.local
Repository revision: d8c9a9dc23e0c6f38c5138cb8fbb4109a5729a35
Repository branch: emacs-28
System Description:  macOS 11.6

Configured using:
 'configure --enable-check-lisp-object-type --with-file-notification=no'

Configured features:
ACL GLIB GNUTLS LCMS2 LIBXML2 MODULES NS PDUMPER PNG RSVG THREADS
TOOLKIT_SCROLL_BARS XIM ZLIB




Added tag(s) patch. Request was from Filipp Gunbin <fgunbin <at> fastmail.fm> to control <at> debbugs.gnu.org. (Sat, 06 Nov 2021 00:03:01 GMT) Full text and rfc822 format available.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#51620; Package emacs. (Sat, 06 Nov 2021 00:14:01 GMT) Full text and rfc822 format available.

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

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Filipp Gunbin <fgunbin <at> fastmail.fm>
Cc: 51620 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#51620: add-hook repeatedly adds same function into
 hook--depth-alist
Date: Sat, 06 Nov 2021 01:13:02 +0100
Filipp Gunbin <fgunbin <at> fastmail.fm> writes:

> diff --git a/lisp/subr.el b/lisp/subr.el
> index 8ff403e113..2b8b6deeb0 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -1868,7 +1868,7 @@ add-hook
>        (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.
> -        (push (cons function depth) (get hook 'hook--depth-alist)))
> +        (setf (alist-get function (get hook 'hook--depth-alist) 0) depth))
>        (setq hook-value
>  	    (if (< 0 depth)
>  		(append hook-value (list function))

So the function would still be compared using `eq' but we would avoid to
add dups of equal conses?  Looks reasonable to me -- Stefan?

TIA,

Michael.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#51620; Package emacs. (Sat, 06 Nov 2021 20:33:01 GMT) Full text and rfc822 format available.

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

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 51620 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#51620: add-hook repeatedly adds same function into
 hook--depth-alist
Date: Sat, 06 Nov 2021 23:31:58 +0300
On 06/11/2021 01:13 +0100, Michael Heerdegen wrote:

> Filipp Gunbin <fgunbin <at> fastmail.fm> writes:
>
>> diff --git a/lisp/subr.el b/lisp/subr.el
>> index 8ff403e113..2b8b6deeb0 100644
>> --- a/lisp/subr.el
>> +++ b/lisp/subr.el
>> @@ -1868,7 +1868,7 @@ add-hook
>>        (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.
>> -        (push (cons function depth) (get hook 'hook--depth-alist)))
>> +        (setf (alist-get function (get hook 'hook--depth-alist) 0) depth))
>>        (setq hook-value
>>  	    (if (< 0 depth)
>>  		(append hook-value (list function))
>
> So the function would still be compared using `eq' but we would avoid to
> add dups of equal conses?  Looks reasonable to me -- Stefan?

Yep, that was the intent.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#51620; Package emacs. (Thu, 11 Nov 2021 16:56:02 GMT) Full text and rfc822 format available.

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

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: control <at> debbugs.gnu.org
Cc: 51620 <at> debbugs.gnu.org
Subject: control message for bug #51620
Date: Thu, 11 Nov 2021 19:55:28 +0300
close 51620 29.1
quit

I've now pushed it in master, and closing this bug.





bug marked as fixed in version 29.1, send any further explanations to 51620 <at> debbugs.gnu.org and Filipp Gunbin <fgunbin <at> fastmail.fm> Request was from Filipp Gunbin <fgunbin <at> fastmail.fm> to control <at> debbugs.gnu.org. (Thu, 11 Nov 2021 16:56:02 GMT) Full text and rfc822 format available.

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Fri, 10 Dec 2021 12:24:05 GMT) Full text and rfc822 format available.

This bug report was last modified 2 years and 99 days ago.

Previous Next


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