* bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors @ 2020-11-14 16:54 Spencer Baugh 2020-11-14 16:54 ` bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer Spencer Baugh 2020-11-14 17:22 ` bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors Eli Zaretskii 0 siblings, 2 replies; 22+ messages in thread From: Spencer Baugh @ 2020-11-14 16:54 UTC (permalink / raw) To: 44638; +Cc: 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer 2020-11-14 16:54 bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors Spencer Baugh @ 2020-11-14 16:54 ` Spencer Baugh 2020-12-02 14:50 ` Michael Albinus 2020-11-14 17:22 ` bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors Eli Zaretskii 1 sibling, 1 reply; 22+ messages in thread From: Spencer Baugh @ 2020-11-14 16:54 UTC (permalink / raw) To: 44639; +Cc: 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer 2020-11-14 16:54 ` bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer Spencer Baugh @ 2020-12-02 14:50 ` Michael Albinus 2021-01-27 3:59 ` Lars Ingebrigtsen 0 siblings, 1 reply; 22+ messages in thread From: Michael Albinus @ 2020-12-02 14:50 UTC (permalink / raw) To: Spencer Baugh; +Cc: 44639 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer 2020-12-02 14:50 ` Michael Albinus @ 2021-01-27 3:59 ` Lars Ingebrigtsen 2021-01-27 16:34 ` Spencer Baugh 0 siblings, 1 reply; 22+ messages in thread From: Lars Ingebrigtsen @ 2021-01-27 3:59 UTC (permalink / raw) To: Michael Albinus; +Cc: Spencer Baugh, 44639 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer 2021-01-27 3:59 ` Lars Ingebrigtsen @ 2021-01-27 16:34 ` Spencer Baugh 2021-01-28 3:29 ` Lars Ingebrigtsen 0 siblings, 1 reply; 22+ messages in thread From: Spencer Baugh @ 2021-01-27 16:34 UTC (permalink / raw) To: Lars Ingebrigtsen, Michael Albinus; +Cc: 44639 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer 2021-01-27 16:34 ` Spencer Baugh @ 2021-01-28 3:29 ` Lars Ingebrigtsen 2021-01-28 14:19 ` Michael Albinus 0 siblings, 1 reply; 22+ messages in thread From: Lars Ingebrigtsen @ 2021-01-28 3:29 UTC (permalink / raw) To: Spencer Baugh; +Cc: Michael Albinus, 44639 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer 2021-01-28 3:29 ` Lars Ingebrigtsen @ 2021-01-28 14:19 ` Michael Albinus 0 siblings, 0 replies; 22+ messages in thread From: Michael Albinus @ 2021-01-28 14:19 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: Spencer Baugh, 44639-done [-- 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors 2020-11-14 16:54 bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors Spencer Baugh 2020-11-14 16:54 ` bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer Spencer Baugh @ 2020-11-14 17:22 ` Eli Zaretskii 2020-11-14 21:19 ` Spencer Baugh 1 sibling, 1 reply; 22+ messages in thread From: Eli Zaretskii @ 2020-11-14 17:22 UTC (permalink / raw) To: Spencer Baugh; +Cc: sbaugh, 44638 > 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors 2020-11-14 17:22 ` bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors Eli Zaretskii @ 2020-11-14 21:19 ` Spencer Baugh 2020-11-30 18:01 ` Spencer Baugh 0 siblings, 1 reply; 22+ messages in thread From: Spencer Baugh @ 2020-11-14 21:19 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 44638 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors 2020-11-14 21:19 ` Spencer Baugh @ 2020-11-30 18:01 ` Spencer Baugh 2020-11-30 18:21 ` Eli Zaretskii 0 siblings, 1 reply; 22+ messages in thread From: Spencer Baugh @ 2020-11-30 18:01 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 44638 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors 2020-11-30 18:01 ` Spencer Baugh @ 2020-11-30 18:21 ` Eli Zaretskii 2020-11-30 18:31 ` Michael Albinus 0 siblings, 1 reply; 22+ messages in thread From: Eli Zaretskii @ 2020-11-30 18:21 UTC (permalink / raw) To: Spencer Baugh; +Cc: 44638 > 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? ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors 2020-11-30 18:21 ` Eli Zaretskii @ 2020-11-30 18:31 ` Michael Albinus 2020-12-01 20:16 ` Michael Albinus 0 siblings, 1 reply; 22+ messages in thread From: Michael Albinus @ 2020-11-30 18:31 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Spencer Baugh, 44638 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors 2020-11-30 18:31 ` Michael Albinus @ 2020-12-01 20:16 ` Michael Albinus 2020-12-02 3:20 ` Eli Zaretskii 0 siblings, 1 reply; 22+ messages in thread From: Michael Albinus @ 2020-12-01 20:16 UTC (permalink / raw) To: Spencer Baugh; +Cc: 44638 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors 2020-12-01 20:16 ` Michael Albinus @ 2020-12-02 3:20 ` Eli Zaretskii 2020-12-02 15:17 ` Michael Albinus 0 siblings, 1 reply; 22+ messages in thread From: Eli Zaretskii @ 2020-12-02 3:20 UTC (permalink / raw) To: Michael Albinus; +Cc: sbaugh, 44638 > 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors 2020-12-02 3:20 ` Eli Zaretskii @ 2020-12-02 15:17 ` Michael Albinus 2020-12-02 15:41 ` Eli Zaretskii 0 siblings, 1 reply; 22+ messages in thread From: Michael Albinus @ 2020-12-02 15:17 UTC (permalink / raw) To: Eli Zaretskii; +Cc: sbaugh, 44638 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors 2020-12-02 15:17 ` Michael Albinus @ 2020-12-02 15:41 ` Eli Zaretskii 2020-12-02 17:05 ` Michael Albinus 0 siblings, 1 reply; 22+ messages in thread From: Eli Zaretskii @ 2020-12-02 15:41 UTC (permalink / raw) To: Michael Albinus; +Cc: sbaugh, 44638 > 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors 2020-12-02 15:41 ` Eli Zaretskii @ 2020-12-02 17:05 ` Michael Albinus 2020-12-02 17:13 ` Eli Zaretskii 0 siblings, 1 reply; 22+ messages in thread From: Michael Albinus @ 2020-12-02 17:05 UTC (permalink / raw) To: Eli Zaretskii; +Cc: sbaugh, 44638 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors 2020-12-02 17:05 ` Michael Albinus @ 2020-12-02 17:13 ` Eli Zaretskii 2020-12-02 17:20 ` Michael Albinus 2020-12-02 17:46 ` Dmitry Gutov 0 siblings, 2 replies; 22+ messages in thread From: Eli Zaretskii @ 2020-12-02 17:13 UTC (permalink / raw) To: Michael Albinus; +Cc: sbaugh, 44638 > 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors 2020-12-02 17:13 ` Eli Zaretskii @ 2020-12-02 17:20 ` Michael Albinus 2020-12-02 17:43 ` Eli Zaretskii 2020-12-03 15:01 ` Michael Albinus 2020-12-02 17:46 ` Dmitry Gutov 1 sibling, 2 replies; 22+ messages in thread From: Michael Albinus @ 2020-12-02 17:20 UTC (permalink / raw) To: Eli Zaretskii; +Cc: sbaugh, 44638 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors 2020-12-02 17:20 ` Michael Albinus @ 2020-12-02 17:43 ` Eli Zaretskii 2020-12-03 15:01 ` Michael Albinus 1 sibling, 0 replies; 22+ messages in thread From: Eli Zaretskii @ 2020-12-02 17:43 UTC (permalink / raw) To: Michael Albinus; +Cc: sbaugh, 44638 > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors 2020-12-02 17:20 ` Michael Albinus 2020-12-02 17:43 ` Eli Zaretskii @ 2020-12-03 15:01 ` Michael Albinus 1 sibling, 0 replies; 22+ messages in thread From: Michael Albinus @ 2020-12-03 15:01 UTC (permalink / raw) To: Eli Zaretskii; +Cc: sbaugh, 44638-done 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors 2020-12-02 17:13 ` Eli Zaretskii 2020-12-02 17:20 ` Michael Albinus @ 2020-12-02 17:46 ` Dmitry Gutov 1 sibling, 0 replies; 22+ messages in thread From: Dmitry Gutov @ 2020-12-02 17:46 UTC (permalink / raw) To: Eli Zaretskii, Michael Albinus; +Cc: sbaugh, 44638 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2021-01-28 14:19 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-14 16:54 bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors Spencer Baugh 2020-11-14 16:54 ` bug#44639: [PATCH 2/2] autorevert: map each watch descriptor to a single buffer Spencer Baugh 2020-12-02 14:50 ` Michael Albinus 2021-01-27 3:59 ` Lars Ingebrigtsen 2021-01-27 16:34 ` Spencer Baugh 2021-01-28 3:29 ` Lars Ingebrigtsen 2021-01-28 14:19 ` Michael Albinus 2020-11-14 17:22 ` bug#44638: [PATCH 1/2] autorevert: don't reuse existing watch descriptors Eli Zaretskii 2020-11-14 21:19 ` Spencer Baugh 2020-11-30 18:01 ` Spencer Baugh 2020-11-30 18:21 ` Eli Zaretskii 2020-11-30 18:31 ` Michael Albinus 2020-12-01 20:16 ` Michael Albinus 2020-12-02 3:20 ` Eli Zaretskii 2020-12-02 15:17 ` Michael Albinus 2020-12-02 15:41 ` Eli Zaretskii 2020-12-02 17:05 ` Michael Albinus 2020-12-02 17:13 ` Eli Zaretskii 2020-12-02 17:20 ` Michael Albinus 2020-12-02 17:43 ` Eli Zaretskii 2020-12-03 15:01 ` Michael Albinus 2020-12-02 17:46 ` Dmitry Gutov
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).