bug#35418: [PATCH] Don't poll auto-revert files that use notification

["Mattias Engdegård" <mattiase@acm.org>]

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

29 apr. 2019 kl. 14.18 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
>> I decided to maintain it as a derived state
>> because it felt silly to replace O(1) code with O(N), and the
>> invariant is clear enough (stated in its doc string). (Some of the
>> places where the variable is updated are O(N) but less frequently
>> executed.)
> 
> Yes, but is N large enough to experience the difference?

These things are tricky to measure, but obviously inefficient code just doesn't feel right to write. For example, generating a list just to see if it is non-empty, when that could be determined in a more straightforward way.

> My proposal was to use it NOT as a predicate, but as a function
> returning the buffer list.

Very well; here is an incremental patch (to make the differences clear). It's a compromise: the derived state is gone, but there are two functions: one for the list of buffers that need to be polled, and one for whether that list would be non-empty.

By the way, the patch now uses functions from cl-lib, not just macros. Is there any reason not to?


[-- Attachment #2: 0001-Eliminate-the-auto-revert-polled-buffers-variable.patch --]
[-- Type: application/octet-stream, Size: 5518 bytes --]

From e3fe91d6b760b8325c352e6220997f569f7f2d44 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Mon, 29 Apr 2019 20:21:32 +0200
Subject: [PATCH] Eliminate the auto-revert--polled-buffers variable

Instead of maintaining the `auto-revert--polled-buffers' variable,
calculate it when needed.  When only its emptiness is of interest,
do so in a more efficient way.
---
 lisp/autorevert.el | 46 ++++++++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index b16b1b5833..6387ef1e92 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -107,7 +107,7 @@
 
 ;; Dependencies:
 
-(eval-when-compile (require 'cl-lib))
+(require 'cl-lib)
 (require 'timer)
 (require 'filenotify)
 
@@ -342,11 +342,6 @@ the list of old buffers.")
 (defvar auto-revert-tail-pos 0
   "Position of last known end of file.")
 
-(defvar auto-revert--polled-buffers ()
-  "List of buffers in Auto-Revert Mode that must be polled.
-It contains the buffers in `auto-revert-buffer-list' whose
-`auto-revert-notify-watch-descriptor' is nil.")
-
 (defun auto-revert-find-file-function ()
   (setq-local auto-revert-tail-pos
               (file-attribute-size (file-attributes buffer-file-name))))
@@ -374,12 +369,8 @@ This has been reported by a file notification event.")
 (defun auto-revert-remove-current-buffer (&optional buffer)
   "Remove BUFFER from `auto-revert-buffer-list'.
 BUFFER defaults to `current-buffer'."
-  (unless buffer
-    (setq buffer (current-buffer)))
   (setq auto-revert-buffer-list
-        (delq buffer auto-revert-buffer-list))
-  (setq auto-revert--polled-buffers
-        (delq buffer auto-revert--polled-buffers)))
+        (delq (or buffer (current-buffer)) auto-revert-buffer-list)))
 
 ;;;###autoload
 (define-minor-mode auto-revert-mode
@@ -399,7 +390,6 @@ without being changed in the part that is already in the buffer."
   (if auto-revert-mode
       (when (not (memq (current-buffer) auto-revert-buffer-list))
         (push (current-buffer) auto-revert-buffer-list)
-        (push (current-buffer) auto-revert--polled-buffers)
         (add-hook
          'kill-buffer-hook
          #'auto-revert-remove-current-buffer
@@ -516,12 +506,26 @@ specifies in the mode line."
                    (not (memq buf auto-revert-buffer-list)))
 	  (auto-revert-notify-rm-watch))))))
 
+(defun auto-revert--polled-buffers ()
+  "List of buffers that need to be polled."
+  (cond (global-auto-revert-mode (buffer-list))
+        (auto-revert-always-poll auto-revert-buffer-list)
+        (t (mapcan (lambda (buffer)
+                     (and (not (buffer-local-value
+                                'auto-revert-notify-watch-descriptor buffer))
+                          (list buffer)))
+                   auto-revert-buffer-list))))
+
+;; Same as above in a boolean context, but cheaper.
 (defun auto-revert--need-polling-p ()
   "Whether periodic polling is required."
   (or global-auto-revert-mode
       (if auto-revert-always-poll
           auto-revert-buffer-list
-        auto-revert--polled-buffers)))
+        (not (cl-every (lambda (buffer)
+                         (buffer-local-value
+                          'auto-revert-notify-watch-descriptor buffer))
+                       auto-revert-buffer-list)))))
 
 (defun auto-revert-set-timer ()
   "Restart or cancel the timer used by Auto-Revert Mode.
@@ -592,8 +596,6 @@ 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)
-        (setq auto-revert--polled-buffers
-              (delq (current-buffer) auto-revert--polled-buffers))
         (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
@@ -644,12 +646,10 @@ system.")
                      ;; A buffer w/o a file, like dired.
                      (null buffer-file-name))
                 (auto-revert-notify-rm-watch)
-                (when (memq buffer auto-revert-buffer-list)
-                  (unless (memq buffer auto-revert--polled-buffers)
-                    (push buffer auto-revert--polled-buffers))
-                  ;; Restart the timer if it wasn't running.
-                  (unless auto-revert-timer
-                    (auto-revert-set-timer))))))
+                ;; Restart the timer if it wasn't running.
+                (when (and (memq buffer auto-revert-buffer-list)
+                           (not auto-revert-timer))
+                  (auto-revert-set-timer)))))
 
         ;; Loop over all buffers, in order to find the intended one.
         (cl-dolist (buffer buffers)
@@ -811,9 +811,7 @@ Auto-Revert Mode from `auto-revert-buffer-list', and for canceling
 the timer when no buffers need to be checked."
 
   (save-match-data
-    (let ((bufs (cond (global-auto-revert-mode (buffer-list))
-                      (auto-revert-always-poll auto-revert-buffer-list)
-                      (t auto-revert--polled-buffers)))
+    (let ((bufs (auto-revert--polled-buffers))
 	  remaining new)
       ;; Buffers with remote contents shall be reverted only if the
       ;; connection is established already.
-- 
2.20.1 (Apple Git-117)