bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Dima Kogan]

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

This is a tracker entry for a report and patch in an emacs-devel mailing
list thread:

 http://lists.gnu.org/archive/html/emacs-devel/2014-10/msg00727.html

The issue is that auto-revert-mode reacts to modifications on a timer,
even if its backend is event-driven. There is a prototype patch in that
thread, attached here also.

On Linux, the two available backends have issues, so this patch wouldn't
be completely effective until those are resolved.

The gfile backend has a problem where emacs isn't using glib correctly,
and the gfile backend itself, in turn, adds artificial delays in how it
deals with inotify. Those are discussed (and partly patched) here:

 http://debbugs.gnu.org/cgi/bugreport.cgi?bug=18861

The inotify backend has an issue where it doesn't deal with more than
one monitored file in a directory correctly:

 http://debbugs.gnu.org/cgi/bugreport.cgi?bug=18880


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-auto-revert-mode-can-now-revert-immediately-in-respo.patch --]
[-- Type: text/x-diff, Size: 3665 bytes --]

From 567d8469dfb143786890c65de58bf2ce887e9ebd Mon Sep 17 00:00:00 2001
From: Dima Kogan <dima@secretsauce.net>
Date: Fri, 24 Oct 2014 19:44:43 -0700
Subject: [PATCH] auto-revert-mode can now revert immediately in response to a
 change event

If we have file notifications, we want to update the auto-revert buffers
immediately when a notification occurs. Since file updates can happen very
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').
---
 lisp/autorevert.el | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index f1074e2..7de6ec1 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -531,6 +531,34 @@ will use an up-to-date value of `auto-revert-interval'"
       ;; Fallback to file checks.
       (set (make-local-variable 'auto-revert-use-notify) nil))))
 
+
+
+;; If we have file notifications, we want to update the auto-revert buffers
+;; immediately when a notification occurs. Since file updates can happen very
+;; 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 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'")
+(make-variable-buffer-local 'auto-revert-buffers-counter-lockedout)
+
+
 (defun auto-revert-notify-handler (event)
   "Handle an EVENT returned from file notification."
   (with-demoted-errors
@@ -566,6 +594,14 @@ will use an up-to-date value of `auto-revert-interval'"
                                 (file-name-nondirectory buffer-file-name)))))
                 ;; Mark buffer modified.
                 (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)
+                  (auto-revert-handler)
+                  (setq auto-revert-buffers-counter-lockedout
+                        auto-revert-buffers-counter))
+
                 ;; No need to check other buffers.
                 (cl-return)))))))))
 
@@ -686,6 +722,10 @@ are checked first the next time this function is called.
 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)
-- 
2.0.0

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Stefan Monnier]

> The issue is that auto-revert-mode reacts to modifications on a timer,
> even if its backend is event-driven. There is a prototype patch in that
> thread, attached here also.

The patch looks OK to me, tho please use defvar-local.


        Stefan

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Michael Albinus]

Dima Kogan <dima@secretsauce.net> writes:

Hi Dima,

I'm sorry that I could not interact earlier. Meanwhile I have read all
the threads about, and I believe your proposed patch is worth to be
applied.

> The issue is that auto-revert-mode reacts to modifications on a timer,
> even if its backend is event-driven. There is a prototype patch in that
> thread, attached here also.
>
> On Linux, the two available backends have issues, so this patch wouldn't
> be completely effective until those are resolved.
>
> The gfile backend has a problem where emacs isn't using glib correctly,
> and the gfile backend itself, in turn, adds artificial delays in how it
> deals with inotify. Those are discussed (and partly patched) here:
>
>  http://debbugs.gnu.org/cgi/bugreport.cgi?bug=18861
>
> The inotify backend has an issue where it doesn't deal with more than
> one monitored file in a directory correctly:
>
>  http://debbugs.gnu.org/cgi/bugreport.cgi?bug=18880

Both bugs are fixed meanwhile.

> From 567d8469dfb143786890c65de58bf2ce887e9ebd Mon Sep 17 00:00:00 2001
> From: Dima Kogan <dima@secretsauce.net>
> Date: Fri, 24 Oct 2014 19:44:43 -0700
> Subject: [PATCH] auto-revert-mode can now revert immediately in response to a
>  change event

I've applied your patch to the master branch (including Stefan's
recommendation to use defvar-local). Could you, please, recheck that
everything works as expected, also wrt the other two bugs you've mentioned?

Best regards, Michael.

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Dima Kogan]

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

> I've applied your patch to the master branch (including Stefan's
> recommendation to use defvar-local). Could you, please, recheck that
> everything works as expected, also wrt the other two bugs you've mentioned?

Hi. Thanks for applying the patch. The basic inotify stuff appears to
work now with multiple files, which is great. I haven't tested gfile
yet.

The notification throttling seems to have a problem. Recipe:

1. rm /tmp/tst; touch /tmp/tst
2. emacs -Q -nw /tmp/tst
3. M-x auto-revert-mode
4. In a separate shell: date >> /tmp/tst

Most of the time I don't see the buffer update, not even after waiting a
bit. After a manual revert, auto-revert-mode does do what it's supposed
to. I haven't debugged this yet. Will do this tomorrow if you don't beat
me to it.

Thanks again
dima

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Michael Albinus]

Dima Kogan <dima@secretsauce.net> writes:

> The notification throttling seems to have a problem. Recipe:
>
> 1. rm /tmp/tst; touch /tmp/tst
> 2. emacs -Q -nw /tmp/tst
> 3. M-x auto-revert-mode
> 4. In a separate shell: date >> /tmp/tst

Well, yesterday I have used a slightly different test:

3. M-x auto-revert-tail-mode
4. In a separate shell:

while /bin/true; do
  date >> /tmp/tst
  sleep 2
done

You see in 5 sec intervals the autorevert due to the loop, and in the
time between just one autorevert triggered directly by the notification
event.

> Thanks again
> dima

Best regards, Michael.

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Dima Kogan]

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

Dima Kogan <dima@secretsauce.net> writes:

> The notification throttling seems to have a problem. Recipe:

Hi. I figured this out. The notification handler was being installed
during a timer update, NOT when auto-revert-mode was initiated. So in
order for the code to work you'd have to

- M-x auto-revert-mode
- wait for the timer. ~5 seconds or so
- update file

If you didn't wait, then this first update would be ignored.

The code to install the handler when needed was already there, but for
some reason it was disabled. Attaching a patch that makes it work for
all of auto-revert-mode, auto-revert-tail-mode and
global-auto-revert-mode. This patch removes

 (let (auto-revert-use-notify) ... )

wrappers to let the existing code to its thing, and for
auto-revert-tail-mode this patch calls (auto-revert-buffers) instead of
(auto-revert-handler), like the other two code paths already do.
Truthfully, it probably does make sense to call (auto-revert-handler)
for the two non-global functions, but the handler-install code would
then need to be moved.

I still need to double-check the gfile stuff, and I'll do that when I
get your ok on this. The gfile also has a delay in glib itself, so this
wouldn't be as nice as inotify until glib is patched.




[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-notification-handler-is-now-installed-when-auto-reve.patch --]
[-- Type: text/x-diff, Size: 1793 bytes --]

From bb2dec478fce20a25a737f16f26c0b338d599552 Mon Sep 17 00:00:00 2001
From: Dima Kogan <dima@secretsauce.net>
Date: Fri, 20 Feb 2015 19:38:17 -0800
Subject: [PATCH] notification handler is now installed when auto-revert-mode
 is entered

---
 lisp/autorevert.el | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 02cef24..4dd021e 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -361,9 +361,8 @@ without being changed in the part that is already in the buffer."
 	  (delq (current-buffer) auto-revert-buffer-list)))
   (auto-revert-set-timer)
   (when auto-revert-mode
-    (let (auto-revert-use-notify)
-      (auto-revert-buffers)
-      (setq auto-revert-tail-mode nil))))
+    (auto-revert-buffers)
+    (setq auto-revert-tail-mode nil)))
 
 
 ;;;###autoload
@@ -417,8 +416,7 @@ Use `auto-revert-mode' for changes other than appends!"
            (y-or-n-p "File changed on disk, content may be missing.  \
 Perform a full revert? ")
            ;; Use this (not just revert-buffer) for point-preservation.
-	   (let (auto-revert-use-notify)
-	     (auto-revert-handler)))
+           (auto-revert-buffers))
       ;; else we might reappend our own end when we save
       (add-hook 'before-save-hook (lambda () (auto-revert-tail-mode 0)) nil t)
       (or (local-variable-p 'auto-revert-tail-pos) ; don't lose prior position
@@ -463,8 +461,7 @@ specifies in the mode line."
   :global t :group 'auto-revert :lighter global-auto-revert-mode-text
   (auto-revert-set-timer)
   (if global-auto-revert-mode
-      (let (auto-revert-use-notify)
-	(auto-revert-buffers))
+      (auto-revert-buffers)
     (dolist (buf (buffer-list))
       (with-current-buffer buf
 	(when auto-revert-use-notify
-- 
2.0.0

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Michael Albinus]

Dima Kogan <dima@secretsauce.net> writes:

Hi Dima,

> I still need to double-check the gfile stuff, and I'll do that when I
> get your ok on this. The gfile also has a delay in glib itself, so this
> wouldn't be as nice as inotify until glib is patched.

Thanks for this. I've committed your patch to the repo. Honestly, I
don't remember why I have suppressed auto-revert-use-notify when
initiating the auto-revert modes, maybe I was simply overcautious. If
there are problems with this change, we will hear soon :-)

Per default, I have enabled gfilenotify as notification library; it
works also w/o problems. Of course, I cannot speak for w32notify, maybe
somebody can run a short test.

Best regards, Michael.

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Sat, 21 Feb 2015 10:57:13 +0100
> Cc: 18958@debbugs.gnu.org
> 
> Per default, I have enabled gfilenotify as notification library; it
> works also w/o problems. Of course, I cannot speak for w32notify, maybe
> somebody can run a short test.

If you can tell me what test to run, I can try doing that.

Thanks.

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> Per default, I have enabled gfilenotify as notification library; it
>> works also w/o problems. Of course, I cannot speak for w32notify, maybe
>> somebody can run a short test.
>
> If you can tell me what test to run, I can try doing that.

I fear we have no ert test case yet, it is a matter of timing. Maybe I
write something next days, or somebody else has a got idea how to do.

Test: Open a file in Emacs, and enable auto-revert-mode or
auto-revert-tail-mode. Change it outside Emacs. The reverted file shall
be shown immediately in Emacs.

Since it is not obvious whether the reversion was triggered by the
notification event directly, or by the auto-revert-buffers call every
auto-revert-interval seconds, you might run the test several times. Note,
that a direct revert triggered by the notification event happens only
once every auto-revert-interval period, in order to defend against fast
series of notification events.

Another test I would also like to see on w32 is for the change I've
committed some few minutes ago: auto-revert of dired buffers. Call
auto-revert-mode on a dired buffer, and create/delete a file in the
respective directory outside Emacs. The dired buffer shall be reverted
immediately as well.

> Thanks.

Thanks, Michael.

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: dima@secretsauce.net,  18958@debbugs.gnu.org
> Date: Sat, 21 Feb 2015 13:43:37 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Per default, I have enabled gfilenotify as notification library; it
> >> works also w/o problems. Of course, I cannot speak for w32notify, maybe
> >> somebody can run a short test.
> >
> > If you can tell me what test to run, I can try doing that.
> 
> I fear we have no ert test case yet, it is a matter of timing. Maybe I
> write something next days, or somebody else has a got idea how to do.
> 
> Test: Open a file in Emacs, and enable auto-revert-mode or
> auto-revert-tail-mode. Change it outside Emacs. The reverted file shall
> be shown immediately in Emacs.

I compared with Emacs 24.4.90, and indeed the master version shows the
updated contents much faster.

> Another test I would also like to see on w32 is for the change I've
> committed some few minutes ago: auto-revert of dired buffers. Call
> auto-revert-mode on a dired buffer, and create/delete a file in the
> respective directory outside Emacs. The dired buffer shall be reverted
> immediately as well.

Likewise: the 25.0.50 version reverts immediately, while 24.4.90
sometimes waits for several seconds (as expected).

So it looks like things work on w32 as intended; thanks.

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Dima Kogan]

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

> Per default, I have enabled gfilenotify as notification library; it
> works also w/o problems.

Hi. I just tested auto-revert-mode with gfilenotify, and it does appear
to work. Thank you very much for merging those patches! Where available
I think it would be better for us to use inotify instead of gfilenotify,
however, because glib currently has a minimum 1-second delay:

 https://bugzilla.gnome.org/show_bug.cgi?id=739322

In a perfect world, this 1-second delay would be removed upstream, and
we'd use gfilenotify for everything. In the meantime, is gfilenotify
better in any way on Linux? If the notifications are unavailable for
some reason (network-mounted file for instance), then emacs simply
decays to the normal polled notifications, right?

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Michael Albinus]

Dima Kogan <dima@secretsauce.net> writes:

> Hi. I just tested auto-revert-mode with gfilenotify, and it does appear
> to work. Thank you very much for merging those patches!

I'm marking this bug as closed.

> In a perfect world, this 1-second delay would be removed upstream, and
> we'd use gfilenotify for everything. In the meantime, is gfilenotify
> better in any way on Linux? If the notifications are unavailable for
> some reason (network-mounted file for instance), then emacs simply
> decays to the normal polled notifications, right?

You mean auto-revert, right? And yes, if `file-notify-add-watch' doesn't
return a proper watch descriptor, auto-revert falls back to its polling
mode.

glib is said to use also polling for file systems where you don't have
native file notification support. This would make it superior to inotify
for mounted filesystems, and alike.

Best regards, Michael.

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Sun, 22 Feb 2015 10:53:03 +0100
> Cc: 18958-done@debbugs.gnu.org
> 
> glib is said to use also polling for file systems where you don't have
> native file notification support. This would make it superior to inotify
> for mounted filesystems, and alike.

The native Windows port of glib either crashes or simply doesn't
generate notifications when file notifications are requested.

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Dima Kogan]

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

> Dima Kogan <dima@secretsauce.net> writes:
>
>> In a perfect world, this 1-second delay would be removed upstream, and
>> we'd use gfilenotify for everything. In the meantime, is gfilenotify
>> better in any way on Linux? If the notifications are unavailable for
>> some reason (network-mounted file for instance), then emacs simply
>> decays to the normal polled notifications, right?
>
> You mean auto-revert, right? And yes, if `file-notify-add-watch' doesn't
> return a proper watch descriptor, auto-revert falls back to its polling
> mode.

Yes, I meant auto-revert.


> glib is said to use also polling for file systems where you don't have
> native file notification support. This would make it superior to inotify
> for mounted filesystems, and alike.

But how is it superior? Didn't you just say that when inotify fails then
emacs polls, just as would happen with gfilenotify? To confirm, I just
used an inotify-configured emacs to auto-revert on a file mounted on a
network samba mount. As expected, the auto-revert-mode does work, but in
a polled instead of event-driven fashion. So unless I'm missing
something, then when you run the current emacs code on Linux both
inotify and gfile work the same way EXCEPT gfile always has a 1-second
notification delay while inotify does not. I'm probably missing
something.

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Michael Albinus]

Dima Kogan <dima@secretsauce.net> writes:

>> glib is said to use also polling for file systems where you don't have
>> native file notification support. This would make it superior to inotify
>> for mounted filesystems, and alike.
>
> But how is it superior? Didn't you just say that when inotify fails then
> emacs polls, just as would happen with gfilenotify? To confirm, I just
> used an inotify-configured emacs to auto-revert on a file mounted on a
> network samba mount. As expected, the auto-revert-mode does work, but in
> a polled instead of event-driven fashion. So unless I'm missing
> something, then when you run the current emacs code on Linux both
> inotify and gfile work the same way EXCEPT gfile always has a 1-second
> notification delay while inotify does not. I'm probably missing
> something.

I meant gfilenotify is superior to inotify wrt file notifications due to
its polling feature. That does not mean auto-revert, since Emacs has its
own polling mechanism here.

Best regards, Michael.

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Dima Kogan]

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

> I meant gfilenotify is superior to inotify wrt file notifications due to
> its polling feature. That does not mean auto-revert, since Emacs has its
> own polling mechanism here.

Ah, ok. This is true. Would you be opposed to setting the default to
"inotify" on Linux since this currently provides a better user
experience there?

bug#18958: 25.0.50; auto-revert-mode reacts slowly even if using an event-driven backend

[Michael Albinus]

Dima Kogan <dima@secretsauce.net> writes:

>> I meant gfilenotify is superior to inotify wrt file notifications due to
>> its polling feature. That does not mean auto-revert, since Emacs has its
>> own polling mechanism here.
>
> Ah, ok. This is true. Would you be opposed to setting the default to
> "inotify" on Linux since this currently provides a better user
> experience there?

I believe this was discussed already back in 2013, when I've contributed
gfilenotify.c. I cannot find the emails, likely there's something in the
archives.

What's the default must be decided by Stefan anyway, as the Emacs
maintainer. Personally, I can live with this 1sec delay of glib. And
glib is linked to Emacs also for other reasons, if not configured off.

Maybe it's better to discuss this on emacs-devel, but in this bug.

Best regards, Michael.