Subject: bug#44638: closed (Re: bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors)

From: [email-masked] (GNU bug Tracking System)
Date: Thu, 03 Dec 2020 15:02:02 +0000

Your bug report

#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
44638: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=44638
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors
From: Michael Albinus <[email-masked]>
To: Eli Zaretskii <[email-masked]>
Cc: [email-masked], [email-masked]
Date: Thu, 03 Dec 2020 16:01:29 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

Michael Albinus <[email-masked]> writes:

> Eli Zaretskii <[email-masked]> writes:
>
>>> > auto-revert-test07-auto-revert-several-buffers succeeds.  Running the
>>> > entire test finds 2 failures:
>>> >
>>> >   2 unexpected results:
>>> >      FAILED  auto-revert-test04-auto-revert-mode-dired
>>> >      FAILED  auto-revert-test05-global-notify
>>> >
>>> > Let me know if I can help you more.
>>>
>>> Well, if the same errors happen also w/o that patch, I believe we are
>>> safe to install the patch in master.
>>
>> Yes, I think those tests always failed for me.
>
> Good, so I will install patch 1/2 in master. Likely tomorrow.

Done, closing the bug.

Best regards, Michael.

Subject: [PATCH 1/2] autorevert: don't reuse existing watch descriptors
From: Spencer Baugh <[email-masked]>
To: [email-masked]
Cc: Spencer Baugh <[email-masked]>
Date: Sat, 14 Nov 2020 11:54:58 -0500
List-Id: <debbugs-submit.debbugs.gnu.org>

Previously, when enabling autorevert for a new buffer, we would search
the buffers already registered with autorevert to see if any of them
had the same filename.

This is very slow with a large number of buffers - with 1000, it takes
2 seconds on my system. This 2-second overhead is paid for every new
file opened.

But this is an unnecesary optimization; registering the same file
twice with file-notify has minimal or no overhead, depending on the
implementation.

In fact, file-notify has some baked-in overhead to support registering
the same file twice without problems. For example, inotify on Linux
returns the same inotify watch descriptor when the same file is
registered twice; file-notify adds an additional uniquifying id so
that all watch descriptors are unique in Emacs, even with inotify.

We can rely on file-notify's existing support for handling the same
file being registered twice. We don't need this slow and complex
logic.

With this code deleted, enabling autorevert for a new buffer is
essentially instant even with 1000 buffers.
---
 lisp/autorevert.el | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 046ea2b5d6..d5bb75c2f1 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -650,30 +650,15 @@ will use an up-to-date value of `auto-revert-interval'."
               (string-match auto-revert-notify-exclude-dir-regexp
 			    (expand-file-name default-directory))
 	      (file-symlink-p (or buffer-file-name default-directory)))
-    ;; Check, whether this has been activated already.
     (let ((file (if buffer-file-name
 		    (expand-file-name buffer-file-name default-directory)
 	          (expand-file-name default-directory))))
-      (maphash
-       (lambda (key _value)
-         (when (and
-                (file-notify-valid-p key)
-                (equal (file-notify--watch-absolute-filename
-                        (gethash key file-notify-descriptors))
-                       (directory-file-name file))
-                (equal (file-notify--watch-callback
-                        (gethash key file-notify-descriptors))
-                       'auto-revert-notify-handler))
-         (setq auto-revert-notify-watch-descriptor key)))
-       auto-revert--buffers-by-watch-descriptor)
-      ;; Create a new watch if needed.
-      (unless auto-revert-notify-watch-descriptor
-        (setq auto-revert-notify-watch-descriptor
-	      (ignore-errors
-		(file-notify-add-watch
-		 file
-                 (if buffer-file-name '(change attribute-change) '(change))
-                 'auto-revert-notify-handler))))
+      (setq auto-revert-notify-watch-descriptor
+	    (ignore-errors
+	      (file-notify-add-watch
+	       file
+               (if buffer-file-name '(change attribute-change) '(change))
+               'auto-revert-notify-handler))))
       (when auto-revert-notify-watch-descriptor
         (setq auto-revert-notify-modified-p t)
         (puthash
@@ -682,7 +667,7 @@ will use an up-to-date value of `auto-revert-interval'."
 	       (gethash auto-revert-notify-watch-descriptor
 		        auto-revert--buffers-by-watch-descriptor))
          auto-revert--buffers-by-watch-descriptor)
-        (add-hook 'kill-buffer-hook #'auto-revert-notify-rm-watch nil t)))))
+        (add-hook 'kill-buffer-hook #'auto-revert-notify-rm-watch nil t))))
 
 ;; If we have file notifications, we want to update the auto-revert buffers
 ;; immediately when a notification occurs. Since file updates can happen very
-- 
2.28.0