bug#66117: 30.0.50; `find-buffer-visiting' is slow when opening large number of buffers

[Ihor Radchenko <yantar92@posteo.net>]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> So, using `with-current-buffer' when looping over all the buffers is
>> certainly not optimal (maybe in other places as well).
>>
>> However, even `buffer-local-value' is still costly - it adds up over 50%
>> run time.
>
> OTOH - I tried to reproduce your experiments here locally -
> `find-buffer-visiting' only takes 12% of the total run time here when
> using the `buffer-local-value' tuned definition of `find-buffer-visiting'.

> When we additionally merge the two loops in `find-buffer-visiting' into
> one loop and avoid the duplicated calls of `buffer-local-value', don't we
> reach a region where maintaining a separate cache doesn't make sense for
> the additional gain?

> And I wonder which part(s) of `buffer-local-value' is/are that costly,
> and why.

I looked closer, and it is not `buffer-local-value' being costly, but
rather the rest of the `find-buffer-visiting'. In particular, regexp
matching in `abbreviate-file-name' and `file-truename'.

I used the following:

(defun find-buffer-visiting (filename &optional predicate)
  "Return the buffer visiting file FILENAME (a string).
This is like `get-file-buffer', except that it checks for any buffer
visiting the same file, possibly under a different name.

If PREDICATE is non-nil, only buffers satisfying it are eligible,
and others are ignored.  PREDICATE is called with the buffer as
the only argument, but not with the buffer as the current buffer.

If there is no such live buffer, return nil."
  (let ((predicate (or predicate #'identity))
        (truename (abbreviate-file-name (file-truename filename))))
    (or (let ((buf (get-file-buffer filename)))
          (when (and buf (funcall predicate buf)) buf))
        (let ((list (buffer-list)) found)
          (while (and (not found) list)
            (if (and (buffer-local-value 'buffer-file-name (car list))
                     (string= (buffer-local-value 'buffer-file-truename (car list)) truename)
                     (funcall predicate (car list)))
                (setq found (car list)))
            (setq list (cdr list)))
          found)
        (let* ((attributes (file-attributes truename))
               (number (file-attribute-file-identifier attributes))
               (list (buffer-list)) found local-filename)
          (and buffer-file-numbers-unique
               (car-safe number)       ;Make sure the inode is not just nil.
               (while (and (not found) list)
		 (setq local-filename (buffer-local-value 'buffer-file-name (car list)))
                 (if (and local-filename
                          (equal (buffer-local-value 'buffer-file-number (car list)) number)
                          ;; Verify this buffer's file number
                          ;; still belongs to its file.
                          (file-exists-p local-filename)
                          (equal (file-attributes (buffer-local-value 'buffer-file-truename (car list)))
                                 attributes)
                          (funcall predicate (car list)))
                     (setq found (car list)))
                 (setq list (cdr list))))
          found))))
(native-compile #'find-buffer-visiting)

Native compilation at the end is important - it reduced runtime
significantly.

> Storing the same information twice - once in a buffer local variable,
> and then again in a cache that needs to be updated separately doesn't
> sound very reasonable.

But see Dmitry's reproducer where even `get-file-buffer' is demonstrated
to be slow in certain scenarios. So, caching (although less rigorous)
might be still necessary.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>