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

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: dmitry@gutov.dev, 66117@debbugs.gnu.org
> Date: Sun, 24 Sep 2023 10:54:34 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Also, even `find-file-noselect' does not use
> >> `set-visited-file-name'.
> >
> > Why does it matter?  We need to catch this in find-file-noselect and
> > in set-visited-file-name anyway.
> 
> Mostly because I feel that I misunderstand where `buffer-file-name' is
> set. `find-file-noselect-1' only sets `buffer-file-name' when
> 
>       (if find-file-visit-truename ;; defcustom, nil by default
>         (setq buffer-file-name (expand-file-name buffer-file-truename)))
> 
> >>   - ~find-alternate-file~
> >>   - ~find-file-noselect-1~ (but not by default?)
> >>   - ~set-visited-file-name~
> >>   - ~file-name-non-special~
> >
> > These _must_ update the cache.
> 
> I feel that I am still missing where `buffer-file-name' is set when
> opening file via C-x C-f. Debugger showed something weird in my testing.

With local files, it seems like insert-file-contents sets it.  So
maybe we should record the file name in the cache in bset_filename.

> >> - Functions setting ~buffer-file-truename~ manually (except those setting 
> >> it to nil):
> >
> > Are the cases where we find the buffer via file's truename significant
> > in the profiles you've seen?
> 
> Not significant for the profiles I got, but I did not want to break the
> existing code.
> 
> > ... if not, these functions are not relevant
> > to the issue at hand.  If the search by truename _is_ significant, we
> > could cache that as well.
> 
> Just to make sure that we are on the same page: the cache I am proposing
> should be complete - if a buffer is missing from the cache, we should be
> sure that there is no matching buffer.

Since we will keep buffer-list (we must), even with this cache
available, we can always leave the current code that scans the buffer
list if the name is not in the cache.  This way, we don't need to
worry to have all the buffers in the cache, only those which are
looked for frequently and need the efficiency.

> `find-buffer-visiting' explicitly checks for `buffer-file-truename'.
> So, if the cache does not account for `buffer-file-truename', there will
> be divergence between the existing code and when using the cache.
> 
> Same argument for `buffer-file-number'

As I said, we could have hash-tables for these as well, if that is
needed.  But I'd like to see the profiles that indicate we do need
them.

> >> > Programs that make these changes are asking for trouble, IMO.  AFAICT,
> >> > find-buffer-visiting will never find such buffers anyway.
> >> 
> >> It would, in its current form. Because it calls `get-file-buffer' that
> >> loops over all the buffers and checks their buffer-local
> >> `buffer-file-name' value, including values set via `setq' in Elisp.
> >
> > Again, which of the loops took the significant time in the profiles
> > you have? the one in get-file-buffer or the ones in
> > find-buffer-visiting?
> 
> Most of the time was taken by `find-buffer-visiting'. Replacing
> `find-buffer-visiting' with `get-file-buffer' in certain (not all)
> places reduced the total runtime by 30%.

So you are saying that 30% of file-visiting buffers are not found by
get-file-buffer?  Or is the 30% increase due to file names for which
there's no corresponding buffer?  If so, does the benchmark indeed
look for so many buffers that don't exist?

> I will try to setup a test on my machine for more detailed data.

Thanks, I think we need to understand the hot spots better, indeed.