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

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

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

29 apr. 2019 kl. 09.53 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
>> Here is an updated patch. There is a new variable,
>> `auto-revert-always-poll', which is t by default.
>> There is also a note in etc/NEWS. Does it merit a mention in the manual as well?
> 
> Yes, please.

There is now a paragraph added to the manual.

By the way, the organisation of this part of the manual could be improved -- don't you agree?

There is a section called Reverting, which starts about `revert-buffer' but then goes on to talk about the auto-revert, global-auto-revert and auto-revert-tail modes and details about the mechanisms behind them: polling, intervals, notification.

Then there is a (sibling) section called Autorevert, which despite its name only talks about auto-reverting non-file buffers.

This can be reorganised in various ways. We could move all autorevert text to a sibling node to Reverting, or to one or more child nodes. In any case, such text shuffling should not be part of this patch.

> I believe it shall be said, that this user option does not compete with
> `auto-revert-use-notify'. Rather, polling is used additionally to file
> notification. When `auto-revert-use-notify' is nil, the value of
> `auto-revert-always-poll' doesn't matter; there will always be polling.

Good point; the doc string has been clarified.

> Saying this, the user option might need another name. What about
> `auto-revert-also-poll'?

Naming is always hard. I started with `auto-revert-avoid-polling' but wanted to avoid a negative name.
I tried `auto-revert-also-poll' but it somehow didn't feel right; not all buffers use notification.
It is nothing I feel strongly about, so if you do prefer that name I'll change, but I've kept the original name in the patch for now.

>> +(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.")
> 
> Is this variable needed? It is used only once in
> `auto-revert--need-polling', and it could be computed easily by

It is also used in `auto-revert-buffers', but you are quite right that it could be a function. 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.)

I can replace it with a function if you want, but the code didn't seem to gain much from doing so.

> `auto-revert--need-polling' shall always return the buffer list, also for
> `global-auto-revert-mode'.

Sorry, it was meant as a predicate and is only used as such.
Clarified by renaming it to `auto-revert--need-polling-p'.

Thank you very much for your review! Updated patch attached.


[-- Attachment #2: 0001-Don-t-poll-auto-revert-files-that-use-notification.patch --]
[-- Type: application/octet-stream, Size: 13578 bytes --]

From 17a48cc4a106112830b1399fff2966bd16b8c23c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 24 Apr 2019 18:39:05 +0200
Subject: [PATCH] Don't poll auto-revert files that use notification

It is a waste to periodically poll files that use change notification
in auto-revert mode; stop doing that.  If no files need polling,
turn off the periodic execution entirely to further avoid wasting power.
Use a timer to inhibit immediate reversion for some time after a
notification.

This change does not apply to files in global-auto-revert-mode, where
polling is still necessary.  It is disabled by default, and enabled by
setting `auto-revert-always-poll' to nil.

* lisp/autorevert.el
(auto-revert--polled-buffers, auto-revert--lockout-interval,
auto-revert--lockout-timer, auto-revert--end-lockout, auto-revert-always-poll,
auto-revert--need-polling-p): New.
(auto-revert-remove-current-buffer, auto-revert-mode,
global-auto-revert-mode, auto-revert-set-timer,
auto-revert-notify-add-watch, auto-revert-buffers,
auto-revert-notify-handler): Maintain and use auto-revert--polled-buffers.
Honour new lockout timer.  Start lockout timer if necessary.
Maintain and use auto-revert--polled-buffers.
(auto-revert-buffers-counter, auto-revert-buffers-counter-lockedout): Remove.

* etc/NEWS (Changes in Specialized Modes and Packages): Mention the change.

* doc/emacs/files.texi (Reverting): Add paragraph describing
auto-revert-always-poll.
---
 doc/emacs/files.texi |  13 +++++
 etc/NEWS             |  10 ++++
 lisp/autorevert.el   | 125 ++++++++++++++++++++++++++++++-------------
 3 files changed, 111 insertions(+), 37 deletions(-)

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index a57428230c..2dd48503b4 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -988,6 +988,19 @@ the polling interval through the variable @code{auto-revert-interval}.
 supported, @code{auto-revert-use-notify} will be @code{nil} by
 default.
 
+@vindex auto-revert-always-poll
+@vindex auto-revert-notify-exclude-dir-regexp
+  By default, Auto-Revert mode will poll files for changes
+periodically even when file notifications are used.  Such polling is
+usually unnecessary, and turning it off may save power by relying on
+notifications only.  To do so, set the variable
+@code{auto-revert-always-poll} to @code{nil}.  However, notification
+is ineffective on certain file systems; mainly network file system on
+Unix-like machines, where files can be altered from other machines.
+To force polling when @code{auto-revert-always-poll} is @code{nil},
+set @code{auto-revert-notify-exclude-dir-regexp} to match files that
+should be excluded from using notification.
+
   One use of Auto-Revert mode is to ``tail'' a file such as a system
 log, so that changes made to that file by other programs are
 continuously displayed.  To do this, just move the point to the end of
diff --git a/etc/NEWS b/etc/NEWS
index 9b32d720b6..cf997aa0c8 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1389,6 +1389,16 @@ Packages deriving from 'js-mode' with 'define-derived-mode' should
 call this function to add enabled syntax extensions to their mode
 name, too.
 
+** Autorevert
+
+*** New variable 'auto-revert-always-poll' for saving power.
+Set this variable to nil to prevent buffers in auto-revert mode from
+being polled for changes periodically.  This reduces the power
+consumption of an idle Emacs, but may fail on some network file
+systems.  Make sure that 'auto-revert-notify-exclude-dir-regexp'
+matches files where notification is not supported.
+The default value is t.
+
 \f
 * New Modes and Packages in Emacs 27.1
 
diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 1d20896c83..b16b1b5833 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -302,6 +302,29 @@ You should set this variable through Custom."
   :type 'regexp
   :version "24.4")
 
+(defcustom auto-revert-always-poll t
+  "Non-nil to poll files in addition to the use of notification.
+
+Set this variable to nil to save power by avoiding polling when
+possible.  Files on file-systems that do not support change
+notifications must match `auto-revert-notify-exclude-dir-regexp'
+for Auto-Revert to work properly in this case.  This typically
+includes files on network file systems on Unix-like machines,
+when those files are modified from another computer.
+
+When non-nil, buffers in Auto-Revert Mode will always be polled
+for changes to their files on disk every `auto-revert-interval'
+seconds, in addition to using notification for those files.
+
+In Global Auto-Revert Mode, polling is always done regardless of
+the value of this variable."
+  :group 'auto-revert
+  :type 'boolean
+  :set (lambda (variable value)
+         (set-default variable value)
+         (auto-revert-set-timer))
+  :version "27.1")
+
 ;; Internal variables:
 
 (defvar auto-revert-buffer-list ()
@@ -319,6 +342,11 @@ 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))))
@@ -346,8 +374,12 @@ 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 (or buffer (current-buffer)) auto-revert-buffer-list)))
+        (delq buffer auto-revert-buffer-list))
+  (setq auto-revert--polled-buffers
+        (delq buffer auto-revert--polled-buffers)))
 
 ;;;###autoload
 (define-minor-mode auto-revert-mode
@@ -367,6 +399,7 @@ 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
@@ -479,9 +512,17 @@ specifies in the mode line."
       (auto-revert-buffers)
     (dolist (buf (buffer-list))
       (with-current-buffer buf
-	(when auto-revert-notify-watch-descriptor
+        (when (and auto-revert-notify-watch-descriptor
+                   (not (memq buf auto-revert-buffer-list)))
 	  (auto-revert-notify-rm-watch))))))
 
+(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)))
+
 (defun auto-revert-set-timer ()
   "Restart or cancel the timer used by Auto-Revert Mode.
 If such a timer is active, cancel it.  Start a new timer if
@@ -492,10 +533,10 @@ will use an up-to-date value of `auto-revert-interval'"
   (if (timerp auto-revert-timer)
       (cancel-timer auto-revert-timer))
   (setq auto-revert-timer
-	(if (or global-auto-revert-mode auto-revert-buffer-list)
-	    (run-with-timer auto-revert-interval
-			    auto-revert-interval
-			    'auto-revert-buffers))))
+	(and (auto-revert--need-polling-p)
+	     (run-with-timer auto-revert-interval
+			     auto-revert-interval
+			     'auto-revert-buffers))))
 
 (defun auto-revert-notify-rm-watch ()
   "Disable file notification for current buffer's associated file."
@@ -551,6 +592,8 @@ 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
@@ -558,24 +601,20 @@ will use an up-to-date value of `auto-revert-interval'"
 ;; often, we want to skip some revert operations so that we don't spend all our
 ;; time reverting the buffer.
 ;;
-;; We do this by reverting immediately in response to the first in a flurry of
-;; notifications. We suppress subsequent notifications until the next time
-;; `auto-revert-buffers' is called (this happens on a timer with a period set by
-;; `auto-revert-interval').
-(defvar auto-revert-buffers-counter 1
-  "Incremented each time `auto-revert-buffers' is called")
-(defvar-local auto-revert-buffers-counter-lockedout 0
-  "Buffer-local value to indicate whether we should immediately
-update the buffer on a notification event or not. If
-
-  (= auto-revert-buffers-counter-lockedout
-     auto-revert-buffers-counter)
-
-then the updates are locked out, and we wait until the next call
-of `auto-revert-buffers' to revert the buffer. If no lockout is
-present, then we revert immediately and set the lockout, so that
-no more reverts are possible until the next call of
-`auto-revert-buffers'")
+;; We do this by reverting immediately in response to the first in a
+;; flurry of notifications. Any notifications during the following
+;; `auto-revert-lockout-interval' seconds are noted but not acted upon
+;; until the end of that interval.
+
+(defconst auto-revert--lockout-interval 2.5
+  "Duration, in seconds, of the Auto-Revert Mode notification lockout.
+This is the quiescence after each notification of a file being
+changed during which no automatic reverting takes place, to
+prevent many updates in rapid succession from overwhelming the
+system.")
+
+(defvar-local auto-revert--lockout-timer nil
+  "Timer awaiting the end of the notification lockout interval, or nil.")
 
 (defun auto-revert-notify-handler (event)
   "Handle an EVENT returned from file notification."
@@ -604,7 +643,13 @@ no more reverts are possible until the next call of
                            (file-name-nondirectory buffer-file-name)))
                      ;; A buffer w/o a file, like dired.
                      (null buffer-file-name))
-                (auto-revert-notify-rm-watch))))
+                (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))))))
 
         ;; Loop over all buffers, in order to find the intended one.
         (cl-dolist (buffer buffers)
@@ -630,11 +675,21 @@ no more reverts are possible until the next call of
                 (setq auto-revert-notify-modified-p t)
 
                 ;; Revert the buffer now if we're not locked out.
-                (when (/= auto-revert-buffers-counter-lockedout
-                          auto-revert-buffers-counter)
+                (unless auto-revert--lockout-timer
                   (auto-revert-handler)
-                  (setq auto-revert-buffers-counter-lockedout
-                        auto-revert-buffers-counter))))))))))
+                  (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.
+If the buffer needs to be reverted, do it now."
+  (when (buffer-live-p buffer)
+    (with-current-buffer buffer
+      (setq auto-revert--lockout-timer nil)
+      (when auto-revert-notify-modified-p
+        (auto-revert-handler)))))
 
 (defun auto-revert-active-p ()
   "Check if auto-revert is active (in current buffer or globally)."
@@ -755,13 +810,10 @@ This function is also responsible for removing buffers no longer in
 Auto-Revert Mode from `auto-revert-buffer-list', and for canceling
 the timer when no buffers need to be checked."
 
-  (setq auto-revert-buffers-counter
-        (1+ auto-revert-buffers-counter))
-
   (save-match-data
-    (let ((bufs (if global-auto-revert-mode
-		    (buffer-list)
-		  auto-revert-buffer-list))
+    (let ((bufs (cond (global-auto-revert-mode (buffer-list))
+                      (auto-revert-always-poll auto-revert-buffer-list)
+                      (t auto-revert--polled-buffers)))
 	  remaining new)
       ;; Buffers with remote contents shall be reverted only if the
       ;; connection is established already.
@@ -810,8 +862,7 @@ the timer when no buffers need to be checked."
 	(setq bufs (cdr bufs)))
       (setq auto-revert-remaining-buffers bufs)
       ;; Check if we should cancel the timer.
-      (when (and (not global-auto-revert-mode)
-		 (null auto-revert-buffer-list))
+      (unless (auto-revert--need-polling-p)
         (if (timerp auto-revert-timer)
             (cancel-timer auto-revert-timer))
 	(setq auto-revert-timer nil)))))
-- 
2.20.1 (Apple Git-117)