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

Previous Next

Package: emacs;

Reported by: Spencer Baugh <sbaugh <at> catern.com>

Date: Sat, 14 Nov 2020 16:56:02 UTC

Severity: normal

Tags: moreinfo, patch

Done: Michael Albinus <michael.albinus <at> gmx.de>

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 44639 in the body.
You can then email your comments to 44639 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#44639; Package emacs. (Sat, 14 Nov 2020 16:56:03 GMT) Full text and rfc822 format available.

Acknowledgement sent to Spencer Baugh <sbaugh <at> catern.com>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Sat, 14 Nov 2020 16:56:03 GMT) Full text and rfc822 format available.

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

From: Spencer Baugh <sbaugh <at> catern.com>
To: bug-gnu-emacs <at> gnu.org
Cc: Spencer Baugh <sbaugh <at> catern.com>
Subject: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer
Date: Sat, 14 Nov 2020 11:54:59 -0500
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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#44639; Package emacs. (Wed, 02 Dec 2020 14:51:01 GMT) Full text and rfc822 format available.

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

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Spencer Baugh <sbaugh <at> catern.com>
Cc: 44639 <at> debbugs.gnu.org
Subject: Re: bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to
 a single buffer
Date: Wed, 02 Dec 2020 15:50:16 +0100
Spencer Baugh <sbaugh <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#44639; Package emacs. (Wed, 27 Jan 2021 04:00:03 GMT) Full text and rfc822 format available.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Spencer Baugh <sbaugh <at> catern.com>, 44639 <at> debbugs.gnu.org
Subject: Re: bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to
 a single buffer
Date: Wed, 27 Jan 2021 04:59:05 +0100
Michael Albinus <michael.albinus <at> gmx.de> writes:

> Spencer Baugh <sbaugh <at> 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




Added tag(s) moreinfo. Request was from Lars Ingebrigtsen <larsi <at> gnus.org> to control <at> debbugs.gnu.org. (Wed, 27 Jan 2021 04:00:03 GMT) Full text and rfc822 format available.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#44639; Package emacs. (Wed, 27 Jan 2021 16:35:01 GMT) Full text and rfc822 format available.

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

From: Spencer Baugh <sbaugh <at> catern.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>, Michael Albinus
 <michael.albinus <at> gmx.de>
Cc: 44639 <at> debbugs.gnu.org
Subject: Re: bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to
 a single buffer
Date: Wed, 27 Jan 2021 11:34:01 -0500
Lars Ingebrigtsen <larsi <at> gnus.org> writes:
> Michael Albinus <michael.albinus <at> gmx.de> writes:
>
>> Spencer Baugh <sbaugh <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#44639; Package emacs. (Thu, 28 Jan 2021 03:30:01 GMT) Full text and rfc822 format available.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Spencer Baugh <sbaugh <at> catern.com>
Cc: Michael Albinus <michael.albinus <at> gmx.de>, 44639 <at> debbugs.gnu.org
Subject: Re: bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to
 a single buffer
Date: Thu, 28 Jan 2021 04:29:16 +0100
Spencer Baugh <sbaugh <at> 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




Reply sent to Michael Albinus <michael.albinus <at> gmx.de>:
You have taken responsibility. (Thu, 28 Jan 2021 14:20:02 GMT) Full text and rfc822 format available.

Notification sent to Spencer Baugh <sbaugh <at> catern.com>:
bug acknowledged by developer. (Thu, 28 Jan 2021 14:20:02 GMT) Full text and rfc822 format available.

Message #24 received at 44639-done <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Spencer Baugh <sbaugh <at> catern.com>, 44639-done <at> debbugs.gnu.org
Subject: Re: bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to
 a single buffer
Date: Thu, 28 Jan 2021 15:19:33 +0100
[Message part 1 (text/plain, inline)]
Lars Ingebrigtsen <larsi <at> 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.

[Message part 2 (text/x-patch, attachment)]

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

This bug report was last modified 4 years and 131 days ago.

Previous Next


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