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

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

[Spencer Baugh]

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

bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer

[Spencer Baugh]

Now that we don't share watch descriptors between buffers, we don't
need to store a list of buffers for each watch descriptor - there
would only be a single buffer in each list. This should have no
functional change.
---
 lisp/autorevert.el | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index d5bb75c2f1..ad39884ab0 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -355,10 +355,10 @@ the list of old buffers.")
 (add-hook 'after-set-visited-file-name-hook
           #'auto-revert-set-visited-file-name)
 
-(defvar auto-revert--buffers-by-watch-descriptor
+(defvar auto-revert--buffer-by-watch-descriptor
   (make-hash-table :test 'equal)
-  "A hash table mapping notification descriptors to lists of buffers.
-The buffers use that descriptor for auto-revert notifications.
+  "A hash table mapping notification descriptors to buffers.
+The buffer uses that descriptor for auto-revert notifications.
 The key is equal to `auto-revert-notify-watch-descriptor' in each
 buffer.")
 
@@ -631,12 +631,9 @@ will use an up-to-date value of `auto-revert-interval'."
 (defun auto-revert-notify-rm-watch ()
   "Disable file notification for current buffer's associated file."
   (let ((desc auto-revert-notify-watch-descriptor)
-        (table auto-revert--buffers-by-watch-descriptor))
+        (table auto-revert--buffer-by-watch-descriptor))
     (when desc
-      (let ((buffers (delq (current-buffer) (gethash desc table))))
-        (if buffers
-            (puthash desc buffers table)
-          (remhash desc table)))
+      (remhash desc table)
       (ignore-errors
 	(file-notify-rm-watch desc))
       (remove-hook 'kill-buffer-hook #'auto-revert-notify-rm-watch t)))
@@ -663,10 +660,8 @@ will use an up-to-date value of `auto-revert-interval'."
         (setq auto-revert-notify-modified-p t)
         (puthash
          auto-revert-notify-watch-descriptor
-         (cons (current-buffer)
-	       (gethash auto-revert-notify-watch-descriptor
-		        auto-revert--buffers-by-watch-descriptor))
-         auto-revert--buffers-by-watch-descriptor)
+         (current-buffer)
+         auto-revert--buffer-by-watch-descriptor)
         (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
@@ -696,8 +691,8 @@ system.")
 	   (action (nth 1 event))
 	   (file (nth 2 event))
 	   (file1 (nth 3 event)) ;; Target of `renamed'.
-	   (buffers (gethash descriptor
-			     auto-revert--buffers-by-watch-descriptor)))
+	   (buffer (gethash descriptor
+			    auto-revert--buffer-by-watch-descriptor)))
       ;; Check, that event is meant for us.
       (cl-assert descriptor)
       ;; Since we watch a directory, a file name must be returned.
@@ -708,7 +703,6 @@ system.")
 
       (if (eq action 'stopped)
           ;; File notification has stopped.  Continue with polling.
-          (cl-dolist (buffer buffers)
             (with-current-buffer buffer
               (when (or
                      ;; A buffer associated with a file.
@@ -721,10 +715,8 @@ system.")
                 (auto-revert-notify-rm-watch)
                 ;; Restart the timer if it wasn't running.
                 (unless auto-revert-timer
-                  (auto-revert-set-timer)))))
+                  (auto-revert-set-timer))))
 
-        ;; Loop over all buffers, in order to find the intended one.
-        (cl-dolist (buffer buffers)
           (when (buffer-live-p buffer)
             (with-current-buffer buffer
               (when (or
@@ -752,7 +744,7 @@ system.")
                   (setq auto-revert--lockout-timer
                         (run-with-timer
                          auto-revert--lockout-interval nil
-                         #'auto-revert--end-lockout buffer)))))))))))
+                         #'auto-revert--end-lockout buffer))))))))))
 
 (defun auto-revert--end-lockout (buffer)
   "End the lockout period after a notification.
-- 
2.28.0

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

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sat, 14 Nov 2020 11:54:58 -0500
> Cc: Spencer Baugh <sbaugh@catern.com>
> 
> 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.

Emacs actually watches the file's directory, not the file itself.  The
directory is what's registered with inotify and other similar
backends.

> 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.

Again, we watch the directory of the file, so what inotify does with
files is not really relevant, IMO.  I wonder what that means for the
changes you propose.

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

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Spencer Baugh <sbaugh@catern.com>
>> Date: Sat, 14 Nov 2020 11:54:58 -0500
>> Cc: Spencer Baugh <sbaugh@catern.com>
>> 
>> 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.
>
> Emacs actually watches the file's directory, not the file itself.  The
> directory is what's registered with inotify and other similar
> backends.
>
>> 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.
>
> Again, we watch the directory of the file, so what inotify does with
> files is not really relevant, IMO.  I wonder what that means for the
> changes you propose.

Ah. Well, the inotify comment was just an example. Even given that Emacs
watches directories rather than files, I think this change is for the
best - the overhead of registering 5 files directly instead of 1
underlying directory is minimal.

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

[Spencer Baugh]

Spencer Baugh <sbaugh@catern.com> writes:
> Eli Zaretskii <eliz@gnu.org> writes:
>> Again, we watch the directory of the file, so what inotify does with
>> files is not really relevant, IMO.  I wonder what that means for the
>> changes you propose.
>
> Ah. Well, the inotify comment was just an example. Even given that Emacs
> watches directories rather than files, I think this change is for the
> best - the overhead of registering 5 files directly instead of 1
> underlying directory is minimal.

So does this kind of change still seem plausible?  Again, the overhead
of additional watches for the same file in the operating system is
minimal, and is rare anyway.

The alternative to this change is to add a new hash table to autorevert
which maps filenames to file-notify watches, so that autorevert can look
up duplicate watches efficiently in O(1) instead of iterating over all
watches in O(n) as it does now.

I started doing that originally, but concluded it was unnecessary since
sharing watches is unnecessary.  But it's a much smaller change to
behavior while still getting the same performance gain, so if you'd
prefer that, I can do it.

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

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@catern.com>
> Cc: 44638@debbugs.gnu.org
> Date: Mon, 30 Nov 2020 13:01:43 -0500
> 
> Spencer Baugh <sbaugh@catern.com> writes:
> > Eli Zaretskii <eliz@gnu.org> writes:
> >> Again, we watch the directory of the file, so what inotify does with
> >> files is not really relevant, IMO.  I wonder what that means for the
> >> changes you propose.
> >
> > Ah. Well, the inotify comment was just an example. Even given that Emacs
> > watches directories rather than files, I think this change is for the
> > best - the overhead of registering 5 files directly instead of 1
> > underlying directory is minimal.
> 
> So does this kind of change still seem plausible?

I'd expect Michael Albinus to chime in on this, as autorevert is his
domain.  Michael?

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> So does this kind of change still seem plausible?
>
> I'd expect Michael Albinus to chime in on this, as autorevert is his
> domain.  Michael?

It's already on my todo list. Hmm, too much items there. I'll try to
find a sufficient time slot tomorrow.

Sorry for not being as responsive as I should :-(

Best regards, Michael.

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

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

> It's already on my todo list. Hmm, too much items there. I'll try to
> find a sufficient time slot tomorrow.

As expected, it takes more time. Currently I'm writing a test case which
covers the problem. This must run then for different backends with and
w/o your patch, at least inotify, gfilenotify and kqueue. If possible
also for w32notify.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  44638@debbugs.gnu.org
> Date: Tue, 01 Dec 2020 21:16:25 +0100
> 
> Michael Albinus <michael.albinus@gmx.de> writes:
> 
> > It's already on my todo list. Hmm, too much items there. I'll try to
> > find a sufficient time slot tomorrow.
> 
> As expected, it takes more time. Currently I'm writing a test case which
> covers the problem. This must run then for different backends with and
> w/o your patch, at least inotify, gfilenotify and kqueue. If possible
> also for w32notify.

Thanks, let me know if I can help with the latter.

bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer

[Michael Albinus]

Spencer Baugh <sbaugh@catern.com> writes:

> Now that we don't share watch descriptors between buffers, we don't
> need to store a list of buffers for each watch descriptor - there
> would only be a single buffer in each list. This should have no
> functional change.

Just for the records, when I apply both patches 1/2 and 2/2,
autorevert-tests fails. So it cannot be applied as such.

Using just patch 1/2 works for the inotify backend. I'll continue to
test. I recommend you to test also your patches by calling

$ make -C test autorevert-tests

Best regards, Michael.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> As expected, it takes more time. Currently I'm writing a test case which
>> covers the problem. This must run then for different backends with and
>> w/o your patch, at least inotify, gfilenotify and kqueue. If possible
>> also for w32notify.
>
> Thanks, let me know if I can help with the latter.

I've pushed a new test of autorevert-tests.el to master,
auto-revert-test07-auto-revert-several-buffers. It ought to cover the
scenario described in this report. It passes for me with the inotify,
gfilenotify and kqueue backends, which is fine. There are some problems
with remote backends (inotifywait and gio), but since there are also
problems with other tests, I wouldn't count on these failures for the
propsed patch.

Eli, could you run autorevert-test on MS Windows, for the w32notify
backend? Either a complete run, or just

$ make -C test autorevert-tests SELECTOR=auto-revert-test07-auto-revert-several-buffers

Thanks, and best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: sbaugh@catern.com,  44638@debbugs.gnu.org
> Date: Wed, 02 Dec 2020 16:17:01 +0100
> 
> Eli, could you run autorevert-test on MS Windows, for the w32notify
> backend? Either a complete run, or just
> 
> $ make -C test autorevert-tests SELECTOR=auto-revert-test07-auto-revert-several-buffers

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

The evidence for the first failure is:

  Test auto-revert-test04-auto-revert-mode-dired condition:
      (ert-test-failed
       ((should-not
	 (string-match name
		       (substring-no-properties ...)))
	:form
	(string-match "auto-revert-testhf8BmX" "  c:/Documents and Settings/Zaretskii/Local Settings/Temp:\12  total used in directory 1178 available 46.5 GiB\12  drwxrwxrwx  1 Zaretskii None      0 2012-04-16  ..\12  drwxrwxrwx  1 Zaretskii None      0 12-02 17:35 .\12  drwxrwxrwx  1 Zaretskii None      0 09-25 21:47 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\12  drwxrwxrwx  1 Zaretskii None      0 12-01 17:26 acrord32_sbx\12  -rw-rw-rw-  1 Zaretskii None  38296 12...(the rest elided)

Not sure what that means.

The second test fails because:

  Test auto-revert-test05-global-notify condition:
      (ert-test-failed
       ((should
	 (buffer-local-value 'auto-revert-notify-watch-descriptor buf-1))
	:form
	(buffer-local-value auto-revert-notify-watch-descriptor #<killed buffer>)
	:value nil))

Let me know if I can help you more.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> Eli, could you run autorevert-test on MS Windows, for the w32notify
>> backend? Either a complete run, or just
>>
>> $ make -C test autorevert-tests
>> SELECTOR=auto-revert-test07-auto-revert-several-buffers
>
> 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.

In general, I need to debug what happens, the ert reports are not
sufficient (for me) to understand what's going wrong. Do you know how to
get an MS Windows VM instance? I would be even willing to pay for a
license.

I have a spare server which runs already VMs, like the FreeBSD instance
I've just taken for the kqueue tests.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: sbaugh@catern.com,  44638@debbugs.gnu.org
> Date: Wed, 02 Dec 2020 18:05:35 +0100
> 
> > 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.

> In general, I need to debug what happens, the ert reports are not
> sufficient (for me) to understand what's going wrong. Do you know how to
> get an MS Windows VM instance? I would be even willing to pay for a
> license.

Sorry, I don't know.  maybe someone else here does.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> 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.

>> In general, I need to debug what happens, the ert reports are not
>> sufficient (for me) to understand what's going wrong. Do you know how to
>> get an MS Windows VM instance? I would be even willing to pay for a
>> license.
>
> Sorry, I don't know.  maybe someone else here does.

Hmm. What would be the canonical address to ask Microsoft directly?

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: sbaugh@catern.com,  44638@debbugs.gnu.org
> Date: Wed, 02 Dec 2020 18:20:01 +0100
> 
> >> Do you know how to get an MS Windows VM instance? I would be even
> >> willing to pay for a license.
> >
> > Sorry, I don't know.  maybe someone else here does.
> 
> Hmm. What would be the canonical address to ask Microsoft directly?

Does the below help?

  https://developer.microsoft.com/en-us/microsoft-edge/tools/vms/
  https://developer.microsoft.com/en-us/windows/downloads/virtual-machines/
  https://www.extremetech.com/computing/198427-how-to-install-windows-10-in-a-virtual-machine
  https://superuser.com/questions/961322/find-a-windows-10-iso-to-install-it-in-a-virtual-machine

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

[Dmitry Gutov]

On 02.12.2020 19:13, Eli Zaretskii wrote:
>> In general, I need to debug what happens, the ert reports are not
>> sufficient (for me) to understand what's going wrong. Do you know how to
>> get an MS Windows VM instance? I would be even willing to pay for a
>> license.
> Sorry, I don't know.  maybe someone else here does.

Check out one of these:

https://developer.microsoft.com/en-us/microsoft-edge/tools/vms/
https://developer.microsoft.com/en-us/windows/downloads/virtual-machines/

These are distributed explicitly for development and testing. They are 
free, but with an expiration date that's fairly close.

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

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

> Eli Zaretskii <eliz@gnu.org> 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.

bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

> Spencer Baugh <sbaugh@catern.com> writes:
>
>> Now that we don't share watch descriptors between buffers, we don't
>> need to store a list of buffers for each watch descriptor - there
>> would only be a single buffer in each list. This should have no
>> functional change.
>
> Just for the records, when I apply both patches 1/2 and 2/2,
> autorevert-tests fails. So it cannot be applied as such.

Spencer, did you do any further work on this patch?

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

bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer

[Spencer Baugh]

Lars Ingebrigtsen <larsi@gnus.org> writes:
> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> Spencer Baugh <sbaugh@catern.com> writes:
>>
>>> Now that we don't share watch descriptors between buffers, we don't
>>> need to store a list of buffers for each watch descriptor - there
>>> would only be a single buffer in each list. This should have no
>>> functional change.
>>
>> Just for the records, when I apply both patches 1/2 and 2/2,
>> autorevert-tests fails. So it cannot be applied as such.
>
> Spencer, did you do any further work on this patch?

Not this patch, no.  Locally I have applied patch 1/2 and not patch
2/2. Patch 2/2 just removes dead code after patch 1/2 removes the
functionality, and I must have gotten it wrong.

But I can report that performance has been fine with patch 1/2 - I
haven't seen the pathological case I was seeing before, and I haven't
seen any new pathological cases to replace it.

bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer

[Lars Ingebrigtsen]

Spencer Baugh <sbaugh@catern.com> writes:

>>> Just for the records, when I apply both patches 1/2 and 2/2,
>>> autorevert-tests fails. So it cannot be applied as such.
>>
>> Spencer, did you do any further work on this patch?
>
> Not this patch, no.  Locally I have applied patch 1/2 and not patch
> 2/2. Patch 2/2 just removes dead code after patch 1/2 removes the
> functionality, and I must have gotten it wrong.
>
> But I can report that performance has been fine with patch 1/2 - I
> haven't seen the pathological case I was seeing before, and I haven't
> seen any new pathological cases to replace it.

Patch 1/2 was applied to Emacs 28 some time ago (and works fine), so
this was just a followup on 2/2 -- whether this clean-up patch (which
apparently doesn't work correctly, according to Michael) is going to get
any further work done, or whether this issue should just be closed...

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

bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer

[Michael Albinus]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

Hi,

> Patch 1/2 was applied to Emacs 28 some time ago (and works fine), so
> this was just a followup on 2/2 -- whether this clean-up patch (which
> apparently doesn't work correctly, according to Michael) is going to get
> any further work done, or whether this issue should just be closed...

I've reworked the patch, using an association list instead of a hash
table. By this, I've fixed also the error which was always evident, but
uncovered only by Spencer's patch.

Pushed to master, I'm closing the bug. The patch itself is appended
below.

Best regards, Michael.


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

*** /tmp/ediffGTBgTB	2021-01-28 15:18:21.881661122 +0100
--- /home/albinus/src/emacs/lisp/autorevert.el	2021-01-28 15:06:38.034456811 +0100
***************
*** 355,364 ****
  (add-hook 'after-set-visited-file-name-hook
            #'auto-revert-set-visited-file-name)

! (defvar auto-revert--buffers-by-watch-descriptor
!   (make-hash-table :test 'equal)
!   "A hash table mapping notification descriptors to lists of buffers.
! The buffers use that descriptor for auto-revert notifications.
  The key is equal to `auto-revert-notify-watch-descriptor' in each
  buffer.")

--- 355,363 ----
  (add-hook 'after-set-visited-file-name-hook
            #'auto-revert-set-visited-file-name)

! (defvar auto-revert--buffer-by-watch-descriptor nil
!   "An association list mapping notification descriptors to buffers.
! The buffer uses that descriptor for auto-revert notifications.
  The key is equal to `auto-revert-notify-watch-descriptor' in each
  buffer.")

***************
*** 630,645 ****

  (defun auto-revert-notify-rm-watch ()
    "Disable file notification for current buffer's associated file."
!   (let ((desc auto-revert-notify-watch-descriptor)
!         (table auto-revert--buffers-by-watch-descriptor))
!     (when desc
!       (let ((buffers (delq (current-buffer) (gethash desc table))))
!         (if buffers
!             (puthash desc buffers table)
!           (remhash desc table)))
!       (ignore-errors
! 	(file-notify-rm-watch desc))
!       (remove-hook 'kill-buffer-hook #'auto-revert-notify-rm-watch t)))
    (setq auto-revert-notify-watch-descriptor nil
  	auto-revert-notify-modified-p nil))

--- 629,640 ----

  (defun auto-revert-notify-rm-watch ()
    "Disable file notification for current buffer's associated file."
!   (when-let ((desc auto-revert-notify-watch-descriptor))
!     (setq auto-revert--buffer-by-watch-descriptor
!           (assoc-delete-all desc auto-revert--buffer-by-watch-descriptor))
!     (ignore-errors
!       (file-notify-rm-watch desc))
!     (remove-hook 'kill-buffer-hook #'auto-revert-notify-rm-watch t))
    (setq auto-revert-notify-watch-descriptor nil
  	auto-revert-notify-modified-p nil))

***************
*** 660,672 ****
                 (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
!          auto-revert-notify-watch-descriptor
!          (cons (current-buffer)
! 	       (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))))

  ;; If we have file notifications, we want to update the auto-revert buffers
--- 655,664 ----
                 (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
!               auto-revert--buffer-by-watch-descriptor
!               (cons (cons auto-revert-notify-watch-descriptor (current-buffer))
!                     auto-revert--buffer-by-watch-descriptor))
          (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
***************
*** 696,703 ****
  	   (action (nth 1 event))
  	   (file (nth 2 event))
  	   (file1 (nth 3 event)) ;; Target of `renamed'.
! 	   (buffers (gethash descriptor
! 			     auto-revert--buffers-by-watch-descriptor)))
        ;; Check, that event is meant for us.
        (cl-assert descriptor)
        ;; Since we watch a directory, a file name must be returned.
--- 688,695 ----
  	   (action (nth 1 event))
  	   (file (nth 2 event))
  	   (file1 (nth 3 event)) ;; Target of `renamed'.
! 	   (buffer (alist-get descriptor auto-revert--buffer-by-watch-descriptor
!                               nil nil #'equal)))
        ;; Check, that event is meant for us.
        (cl-assert descriptor)
        ;; Since we watch a directory, a file name must be returned.
***************
*** 706,714 ****
        (when auto-revert-debug
          (message "auto-revert-notify-handler %S" event))

!       (if (eq action 'stopped)
!           ;; File notification has stopped.  Continue with polling.
!           (cl-dolist (buffer buffers)
              (with-current-buffer buffer
                (when (or
                       ;; A buffer associated with a file.
--- 698,706 ----
        (when auto-revert-debug
          (message "auto-revert-notify-handler %S" event))

!       (when (buffer-live-p buffer)
!         (if (eq action 'stopped)
!             ;; File notification has stopped.  Continue with polling.
              (with-current-buffer buffer
                (when (or
                       ;; A buffer associated with a file.
***************
*** 721,758 ****
                  (auto-revert-notify-rm-watch)
                  ;; Restart the timer if it wasn't running.
                  (unless auto-revert-timer
!                   (auto-revert-set-timer)))))

!         ;; Loop over all buffers, in order to find the intended one.
!         (cl-dolist (buffer buffers)
!           (when (buffer-live-p buffer)
!             (with-current-buffer buffer
!               (when (or
!                      ;; A buffer associated with a file.
!                      (and (stringp buffer-file-name)
!                           (or
!                            (and (memq
!                                  action '(attribute-changed changed created))
!                                 (string-equal
!                                  (file-name-nondirectory file)
!                                  (file-name-nondirectory buffer-file-name)))
!                            (and (eq action 'renamed)
!                                 (string-equal
!                                  (file-name-nondirectory file1)
!                                  (file-name-nondirectory buffer-file-name)))))
!                      ;; A buffer w/o a file, like dired.
!                      (and (null buffer-file-name)
!                           (memq action '(created renamed deleted))))
!                 ;; Mark buffer modified.
!                 (setq auto-revert-notify-modified-p t)
!
!                 ;; Revert the buffer now if we're not locked out.
!                 (unless auto-revert--lockout-timer
!                   (auto-revert-handler)
!                   (setq auto-revert--lockout-timer
!                         (run-with-timer
!                          auto-revert--lockout-interval nil
!                          #'auto-revert--end-lockout buffer)))))))))))

  (defun auto-revert--end-lockout (buffer)
    "End the lockout period after a notification.
--- 713,747 ----
                  (auto-revert-notify-rm-watch)
                  ;; Restart the timer if it wasn't running.
                  (unless auto-revert-timer
!                   (auto-revert-set-timer))))

!           (with-current-buffer buffer
!             (when (or
!                    ;; A buffer associated with a file.
!                    (and (stringp buffer-file-name)
!                         (or
!                          (and (memq
!                                action '(attribute-changed changed created))
!                               (string-equal
!                                (file-name-nondirectory file)
!                                (file-name-nondirectory buffer-file-name)))
!                          (and (eq action 'renamed)
!                               (string-equal
!                                (file-name-nondirectory file1)
!                                (file-name-nondirectory buffer-file-name)))))
!                    ;; A buffer w/o a file, like dired.
!                    (and (null buffer-file-name)
!                         (memq action '(created renamed deleted))))
!               ;; Mark buffer modified.
!               (setq auto-revert-notify-modified-p t)
!
!               ;; Revert the buffer now if we're not locked out.
!               (unless auto-revert--lockout-timer
!                 (auto-revert-handler)
!                 (setq auto-revert--lockout-timer
!                       (run-with-timer
!                        auto-revert--lockout-interval nil
!                        #'auto-revert--end-lockout buffer))))))))))

  (defun auto-revert--end-lockout (buffer)
    "End the lockout period after a notification.