GNU bug report logs -
#66117
30.0.50; `find-buffer-visiting' is slow when opening large number of buffers
Previous Next
Reported by: Ihor Radchenko <yantar92 <at> posteo.net>
Date: Wed, 20 Sep 2023 08:53:02 UTC
Severity: minor
Found in version 30.0.50
Done: Eli Zaretskii <eliz <at> gnu.org>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 66117 in the body.
You can then email your comments to 66117 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
dmitry <at> gutov.dev, bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 20 Sep 2023 08:53:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Ihor Radchenko <yantar92 <at> posteo.net>:
New bug report received and forwarded. Copy sent to
dmitry <at> gutov.dev, bug-gnu-emacs <at> gnu.org.
(Wed, 20 Sep 2023 08:53:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
X-Debbugs-CC: Dmitry Gutov <dmitry <at> gutov.dev>
Hello,
Background: Some Org users (especially org-roam users) often complain
that Org agenda is very slow (sometimes, tens of seconds or minutes)
when running it right after startup. The cause is often not Org agenda
itself, but Emacs opening files slowly.
In some cases, the main reason files are opened slowly is user-defined
hooks or various packages adding advices mindlessly. However, Emacs
itself is also taking a significant fraction of time when opening a
large number of buffers.
I have recently got data on opening a large number of Org files in
Emacs that clearly demonstrates that `file-buffer-visiting' (and thus
`find-file') scale poorly when the number of buffers increases. This is
especially clear when Org mode needs to open several hundreds of
not-yet-opened Org files in a new Emacs session.
In the attached profile, ~500 Org buffers are being opened and calls to
`find-buffer-visiting' take 12.4 sec out of total 30.9 sec - 1/3 of all
the execution time. After replacing some of the calls to
`find-buffer-visiting' with `get-file-buffer', the total execution time
decreased to ~20 sec, confirming that `find-buffer-visiting' is
contributing significantly to slow performance.
Would it be possible to implement some kind of caching mechanism to be
used by `find-buffer-visiting'?
[org_agenda_profile_2 (application/octet-stream, attachment)]
[Message part 3 (text/plain, inline)]
In GNU Emacs 30.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version
3.24.38, cairo version 1.17.8) of 2023-09-19 built on localhost
Repository revision: 6721c616653279913f51f211b80614bff82c24c3
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
System Description: Gentoo Linux
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 22 Sep 2023 01:38:02 GMT)
Full text and
rfc822 format available.
Message #8 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Hi Ihor,
On 20/09/2023 11:53, Ihor Radchenko wrote:
> In the attached profile, ~500 Org buffers are being opened and calls to
> `find-buffer-visiting' take 12.4 sec out of total 30.9 sec - 1/3 of all
> the execution time. After replacing some of the calls to
> `find-buffer-visiting' with `get-file-buffer', the total execution time
> decreased to ~20 sec, confirming that `find-buffer-visiting' is
> contributing significantly to slow performance.
>
> Would it be possible to implement some kind of caching mechanism to be
> used by `find-buffer-visiting'?
I'm guessing you Cc'd me because of an existing comment inside xref.el?
As you can see I decided not to use this function there, but even
get-file-buffer wasn't as fast as I would've wanted, so there's a
quick-and-dirty caching solution for sequential lookups (which assumes
that the same file would be looked up multiple times in a row).
Putting aside the perspective of maintaining a cache in the core
(hopefully this will be discussed later in this feature request), any
chance you could initialize that cache yourself first (by iterating
through buffers and building file-truename -> buffer map), before you
are doing those many lookups? Somewhere in org-agenda-list, perhaps.
That should change the complexity from N*M to N+M, more or less.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 22 Sep 2023 11:03:01 GMT)
Full text and
rfc822 format available.
Message #11 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Dmitry Gutov <dmitry <at> gutov.dev> writes:
>> Would it be possible to implement some kind of caching mechanism to be
>> used by `find-buffer-visiting'?
>
> I'm guessing you Cc'd me because of an existing comment inside xref.el?
Actually, mostly because I though that this discussion might be of
interest for you.
> As you can see I decided not to use this function there, but even
> get-file-buffer wasn't as fast as I would've wanted, so there's a
> quick-and-dirty caching solution for sequential lookups (which assumes
> that the same file would be looked up multiple times in a row).
For me, the situation is different - a new not-yet-open file is looked
up multiple times in a row, which is a bit more tricky.
> Putting aside the perspective of maintaining a cache in the core
> (hopefully this will be discussed later in this feature request), any
> chance you could initialize that cache yourself first (by iterating
> through buffers and building file-truename -> buffer map), before you
> are doing those many lookups? Somewhere in org-agenda-list, perhaps.
>
> That should change the complexity from N*M to N+M, more or less.
Sure. That's my plan to address older Emacs versions. But such iteration
will involve copy-pasting a big part of `find-buffer-visiting' source
code, which is not something I want to maintain in a long term (what if
`find-buffer-visiting' changes in future?).
Moreover, `find-buffer-visiting' is called by `find-file-noselect', and
I simply cannot make use of cache there without modifying the code
upstream; or writing my own version of `find-file-noselect' - bad idea
maintenance-wise.
I think that the best way that will benefit more than Org mode is
arranging internal cache that will link buffer-file-name,
buffer-file-truename, and buffer-file-number with buffers; and maintain
the correctness of the cache if buffer-file-name changes for any reason.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 22 Sep 2023 12:12:02 GMT)
Full text and
rfc822 format available.
Message #14 received at 66117 <at> debbugs.gnu.org (full text, mbox):
On 22/09/2023 14:03, Ihor Radchenko wrote:
> Dmitry Gutov <dmitry <at> gutov.dev> writes:
>
>>> Would it be possible to implement some kind of caching mechanism to be
>>> used by `find-buffer-visiting'?
>>
>> I'm guessing you Cc'd me because of an existing comment inside xref.el?
>
> Actually, mostly because I though that this discussion might be of
> interest for you.
All right, thank you for that.
>> As you can see I decided not to use this function there, but even
>> get-file-buffer wasn't as fast as I would've wanted, so there's a
>> quick-and-dirty caching solution for sequential lookups (which assumes
>> that the same file would be looked up multiple times in a row).
>
> For me, the situation is different - a new not-yet-open file is looked
> up multiple times in a row, which is a bit more tricky.
This could be handled with a locally held dictionary as well, but...
>> Putting aside the perspective of maintaining a cache in the core
>> (hopefully this will be discussed later in this feature request), any
>> chance you could initialize that cache yourself first (by iterating
>> through buffers and building file-truename -> buffer map), before you
>> are doing those many lookups? Somewhere in org-agenda-list, perhaps.
>>
>> That should change the complexity from N*M to N+M, more or less.
>
> Sure. That's my plan to address older Emacs versions. But such iteration
> will involve copy-pasting a big part of `find-buffer-visiting' source
> code, which is not something I want to maintain in a long term (what if
> `find-buffer-visiting' changes in future?).
>
> Moreover, `find-buffer-visiting' is called by `find-file-noselect', and
> I simply cannot make use of cache there without modifying the code
> upstream; or writing my own version of `find-file-noselect' - bad idea
> maintenance-wise.
...if most of said calls are done through find-file-noselect, I suppose
that solution is a no-go.
> I think that the best way that will benefit more than Org mode is
> arranging internal cache that will link buffer-file-name,
> buffer-file-truename, and buffer-file-number with buffers; and maintain
> the correctness of the cache if buffer-file-name changes for any reason.
I think that is doable.
It probably won't regress the performance of any particular scenario
either, but we should benchmark opening a bunch of files this way anyway
(might actually get faster, due to find-buffer-visiting calls).
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 22 Sep 2023 12:41:01 GMT)
Full text and
rfc822 format available.
Message #17 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Dmitry Gutov <dmitry <at> gutov.dev> writes:
>> Moreover, `find-buffer-visiting' is called by `find-file-noselect', and
>> I simply cannot make use of cache there without modifying the code
>> upstream; or writing my own version of `find-file-noselect' - bad idea
>> maintenance-wise.
>
> ...if most of said calls are done through find-file-noselect, I suppose
> that solution is a no-go.
Not a no-go, but not a complete solution either.
I asked the user who provided the profiler report I have attached to
replace `find-buffer-visiting' with `get-file-buffer' in the relevant Org
sources and it did lead to ~30% runtime reduction.
However, upon recording the profiler report with that change,
`find-buffer-visiting' still took a significant fraction of CPU time.
So, I decided to reach out upstream.
Will it be acceptable to implement the cache using variable watchers?
>> I think that the best way that will benefit more than Org mode is
>> arranging internal cache that will link buffer-file-name,
>> buffer-file-truename, and buffer-file-number with buffers; and maintain
>> the correctness of the cache if buffer-file-name changes for any reason.
>
> I think that is doable.
>
> It probably won't regress the performance of any particular scenario
> either, but we should benchmark opening a bunch of files this way anyway
> (might actually get faster, due to find-buffer-visiting calls).
The regression might happen when the number of buffers is small -
when hash tables become slower compared to simple list lookup.
But in such scenario, we will be talking about very small absolute
runtimes anyway, so it should probably not matter in practice.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 22 Sep 2023 13:00:02 GMT)
Full text and
rfc822 format available.
Message #20 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> Cc: 66117 <at> debbugs.gnu.org
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Date: Fri, 22 Sep 2023 15:41:01 +0300
>
> Will it be acceptable to implement the cache using variable watchers?
I don't like us using variable watchers for such stuff.
Why cannot we just cache the correspondence between files and buffers
in some hash-table?
> The regression might happen when the number of buffers is small -
> when hash tables become slower compared to simple list lookup.
> But in such scenario, we will be talking about very small absolute
> runtimes anyway, so it should probably not matter in practice.
Alternatively, only create the hash-table when the number of buffers
exceeds some threshold.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 22 Sep 2023 13:04:01 GMT)
Full text and
rfc822 format available.
Message #23 received at 66117 <at> debbugs.gnu.org (full text, mbox):
On 22/09/2023 15:41, Ihor Radchenko wrote:
> Dmitry Gutov <dmitry <at> gutov.dev> writes:
>
>>> Moreover, `find-buffer-visiting' is called by `find-file-noselect', and
>>> I simply cannot make use of cache there without modifying the code
>>> upstream; or writing my own version of `find-file-noselect' - bad idea
>>> maintenance-wise.
>>
>> ...if most of said calls are done through find-file-noselect, I suppose
>> that solution is a no-go.
>
> Not a no-go, but not a complete solution either.
> I asked the user who provided the profiler report I have attached to
> replace `find-buffer-visiting' with `get-file-buffer' in the relevant Org
> sources and it did lead to ~30% runtime reduction.
>
> However, upon recording the profiler report with that change,
> `find-buffer-visiting' still took a significant fraction of CPU time.
> So, I decided to reach out upstream.
Because the rest of the calls are made from Org?
I see this in the profile, though:
6247 20% - editorconfig--advice-find-file-noselect
There seems to be a lot of timers in that profile, and this is in one of
them (under "Automatic GC" which takes up 53% of the time).
So one was to improve this in the meantime would be to bring it up with
"editorconfig" authors. I would even question the wisdom of advising
find-file-noselect.
> Will it be acceptable to implement the cache using variable watchers?
I think variable watchers are mostly for debugging? I was thinking of a
straight approach where the base primitives that visit a file or kill a
buffer update said cache.
>>> I think that the best way that will benefit more than Org mode is
>>> arranging internal cache that will link buffer-file-name,
>>> buffer-file-truename, and buffer-file-number with buffers; and maintain
>>> the correctness of the cache if buffer-file-name changes for any reason.
>>
>> I think that is doable.
>>
>> It probably won't regress the performance of any particular scenario
>> either, but we should benchmark opening a bunch of files this way anyway
>> (might actually get faster, due to find-buffer-visiting calls).
>
> The regression might happen when the number of buffers is small -
> when hash tables become slower compared to simple list lookup.
> But in such scenario, we will be talking about very small absolute
> runtimes anyway, so it should probably not matter in practice.
I suppose.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 22 Sep 2023 13:11:01 GMT)
Full text and
rfc822 format available.
Message #26 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> Cc: 66117 <at> debbugs.gnu.org
> Date: Fri, 22 Sep 2023 16:02:59 +0300
> From: Dmitry Gutov <dmitry <at> gutov.dev>
>
> I think variable watchers are mostly for debugging?
It is also okay to use watchers when there's no other way of reacting
to a change. For example, if we need to do something when some
variable is changed that is exposed to Lisp.
> I was thinking of a straight approach where the base primitives that
> visit a file or kill a buffer update said cache.
Yes, in this case find-file-noselect could update the cache.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 22 Sep 2023 13:30:02 GMT)
Full text and
rfc822 format available.
Message #29 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> Will it be acceptable to implement the cache using variable watchers?
>
> I don't like us using variable watchers for such stuff.
>
> Why cannot we just cache the correspondence between files and buffers
> in some hash-table?
Because `buffer-file-name' can be modified from Lisp (via
`set-visited-file-name' or directly). Same for `buffer-file-truename'
and `buffer-file-number'.
Or should we just assume that these variables remain unchanged other
than by primitives?
>> The regression might happen when the number of buffers is small -
>> when hash tables become slower compared to simple list lookup.
>> But in such scenario, we will be talking about very small absolute
>> runtimes anyway, so it should probably not matter in practice.
>
> Alternatively, only create the hash-table when the number of buffers
> exceeds some threshold.
Indeed, this is a good idea.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 22 Sep 2023 13:38:02 GMT)
Full text and
rfc822 format available.
Message #32 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Dmitry Gutov <dmitry <at> gutov.dev> writes:
>> However, upon recording the profiler report with that change,
>> `find-buffer-visiting' still took a significant fraction of CPU time.
>> So, I decided to reach out upstream.
>
> Because the rest of the calls are made from Org?
I am not 100% sure what you mean.
> I see this in the profile, though:
>
> 6247 20% - editorconfig--advice-find-file-noselect
>
> There seems to be a lot of timers in that profile, and this is in one of
> them (under "Automatic GC" which takes up 53% of the time).
>
> So one was to improve this in the meantime would be to bring it up with
> "editorconfig" authors. I would even question the wisdom of advising
> find-file-noselect.
Nope. If you dig deeper, you will see that it is an around advice that
later calls the original `find-file-noselect', where we spend most of
the time:
6247 20% - editorconfig--advice-find-file-noselect
6247 20% - let
6247 20% - let
6247 20% - setq
6247 20% - apply
1773 5% - #<lambda -0xd6e9bb25a48132a>
1773 5% - if
1773 5% - if
1773 5% - let*
1773 5% - and
1773 5% + find-buffer-visiting
1666 5% - #<lambda -0xd6e9bb25a48132a>
1666 5% - if
1666 5% - if
1666 5% - let*
1666 5% - and
1666 5% + find-buffer-visiting
1006 3% - #<lambda -0xd6e9bb25a48132a>
1006 3% - if
1006 3% - if
1006 3% - let*
1006 3% - if
1006 3% - setq
1006 3% + create-file-buffer
830 2% - #<lambda -0xd6e9bb25a48132a>
830 2% - if
830 2% - if
830 2% - let*
830 2% - and
830 2% + find-buffer-visiting
776 2% - #<lambda -0xd6e9bb25a48132a>
776 2% - if
776 2% - if
776 2% - let*
776 2% - and
776 2% + find-buffer-visiting
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 22 Sep 2023 13:46:01 GMT)
Full text and
rfc822 format available.
Message #35 received at 66117 <at> debbugs.gnu.org (full text, mbox):
On 22/09/2023 16:38, Ihor Radchenko wrote:
> Nope. If you dig deeper, you will see that it is an around advice that
> later calls the original `find-file-noselect', where we spend most of
> the time:
I don't see find-file-noselect in the callstack, but I suppose it's
possible that the profiler just skips it, printing just the name of the
advice instead.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 22 Sep 2023 14:58:01 GMT)
Full text and
rfc822 format available.
Message #38 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Fri, 22 Sep 2023 13:30:41 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> Will it be acceptable to implement the cache using variable watchers?
> >
> > I don't like us using variable watchers for such stuff.
> >
> > Why cannot we just cache the correspondence between files and buffers
> > in some hash-table?
>
> Because `buffer-file-name' can be modified from Lisp (via
> `set-visited-file-name' or directly). Same for `buffer-file-truename'
> and `buffer-file-number'.
You could update the cache in set-visited-file-name, and ignore
direct changes.
> Or should we just assume that these variables remain unchanged other
> than by primitives?
Programs that make these changes are asking for trouble, IMO. AFAICT,
find-buffer-visiting will never find such buffers anyway.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 23 Sep 2023 08:22:02 GMT)
Full text and
rfc822 format available.
Message #41 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> Because `buffer-file-name' can be modified from Lisp (via
>> `set-visited-file-name' or directly). Same for `buffer-file-truename'
>> and `buffer-file-number'.
>
> You could update the cache in set-visited-file-name, and ignore
> direct changes.
I have eyeballed Emacs sources, and it looks like a huge number of
libraries sets `buffer-file-name' directly. Also, even
`find-file-noselect' does not use `set-visited-file-name'.
- Functions setting ~buffer-file-name~ manually (except those setting it to nil):
- tests
- ~vc-find-revision-no-save~
- ~url-insert-buffer-contents~
- ~plstore-open~
- ~protect-innocence-hook~ (really?)
- ~tramp-handle-insert-file-contents~
- ~tramp-archive-handle-insert-file-contents~
- ~mailcap-view-file~
- ~ange-ftp-parse-netrc~
- ~ange-ftp-write-region~
- ~ange-ftp-insert-file-contents~
- ~mh-display-msg~
- ~mh-make-folder~
- ~mh-read-draft~
- ~feedmail-vm-mail-mode~
- ~feedmail-send-it~
- ~jka-compr-write-region~
- ~jka-compr-insert-file-contents~
- ~image-dired-write-tags~
- ~image-dired-remove-tag~
- ~image-dired-write-comments~
- ~hfy-buffer~
- ~nndraft-request-associate-buffer~
- ~nndraft-auto-save-file-name~
- ~nnbabyl-create-mbox~
- ~mm-display-inline-fontify~
- ~mm-url-insert-file-contents~
- ~mm-extern-url~
- ~message-send-mail-with-mh~
- ~message-set-auto-save-file-name~
- ~gnus-dribble-read-file~
- ~gnus-save-newsrc-file~
- ~gnus-gnus-to-newsrc-format~
- ~gnus-mime-copy-part~
- ~find-alternate-file~
- ~find-file-noselect-1~ (but not by default?)
- ~set-visited-file-name~
- ~file-name-non-special~
- ~erc-dcc-find-file~
- ~epa-file-insert-file-contents~
- ~epa-file-write-region~
- ~save-completions-to-file~
- ~load-completions-from-file~
- ~archive-extract~
- Functions setting ~buffer-file-truename~ manually (except those setting it to nil):
- ~tar-extract~
- ~find-alternate-file~
- ~find-file-noselect-1~
- ~set-visited-file-name~
- ~revert-buffer--default~
- ~archive-extract~
- Functions setting ~buffer-file-number~ manually (except those setting it to nil):
- ~find-alternate-file~
- ~find-file-noselect-1~
- ~set-visited-file-name~
- ~basic-save-buffer~
>> Or should we just assume that these variables remain unchanged other
>> than by primitives?
>
> 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.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 23 Sep 2023 08:58:01 GMT)
Full text and
rfc822 format available.
Message #44 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Sat, 23 Sep 2023 08:22:17 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> Because `buffer-file-name' can be modified from Lisp (via
> >> `set-visited-file-name' or directly). Same for `buffer-file-truename'
> >> and `buffer-file-number'.
> >
> > You could update the cache in set-visited-file-name, and ignore
> > direct changes.
>
> I have eyeballed Emacs sources, and it looks like a huge number of
> libraries sets `buffer-file-name' directly.
I think this is an exaggeration, see below.
> 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.
> - Functions setting ~buffer-file-name~ manually (except those setting it to nil):
> - tests
> - ~vc-find-revision-no-save~
> - ~url-insert-buffer-contents~
> - ~plstore-open~
> - ~protect-innocence-hook~ (really?)
> - ~tramp-handle-insert-file-contents~
> - ~tramp-archive-handle-insert-file-contents~
> - ~mailcap-view-file~
> - ~ange-ftp-parse-netrc~
> - ~ange-ftp-write-region~
> - ~ange-ftp-insert-file-contents~
> - ~mh-display-msg~
> - ~mh-make-folder~
> - ~mh-read-draft~
> - ~feedmail-vm-mail-mode~
> - ~feedmail-send-it~
> - ~jka-compr-write-region~
> - ~jka-compr-insert-file-contents~
> - ~image-dired-write-tags~
> - ~image-dired-remove-tag~
> - ~image-dired-write-comments~
> - ~hfy-buffer~
> - ~nndraft-request-associate-buffer~
> - ~nndraft-auto-save-file-name~
> - ~nnbabyl-create-mbox~
> - ~mm-display-inline-fontify~
> - ~mm-url-insert-file-contents~
> - ~mm-extern-url~
> - ~message-send-mail-with-mh~
> - ~message-set-auto-save-file-name~
> - ~gnus-dribble-read-file~
> - ~gnus-save-newsrc-file~
> - ~gnus-gnus-to-newsrc-format~
> - ~gnus-mime-copy-part~
IMO, these are not important for your purpose. The few buffers whose
buffer-file-name is set as in those functions are not going to get in
the way of your looking for a buffer that visits a specific name. And
if a few of them do, we can always add the cache-updating code to them.
> - ~find-alternate-file~
> - ~find-file-noselect-1~ (but not by default?)
> - ~set-visited-file-name~
> - ~file-name-non-special~
These _must_ update the cache.
> - ~erc-dcc-find-file~
> - ~epa-file-insert-file-contents~
> - ~epa-file-write-region~
> - ~save-completions-to-file~
> - ~load-completions-from-file~
> - ~archive-extract~
These belong to the first group, I think.
> - 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? 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.
> - ~tar-extract~
> - ~find-alternate-file~
> - ~find-file-noselect-1~
> - ~set-visited-file-name~
> - ~revert-buffer--default~
> - ~archive-extract~
>
>
> - Functions setting ~buffer-file-number~ manually (except those setting it to nil):
Same question as for truename above.
> - ~find-alternate-file~
> - ~find-file-noselect-1~
> - ~set-visited-file-name~
> - ~basic-save-buffer~
>
>
> >> Or should we just assume that these variables remain unchanged other
> >> than by primitives?
> >
> > 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?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 24 Sep 2023 10:54:01 GMT)
Full text and
rfc822 format available.
Message #47 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> 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.
>> - 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.
`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'
>> > 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%. I do not have more granular data
because the profiler did not give very granular data for the internals
of `find-buffer-visiting'.
I will try to setup a test on my machine for more detailed data.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 24 Sep 2023 12:52:01 GMT)
Full text and
rfc822 format available.
Message #50 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Sun, 24 Sep 2023 10:54:34 +0000
>
> Eli Zaretskii <eliz <at> 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 26 Sep 2023 08:54:02 GMT)
Full text and
rfc822 format available.
Message #53 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> 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.
Thanks for the pointer!
AFAIU, the relevant code is
if (NILP (handler))
{
current_buffer->modtime = mtime;
current_buffer->modtime_size = st.st_size;
bset_filename (current_buffer, orig_filename);
However, it looks like file handlers are responsible for setting the
filename. So,
> - ~tramp-handle-insert-file-contents~
> - ~tramp-archive-handle-insert-file-contents~
> - ~ange-ftp-insert-file-contents~
> - ~jka-compr-insert-file-contents~
> - ~mm-url-insert-file-contents~
> - ~epa-file-insert-file-contents~
may also need to handle the caching. And also all the third-party handlers.
>> 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.
I need to elaborate then.
The problem Org faces happens when we open a file that is not yet opened
in Emacs. So, the FILENAME in question is missing from the buffer list
and `find-buffer-visiting' must (1) traverse every buffer in
`get-file-buffer'; (2) traverse every buffer again, checking
`buffer-file-name' values; (3) traverse every buffer yet again, checking
for `buffer-file-number'. We have the worst-case scenario for the
current code when the buffer with a given file name is not available and
all the checks fail.
To address the above scenario, it is not enough to cache _some_ buffer
names. Because not-yet-open FILENAME will be missing from the cache, but
we will still have to go through the above process, which is slow.
What is needed is a _complete_ cache, so that the fact that FILENAME is
missing there means that no buffer associated with FILENAME is open in
Emacs.
>> `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.
I hope that the above clarified why I want to cache everything.
>> 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?
The rough code flow for the profile I attached to the initial message
is: For each of 500 files used to build agenda: (1) check if file is
open in Emacs via `find-buffer-visiting' and open it if not yet open;
(2) search the file to find matching headings to be added to agenda.
The total CPU time spend building agenda from fresh Emacs decreased by
1/3 (~10 seconds) by replacing calls to `find-buffer-visiting' with
`get-file-buffer'. And this replacement did not yet replace every call
to `find-buffer-visiting' (in particular, find-file-no-select by itself
also calls `find-buffer-visiting'; I replaced no more than half of the
calls only). I estimate that over half of the 30 seconds building agenda
was spent repeatedly searching over all the buffers.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 26 Sep 2023 11:12:01 GMT)
Full text and
rfc822 format available.
Message #56 received at 66117 <at> debbugs.gnu.org (full text, mbox):
On 24/09/2023 15:50, Eli Zaretskii wrote:
>>> ... 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.
If the cache was exhaustive (i.e. we could guarantee that
'find-buffer-visiting' is always fast), we could use it in Xref as well.
Instead of doing the memoization dance in xref--find-file-buffer which
still doesn't account for symlinks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 26 Sep 2023 13:06:02 GMT)
Full text and
rfc822 format available.
Message #59 received at 66117 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Ihor Radchenko <yantar92 <at> posteo.net> writes:
> 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%. I do not have more granular data
> because the profiler did not give very granular data for the internals
> of `find-buffer-visiting'.
>
> I will try to setup a test on my machine for more detailed data.
Here is a reproducer anyone can try locally:
1. Create a dummy set of 1000 files in /tmp/test/:
(dotimes (i 1000) (with-temp-file (format "/tmp/test/%d.org" i) (insert "* This is test")))
2. emacs -Q
3. Open all the 1000 files one by one:
(dolist (file (directory-files "/tmp/test/" t "org"))
(unless (find-buffer-visiting file) (find-file-noselect file)))
Step (3) takes 18.8 seconds on my machine. The CPU profile attached as
cpu-profile.
If one uses `get-file-buffer' instead of `find-buffer-visiting', the
total runtime becomes 5.1 sec - almost 4x faster.
To test:
(dolist (file (directory-files "/tmp/test/" t "org"))
(unless (get-file-buffer file)
(cl-letf (((symbol-function 'find-buffer-visiting)
(lambda (file &optional predicate)
(when-let ((buf (get-file-buffer file)))
(and (funcall predicate buf) buf)))))
(find-file-noselect file))))
With `get-file-buffer' instead of `find-buffer-visiting', matching
against the opened buffers no longer dominates the profiler. See the
attached cpu-profile-get-file-buffer.
So, it looks like caching `get-file-buffer' is not really necessary.
From the profile, the slowest parts of `find-buffer-visiting' are the
two loops checking `buffer-file-truename' and `buffer-file-number' with
most of the time apparently spent executing `with-current-buffer'. I
tested whether `with-current-buffer' is the culprit by replacing it with
`buffer-local-value' calls:
(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)
(and buffer-file-numbers-unique
(car-safe number) ;Make sure the inode is not just nil.
(while (and (not found) list)
(if (and (buffer-local-value 'buffer-file-name (car list))
(equal (buffer-local-value 'buffer-file-number (car list)) number)
;; Verify this buffer's file number
;; still belongs to its file.
(file-exists-p (buffer-local-value 'buffer-file-name (car list)))
(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))))
The result is 7.8 sec execution time - much better compared to 18.8
seconds in `with-current-buffer' version, but still worse compared to
5.1 sec in `get-file-buffer' version. See the attached
cpu-profile-buffer-local-value.
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.
Also, looking at the 5.1 sec profile, there are other things that may
slow down opening a large number of files:
0. GC (as usual)
1. hack-local-variables
2. vc-refresh-state
3. uniquify--create-file-buffer-advice -> uniquify-rationalize-file-buffer-names
4. Org mode loading (nothing new here for me)
[cpu-profile (application/octet-stream, attachment)]
[cpu-profile-get-file-buffer (application/octet-stream, attachment)]
[cpu-profile-buffer-local-value (application/octet-stream, attachment)]
[Message part 5 (text/plain, inline)]
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 26 Sep 2023 14:19:02 GMT)
Full text and
rfc822 format available.
Message #62 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Ihor Radchenko <yantar92 <at> posteo.net> writes:
Hi Ihor,
>> 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.
>
> Thanks for the pointer!
> AFAIU, the relevant code is
>
> if (NILP (handler))
> {
> current_buffer->modtime = mtime;
> current_buffer->modtime_size = st.st_size;
> bset_filename (current_buffer, orig_filename);
>
> However, it looks like file handlers are responsible for setting the
> filename. So,
>
> > - ~tramp-handle-insert-file-contents~
> > - ~tramp-archive-handle-insert-file-contents~
> > - ~ange-ftp-insert-file-contents~
> > - ~jka-compr-insert-file-contents~
> > - ~mm-url-insert-file-contents~
> > - ~epa-file-insert-file-contents~
>
> may also need to handle the caching. And also all the third-party handlers.
That might be hard to synchronize. Why not fill the cache after that
code snippet? The handlers have already done their job, buffer-file-name
is set. And so does bset_filename in the local case.
Best regards, Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 27 Sep 2023 23:32:01 GMT)
Full text and
rfc822 format available.
Message #65 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Ihor Radchenko <yantar92 <at> 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.
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.
Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 29 Sep 2023 07:33:01 GMT)
Full text and
rfc822 format available.
Message #68 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Tue, 26 Sep 2023 13:06:04 +0000
>
> Here is a reproducer anyone can try locally:
>
> 1. Create a dummy set of 1000 files in /tmp/test/:
> (dotimes (i 1000) (with-temp-file (format "/tmp/test/%d.org" i) (insert "* This is test")))
>
> 2. emacs -Q
> 3. Open all the 1000 files one by one:
> (dolist (file (directory-files "/tmp/test/" t "org"))
> (unless (find-buffer-visiting file) (find-file-noselect file)))
>
> Step (3) takes 18.8 seconds on my machine. The CPU profile attached as
> cpu-profile.
Since find-file-noselect calls find-buffer-visiting internally, I'm
not sure the above test case makes sense. A Lisp program should feel
free to call find-file-noselect directly, and Emacs will find the
visiting buffer, if it already exists, as part of the job of
find-file-noselect.
Let's please focus on test cases where the Lisp code being benchmarked
doesn't do any unnecessary stuff, since what's at stake is a
significant change in our internals.
> If one uses `get-file-buffer' instead of `find-buffer-visiting', the
> total runtime becomes 5.1 sec - almost 4x faster.
This is also not very interesting, since find-file-noselect calls
get-file-buffer as well.
> So, it looks like caching `get-file-buffer' is not really necessary.
I don't think we are ready for conclusions yet, see above.
> >From the profile, the slowest parts of `find-buffer-visiting' are the
> two loops checking `buffer-file-truename' and `buffer-file-number' with
> most of the time apparently spent executing `with-current-buffer'. I
> tested whether `with-current-buffer' is the culprit by replacing it with
> `buffer-local-value' calls:
If we come to the conclusion that those loops in find-buffer-visiting
are the hot spot, the right thing is to implement them in C, where we
don't need to use the equivalent of with-current-buffer to examine the
truename and file-number of every buffer, we can just access them
directly.
> The result is 7.8 sec execution time - much better compared to 18.8
> seconds in `with-current-buffer' version, but still worse compared to
> 5.1 sec in `get-file-buffer' version. See the attached
> cpu-profile-buffer-local-value.
As explained above, both the 18.8 and the 5.1 figures are not good
base lines upon which to make decisions.
> So, using `with-current-buffer' when looping over all the buffers is
> certainly not optimal (maybe in other places as well).
with-current-buffer is normally very expensive. Which is why any
performance-critical loop should try to avoid it as much as possible.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 29 Sep 2023 13:56:02 GMT)
Full text and
rfc822 format available.
Message #71 received at 66117 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:
>> 3. Open all the 1000 files one by one:
>> (dolist (file (directory-files "/tmp/test/" t "org"))
>> (unless (find-buffer-visiting file) (find-file-noselect file)))
>>
>> Step (3) takes 18.8 seconds on my machine. The CPU profile attached as
>> cpu-profile.
>
> Since find-file-noselect calls find-buffer-visiting internally, I'm
> not sure the above test case makes sense. A Lisp program should feel
> free to call find-file-noselect directly, and Emacs will find the
> visiting buffer, if it already exists, as part of the job of
> find-file-noselect.
>
> Let's please focus on test cases where the Lisp code being benchmarked
> doesn't do any unnecessary stuff, since what's at stake is a
> significant change in our internals.
The reason I left an extra `find-buffer-visiting' call was because Org
mode does it (for a reason - we need information if a file was already
open or not).
You may as well do
(dolist (file (directory-files "/tmp/test/" t "org"))
(find-file-noselect file))
as step (3).
The same conclusions will hold - `find-file-noselect' calls
`find-buffer-visiting' as well and it also takes most of the CPU time.
I am attaching an updated set of the same profiles, but based on the
above `dolist' that only calls `find-file-noselect'.
The run times are now: 12.0 seconds, 5.3 seconds, and 6.6 seconds.
>> If one uses `get-file-buffer' instead of `find-buffer-visiting', the
>> total runtime becomes 5.1 sec - almost 4x faster.
>
> This is also not very interesting, since find-file-noselect calls
> get-file-buffer as well.
No. `find-file-noselect' calls `find-buffer-visiting'.
> If we come to the conclusion that those loops in find-buffer-visiting
> are the hot spot, the right thing is to implement them in C, where we
> don't need to use the equivalent of with-current-buffer to examine the
> truename and file-number of every buffer, we can just access them
> directly.
I still think that my previous conclusions are true. And I agree that
rewriting these expensive loops in C makes sense. Maybe two new
subroutines to find buffer by `buffer-file-truename' and by
`buffer-file-number'? They will be an equivalent of `get-file-buffer'
that searches by `buffer-file-name'.
>> So, using `with-current-buffer' when looping over all the buffers is
>> certainly not optimal (maybe in other places as well).
>
> with-current-buffer is normally very expensive. Which is why any
> performance-critical loop should try to avoid it as much as possible.
Aside: this reminds me about obsoletion of generalized buffer-local
variable. AFAIU, there is currently no way to set buffer-local value in
buffer without setting that buffer to current. It would be nice if such
setting were possible, especially in performance-critical code.
[cpu-profile (application/octet-stream, attachment)]
[cpu-profile-get-file-buffer (application/octet-stream, attachment)]
[cpu-profile-buffer-local-value (application/octet-stream, attachment)]
[Message part 5 (text/plain, inline)]
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 29 Sep 2023 16:14:02 GMT)
Full text and
rfc822 format available.
Message #74 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Fri, 29 Sep 2023 13:56:40 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> 3. Open all the 1000 files one by one:
> >> (dolist (file (directory-files "/tmp/test/" t "org"))
> >> (unless (find-buffer-visiting file) (find-file-noselect file)))
> >>
> >> Step (3) takes 18.8 seconds on my machine. The CPU profile attached as
> >> cpu-profile.
> >
> > Since find-file-noselect calls find-buffer-visiting internally, I'm
> > not sure the above test case makes sense. A Lisp program should feel
> > free to call find-file-noselect directly, and Emacs will find the
> > visiting buffer, if it already exists, as part of the job of
> > find-file-noselect.
> >
> > Let's please focus on test cases where the Lisp code being benchmarked
> > doesn't do any unnecessary stuff, since what's at stake is a
> > significant change in our internals.
>
> The reason I left an extra `find-buffer-visiting' call was because Org
> mode does it (for a reason - we need information if a file was already
> open or not).
>
> You may as well do
>
> (dolist (file (directory-files "/tmp/test/" t "org"))
> (find-file-noselect file))
>
> as step (3).
>
> The same conclusions will hold - `find-file-noselect' calls
> `find-buffer-visiting' as well and it also takes most of the CPU time.
>
> I am attaching an updated set of the same profiles, but based on the
> above `dolist' that only calls `find-file-noselect'.
>
> The run times are now: 12.0 seconds, 5.3 seconds, and 6.6 seconds.
12 sec is quite a far cry from 18.8, won't you agree?
> >> If one uses `get-file-buffer' instead of `find-buffer-visiting', the
> >> total runtime becomes 5.1 sec - almost 4x faster.
> >
> > This is also not very interesting, since find-file-noselect calls
> > get-file-buffer as well.
>
> No. `find-file-noselect' calls `find-buffer-visiting'.
Unless we use different Emacsen, find-file-noselect calls both
get-file-buffer and find-buffer-visiting:
(let* ((buf (get-file-buffer filename)) <<<<<<<<<<<<<<<<<<<<<<<<<<<<
(truename (abbreviate-file-name (file-truename filename)))
(attributes (file-attributes truename))
(number (file-attribute-file-identifier attributes))
;; Find any buffer for a file that has same truename.
(other (and (not buf)
(find-buffer-visiting <<<<<<<<<<<<<<<<<<<<<<<<<<<<
filename
;; We want to filter out buffers that we've
;; visited via symlinks and the like, where
;; the symlink no longer exists.
(lambda (buffer)
(let ((file (buffer-local-value
'buffer-file-name buffer)))
(and file (file-exists-p file))))))))
> > If we come to the conclusion that those loops in find-buffer-visiting
> > are the hot spot, the right thing is to implement them in C, where we
> > don't need to use the equivalent of with-current-buffer to examine the
> > truename and file-number of every buffer, we can just access them
> > directly.
>
> I still think that my previous conclusions are true. And I agree that
> rewriting these expensive loops in C makes sense. Maybe two new
> subroutines to find buffer by `buffer-file-truename' and by
> `buffer-file-number'?
Yes, that's what I had in mind.
> >> So, using `with-current-buffer' when looping over all the buffers is
> >> certainly not optimal (maybe in other places as well).
> >
> > with-current-buffer is normally very expensive. Which is why any
> > performance-critical loop should try to avoid it as much as possible.
>
> Aside: this reminds me about obsoletion of generalized buffer-local
> variable. AFAIU, there is currently no way to set buffer-local value in
> buffer without setting that buffer to current. It would be nice if such
> setting were possible, especially in performance-critical code.
Maybe, but is there any performance-critical code which needs that?
Severity set to 'minor' from 'normal'
Request was from
Stefan Kangas <stefankangas <at> gmail.com>
to
control <at> debbugs.gnu.org.
(Sat, 30 Sep 2023 23:28:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 03 Oct 2023 03:27:02 GMT)
Full text and
rfc822 format available.
Message #79 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> > Aside: this reminds me about obsoletion of generalized buffer-local
> > variable. AFAIU, there is currently no way to set buffer-local value in
> > buffer without setting that buffer to current. It would be nice if such
> > setting were possible, especially in performance-critical code.
>
> Maybe, but is there any performance-critical code which needs that?
I don't know, but the gv-setter of `buffer-local-value' also calls
`with-current-buffer', so using it was not better performance-wise.
Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 03 Oct 2023 06:02:02 GMT)
Full text and
rfc822 format available.
Message #82 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Michael Heerdegen <michael_heerdegen <at> web.de>
> Cc: Ihor Radchenko <yantar92 <at> posteo.net>, dmitry <at> gutov.dev,
> 66117 <at> debbugs.gnu.org
> Date: Tue, 03 Oct 2023 05:25:39 +0200
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > > Aside: this reminds me about obsoletion of generalized buffer-local
> > > variable. AFAIU, there is currently no way to set buffer-local value in
> > > buffer without setting that buffer to current. It would be nice if such
> > > setting were possible, especially in performance-critical code.
> >
> > Maybe, but is there any performance-critical code which needs that?
>
> I don't know, but the gv-setter of `buffer-local-value' also calls
> `with-current-buffer', so using it was not better performance-wise.
If we discover enough performance-critical cases with that, we could
provide a special way, implemented in C, of doing that efficiently.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 04 Oct 2023 10:57:02 GMT)
Full text and
rfc822 format available.
Message #85 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Michael Albinus <michael.albinus <at> gmx.de> writes:
>> may also need to handle the caching. And also all the third-party handlers.
>
> That might be hard to synchronize. Why not fill the cache after that
> code snippet? The handlers have already done their job, buffer-file-name
> is set. And so does bset_filename in the local case.
Indeed you are right.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 04 Oct 2023 10:58:02 GMT)
Full text and
rfc822 format available.
Message #88 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Dmitry Gutov <dmitry <at> gutov.dev> writes:
>> Would it be possible to implement some kind of caching mechanism to be
>> used by `find-buffer-visiting'?
>
> I'm guessing you Cc'd me because of an existing comment inside xref.el?
> As you can see I decided not to use this function there, but even
> get-file-buffer wasn't as fast as I would've wanted, so there's a
> quick-and-dirty caching solution for sequential lookups (which assumes
> that the same file would be looked up multiple times in a row).
It looks like my example is quite different from yours.
Do you have a reproducer demonstrating `get-file-buffer' being slow?
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 04 Oct 2023 11:47:02 GMT)
Full text and
rfc822 format available.
Message #91 received at 66117 <at> debbugs.gnu.org (full text, mbox):
On 04/10/2023 13:58, Ihor Radchenko wrote:
> Dmitry Gutov<dmitry <at> gutov.dev> writes:
>
>>> Would it be possible to implement some kind of caching mechanism to be
>>> used by `find-buffer-visiting'?
>> I'm guessing you Cc'd me because of an existing comment inside xref.el?
>> As you can see I decided not to use this function there, but even
>> get-file-buffer wasn't as fast as I would've wanted, so there's a
>> quick-and-dirty caching solution for sequential lookups (which assumes
>> that the same file would be looked up multiple times in a row).
> It looks like my example is quite different from yours.
> Do you have a reproducer demonstrating `get-file-buffer' being slow?
I retested, and it seems most people here won't be impressed by my
example. Anyway:
1. Call (benchmark-run 1 (project-find-regexp "emacs") in an Emacs repo
checkout.
2. Change xref--collect-matches to call get-file-buffer instead of
xref--find-file-buffer (which does some caching). 'M-x
byte-compile-file', (load "xref.elc"), and run the benchmark again.
What I get here, is 3.60s vs 5.03.
It's a 1.40s difference over ~95000 calls, though (this is how many
matches are produced). Which is meaningful for my case, but probably not
for yours.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 05 Oct 2023 11:27:01 GMT)
Full text and
rfc822 format available.
Message #94 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Dmitry Gutov <dmitry <at> gutov.dev> writes:
> I retested, and it seems most people here won't be impressed by my
> example. Anyway:
>
> 1. Call (benchmark-run 1 (project-find-regexp "emacs") in an Emacs repo
> checkout.
> 2. Change xref--collect-matches to call get-file-buffer instead of
> xref--find-file-buffer (which does some caching). 'M-x
> byte-compile-file', (load "xref.elc"), and run the benchmark again.
>
> What I get here, is 3.60s vs 5.03.
>
> It's a 1.40s difference over ~95000 calls, though (this is how many
> matches are produced). Which is meaningful for my case, but probably not
> for yours.
I can reproduce:
;; original code
(benchmark-run 10 (project-find-regexp "emacs")) ; => (26.459992888000002 17 7.026558985999998)
;; changed `xref--find-file-buffer' to `get-file-buffer'
(benchmark-run 10 (project-find-regexp "emacs")) ; => (43.623003254000004 18 8.161839762000001)
Looking into `xref--find-file-buffer', all it does is memoizing the
previous call. I think that we can easily achieve the same performance
improvement by maintaining approximate cache for Fget_file_buffer -
something like
(if (and (gethash file cache)
(equal (buffer-local-value 'buffer-file-name (gethash ...)) file))
<return cached>
<compute and cache the result>)
Or maybe even cache the last call separately to not pay `gethash' call price.
100% reliable cache is not necessary in this scenario. And not necessary
in my scenario, as I have found via my benchmarks in another message.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 05 Oct 2023 14:25:01 GMT)
Full text and
rfc822 format available.
Message #97 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Michael Heerdegen <michael_heerdegen <at> web.de> writes:
> Ihor Radchenko <yantar92 <at> 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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 05 Oct 2023 17:16:02 GMT)
Full text and
rfc822 format available.
Message #100 received at 66117 <at> debbugs.gnu.org (full text, mbox):
On 05/10/2023 14:27, Ihor Radchenko wrote:
> Looking into `xref--find-file-buffer', all it does is memoizing the
> previous call. I think that we can easily achieve the same performance
> improvement by maintaining approximate cache for Fget_file_buffer -
> something like
> (if (and (gethash file cache)
> (equal (buffer-local-value 'buffer-file-name (gethash ...)) file))
> <return cached>
> <compute and cache the result>)
>
> Or maybe even cache the last call separately to not pay `gethash' call price.
That works for the Xref's scenario (where, when there are many matches,
they would tend to come several per file), but I'm not sure it's a good
caching strategy in general.
And applications that are suited for it might as well write a small
wrapper like I did.
> 100% reliable cache is not necessary in this scenario. And not necessary
> in my scenario, as I have found via my benchmarks in another message.
IIUC a cache that keys on non-abbreviated/non-expanded filenames would
be able to speed all that up anyway. But a more algorithmic improvement
first would be better, of course.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 07 Oct 2023 08:25:02 GMT)
Full text and
rfc822 format available.
Message #103 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> >> If one uses `get-file-buffer' instead of `find-buffer-visiting', the
>> >> total runtime becomes 5.1 sec - almost 4x faster.
>> >
>> > This is also not very interesting, since find-file-noselect calls
>> > get-file-buffer as well.
>>
>> No. `find-file-noselect' calls `find-buffer-visiting'.
>
> Unless we use different Emacsen, find-file-noselect calls both
> get-file-buffer and find-buffer-visiting:
>
> (let* ((buf (get-file-buffer filename)) <<<<<<<<<<<<<<<<<<<<<<<<<<<<
We are probably mis-communicating. My point is that `get-file-buffer' is
very fast (for my purposes). So, it does not matter as much if it is
called somewhere else and how many times.
Although, as Dmitry demonstrated, `get-file-buffer' might be slow in
certain scenarios. But let's postpone that discussion after we handle
the much slower `find-buffer-visiting'.
>> I still think that my previous conclusions are true. And I agree that
>> rewriting these expensive loops in C makes sense. Maybe two new
>> subroutines to find buffer by `buffer-file-truename' and by
>> `buffer-file-number'?
>
> Yes, that's what I had in mind.
I looked closer, and there is already `get_truename_buffer', which can
simply be exposed to Lisp.
`buffer-file-number' is a bit more tricky - it is not defined in C, but
in files.el. However, I am wondering if this variable should be moved to
C or maybe into the buffer object. `make-indirect-buffer' (defined in C)
has
Fset (intern ("buffer-save-without-query"), Qnil);
Fset (intern ("buffer-file-number"), Qnil);
WDYT?
>> Aside: this reminds me about obsoletion of generalized buffer-local
>> variable. AFAIU, there is currently no way to set buffer-local value in
>> buffer without setting that buffer to current. It would be nice if such
>> setting were possible, especially in performance-critical code.
>
> Maybe, but is there any performance-critical code which needs that?
For example, org-element.el needs to set buffer-local values in base
buffer of an indirect buffer every time buffer text is being edited.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 07 Oct 2023 08:49:02 GMT)
Full text and
rfc822 format available.
Message #106 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Sat, 07 Oct 2023 08:25:06 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> >> If one uses `get-file-buffer' instead of `find-buffer-visiting', the
> >> >> total runtime becomes 5.1 sec - almost 4x faster.
> >> >
> >> > This is also not very interesting, since find-file-noselect calls
> >> > get-file-buffer as well.
> >>
> >> No. `find-file-noselect' calls `find-buffer-visiting'.
> >
> > Unless we use different Emacsen, find-file-noselect calls both
> > get-file-buffer and find-buffer-visiting:
> >
> > (let* ((buf (get-file-buffer filename)) <<<<<<<<<<<<<<<<<<<<<<<<<<<<
>
> We are probably mis-communicating. My point is that `get-file-buffer' is
> very fast (for my purposes). So, it does not matter as much if it is
> called somewhere else and how many times.
It matters to me. Timing code must be very accurate and must not
modify the code in question, certainly not by invoking the same
primitives that are already called by the code being times.
I must say that this discussion is very frustrating from my POV. Lots
of information, a large portion of it irrelevant, and very little
systematical analysis of the involved code, its actual performance,
and the conclusions with numbers to back them up. On top of that,
gaps for a week or more between a message and a response, something
that makes it hard to follows the discussion. We should be able to do
better.
> >> I still think that my previous conclusions are true. And I agree that
> >> rewriting these expensive loops in C makes sense. Maybe two new
> >> subroutines to find buffer by `buffer-file-truename' and by
> >> `buffer-file-number'?
> >
> > Yes, that's what I had in mind.
>
> I looked closer, and there is already `get_truename_buffer', which can
> simply be exposed to Lisp.
>
> `buffer-file-number' is a bit more tricky - it is not defined in C, but
> in files.el. However, I am wondering if this variable should be moved to
> C or maybe into the buffer object. `make-indirect-buffer' (defined in C)
> has
>
> Fset (intern ("buffer-save-without-query"), Qnil);
> Fset (intern ("buffer-file-number"), Qnil);
>
> WDYT?
TTTT, I don't know what to think.
From my POV, there are two alternatives here:
. expose several new primitives to Lisp to make find-buffer-visiting
faster without changing the way we store the file-to-buffer
association information
. introduce caches or change the way file-to-buffer associations are
stored to speed up find-buffer-visiting
What I'd like to see is that someone implements the first idea, and
times find-buffer-visiting after that to see if it becomes fast
enough. Then we can discuss whether anything else is needed.
> >> Aside: this reminds me about obsoletion of generalized buffer-local
> >> variable. AFAIU, there is currently no way to set buffer-local value in
> >> buffer without setting that buffer to current. It would be nice if such
> >> setting were possible, especially in performance-critical code.
> >
> > Maybe, but is there any performance-critical code which needs that?
>
> For example, org-element.el needs to set buffer-local values in base
> buffer of an indirect buffer every time buffer text is being edited.
And that is performance-critical? in what way and in which situations?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 07 Oct 2023 09:29:01 GMT)
Full text and
rfc822 format available.
Message #109 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> We are probably mis-communicating. My point is that `get-file-buffer' is
>> very fast (for my purposes). So, it does not matter as much if it is
>> called somewhere else and how many times.
>
> It matters to me. Timing code must be very accurate and must not
> modify the code in question, certainly not by invoking the same
> primitives that are already called by the code being times.
My point is that an extra call to `get-file-buffer' in (unless (get-file-buffer ...) ...)
does not change the profiling results - `get-file-buffer' only takes 49
samples out of 5157 (<1%). Calling `get-file-buffer' multiple times
literally make no difference in my benchmark.
> I must say that this discussion is very frustrating from my POV. Lots
> of information, a large portion of it irrelevant, and very little
> systematical analysis of the involved code, its actual performance,
> and the conclusions with numbers to back them up.
Sorry, but I do not fully understand what you expected. I tried my best
to provide all the details of my testing during the course of this
branch of the discussion.
In particular, see
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#59 and later
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#71 where I addressed
your concern about calling `find-buffer-visiting'/`get-file-buffer'
unnecessarily in the testing code. I provided the code, the timing, and
the raw data from the profiler report.
Please, let me know if you need more.
> ... On top of that,
> gaps for a week or more between a message and a response, something
> that makes it hard to follows the discussion. We should be able to do
> better.
I can understand the frustration, but I just have as much time as I can
allocate. I was mostly AFC for about a week and was not able to reply to
many of the Emacs-related discussions.
Also, I am used to mailing list rule that one month is allowed for a
reply (https://orgmode.org/worg/org-mailing-list.html#i-didnt-receive-an-answer).
I thought that emacs-devel is not different.
>> I looked closer, and there is already `get_truename_buffer', which can
>> simply be exposed to Lisp.
>>
>> `buffer-file-number' is a bit more tricky - it is not defined in C, but
>> in files.el. However, I am wondering if this variable should be moved to
>> C or maybe into the buffer object. `make-indirect-buffer' (defined in C)
>> has
>>
>> Fset (intern ("buffer-save-without-query"), Qnil);
>> Fset (intern ("buffer-file-number"), Qnil);
>>
>> WDYT?
>
> TTTT, I don't know what to think.
>
> From my POV, there are two alternatives here:
>
> . expose several new primitives to Lisp to make find-buffer-visiting
> faster without changing the way we store the file-to-buffer
> association information
> . introduce caches or change the way file-to-buffer associations are
> stored to speed up find-buffer-visiting
>
> What I'd like to see is that someone implements the first idea, and
> times find-buffer-visiting after that to see if it becomes fast
> enough. Then we can discuss whether anything else is needed.
Agree. That's what I was trying to do. However, one of the new primitives
will require searching a buffer with given value of `buffer-file-number'
- the variable not defined in C. That's why my question - I can either
query buffer-local variable in generic way via `Fbuffer_local_value' or
faster, by adding `buffer-file-number' to buffer object. The latter also
makes sense in the context of `make-indirect-buffer' implementation.
>> >> Aside: this reminds me about obsoletion of generalized buffer-local
>> >> variable. AFAIU, there is currently no way to set buffer-local value in
>> >> buffer without setting that buffer to current. It would be nice if such
>> >> setting were possible, especially in performance-critical code.
>> >
>> > Maybe, but is there any performance-critical code which needs that?
>>
>> For example, org-element.el needs to set buffer-local values in base
>> buffer of an indirect buffer every time buffer text is being edited.
>
> And that is performance-critical? in what way and in which situations?
When making a large number of edits in Org buffer. The code I am
referring to runs in `after-change-functions' and therefore should be as
fast as possible.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 07 Oct 2023 10:58:02 GMT)
Full text and
rfc822 format available.
Message #112 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Sat, 07 Oct 2023 09:29:15 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > From my POV, there are two alternatives here:
> >
> > . expose several new primitives to Lisp to make find-buffer-visiting
> > faster without changing the way we store the file-to-buffer
> > association information
> > . introduce caches or change the way file-to-buffer associations are
> > stored to speed up find-buffer-visiting
> >
> > What I'd like to see is that someone implements the first idea, and
> > times find-buffer-visiting after that to see if it becomes fast
> > enough. Then we can discuss whether anything else is needed.
>
> Agree. That's what I was trying to do. However, one of the new primitives
> will require searching a buffer with given value of `buffer-file-number'
> - the variable not defined in C.
buffer-file-number is defined as follows:
(file-attribute-file-identifier
(file-attributes buffer-file-name)))
whereas file-attribute-file-identifier is
(defsubst file-attribute-file-identifier (attributes)
"The inode and device numbers in ATTRIBUTES returned by `file-attributes'.
The value is a list of the form (INODENUM DEVICE), where DEVICE could be
either a single number or a cons cell of two numbers.
This tuple of numbers uniquely identifies the file."
(nthcdr 10 attributes))
So in C this is:
list2 (INT_TO_INTEGER (s.st_ino), INT_TO_INTEGER (s.st_dev));
> >> For example, org-element.el needs to set buffer-local values in base
> >> buffer of an indirect buffer every time buffer text is being edited.
> >
> > And that is performance-critical? in what way and in which situations?
>
> When making a large number of edits in Org buffer. The code I am
> referring to runs in `after-change-functions' and therefore should be as
> fast as possible.
I'd need to see measurements for that. In any case, this is a
separate issue.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 07 Oct 2023 11:08:02 GMT)
Full text and
rfc822 format available.
Message #115 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> Agree. That's what I was trying to do. However, one of the new primitives
>> will require searching a buffer with given value of `buffer-file-number'
>> - the variable not defined in C.
>
> buffer-file-number is defined as follows:
>
> (file-attribute-file-identifier
> (file-attributes buffer-file-name)))
>
> whereas file-attribute-file-identifier is
>
> (defsubst file-attribute-file-identifier (attributes)
> ...
> (nthcdr 10 attributes))
>
> So in C this is:
>
> list2 (INT_TO_INTEGER (s.st_ino), INT_TO_INTEGER (s.st_dev));
But how to get `s'? Do you mean that we should re-run `sys_fstat' on
every buffer? That's slow (query to FS), isn't it?
> I'd need to see measurements for that. In any case, this is a
> separate issue.
Ok. I will look into it later and open a separate bug report if 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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 07 Oct 2023 11:25:02 GMT)
Full text and
rfc822 format available.
Message #118 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Sat, 07 Oct 2023 11:08:10 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > So in C this is:
> >
> > list2 (INT_TO_INTEGER (s.st_ino), INT_TO_INTEGER (s.st_dev));
>
> But how to get `s'? Do you mean that we should re-run `sys_fstat' on
> every buffer? That's slow (query to FS), isn't it?
No, it's very fast, at least on Posix hosts. But if we decide it
isn't fast enough, we can record that in the buffer when the file is
visited.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 07 Oct 2023 11:43:02 GMT)
Full text and
rfc822 format available.
Message #121 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>
>> > So in C this is:
>> >
>> > list2 (INT_TO_INTEGER (s.st_ino), INT_TO_INTEGER (s.st_dev));
>>
>> But how to get `s'? Do you mean that we should re-run `sys_fstat' on
>> every buffer? That's slow (query to FS), isn't it?
>
> No, it's very fast, at least on Posix hosts. But if we decide it
> isn't fast enough, we can record that in the buffer when the file is
> visited.
I am mostly concerned about network FS, where any file operation can be
slow.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 07 Oct 2023 12:06:02 GMT)
Full text and
rfc822 format available.
Message #124 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Sat, 07 Oct 2023 11:43:38 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> Eli Zaretskii <eliz <at> gnu.org> writes:
> >>
> >> > So in C this is:
> >> >
> >> > list2 (INT_TO_INTEGER (s.st_ino), INT_TO_INTEGER (s.st_dev));
> >>
> >> But how to get `s'? Do you mean that we should re-run `sys_fstat' on
> >> every buffer? That's slow (query to FS), isn't it?
> >
> > No, it's very fast, at least on Posix hosts. But if we decide it
> > isn't fast enough, we can record that in the buffer when the file is
> > visited.
>
> I am mostly concerned about network FS, where any file operation can be
> slow.
My advice is to have a prototype working, then time it on local
filesystems. Optimizing for networked filesystems is the next step,
assuming it is needed. Please keep in mind that the current code does
that as well, only from Lisp: the call to file-attributes calls
'stat', then conses the 11-member list that is the return value. So
the C implementation cannot possibly be worse.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 08 Oct 2023 09:00:02 GMT)
Full text and
rfc822 format available.
Message #127 received at 66117 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:
> My advice is to have a prototype working, then time it on local
> filesystems.
See the attached patch.
I used the following simplified reproducer that does not involve loading
Org (which skews the relative numbers as Org loading is relatively slow):
(dotimes (i 1000) (with-temp-file (format "/tmp/test/%d.txt" i) (insert "* This is test")))
(dolist (file (directory-files "/tmp/test/" t "txt"))
(find-file-noselect file))
Without the patch (cpu-profiler-without-patch): 4.3 sec
With the patch (cpu-profiler-w-patch ): 2.5 sec
> ... Optimizing for networked filesystems is the next step,
> assuming it is needed. Please keep in mind that the current code does
> that as well, only from Lisp: the call to file-attributes calls
> 'stat', then conses the 11-member list that is the return value. So
> the C implementation cannot possibly be worse.
I left the `file-attributes' call in Elisp. Looking at the
cpu-profiler-w-patch, `find-buffer-visiting' is no longer the main
contributor to CPU time. I am not sure if we really need to squeeze the
performance yet further from `find-buffer-visiting' - `file-attributes'
is taking pretty much no time:
(reverse call-tree)
924 36% Automatic GC
173 6% + inhibit-local-variables-p
172 6% + locate-dominating-file
139 5% + abbreviate-file-name
113 4% + dir-locals--all-files
109 4% + file-truename
92 3% + find-buffer-visiting
...
6 0% + file-attributes
comapare with cpu-profiler-without-patch:
(reverse call-tree)
1714 39% + find-buffer-visiting
1131 26% Automatic GC
202 4% + locate-dominating-file
147 3% + abbreviate-file-name
140 3% + inhibit-local-variables-p
104 2% + dir-locals--all-files
98 2% + uniquify-rationalize-file-buffer-names
91 2% + file-truename
[0001-Improve-performance-of-find-buffer-visiting-bug-6611.patch (text/x-patch, inline)]
From 3b6a9eec3ccc9ff69dbce10d207bddb3e7611606 Mon Sep 17 00:00:00 2001
Message-ID: <3b6a9eec3ccc9ff69dbce10d207bddb3e7611606.1696755521.git.yantar92 <at> posteo.net>
From: Ihor Radchenko <yantar92 <at> posteo.net>
Date: Sun, 8 Oct 2023 11:48:42 +0300
Subject: [PATCH] Improve performance of `find-buffer-visiting' (bug#66117)
* src/buffer.c (Fget_truename_buffer): Expose `get_truename_buffer' to
Elisp.
(Ffind_buffer): New subr searching for a live buffer with a given
value of buffer-local variable.
(syms_of_buffer): Register the new added subroutines.
* src/filelock.c (lock_file): Use the new `Fget_truename_buffer' name.
* src/lisp.h:
* test/manual/etags/c-src/emacs/src/lisp.h: Remove no-longer-necessary
extern declarations for `get_truename_buffer'.
* lisp/files.el (find-buffer-visiting): Refactor, using subroutines to
search for buffers instead of slow manual Elisp iterations.
---
lisp/files.el | 54 ++++++++++--------------
src/buffer.c | 25 ++++++++++-
src/filelock.c | 2 +-
src/lisp.h | 1 -
test/manual/etags/c-src/emacs/src/lisp.h | 1 -
5 files changed, 47 insertions(+), 36 deletions(-)
diff --git a/lisp/files.el b/lisp/files.el
index 884c6b74247..dde1a2cc136 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -2183,37 +2183,29 @@ find-buffer-visiting
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)
- (with-current-buffer (car list)
- (if (and buffer-file-name
- (string= buffer-file-truename truename)
- (funcall predicate (current-buffer)))
- (setq found (car list))))
- (setq list (cdr list)))
- found)
- (let* ((attributes (file-attributes truename))
- (number (file-attribute-file-identifier attributes))
- (list (buffer-list)) found)
- (and buffer-file-numbers-unique
- (car-safe number) ;Make sure the inode is not just nil.
- (while (and (not found) list)
- (with-current-buffer (car list)
- (if (and buffer-file-name
- (equal buffer-file-number number)
- ;; Verify this buffer's file number
- ;; still belongs to its file.
- (file-exists-p buffer-file-name)
- (equal (file-attributes buffer-file-truename)
- attributes)
- (funcall predicate (current-buffer)))
- (setq found (car list))))
- (setq list (cdr list))))
- found))))
+ (or (let ((buf (get-file-buffer filename)))
+ (when (and buf (or (not predicate) (funcall predicate buf))) buf))
+ (let ((truename (abbreviate-file-name (file-truename filename))))
+ (or
+ (let ((buf (get-truename-buffer truename)))
+ (when (and buf (buffer-local-value 'buffer-file-name buf)
+ (or (not predicate) (funcall predicate buf)))
+ buf))
+ (let* ((attributes (file-attributes truename))
+ (number (file-attribute-file-identifier attributes)))
+ (and buffer-file-numbers-unique
+ (car-safe number) ;Make sure the inode is not just nil.
+ (let ((buf (find-buffer 'buffer-file-number number)))
+ (when (and buf (buffer-local-value 'buffer-file-name buf)
+ ;; Verify this buffer's file number
+ ;; still belongs to its file.
+ (file-exists-p buffer-file-name)
+ (equal (file-attributes buffer-file-truename)
+ attributes)
+ (or (not predicate)
+ (funcall predicate (current-buffer))))
+ buf))))))))
+
(defcustom find-file-wildcards t
"Non-nil means file-visiting commands should handle wildcards.
diff --git a/src/buffer.c b/src/buffer.c
index a7299f4a49e..12f226d8249 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -519,8 +519,11 @@ DEFUN ("get-file-buffer", Fget_file_buffer, Sget_file_buffer, 1, 1, 0,
return Qnil;
}
-Lisp_Object
-get_truename_buffer (register Lisp_Object filename)
+DEFUN ("get-truename-buffer", Fget_truename_buffer, Sget_truename_buffer, 1, 1, 0,
+ doc: /* Return the buffer with `file-truename' equal to FILENAME (a string).
+If there is no such live buffer, return nil.
+See also `find-buffer-visiting'. */)
+ (register Lisp_Object filename)
{
register Lisp_Object tail, buf;
@@ -533,6 +536,22 @@ get_truename_buffer (register Lisp_Object filename)
return Qnil;
}
+DEFUN ("find-buffer", Ffind_buffer, Sfind_buffer, 2, 2, 0,
+ doc: /* Return the buffer with buffer-local VARIABLE equal to VALUE.
+ If there is no such live buffer, return nil.
+See also `find-buffer-visiting'. */)
+ (Lisp_Object variable, Lisp_Object value)
+{
+ register Lisp_Object tail, buf;
+
+ FOR_EACH_LIVE_BUFFER (tail, buf)
+ {
+ if (!NILP (Fequal (value, Fbuffer_local_value(variable, buf))))
+ return buf;
+ }
+ return Qnil;
+}
+
/* Run buffer-list-update-hook if Vrun_hooks is non-nil and BUF does
not have buffer hooks inhibited. */
@@ -6010,6 +6029,8 @@ Functions (implicitly) running this hook are `get-buffer-create',
defsubr (&Sbuffer_list);
defsubr (&Sget_buffer);
defsubr (&Sget_file_buffer);
+ defsubr (&Sget_truename_buffer);
+ defsubr (&Sfind_buffer);
defsubr (&Sget_buffer_create);
defsubr (&Smake_indirect_buffer);
defsubr (&Sgenerate_new_buffer_name);
diff --git a/src/filelock.c b/src/filelock.c
index c2b306ab47d..9ce51c724b1 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -563,7 +563,7 @@ lock_file (Lisp_Object fn)
/* See if this file is visited and has changed on disk since it was
visited. */
- Lisp_Object subject_buf = get_truename_buffer (fn);
+ Lisp_Object subject_buf = Fget_truename_buffer (fn);
if (!NILP (subject_buf)
&& NILP (Fverify_visited_file_modtime (subject_buf))
&& !NILP (Ffile_exists_p (fn))
diff --git a/src/lisp.h b/src/lisp.h
index 39aa51531fe..544b1691556 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4715,7 +4715,6 @@ XMODULE_FUNCTION (Lisp_Object o)
Lisp_Object, Lisp_Object, Lisp_Object);
extern bool overlay_touches_p (ptrdiff_t);
extern Lisp_Object other_buffer_safely (Lisp_Object);
-extern Lisp_Object get_truename_buffer (Lisp_Object);
extern void init_buffer_once (void);
extern void init_buffer (void);
extern void syms_of_buffer (void);
diff --git a/test/manual/etags/c-src/emacs/src/lisp.h b/test/manual/etags/c-src/emacs/src/lisp.h
index aa8dc8c9a66..19463828270 100644
--- a/test/manual/etags/c-src/emacs/src/lisp.h
+++ b/test/manual/etags/c-src/emacs/src/lisp.h
@@ -4075,7 +4075,6 @@ intern_c_string (const char *str)
Lisp_Object, Lisp_Object, Lisp_Object);
extern bool overlay_touches_p (ptrdiff_t);
extern Lisp_Object other_buffer_safely (Lisp_Object);
-extern Lisp_Object get_truename_buffer (Lisp_Object);
extern void init_buffer_once (void);
extern void init_buffer (int);
extern void syms_of_buffer (void);
--
2.42.0
[cpu-profiler-without-patch (application/octet-stream, attachment)]
[cpu-profiler-w-patch (application/octet-stream, attachment)]
[Message part 5 (text/plain, inline)]
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 08 Oct 2023 09:58:01 GMT)
Full text and
rfc822 format available.
Message #130 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Sun, 08 Oct 2023 09:00:49 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > My advice is to have a prototype working, then time it on local
> > filesystems.
>
> See the attached patch.
Thanks. This still has some unnecessary overhead (I actually thought
about moving the entire find-buffer-visiting into C, not replacing it
with Lisp that calls new primitives). But this already makes
find-buffer-visiting take an insignificant portion of CPU time, so I
think it proves that the idea is workable:
> I left the `file-attributes' call in Elisp. Looking at the
> cpu-profiler-w-patch, `find-buffer-visiting' is no longer the main
> contributor to CPU time. I am not sure if we really need to squeeze the
> performance yet further from `find-buffer-visiting' - `file-attributes'
> is taking pretty much no time:
>
> (reverse call-tree)
> 924 36% Automatic GC
> 173 6% + inhibit-local-variables-p
> 172 6% + locate-dominating-file
> 139 5% + abbreviate-file-name
> 113 4% + dir-locals--all-files
> 109 4% + file-truename
> 92 3% + find-buffer-visiting
> ...
> 6 0% + file-attributes
>
> comapare with cpu-profiler-without-patch:
>
> (reverse call-tree)
> 1714 39% + find-buffer-visiting
> 1131 26% Automatic GC
> 202 4% + locate-dominating-file
> 147 3% + abbreviate-file-name
> 140 3% + inhibit-local-variables-p
> 104 2% + dir-locals--all-files
> 98 2% + uniquify-rationalize-file-buffer-names
> 91 2% + file-truename
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 08 Oct 2023 11:18:02 GMT)
Full text and
rfc822 format available.
Message #133 received at 66117 <at> debbugs.gnu.org (full text, mbox):
On 08/10/2023 12:00, Ihor Radchenko wrote:
> I am not sure if we really need to squeeze the
> performance yet further from `find-buffer-visiting' - `file-attributes'
> is taking pretty much no time:
If file-attributes is the main source of consing, it could be the reason
for the line
924 36% Automatic GC
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 08 Oct 2023 11:25:02 GMT)
Full text and
rfc822 format available.
Message #136 received at 66117 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Dmitry Gutov <dmitry <at> gutov.dev> writes:
> On 08/10/2023 12:00, Ihor Radchenko wrote:
>> I am not sure if we really need to squeeze the
>> performance yet further from `find-buffer-visiting' - `file-attributes'
>> is taking pretty much no time:
>
> If file-attributes is the main source of consing, it could be the reason
> for the line
>
> 924 36% Automatic GC
No, the main source of consing, according to the memory profiler, is
`inhibit-local-variables-p':
170,592,016 49% + inhibit-local-variables-p
45,619,488 13% + string-match
28,171,152 8% + insert-file-contents
26,066,069 7% + file-name-sans-versions
18,699,245 5% + file-remote-p
The full memory profile is attached.
[mem-profile-w-patch (application/octet-stream, attachment)]
[Message part 3 (text/plain, inline)]
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 08 Oct 2023 11:46:02 GMT)
Full text and
rfc822 format available.
Message #139 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 66117 <at> debbugs.gnu.org
> Date: Sun, 08 Oct 2023 11:25:20 +0000
>
> > If file-attributes is the main source of consing, it could be the reason
> > for the line
> >
> > 924 36% Automatic GC
>
> No, the main source of consing, according to the memory profiler, is
> `inhibit-local-variables-p':
The so-called "memory" profiler doesn't profile memory usage...
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 08 Oct 2023 12:10:02 GMT)
Full text and
rfc822 format available.
Message #142 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> > If file-attributes is the main source of consing, it could be the reason
>> > for the line
>> >
>> > 924 36% Automatic GC
>>
>> No, the main source of consing, according to the memory profiler, is
>> `inhibit-local-variables-p':
>
> The so-called "memory" profiler doesn't profile memory usage...
This is annoying... Would be so nice if there were an actual profiler
for consing.
I tried
- (let* ((attributes (file-attributes truename))
+ (let* ((attributes (and nil (file-attributes truename)))
which should effectively eliminate the calls to `file-attributes' from
`find-file-visiting'. The GC time did not change significantly:
916 38% Automatic GC
213 8% + locate-dominating-file
157 6% + abbreviate-file-name
141 5% + inhibit-local-variables-p
120 5% + file-truename
108 4% + dir-locals--all-files
60 2% + file-name-sans-versions
54 2% + insert-file-contents
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 08 Oct 2023 12:13:02 GMT)
Full text and
rfc822 format available.
Message #145 received at 66117 <at> debbugs.gnu.org (full text, mbox):
On 08/10/2023 14:25, Ihor Radchenko wrote:
> Dmitry Gutov <dmitry <at> gutov.dev> writes:
>
>> On 08/10/2023 12:00, Ihor Radchenko wrote:
>>> I am not sure if we really need to squeeze the
>>> performance yet further from `find-buffer-visiting' - `file-attributes'
>>> is taking pretty much no time:
>>
>> If file-attributes is the main source of consing, it could be the reason
>> for the line
>>
>> 924 36% Automatic GC
>
> No, the main source of consing, according to the memory profiler, is
> `inhibit-local-variables-p':
>
> 170,592,016 49% + inhibit-local-variables-p
> 45,619,488 13% + string-match
> 28,171,152 8% + insert-file-contents
> 26,066,069 7% + file-name-sans-versions
> 18,699,245 5% + file-remote-p
>
> The full memory profile is attached.
That's odd. Do many/any of your files being visited match
inhibit-local-variables-suffixes or inhibit-local-variables-regexps?
This might help a little bit:
diff --git a/lisp/files.el b/lisp/files.el
index b99ccf66d8a..8cab08bc6b6 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -3208,7 +3208,7 @@ inhibit-local-variables-p
sufs)
(setq name (substring name 0 (match-beginning 0))))
(while (and temp
- (not (string-match (car temp) name)))
+ (not (string-match-p (car temp) name)))
(setq temp (cdr temp)))
temp))
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 08 Oct 2023 12:20:02 GMT)
Full text and
rfc822 format available.
Message #148 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Dmitry Gutov <dmitry <at> gutov.dev> writes:
>> The full memory profile is attached.
>
> That's odd. Do many/any of your files being visited match
> inhibit-local-variables-suffixes or inhibit-local-variables-regexps?
I used the same reproducer. You can try it on your side if you want.
> This might help a little bit:
>
> - (not (string-match (car temp) name)))
> + (not (string-match-p (car temp) name)))
It is the first thing I tried. Nope.
And, as Eli pointed, memory profiler is not measuring consing... So, it
is useless for the purposes of reducing GC time.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 08 Oct 2023 12:29:01 GMT)
Full text and
rfc822 format available.
Message #151 received at 66117 <at> debbugs.gnu.org (full text, mbox):
On 08/10/2023 14:44, Eli Zaretskii wrote:
>> From: Ihor Radchenko<yantar92 <at> posteo.net>
>> Cc: Eli Zaretskii<eliz <at> gnu.org>,66117 <at> debbugs.gnu.org
>> Date: Sun, 08 Oct 2023 11:25:20 +0000
>>
>>> If file-attributes is the main source of consing, it could be the reason
>>> for the line
>>>
>>> 924 36% Automatic GC
>> No, the main source of consing, according to the memory profiler, is
>> `inhibit-local-variables-p':
> The so-called "memory" profiler doesn't profile memory usage...
It does seem to be recording allocations (in push_handler_nosignal).
Are you making the point that it doesn't record the consing values (i.e.
the real numbers that affect the time spent in GC)?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Mon, 09 Oct 2023 00:50:01 GMT)
Full text and
rfc822 format available.
Message #154 received at 66117 <at> debbugs.gnu.org (full text, mbox):
On 08/10/2023 15:20, Ihor Radchenko wrote:
> Dmitry Gutov <dmitry <at> gutov.dev> writes:
>
>>> The full memory profile is attached.
>>
>> That's odd. Do many/any of your files being visited match
>> inhibit-local-variables-suffixes or inhibit-local-variables-regexps?
>
> I used the same reproducer. You can try it on your side if you want.
Tried it, didn't find anything obvious (that helped) either. Thanks.
>> This might help a little bit:
>>
>> - (not (string-match (car temp) name)))
>> + (not (string-match-p (car temp) name)))
>
> It is the first thing I tried. Nope.
>
> And, as Eli pointed, memory profiler is not measuring consing... So, it
> is useless for the purposes of reducing GC time.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Mon, 09 Oct 2023 10:02:01 GMT)
Full text and
rfc822 format available.
Message #157 received at 66117 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:
>> > My advice is to have a prototype working, then time it on local
>> > filesystems.
>>
>> See the attached patch.
>
> Thanks. This still has some unnecessary overhead (I actually thought
> about moving the entire find-buffer-visiting into C, not replacing it
> with Lisp that calls new primitives). But this already makes
> find-buffer-visiting take an insignificant portion of CPU time, so I
> think it proves that the idea is workable:
I looked into rewriting `find-buffer-visiting' fully in C, but it
appears to be rather tricky because it calls `file-truename' and
`abbreviate-file-name', which are Elisp functions recursively calling
other Elisp functions.
And `abbreviate-file-name' is the slowest function showing up in the
profiler (with my patch applied):
;; Increased the number of files to get more than ~2 sec CPU samples.
(dotimes (i 3000) (with-temp-file (format "/tmp/test/%d.txt" i) (insert "* This is test")))
(dolist (file (directory-files "/tmp/test/" t "txt"))
(find-file-noselect file))
(cpu-profile-w-patch-3000-files, reverse call tree)
3130 29% Automatic GC
1305 12% + abbreviate-file-name
845 7% + inhibit-local-variables-p
842 7% + uniquify-rationalize-file-buffer-names
719 6% + find-buffer-visiting
619 5% + locate-dominating-file
350 3% + dir-locals--all-files
332 3% + set-auto-mode--apply-alist
283 2% + hack-local-variables--find-variables
272 2% + file-truename
I am not sure if we want to go deeper into the rabbit hole, but if we
do, here is also the perf profile:
25.35% emacs emacs [.] set_default_internal
15.73% emacs emacs [.] process_mark_stack
10.18% emacs emacs [.] re_match_2_internal
8.88% emacs emacs [.] assq_no_quit
2.23% emacs emacs [.] Fstring_equal
2.01% emacs emacs [.] pdumper_marked_p_impl
The main contributor is set_default_internal. I _suspect_ (because we
have non-linear scaling with the number of buffers - 3x increase in the
number of files lead to ~6x increase in the run time) that the main
contributor here is let-binding `case-fold-search' - special per-buffer
variable with no default value. In particular, `specbind' has the
following comment:
/* If SYMBOL is a per-buffer variable which doesn't have a
buffer-local value here, make the `let' change the global
value by changing the value of SYMBOL in all buffers not
having their own value. This is consistent with what
happens with other buffer-local variables. */
which implies that the binding will iterate over all the buffers,
leading to non-linear scaling in our scenario.
[cpu-profile-w-patch-3000-files (application/octet-stream, attachment)]
[Message part 3 (text/plain, inline)]
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Mon, 09 Oct 2023 10:16:01 GMT)
Full text and
rfc822 format available.
Message #160 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Ihor Radchenko <yantar92 <at> posteo.net> writes:
> The main contributor is set_default_internal. I _suspect_ (because we
> have non-linear scaling with the number of buffers - 3x increase in the
> number of files lead to ~6x increase in the run time) that the main
> contributor here is let-binding `case-fold-search' - special per-buffer
> variable with no default value.
My suspicion appears to be correct - removing let-binding for
case-fold-search from `abbreviate-file-name' and
`inhibit-local-variables-p' made them disappear from the top of the
reverse call tree.
Then, considering the ubiquity of let-binding case-fold-search across
Elisp code, speeding up let-binding of special per-buffer variables may
give us performance boost across Emacs.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 14:22:02 GMT)
Full text and
rfc822 format available.
Message #163 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Ihor Radchenko <yantar92 <at> posteo.net> writes:
> Ihor Radchenko <yantar92 <at> posteo.net> writes:
>
>> The main contributor is set_default_internal. I _suspect_ (because we
>> have non-linear scaling with the number of buffers - 3x increase in the
>> number of files lead to ~6x increase in the run time) that the main
>> contributor here is let-binding `case-fold-search' - special per-buffer
>> variable with no default value.
>
> My suspicion appears to be correct - removing let-binding for
> case-fold-search from `abbreviate-file-name' and
> `inhibit-local-variables-p' made them disappear from the top of the
> reverse call tree.
>
> Then, considering the ubiquity of let-binding case-fold-search across
> Elisp code, speeding up let-binding of special per-buffer variables may
> give us performance boost across Emacs.
What do you think about introducing something like
struct buffer buffer_overrides;
It will be similar to buffer_defaults in buffer.c, but will hold the
overriding values of buffer-local variables that are to be used instead
of proper buffer-local variables. That way, we can speed up let-binding
of case-fold-search.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 14:51:02 GMT)
Full text and
rfc822 format available.
Message #166 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Tue, 12 Dec 2023 14:24:35 +0000
>
> Ihor Radchenko <yantar92 <at> posteo.net> writes:
>
> > Ihor Radchenko <yantar92 <at> posteo.net> writes:
> >
> >> The main contributor is set_default_internal. I _suspect_ (because we
> >> have non-linear scaling with the number of buffers - 3x increase in the
> >> number of files lead to ~6x increase in the run time) that the main
> >> contributor here is let-binding `case-fold-search' - special per-buffer
> >> variable with no default value.
> >
> > My suspicion appears to be correct - removing let-binding for
> > case-fold-search from `abbreviate-file-name' and
> > `inhibit-local-variables-p' made them disappear from the top of the
> > reverse call tree.
> >
> > Then, considering the ubiquity of let-binding case-fold-search across
> > Elisp code, speeding up let-binding of special per-buffer variables may
> > give us performance boost across Emacs.
>
> What do you think about introducing something like
>
> struct buffer buffer_overrides;
>
> It will be similar to buffer_defaults in buffer.c, but will hold the
> overriding values of buffer-local variables that are to be used instead
> of proper buffer-local variables. That way, we can speed up let-binding
> of case-fold-search.
I'm sorry, I don't think I understand how this will work and avoid the
problems we have with let-binding. So I added Stefan, in the hope
that he might understand what I am evidently missing.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 16:31:01 GMT)
Full text and
rfc822 format available.
Message #169 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> What do you think about introducing something like
>>
>> struct buffer buffer_overrides;
>>
>> It will be similar to buffer_defaults in buffer.c, but will hold the
>> overriding values of buffer-local variables that are to be used instead
>> of proper buffer-local variables. That way, we can speed up let-binding
>> of case-fold-search.
>
> I'm sorry, I don't think I understand how this will work and avoid the
> problems we have with let-binding. So I added Stefan, in the hope
> that he might understand what I am evidently missing.
Let me elaborate.
- In eval.c, `Flet' always uses `specbind' to bind `case-fold-search'.
Because `case-fold-search' is declared special (u.s.declared_special =
true).
- For `case-fold-search', `specbind' does
/* If SYMBOL is a per-buffer variable which doesn't have a
buffer-local value here, make the `let' change the global
value by changing the value of SYMBOL in all buffers not
having their own value. This is consistent with what
happens with other buffer-local variables. */
if (NILP (Flocal_variable_p (symbol, Qnil)))
specpdl_ptr->let.kind = SPECPDL_LET_DEFAULT;
- Later, `specbind' calls `do_specbind' where SPECPDL_LET_DEFAULT causes
a call to `set_default_internal'.
- Then, `set_default_internal', for any variable with positive
buffer_local_flags loops over all the live buffers to overwrite the
buffer-local value.
This loop is the bottleneck, making let-binding
of `case-fold-search' (and any other variable marked in buffer.c:4691
block) scale with the number of open buffers.
- What I propose is to avoid this loop in `do_specbind' altogether.
Instead of having to loop through all the buffers to set temporary
buffer-local value, I propose to introduce special buffer object
`buffer_overrides' that will hold such temporary bindings.
Then, we can change BVAR to something like
#define BVAR(buf, field) (buffer_overrides->field ## _ == UNSET ?\
(buf)->field ## _ :\
buffer_overrides->field ## _)
and replace the loop with simply setting buffer_overrides slot.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 17:27:02 GMT)
Full text and
rfc822 format available.
Message #172 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, dmitry <at> gutov.dev,
> 66117 <at> debbugs.gnu.org
> Date: Tue, 12 Dec 2023 16:33:05 +0000
>
> - Then, `set_default_internal', for any variable with positive
> buffer_local_flags loops over all the live buffers to overwrite the
> buffer-local value.
>
> This loop is the bottleneck, making let-binding
> of `case-fold-search' (and any other variable marked in buffer.c:4691
> block) scale with the number of open buffers.
>
> - What I propose is to avoid this loop in `do_specbind' altogether.
> Instead of having to loop through all the buffers to set temporary
> buffer-local value, I propose to introduce special buffer object
> `buffer_overrides' that will hold such temporary bindings.
> Then, we can change BVAR to something like
>
> #define BVAR(buf, field) (buffer_overrides->field ## _ == UNSET ?\
> (buf)->field ## _ :\
> buffer_overrides->field ## _)
>
> and replace the loop with simply setting buffer_overrides slot.
So for starters, we make each BVAR more expensive, i.e. make Emacs
uniformly slower (because we call BVAR all over the place). And then
I'm not sure I understand how buffer_overrides will get the values of
those temporary bindings without having to use the same loops when we
have let-binding within another one. Also, what about threads?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 17:42:01 GMT)
Full text and
rfc822 format available.
Message #175 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> #define BVAR(buf, field) (buffer_overrides->field ## _ == UNSET ?\
>> (buf)->field ## _ :\
>> buffer_overrides->field ## _)
>>
>> and replace the loop with simply setting buffer_overrides slot.
>
> So for starters, we make each BVAR more expensive, i.e. make Emacs
> uniformly slower (because we call BVAR all over the place).
Yes, although I do not believe that it will have large impact in
practice. It is just an extra == comparison.
> ... And then
> I'm not sure I understand how buffer_overrides will get the values of
> those temporary bindings without having to use the same loops when we
> have let-binding within another one. Also, what about threads?
Just as any other global state variable - via `specbind'. Entering inner
let will push the previous value into specpdl and recover it upon
exiting.
Threads work similarly, maintaining their own local specpdl queue heads
that make sure that global state is recorded and rewinded as needed.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 18:38:02 GMT)
Full text and
rfc822 format available.
Message #178 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: monnier <at> iro.umontreal.ca, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Tue, 12 Dec 2023 17:44:15 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> #define BVAR(buf, field) (buffer_overrides->field ## _ == UNSET ?\
> >> (buf)->field ## _ :\
> >> buffer_overrides->field ## _)
> >>
> >> and replace the loop with simply setting buffer_overrides slot.
> >
> > So for starters, we make each BVAR more expensive, i.e. make Emacs
> > uniformly slower (because we call BVAR all over the place).
>
> Yes, although I do not believe that it will have large impact in
> practice. It is just an extra == comparison.
We've seen how similar extra comparisons slow down Emacs when we
introduced symbols-with-location. IMO, BVAR might be even more
expensive, since it is used much more frequently and widely.
> > ... And then
> > I'm not sure I understand how buffer_overrides will get the values of
> > those temporary bindings without having to use the same loops when we
> > have let-binding within another one. Also, what about threads?
>
> Just as any other global state variable - via `specbind'. Entering inner
> let will push the previous value into specpdl and recover it upon
> exiting.
And that will not require looping similar to what we have now?
> Threads work similarly, maintaining their own local specpdl queue heads
> that make sure that global state is recorded and rewinded as needed.
I don't think I follow you here.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 19:11:02 GMT)
Full text and
rfc822 format available.
Message #181 received at 66117 <at> debbugs.gnu.org (full text, mbox):
On 12/12/2023 20:36, Eli Zaretskii wrote:
>> From: Ihor Radchenko<yantar92 <at> posteo.net>
>> Cc:monnier <at> iro.umontreal.ca,dmitry <at> gutov.dev,66117 <at> debbugs.gnu.org
>> Date: Tue, 12 Dec 2023 17:44:15 +0000
>>
>> Eli Zaretskii<eliz <at> gnu.org> writes:
>>
>>>> #define BVAR(buf, field) (buffer_overrides->field ## _ == UNSET ?\
>>>> (buf)->field ## _ :\
>>>> buffer_overrides->field ## _)
>>>>
>>>> and replace the loop with simply setting buffer_overrides slot.
>>> So for starters, we make each BVAR more expensive, i.e. make Emacs
>>> uniformly slower (because we call BVAR all over the place).
>> Yes, although I do not believe that it will have large impact in
>> practice. It is just an extra == comparison.
> We've seen how similar extra comparisons slow down Emacs when we
> introduced symbols-with-location. IMO, BVAR might be even more
> expensive, since it is used much more frequently and widely.
We also have dynamic variable bindings all over the place, so it might
be worth a try (with measurements).
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 19:16:01 GMT)
Full text and
rfc822 format available.
Message #184 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> > So for starters, we make each BVAR more expensive, i.e. make Emacs
>> > uniformly slower (because we call BVAR all over the place).
>>
>> Yes, although I do not believe that it will have large impact in
>> practice. It is just an extra == comparison.
>
> We've seen how similar extra comparisons slow down Emacs when we
> introduced symbols-with-location. IMO, BVAR might be even more
> expensive, since it is used much more frequently and widely.
It might. Do you know a good way to test such thing? Because I do not
think that a mere suspicion that this change might make things slower
should stop the discussion.
>> > ... And then
>> > I'm not sure I understand how buffer_overrides will get the values of
>> > those temporary bindings without having to use the same loops when we
>> > have let-binding within another one. Also, what about threads?
>>
>> Just as any other global state variable - via `specbind'. Entering inner
>> let will push the previous value into specpdl and recover it upon
>> exiting.
>
> And that will not require looping similar to what we have now?
No.
AFAIK, the way let-binding for dynamic scoping works is:
1. push existing variable value to specpdl
2. assign a new value
3. when leaving let scope, pop the record from specpdl and assign the
old value
What I propose is changing the way value is assigned. Now, the assignment of
buffer-local variable that is recorded into buffer object slot involves
doing it in all the live buffers. I propose to do it in a special buffer
object instead, which is strictly faster.
>> Threads work similarly, maintaining their own local specpdl queue heads
>> that make sure that global state is recorded and rewinded as needed.
>
> I don't think I follow you here.
See `rebind_for_thread_switch' and `unbind_for_thread_switch'.
The workhorse is `specpdl_unrewind', which see.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 19:18:01 GMT)
Full text and
rfc822 format available.
Message #187 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> Date: Tue, 12 Dec 2023 21:10:13 +0200
> Cc: monnier <at> iro.umontreal.ca, 66117 <at> debbugs.gnu.org
> From: Dmitry Gutov <dmitry <at> gutov.dev>
>
> On 12/12/2023 20:36, Eli Zaretskii wrote:
> >> From: Ihor Radchenko<yantar92 <at> posteo.net>
> >> Cc:monnier <at> iro.umontreal.ca,dmitry <at> gutov.dev,66117 <at> debbugs.gnu.org
> >> Date: Tue, 12 Dec 2023 17:44:15 +0000
> >>
> >> Eli Zaretskii<eliz <at> gnu.org> writes:
> >>
> >>>> #define BVAR(buf, field) (buffer_overrides->field ## _ == UNSET ?\
> >>>> (buf)->field ## _ :\
> >>>> buffer_overrides->field ## _)
> >>>>
> >>>> and replace the loop with simply setting buffer_overrides slot.
> >>> So for starters, we make each BVAR more expensive, i.e. make Emacs
> >>> uniformly slower (because we call BVAR all over the place).
> >> Yes, although I do not believe that it will have large impact in
> >> practice. It is just an extra == comparison.
> > We've seen how similar extra comparisons slow down Emacs when we
> > introduced symbols-with-location. IMO, BVAR might be even more
> > expensive, since it is used much more frequently and widely.
>
> We also have dynamic variable bindings all over the place
But not all of them are special like case-fold-search.
> so it might be worth a try (with measurements).
I didn't say it isn't worth a try.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 19:21:02 GMT)
Full text and
rfc822 format available.
Message #190 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: monnier <at> iro.umontreal.ca, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Tue, 12 Dec 2023 19:18:19 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> > So for starters, we make each BVAR more expensive, i.e. make Emacs
> >> > uniformly slower (because we call BVAR all over the place).
> >>
> >> Yes, although I do not believe that it will have large impact in
> >> practice. It is just an extra == comparison.
> >
> > We've seen how similar extra comparisons slow down Emacs when we
> > introduced symbols-with-location. IMO, BVAR might be even more
> > expensive, since it is used much more frequently and widely.
>
> It might. Do you know a good way to test such thing? Because I do not
> think that a mere suspicion that this change might make things slower
> should stop the discussion.
To measure such things, you need to run some benchmarks.
Byte-compiling might be one such kind; "make bootstrap" could be
another; scrolling one line at a time through a large file could be
yet another one. Several such benchmarks were posted to the lists on
several similar occasions.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 20:03:01 GMT)
Full text and
rfc822 format available.
Message #193 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> - What I propose is to avoid this loop in `do_specbind' altogether.
> Instead of having to loop through all the buffers to set temporary
> buffer-local value, I propose to introduce special buffer object
> `buffer_overrides' that will hold such temporary bindings.
> Then, we can change BVAR to something like
>
> #define BVAR(buf, field) (buffer_overrides->field ## _ == UNSET ?\
> (buf)->field ## _ :\
> buffer_overrides->field ## _)
If we're willing to pay the cost of such a test, then we don't need
a new `buffer_overrides`, we can just set `buf->field` to a special value
(e.g. `Qunbound`, tho we'll probably need to use a new similar special
value) and then do:
#define BVAR(buf, field) ((buf)->field ## _ != Qunbound ?\
(buf)->field ## _ :\
buffer_defaults->field ## _)
This said, in the case of `case-fold-search` I think there's a simpler
solution: make it "buffer-local only", meaning that let-binding it
affects only the current buffer (rather than affecting all the buffers
which have not made it buffer-local yet).
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 20:48:02 GMT)
Full text and
rfc822 format available.
Message #196 received at 66117 <at> debbugs.gnu.org (full text, mbox):
On 12/12/2023 22:01, Stefan Monnier wrote:
> This said, in the case of `case-fold-search` I think there's a simpler
> solution: make it "buffer-local only", meaning that let-binding it
> affects only the current buffer (rather than affecting all the buffers
> which have not made it buffer-local yet).
Wouldn't that be a breaking change? And a non-obvious one.
E.g. company-dabbrev-code creates a dynamic binding for case-fold-search
in one place and then calls a function (company-dabbrev--search) that
iterates across all buffers with re-search-forward.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 22:59:02 GMT)
Full text and
rfc822 format available.
Message #199 received at 66117 <at> debbugs.gnu.org (full text, mbox):
>> This said, in the case of `case-fold-search` I think there's a simpler
>> solution: make it "buffer-local only", meaning that let-binding it
>> affects only the current buffer (rather than affecting all the buffers
>> which have not made it buffer-local yet).
> Wouldn't that be a breaking change? And a non-obvious one.
Yup.
> E.g. company-dabbrev-code creates a dynamic binding for case-fold-search in
> one place and then calls a function (company-dabbrev--search) that iterates
> across all buffers with re-search-forward.
I think we call this collateral damage.
My suggestion was indeed not completely serious, tho I do think in the
long run it's what we should do.
BTW, regarding making BVAR slower, we could reduce its cost by
introducing two versions of BVAR, so we only pay the price when
accessing one of the problematic variables like `case-fold-search`.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 23:11:01 GMT)
Full text and
rfc822 format available.
Message #202 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> E.g. company-dabbrev-code creates a dynamic binding for case-fold-search in
> one place and then calls a function (company-dabbrev--search) that iterates
> across all buffers with re-search-forward.
BTW, that `company-dabbrev-code` function may not do what you expect
in those buffers that have set `case-fold-search` buffer-locally.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 23:41:02 GMT)
Full text and
rfc822 format available.
Message #205 received at 66117 <at> debbugs.gnu.org (full text, mbox):
On 13/12/2023 00:57, Stefan Monnier wrote:
>>> This said, in the case of `case-fold-search` I think there's a simpler
>>> solution: make it "buffer-local only", meaning that let-binding it
>>> affects only the current buffer (rather than affecting all the buffers
>>> which have not made it buffer-local yet).
>> Wouldn't that be a breaking change? And a non-obvious one.
> Yup.
>
>> E.g. company-dabbrev-code creates a dynamic binding for case-fold-search in
>> one place and then calls a function (company-dabbrev--search) that iterates
>> across all buffers with re-search-forward.
> I think we call this collateral damage.
>
> My suggestion was indeed not completely serious, tho I do think in the
> long run it's what we should do.
>
> BTW, regarding making BVAR slower, we could reduce its cost by
> introducing two versions of BVAR, so we only pay the price when
> accessing one of the problematic variables like `case-fold-search`.
Perhaps in the long run it might be good to do the opposite? Introduce
never-buffer-local variables, and make case-fold-search one of them.
I do see a few hits for
(setq-local case-fold-search
across the Emacs codebase, but in none of these I can clearly understand
the end goal (is that for interactive isearch?). Except for
ido-make-buffer-list-1, which is done purely for performance, working
around a problem related to what's being discussed here (df4991093b94).
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 23:42:01 GMT)
Full text and
rfc822 format available.
Message #208 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> E.g. company-dabbrev-code creates a dynamic binding for case-fold-search in
>> one place and then calls a function (company-dabbrev--search) that iterates
>> across all buffers with re-search-forward.
>
> BTW, that `company-dabbrev-code` function may not do what you expect
> in those buffers that have set `case-fold-search` buffer-locally.
Yup.
Steps to reproduce:
1. emacs -Q
2. M-: (setq-local case-fold-search nil)
3. C-x b <RET> (to *Messages*)
4. M-: (let ((case-fold-search t)) (with-current-buffer (get-buffer "*scratch*") (search-forward "CREATE")))
5. Observe search failing
If step (2) is omitted, search succeeds.
Although, I am not sure if I like that let-binding is not reliable for
buffer-locals. It is simply confusing from the point of view of let
usage; even if documented. I'd rather go for `let' becoming more consistent.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 12 Dec 2023 23:49:02 GMT)
Full text and
rfc822 format available.
Message #211 received at 66117 <at> debbugs.gnu.org (full text, mbox):
On 13/12/2023 01:43, Ihor Radchenko wrote:
> Stefan Monnier<monnier <at> iro.umontreal.ca> writes:
>
>>> E.g. company-dabbrev-code creates a dynamic binding for case-fold-search in
>>> one place and then calls a function (company-dabbrev--search) that iterates
>>> across all buffers with re-search-forward.
>> BTW, that `company-dabbrev-code` function may not do what you expect
>> in those buffers that have set `case-fold-search` buffer-locally.
> Yup.
>
> Steps to reproduce:
>
> 1. emacs -Q
> 2. M-: (setq-local case-fold-search nil)
> 3. C-x b <RET> (to*Messages*)
> 4. M-: (let ((case-fold-search t)) (with-current-buffer (get-buffer "*scratch*") (search-forward "CREATE")))
> 5. Observe search failing
>
> If step (2) is omitted, search succeeds.
That's a good data point, but since I don't understand why anybody would
make (or does make) case-fold-search buffer-local, I'm not going to
hurry to fix those cases.
> Although, I am not sure if I like that let-binding is not reliable for
> buffer-locals. It is simply confusing from the point of view of let
> usage; even if documented. I'd rather go for `let' becoming more consistent.
That sounds attractive (ignoring all buffer-locals when let-bound?), but
I'm guessing there are enough valid use cases for this behavior.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 13 Dec 2023 03:52:01 GMT)
Full text and
rfc822 format available.
Message #214 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> Perhaps in the long run it might be good to do the opposite? Introduce
> never-buffer-local variables, and make case-fold-search one of them.
>
> I do see a few hits for
>
> (setq-local case-fold-search
>
> across the Emacs codebase, but in none of these I can clearly understand the
> end goal (is that for interactive isearch?).
If it's really rare, then the simpler&better solution is to move it out
of the `struct buffer` altogether (i.e. make it normal DEFVAR_LISP or
DEFVAR_BOOL).
> Except for
> ido-make-buffer-list-1, which is done purely for performance, working
> around a problem related to what's being discussed here (df4991093b94).
Of course, ideally there should be no such var for `re-search-*`
and friends. Such vars are good for user-configs impacting the behavior
of commands but they're a bit clunky when used as implicit parameters to
low-level ELisp primitives.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 13 Dec 2023 03:56:02 GMT)
Full text and
rfc822 format available.
Message #217 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> That sounds attractive (ignoring all buffer-locals when let-bound?),
> but I'm guessing there are enough valid use cases for this behavior.
Yup, that would break lots of existing code, indeed, and in subtle
enough ways that people would go crazy.
Luckily, there's no "100% ideal" way buffer-local and scoped bindings
can interact. All choices suck in some circumstances, so there's really
no good reason to lament the current choice: it's no worse than
the alternatives, AFAIK.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 13 Dec 2023 12:12:01 GMT)
Full text and
rfc822 format available.
Message #220 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Ihor Radchenko <yantar92 <at> posteo.net>, Eli Zaretskii <eliz <at> gnu.org>,
> 66117 <at> debbugs.gnu.org
> Date: Tue, 12 Dec 2023 22:55:15 -0500
>
> > That sounds attractive (ignoring all buffer-locals when let-bound?),
> > but I'm guessing there are enough valid use cases for this behavior.
>
> Yup, that would break lots of existing code, indeed, and in subtle
> enough ways that people would go crazy.
>
> Luckily, there's no "100% ideal" way buffer-local and scoped bindings
> can interact. All choices suck in some circumstances, so there's really
> no good reason to lament the current choice: it's no worse than
> the alternatives, AFAIK.
Btw, the title of this bug is about find-buffer-visiting, and AFAIR I
proposed to have a primitive written in C that replaces
find-buffer-visiting without a need to actually switch to each buffer
(which is what triggers the costly rebinding of buffer-local variables
like case-fold-search). So why we are talking about case-fold-search
instead of solving the original problem? If we want to discuss the
case-fold-search issue, IMO we should discuss it in a separate bug
report.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 13 Dec 2023 13:04:02 GMT)
Full text and
rfc822 format available.
Message #223 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> Btw, the title of this bug is about find-buffer-visiting, and AFAIR I
> proposed to have a primitive written in C that replaces
> find-buffer-visiting without a need to actually switch to each buffer
> (which is what triggers the costly rebinding of buffer-local variables
> like case-fold-search). So why we are talking about case-fold-search
> instead of solving the original problem? If we want to discuss the
> case-fold-search issue, IMO we should discuss it in a separate bug
> report.
The bottlenecks now (after applying my patch for `find-buffer-visiting')
are `abbreviate-file-name' and `inhibit-local-variables-p'; not the
`find-buffer-visiting' per se (both are slow because they let-bind
case-fold-search).
AFAIU, it is very hard to rewrite them in C, as I explained in
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#157
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 13 Dec 2023 13:27:02 GMT)
Full text and
rfc822 format available.
Message #226 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, dmitry <at> gutov.dev,
> 66117 <at> debbugs.gnu.org
> Date: Wed, 13 Dec 2023 13:06:06 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > Btw, the title of this bug is about find-buffer-visiting, and AFAIR I
> > proposed to have a primitive written in C that replaces
> > find-buffer-visiting without a need to actually switch to each buffer
> > (which is what triggers the costly rebinding of buffer-local variables
> > like case-fold-search). So why we are talking about case-fold-search
> > instead of solving the original problem? If we want to discuss the
> > case-fold-search issue, IMO we should discuss it in a separate bug
> > report.
>
> The bottlenecks now (after applying my patch for `find-buffer-visiting')
> are `abbreviate-file-name' and `inhibit-local-variables-p'; not the
> `find-buffer-visiting' per se (both are slow because they let-bind
> case-fold-search).
Then let's see about implementing in C of these two, or their subsets
needed by find-buffer-visiting, because case-insensitive operations in
C don't need to bind case-fold-search.
Another possibility is to rewrite the parts of these two functions
that need case-insensitive matching in a way that doesn't need to bind
case-fold-search. For example, case-insensitive match against "abcd"
using string-match can be replaced by a case-sensitive match against
"[aA][bB][cC][dD]" or by a call to compare-strings.
> AFAIU, it is very hard to rewrite them in C, as I explained in
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#157
The only explanation I see there is that they "recursively call
other Elisp functions". Is there any other reason?
My point is that by trying to speed-up let-binding of case-fold-search
and similar variables you are trying to solve a much more general
problem, and a much harder one at that. IME, if an idea for a
solution leads to a more general and harder-to-solve problem than the
original, then the idea should be dropped, and one should look for
alternative ideas.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 13 Dec 2023 13:41:01 GMT)
Full text and
rfc822 format available.
Message #229 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> AFAIU, it is very hard to rewrite them in C, as I explained in
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#157
>
> The only explanation I see there is that they "recursively call
> other Elisp functions". Is there any other reason?
The main problem I see is that `abbreviate-file-name' uses a number of
variables, including customizations, defined in ELisp. I suspect that
moving all of those to Elisp will neither be easy nor practical.
> My point is that by trying to speed-up let-binding of case-fold-search
> and similar variables you are trying to solve a much more general
> problem, and a much harder one at that. IME, if an idea for a
> solution leads to a more general and harder-to-solve problem than the
> original, then the idea should be dropped, and one should look for
> alternative ideas.
The simplest solution would be what `ido-make-buffer-list-1' does:
;; Each call to ido-ignore-item-p LET-binds case-fold-search.
;; That is slow if there's no buffer-local binding available,
;; roughly O(number of buffers). This hack avoids it.
(setq-local case-fold-search nil)
But it will only solve specific problem with `find-buffer-visiting' and
we can always go for it if we cannot find anything better.
I thought that trying to solve a more general problem would benefit more
code - let-binding case-fold-search is extremely common across packages.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 13 Dec 2023 13:53:02 GMT)
Full text and
rfc822 format available.
Message #232 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: monnier <at> iro.umontreal.ca, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Wed, 13 Dec 2023 13:43:23 +0000
>
> The simplest solution would be what `ido-make-buffer-list-1' does:
>
> ;; Each call to ido-ignore-item-p LET-binds case-fold-search.
> ;; That is slow if there's no buffer-local binding available,
> ;; roughly O(number of buffers). This hack avoids it.
> (setq-local case-fold-search nil)
>
> But it will only solve specific problem with `find-buffer-visiting' and
> we can always go for it if we cannot find anything better.
Which other popular functions need to loop through all the buffers in
Lisp?
> I thought that trying to solve a more general problem would benefit more
> code - let-binding case-fold-search is extremely common across packages.
IMO, the general way you are trying to solve this makes this a very
hard problem with potentially Emacs-wide implications. So we might as
well look for easier alternatives.
We could, for example, document the above trick, for those who need
it.
Or we could have a separate variable, which would not be a defcustom
nor automatically buffer-local, and will have the same effect as
case-fold-search on low-level searching and matching functions.
Btw, are you aware that many case-insensitive operations in Emacs
depend also on the case table in effect, which can also be
buffer-local? So case-insensitive operations in Lisp can be
unexpectedly affected by stuff like the current buffer. One more
reason to use them as little as possible.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 13 Dec 2023 14:14:02 GMT)
Full text and
rfc822 format available.
Message #235 received at 66117 <at> debbugs.gnu.org (full text, mbox):
>> But it will only solve specific problem with `find-buffer-visiting' and
>> we can always go for it if we cannot find anything better.
> Which other popular functions need to loop through all the buffers in
> Lisp?
Not sure why that matters. The performance problem comes from repeated
uses of `(let ((case-fold-search ..)) ...)` where the repetition can be
due to anything (not only enumerating buffers).
The "loop through buffers" happens when entering and leaving the `let`,
because it has to `set/unset` the corresponding filed of the `struct
buffer` of all the buffers whose `case-fold-search` is "global".
Making it a DEFVAR_LISP rather than a DEFVAR_PER_BUFFER will fix this
problem (at the cost of making it less efficient to `set-buffer` when
that variable has been made buffer-local).
DEFVAR_PER_BUFFER should be for variables which are usually
buffer-local, and apparently that's not the case of `case-fold-search`,
so I'm starting to think that changing it from DEFVAR_PER_BUFFER to
DEFVAR_LISP might be the better option.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 13 Dec 2023 14:30:02 GMT)
Full text and
rfc822 format available.
Message #238 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> ;; Each call to ido-ignore-item-p LET-binds case-fold-search.
>> ;; That is slow if there's no buffer-local binding available,
>> ;; roughly O(number of buffers). This hack avoids it.
>> (setq-local case-fold-search nil)
>>
>> But it will only solve specific problem with `find-buffer-visiting' and
>> we can always go for it if we cannot find anything better.
>
> Which other popular functions need to loop through all the buffers in
> Lisp?
Org mode... When activating the major mode, Org binds case-fold-search
many times - for each Org buffer being opened. The same with happen for
other major modes that let-bind case-fold-search during initialization.
Also, various multi-buffer search functions, like multi-occur, tag
search (etags.el), fileloop.el.
>> I thought that trying to solve a more general problem would benefit more
>> code - let-binding case-fold-search is extremely common across packages.
>
> IMO, the general way you are trying to solve this makes this a very
> hard problem with potentially Emacs-wide implications. So we might as
> well look for easier alternatives.
The solution with Emacs-wide implications is not what I proposed, if you
are talking about breaking changes in let.
What I proposed might slow thing down because of more complex BVAR, but
that is yet to be proven. I want to try first before giving up.
> We could, for example, document the above trick, for those who need
> it.
>
> Or we could have a separate variable, which would not be a defcustom
> nor automatically buffer-local, and will have the same effect as
> case-fold-search on low-level searching and matching functions.
That's indeed another option. Also, we may introduce something similar
to `with-syntax-table' macro - `with-case-fold' and then throw a warning
if case-fold-search is let-bound directly.
> Btw, are you aware that many case-insensitive operations in Emacs
> depend also on the case table in effect, which can also be
> buffer-local? So case-insensitive operations in Lisp can be
> unexpectedly affected by stuff like the current buffer. One more
> reason to use them as little as possible.
... and syntax-table can be set in text properties; so not just affected
by current buffer - also by position in buffer.
Yet, it is sometimes necessary and there is no simple way around
available.
In the past, me and Mattias Engdegård even discussed a possibility to
incorporate case-fold-search flag into regexps directly
(https://debbugs.gnu.org/cgi/bugreport.cgi?bug=63225#74)
That way, things would be less affected by the buffer-locals.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 13 Dec 2023 14:59:02 GMT)
Full text and
rfc822 format available.
Message #241 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Ihor Radchenko <yantar92 <at> posteo.net>, dmitry <at> gutov.dev,
> 66117 <at> debbugs.gnu.org
> Date: Wed, 13 Dec 2023 09:12:39 -0500
>
> >> But it will only solve specific problem with `find-buffer-visiting' and
> >> we can always go for it if we cannot find anything better.
> > Which other popular functions need to loop through all the buffers in
> > Lisp?
>
> Not sure why that matters.
Because if using the setq-local trick solves this problem, we could do
it in those other places as well.
> The performance problem comes from repeated
> uses of `(let ((case-fold-search ..)) ...)` where the repetition can be
> due to anything (not only enumerating buffers).
>
> The "loop through buffers" happens when entering and leaving the `let`,
> because it has to `set/unset` the corresponding filed of the `struct
> buffer` of all the buffers whose `case-fold-search` is "global".
That's not the loop through buffers I had in mind. I meant the loop
in find-buffer-visiting.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 13 Dec 2023 15:24:02 GMT)
Full text and
rfc822 format available.
Message #244 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: monnier <at> iro.umontreal.ca, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org,
> Mattias Engdegård <mattias.engdegard <at> gmail.com>
> Date: Wed, 13 Dec 2023 14:32:21 +0000
>
> > Btw, are you aware that many case-insensitive operations in Emacs
> > depend also on the case table in effect, which can also be
> > buffer-local? So case-insensitive operations in Lisp can be
> > unexpectedly affected by stuff like the current buffer. One more
> > reason to use them as little as possible.
>
> ... and syntax-table can be set in text properties; so not just affected
> by current buffer - also by position in buffer.
If you search the buffer text, then having syntax-table on that text
is less surprising: after all, it only affects the text which has this
property.
But buffer-local case-table is much more subtle: it is in effect even
if you match strings, not portions of buffer text, and only because
some buffer _happens_ to be the current buffer when, say, string-match
is called.
> In the past, me and Mattias Engdegård even discussed a possibility to
> incorporate case-fold-search flag into regexps directly
> (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=63225#74)
> That way, things would be less affected by the buffer-locals.
Indeed.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 14:18:02 GMT)
Full text and
rfc822 format available.
Message #247 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> ... and syntax-table can be set in text properties; so not just affected
>> by current buffer - also by position in buffer.
>
> If you search the buffer text, then having syntax-table on that text
> is less surprising: after all, it only affects the text which has this
> property.
>
> But buffer-local case-table is much more subtle: it is in effect even
> if you match strings, not portions of buffer text, and only because
> some buffer _happens_ to be the current buffer when, say, string-match
> is called.
Good point. I did not even think about this subtlety.
Then, I think that having a global variable like
`case-fold-search-override' could make things more predictable if we
advertise it for let-bindings.
Also, it is much easier to implement.
WDYT?
>> In the past, me and Mattias Engdegård even discussed a possibility to
>> incorporate case-fold-search flag into regexps directly
>> (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=63225#74)
>> That way, things would be less affected by the buffer-locals.
>
> Indeed.
There are multiple ways to implement this. Though I think that this part
of the discussion should continue in bug#63225 or even a separate
feature request.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 16:42:02 GMT)
Full text and
rfc822 format available.
Message #250 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: monnier <at> iro.umontreal.ca, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org,
> mattias.engdegard <at> gmail.com
> Date: Thu, 14 Dec 2023 14:20:18 +0000
>
> Then, I think that having a global variable like
> `case-fold-search-override' could make things more predictable if we
> advertise it for let-bindings.
> Also, it is much easier to implement.
> WDYT?
I'd choose a different name (since this is supposed to affect any
matching, not just search).
And then we will need to decide which case-equivalence table this will
use. The current default is affected by language-environment, so it
might not be a good idea (it causes known problems in Turkish locales,
for example). Perhaps we need a new standard kind of table for that.
But for starters, a new Lisp-only global variable should be good, I
think. Stefan, any comments?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 17:08:02 GMT)
Full text and
rfc822 format available.
Message #253 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> But for starters, a new Lisp-only global variable should be good, I
> think. Stefan, any comments?
I must say I don't understand the reasoning behind this.
What would it do different from `case-fold-search`?
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 17:15:02 GMT)
Full text and
rfc822 format available.
Message #256 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Ihor Radchenko <yantar92 <at> posteo.net>, dmitry <at> gutov.dev,
> 66117 <at> debbugs.gnu.org, mattias.engdegard <at> gmail.com
> Date: Thu, 14 Dec 2023 12:07:13 -0500
>
> > But for starters, a new Lisp-only global variable should be good, I
> > think. Stefan, any comments?
>
> I must say I don't understand the reasoning behind this.
> What would it do different from `case-fold-search`?
It won't be buffer-local-if-changed, so binding it will not be costly.
It also won't be a defcustom, so let-binding it will not step on the
user's preferences.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 18:13:01 GMT)
Full text and
rfc822 format available.
Message #259 received at 66117 <at> debbugs.gnu.org (full text, mbox):
>> > But for starters, a new Lisp-only global variable should be good, I
>> > think. Stefan, any comments?
>>
>> I must say I don't understand the reasoning behind this.
>> What would it do different from `case-fold-search`?
>
> It won't be buffer-local-if-changed, so binding it will not be costly.
As mentioned elsewhere in this bug-report we can make `case-fold-search`
into a `DEFVAR_LISP`: it would have no visible impact to ELisp and would
avoid the costly let-bindings.
> It also won't be a defcustom, so let-binding it will not step on the
> user's preferences.
Hmm... when/where do existing let-bindings of `case-fold-search` step on
user settings of that var?
IME the problem is rather the opposite: most calls to search functions
don't explicitly let-bind `case-fold-search` and instead rely naively on
the default value and are thus susceptible to bugs if/when someone sets
the custom var (globally or buffer-locally).
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 18:28:01 GMT)
Full text and
rfc822 format available.
Message #262 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> It won't be buffer-local-if-changed, so binding it will not be costly.
>
> As mentioned elsewhere in this bug-report we can make `case-fold-search`
> into a `DEFVAR_LISP`: it would have no visible impact to ELisp and would
> avoid the costly let-bindings.
Wouldn't DEFVAR_LISP break major modes that do (setq case-fold-search ...)?
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 18:43:01 GMT)
Full text and
rfc822 format available.
Message #265 received at 66117 <at> debbugs.gnu.org (full text, mbox):
>>> It won't be buffer-local-if-changed, so binding it will not be costly.
>> As mentioned elsewhere in this bug-report we can make `case-fold-search`
>> into a `DEFVAR_LISP`: it would have no visible impact to ELisp and would
>> avoid the costly let-bindings.
> Wouldn't DEFVAR_LISP break major modes that do (setq case-fold-search ...)?
No, it makes no difference to Lisp. It only changes the implementation
strategy: DEFVAR_LISP stores the value in a global C variable (which
thus requires more processing when the var is buffer-local, more
specifically updating the C var when we `set-buffer`), whereas
DEFVAR_PER_BUFFER stores the value inside the buffer object (which
thus requires more processing when the var is *not* buffer-local since
the global value is duplicated in all the buffers where it's not made
buffer-local).
DEFVAR_PER_BUFFER are thus the better option for vars which are set
buffer-locally in most buffers whereas DEFVAR_LISP is the better option
for vars which are made buffer-local only in a few buffers.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 18:50:02 GMT)
Full text and
rfc822 format available.
Message #268 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: yantar92 <at> posteo.net, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org,
> mattias.engdegard <at> gmail.com
> Date: Thu, 14 Dec 2023 13:11:58 -0500
>
> >> > But for starters, a new Lisp-only global variable should be good, I
> >> > think. Stefan, any comments?
> >>
> >> I must say I don't understand the reasoning behind this.
> >> What would it do different from `case-fold-search`?
> >
> > It won't be buffer-local-if-changed, so binding it will not be costly.
>
> As mentioned elsewhere in this bug-report we can make `case-fold-search`
> into a `DEFVAR_LISP`: it would have no visible impact to ELisp and would
> avoid the costly let-bindings.
Didn't we just agree that would be a breaking change?
> > It also won't be a defcustom, so let-binding it will not step on the
> > user's preferences.
>
> Hmm... when/where do existing let-bindings of `case-fold-search` step on
> user settings of that var?
Each time we let-bind it in code used in searching and/or matching
commands.
> IME the problem is rather the opposite: most calls to search functions
> don't explicitly let-bind `case-fold-search` and instead rely naively on
> the default value and are thus susceptible to bugs if/when someone sets
> the custom var (globally or buffer-locally).
I don't see that as a bug: the user said he/she wants the search to be
case-insensitive, so they should get what they asked for.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 19:00:02 GMT)
Full text and
rfc822 format available.
Message #271 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>>> It won't be buffer-local-if-changed, so binding it will not be costly.
>>> As mentioned elsewhere in this bug-report we can make `case-fold-search`
>>> into a `DEFVAR_LISP`: it would have no visible impact to ELisp and would
>>> avoid the costly let-bindings.
>> Wouldn't DEFVAR_LISP break major modes that do (setq case-fold-search ...)?
>
> No, it makes no difference to Lisp. It only changes the implementation
> strategy: DEFVAR_LISP stores the value in a global C variable (which
> thus requires more processing when the var is buffer-local, more
> specifically updating the C var when we `set-buffer`), whereas
> DEFVAR_PER_BUFFER stores the value inside the buffer object (which
> thus requires more processing when the var is *not* buffer-local since
> the global value is duplicated in all the buffers where it's not made
> buffer-local).
Do I understand correctly that what is needed to implement your
suggestion is
1. Remove case_fold_search_ slot from buffer objects
2. Use DEFVAR_LISP for case-fold-search
3. Declare case-fold-search buffer-local
4. Replace BVAR(...) references in C with Vcase_fold_search
?
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 19:13:02 GMT)
Full text and
rfc822 format available.
Message #274 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> I must say I don't understand the reasoning behind this.
>> What would it do different from `case-fold-search`?
>
> It won't be buffer-local-if-changed, so binding it will not be costly.
> It also won't be a defcustom, so let-binding it will not step on the
> user's preferences.
In my mind, another benefit is that the new variable will not be
affected by buffer-local values - (let (case-fold-search-override value) ...)
will be guaranteed to use VALUE everywhere, even if current buffer is
changed. This will make the case I described in
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#208 more predictable
(and, since it is a new variable, will not break any existing code).
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 19:38:02 GMT)
Full text and
rfc822 format available.
Message #277 received at 66117 <at> debbugs.gnu.org (full text, mbox):
>>>>> It won't be buffer-local-if-changed, so binding it will not be costly.
>>>> As mentioned elsewhere in this bug-report we can make `case-fold-search`
>>>> into a `DEFVAR_LISP`: it would have no visible impact to ELisp and would
>>>> avoid the costly let-bindings.
>>> Wouldn't DEFVAR_LISP break major modes that do (setq case-fold-search ...)?
>>
>> No, it makes no difference to Lisp. It only changes the implementation
>> strategy: DEFVAR_LISP stores the value in a global C variable (which
>> thus requires more processing when the var is buffer-local, more
>> specifically updating the C var when we `set-buffer`), whereas
>> DEFVAR_PER_BUFFER stores the value inside the buffer object (which
>> thus requires more processing when the var is *not* buffer-local since
>> the global value is duplicated in all the buffers where it's not made
>> buffer-local).
>
> Do I understand correctly that what is needed to implement your
> suggestion is
>
> 1. Remove case_fold_search_ slot from buffer objects
> 2. Use DEFVAR_LISP for case-fold-search
> 3. Declare case-fold-search buffer-local
> 4. Replace BVAR(...) references in C with Vcase_fold_search
Yes (assuming that 3 refers to the use of `make-variable-buffer-local`).
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 19:51:01 GMT)
Full text and
rfc822 format available.
Message #280 received at 66117 <at> debbugs.gnu.org (full text, mbox):
>> >> > But for starters, a new Lisp-only global variable should be good, I
>> >> > think. Stefan, any comments?
>> >>
>> >> I must say I don't understand the reasoning behind this.
>> >> What would it do different from `case-fold-search`?
>> >
>> > It won't be buffer-local-if-changed, so binding it will not be costly.
>>
>> As mentioned elsewhere in this bug-report we can make `case-fold-search`
>> into a `DEFVAR_LISP`: it would have no visible impact to ELisp and would
>> avoid the costly let-bindings.
> Didn't we just agree that would be a breaking change?
No, when? where?
You might be confusing that proposition to my proposition to make it
always-buffer-local (like `mode-name` or `buffer-file-name`).
>> > It also won't be a defcustom, so let-binding it will not step on the
>> > user's preferences.
>> Hmm... when/where do existing let-bindings of `case-fold-search` step on
>> user settings of that var?
> Each time we let-bind it in code used in searching and/or matching
> commands.
I don't think that true of all those let-binding.
Obviously the mere fact that the let-binding takes precedence means that
we override the user's setting, but that's just an internal technical
detail. To "step on the user's preference" we additionally need
a situation where the user-visible result is not what the user wanted.
Which of those let-binding can really be said to "step on the user's
preference"?
AFAICT most/all the times we do that, it's because we do a search that's
"internal" to some operation and has thus no reason to obey the custom
setting ,which AFAICT is meant to affect interactive uses like Isearch
(tho Isearch doesn't Isearch doesn't pay attention to
`case-fold-search`, AFAICT, so really the user-visible effect of setting
`case-fold-search` is quite limited).
I don't think that when users set `case-fold-search` to t they mean that
indentation and highlighting should treat `STrucT` as a keyword in
C mode, right?
>> IME the problem is rather the opposite: most calls to search functions
>> don't explicitly let-bind `case-fold-search` and instead rely naively on
>> the default value and are thus susceptible to bugs if/when someone sets
>> the custom var (globally or buffer-locally).
>
> I don't see that as a bug: the user said he/she wants the search to be
> case-insensitive, so they should get what they asked for.
But that's only true if the user runs a search command.
If the search is done within a non-search command (e.g. indentation),
then the result is often not what the user wanted.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 19:57:02 GMT)
Full text and
rfc822 format available.
Message #283 received at 66117 <at> debbugs.gnu.org (full text, mbox):
>>> I must say I don't understand the reasoning behind this.
>>> What would it do different from `case-fold-search`?
>> It won't be buffer-local-if-changed, so binding it will not be costly.
>> It also won't be a defcustom, so let-binding it will not step on the
>> user's preferences.
> In my mind, another benefit is that the new variable will not be
> affected by buffer-local values - (let (case-fold-search-override value) ...)
> will be guaranteed to use VALUE everywhere, even if current buffer is
> changed. This will make the case I described in
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#208 more predictable
> (and, since it is a new variable, will not break any existing code).
But of course, as always it cuts both ways. E.g. if you do
(let ((case-fold-search-override t))
(dolist (buf (buffer-list))
(when (string-match "foo" buffer-file-name)
(do-something))))
your `case-fold-search-override` will override all the careful
`case-fold-search` let-bindings used during the execution of `do-something`
which will likely lead to new bugs.
[ Say, if `do-something` ends up calling `syntax-ppss` which ends up
running `syntax-propertize` which often needs to perform
case-sensitive searches. ]
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 20:25:01 GMT)
Full text and
rfc822 format available.
Message #286 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: yantar92 <at> posteo.net, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org,
> mattias.engdegard <at> gmail.com
> Date: Thu, 14 Dec 2023 14:49:54 -0500
>
> >> As mentioned elsewhere in this bug-report we can make `case-fold-search`
> >> into a `DEFVAR_LISP`: it would have no visible impact to ELisp and would
> >> avoid the costly let-bindings.
> > Didn't we just agree that would be a breaking change?
>
> No, when? where?
>
> You might be confusing that proposition to my proposition to make it
> always-buffer-local (like `mode-name` or `buffer-file-name`).
Maybe. This discussion has enough confusion to confuse everyone.
> >> > It also won't be a defcustom, so let-binding it will not step on the
> >> > user's preferences.
> >> Hmm... when/where do existing let-bindings of `case-fold-search` step on
> >> user settings of that var?
> > Each time we let-bind it in code used in searching and/or matching
> > commands.
>
> I don't think that true of all those let-binding.
Not all, just some.
> Obviously the mere fact that the let-binding takes precedence means that
> we override the user's setting, but that's just an internal technical
> detail. To "step on the user's preference" we additionally need
> a situation where the user-visible result is not what the user wanted.
Yes, and you are sure there aren't such cases, what with our
willy-nilly binding this variable whenever we need that?
> AFAICT most/all the times we do that, it's because we do a search that's
> "internal" to some operation and has thus no reason to obey the custom
> setting ,which AFAICT is meant to affect interactive uses like Isearch
> (tho Isearch doesn't Isearch doesn't pay attention to
> `case-fold-search`, AFAICT, so really the user-visible effect of setting
> `case-fold-search` is quite limited).
This is wishful thinking not based by any data.
> >> IME the problem is rather the opposite: most calls to search functions
> >> don't explicitly let-bind `case-fold-search` and instead rely naively on
> >> the default value and are thus susceptible to bugs if/when someone sets
> >> the custom var (globally or buffer-locally).
> >
> > I don't see that as a bug: the user said he/she wants the search to be
> > case-insensitive, so they should get what they asked for.
>
> But that's only true if the user runs a search command.
That's what I had in mind, and even said so.
> If the search is done within a non-search command (e.g. indentation),
> then the result is often not what the user wanted.
My point is that we use case-fold-search for two different purposes,
and so it is good to have 2 variables, one each for every purpose.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 20:58:02 GMT)
Full text and
rfc822 format available.
Message #289 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> Maybe. This discussion has enough confusion to confuse everyone.
I've already been fooled more than once in this thread, indeed :-)
>> Obviously the mere fact that the let-binding takes precedence means that
>> we override the user's setting, but that's just an internal technical
>> detail. To "step on the user's preference" we additionally need
>> a situation where the user-visible result is not what the user wanted.
>
> Yes, and you are sure there aren't such cases,
Oh, no, I'm definitely not sure, but since fixing this problem was cited
as part of the motivation for the introduction of
`case-fold-search-override`, I hope we have at least one or two concrete
cases showing that it's a real problem.
>> AFAICT most/all the times we do that, it's because we do a search that's
>> "internal" to some operation and has thus no reason to obey the custom
>> setting, which AFAICT is meant to affect interactive uses like Isearch
>> (tho Isearch doesn't Isearch doesn't pay attention to
>> `case-fold-search`, AFAICT, so really the user-visible effect of setting
>> `case-fold-search` is quite limited).
> This is wishful thinking not based by any data.
Really? There are many let-bindings of `case-fold-search` in the tree,
so it's hard to get a clear picture, but a random sampling of them
suggests they mostly wrap searches where the regexps are not provided by
the user but by the wrapped code (they're typically hard-coded).
Admittedly, that doesn't mean that it would be wrong to obey the user's choice
of `case-fold-search` there, but I think it clearly leans this way.
>> >> IME the problem is rather the opposite: most calls to search functions
>> >> don't explicitly let-bind `case-fold-search` and instead rely naively on
>> >> the default value and are thus susceptible to bugs if/when someone sets
>> >> the custom var (globally or buffer-locally).
>> > I don't see that as a bug: the user said he/she wants the search to be
>> > case-insensitive, so they should get what they asked for.
>> But that's only true if the user runs a search command.
> That's what I had in mind, and even said so.
But I was talking about code which performs a search that's internal to
the command rather than being the purpose of the command. Things like
searches performed during indentation, searches performed to parse
the output of processes, ...
>> If the search is done within a non-search command (e.g. indentation),
>> then the result is often not what the user wanted.
> My point is that we use case-fold-search for two different purposes,
> and so it is good to have 2 variables, one each for every purpose.
We do use it for 2 different purposes, indeed.
And I have seen this lead to bugs.
But the bugs I recall went in the direction "user setting unexpectedly
affecting internal searches" rather than "let-binding unexpected
affecting user searches".
I must admit that I can't point to concrete examples of the kind of bugs
"I recall". But I don't know of examples of bugs caused by let-bindings
overriding user settings either.
So all in all, this seems like a weak justification for any change.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 21:33:02 GMT)
Full text and
rfc822 format available.
Message #292 received at 66117 <at> debbugs.gnu.org (full text, mbox):
On 14/12/2023 22:57, Stefan Monnier wrote:
> But the bugs I recall went in the direction "user setting unexpectedly
> affecting internal searches" rather than "let-binding unexpected
> affecting user searches".
>
> I must admit that I can't point to concrete examples of the kind of bugs
> "I recall".
You just pointed at one in company-dabbrev several messages ago.
Nobody ever reported this, though, so I have to assume that "user
settings" for case-fold-search are rare enough.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 14 Dec 2023 23:04:02 GMT)
Full text and
rfc822 format available.
Message #295 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Dmitry Gutov [2023-12-14 23:32:01] wrote:
> On 14/12/2023 22:57, Stefan Monnier wrote:
>> But the bugs I recall went in the direction "user setting unexpectedly
>> affecting internal searches" rather than "let-binding unexpected
>> affecting user searches".
>> I must admit that I can't point to concrete examples of the kind of bugs
>> "I recall".
>
> You just pointed at one in company-dabbrev several messages ago.
> Nobody ever reported this, though, so I have to assume that "user settings"
> for case-fold-search are rare enough.
I suspect that buffer-local settings are indeed rare, and I'd
expect them to come from packages rather than from users.
I was instead thinking of cases where the global setting (more likely to
be set by the user) incorrectly affected a package's behavior.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 15 Dec 2023 10:17:02 GMT)
Full text and
rfc822 format available.
Message #298 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> In my mind, another benefit is that the new variable will not be
>> affected by buffer-local values - (let (case-fold-search-override value) ...)
>> will be guaranteed to use VALUE everywhere, even if current buffer is
>> changed. This will make the case I described in
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#208 more predictable
>> (and, since it is a new variable, will not break any existing code).
>
> But of course, as always it cuts both ways. E.g. if you do
>
> (let ((case-fold-search-override t))
> (dolist (buf (buffer-list))
> (when (string-match "foo" buffer-file-name)
> (do-something))))
>
> your `case-fold-search-override` will override all the careful
> `case-fold-search` let-bindings used during the execution of `do-something`
> which will likely lead to new bugs.
Good point. I was hoping that together with this new variable we also
add a compiler warning encouraging to use the case-fold-search-override
_instead_ of case-fold-search everywhere. But I can indeed see how
people can get even more confused in such scenario.
Within the scope of this particular bug report, I am now leaning more
towards your idea with DEFVAR_LISP then.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 15 Dec 2023 10:59:01 GMT)
Full text and
rfc822 format available.
Message #301 received at 66117 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> Do I understand correctly that what is needed to implement your
>> suggestion is
>>
>> 1. Remove case_fold_search_ slot from buffer objects
>> 2. Use DEFVAR_LISP for case-fold-search
>> 3. Declare case-fold-search buffer-local
>> 4. Replace BVAR(...) references in C with Vcase_fold_search
>
> Yes (assuming that 3 refers to the use of `make-variable-buffer-local`).
See the attached patch.
I used my `find-buffer-visiting' benchmark
(dolist (file (directory-files "/tmp/test/" t "txt"))
(find-file-noselect file))
Without the patch: 10.3 sec
With the patch: 7.0 sec
Without the patch, the reverse profiler tree is
2728 26% Automatic GC
1187 11% + abbreviate-file-name ;; <---
1080 10% + inhibit-local-variables-p ;; <---
913 8% + uniquify-rationalize-file-buffer-names
816 7% + find-buffer-visiting
547 5% + locate-dominating-file
With the patch, the `abbreviate-file-name' and
`inhibit-local-variables-p' no longer appear on top
2559 35% Automatic GC
892 12% + uniquify-rationalize-file-buffer-names
704 9% + find-buffer-visiting
473 6% + locate-dominating-file
295 4% + dir-locals--all-files
241 3% + generate-new-buffer
219 3% + file-truename
169 2% + inhibit-local-variables-p
128 1% + abbreviate-file-name
[0001-Improve-performance-let-binding-case-fold-search-bug.patch (text/x-patch, inline)]
From fb7677be03e8c49d6a549f0d993b4df6f0ea404d Mon Sep 17 00:00:00 2001
Message-ID: <fb7677be03e8c49d6a549f0d993b4df6f0ea404d.1702638101.git.yantar92 <at> posteo.net>
From: Ihor Radchenko <yantar92 <at> posteo.net>
Date: Fri, 15 Dec 2023 11:47:45 +0100
Subject: [PATCH] Improve performance let-binding `case-fold-search'
(bug#66117)
* src/buffer.h: Remove case_fold_search_ buffer object slot.
* src/buffer.c (bset_case_fold_search): Remove - no longer needed.
(init_buffer_once): Remove removed buffer slot init.
(syms_of_buffer): Use DEFVAR_LISP to define `case-fold-search' and
declare it buffer-local.
* src/minibuf.c (syms_of_minibuf): Remove DEFSYM call for
`case-fold-search' symbol. It now lives in `syms_of_buffer'.
* src/editfns.c (Fcompare_buffer_substrings):
(Fchar_equal):
* src/search.c (looking_at_1):
(string_match_1):
(search_command):
(Fre__describe_compiled): Adjust C queries to `case-fold-search' value
to use C globals instead of BVAR macro.
* doc/lispref/internals.texi (Buffer Internals): Do not list
`case_fold_search' slot.
See https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#259
When used as buffer slot, let-binding `case-fold-search' would scale
with the number of live buffers and can be slow. This change makes
let-binding much faster at the cost of slightly slower `set-buffer'.
---
doc/lispref/internals.texi | 1 -
src/buffer.c | 17 ++++++-----------
src/buffer.h | 1 -
src/editfns.c | 4 ++--
src/minibuf.c | 1 -
src/search.c | 10 +++++-----
6 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/doc/lispref/internals.texi b/doc/lispref/internals.texi
index 1fba683223e..4b4edda6d46 100644
--- a/doc/lispref/internals.texi
+++ b/doc/lispref/internals.texi
@@ -2519,7 +2519,6 @@ Buffer Internals
@item mode_line_format
@itemx header_line_format
-@itemx case_fold_search
@itemx tab_width
@itemx fill_column
@itemx left_margin
diff --git a/src/buffer.c b/src/buffer.c
index 12f226d8249..919c470d9e5 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -210,11 +210,6 @@ bset_buffer_file_coding_system (struct buffer *b, Lisp_Object val)
b->buffer_file_coding_system_ = val;
}
static void
-bset_case_fold_search (struct buffer *b, Lisp_Object val)
-{
- b->case_fold_search_ = val;
-}
-static void
bset_ctl_arrow (struct buffer *b, Lisp_Object val)
{
b->ctl_arrow_ = val;
@@ -4692,7 +4687,6 @@ init_buffer_once (void)
XSETFASTINT (BVAR (&buffer_local_flags, mode_line_format), idx); ++idx;
XSETFASTINT (BVAR (&buffer_local_flags, abbrev_mode), idx); ++idx;
XSETFASTINT (BVAR (&buffer_local_flags, overwrite_mode), idx); ++idx;
- XSETFASTINT (BVAR (&buffer_local_flags, case_fold_search), idx); ++idx;
XSETFASTINT (BVAR (&buffer_local_flags, auto_fill_function), idx); ++idx;
XSETFASTINT (BVAR (&buffer_local_flags, selective_display), idx); ++idx;
XSETFASTINT (BVAR (&buffer_local_flags, selective_display_ellipses), idx); ++idx;
@@ -4785,7 +4779,6 @@ init_buffer_once (void)
bset_tab_line_format (&buffer_defaults, Qnil);
bset_abbrev_mode (&buffer_defaults, Qnil);
bset_overwrite_mode (&buffer_defaults, Qnil);
- bset_case_fold_search (&buffer_defaults, Qt);
bset_auto_fill_function (&buffer_defaults, Qnil);
bset_selective_display (&buffer_defaults, Qnil);
bset_selective_display_ellipses (&buffer_defaults, Qt);
@@ -5215,10 +5208,6 @@ syms_of_buffer (void)
doc: /* Non-nil if Abbrev mode is enabled.
Use the command `abbrev-mode' to change this variable. */);
- DEFVAR_PER_BUFFER ("case-fold-search", &BVAR (current_buffer, case_fold_search),
- Qnil,
- doc: /* Non-nil if searches and matches should ignore case. */);
-
DEFVAR_PER_BUFFER ("fill-column", &BVAR (current_buffer, fill_column),
Qintegerp,
doc: /* Column beyond which automatic line-wrapping should happen.
@@ -5951,6 +5940,12 @@ Functions (implicitly) running this hook are `get-buffer-create',
This is the default. If nil, auto-save file deletion is inhibited. */);
delete_auto_save_files = 1;
+ DEFVAR_LISP ("case-fold-search", Vcase_fold_search,
+ doc: /* Non-nil if searches and matches should ignore case. */);
+ Vcase_fold_search = Qt;
+ DEFSYM (Qcase_fold_search, "case-fold-search");
+ Fmake_variable_buffer_local (Qcase_fold_search);
+
DEFVAR_LISP ("clone-indirect-buffer-hook", Vclone_indirect_buffer_hook,
doc: /* Normal hook to run in the new buffer at the end of `make-indirect-buffer'.
diff --git a/src/buffer.h b/src/buffer.h
index b2bd15657dc..399c6585158 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -379,7 +379,6 @@ #define BVAR(buf, field) ((buf)->field ## _)
/* Values of several buffer-local variables. */
/* tab-width is buffer-local so that redisplay can find it
in buffers that are not current. */
- Lisp_Object case_fold_search_;
Lisp_Object tab_width_;
Lisp_Object fill_column_;
Lisp_Object left_margin_;
diff --git a/src/editfns.c b/src/editfns.c
index 49d552c4a75..aa410ea7091 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -1785,7 +1785,7 @@ DEFUN ("compare-buffer-substrings", Fcompare_buffer_substrings, Scompare_buffer_
register EMACS_INT begp1, endp1, begp2, endp2, temp;
register struct buffer *bp1, *bp2;
register Lisp_Object trt
- = (!NILP (BVAR (current_buffer, case_fold_search))
+ = (!NILP (Vcase_fold_search)
? BVAR (current_buffer, case_canon_table) : Qnil);
ptrdiff_t chars = 0;
ptrdiff_t i1, i2, i1_byte, i2_byte;
@@ -4367,7 +4367,7 @@ DEFUN ("char-equal", Fchar_equal, Schar_equal, 2, 2, 0,
if (XFIXNUM (c1) == XFIXNUM (c2))
return Qt;
- if (NILP (BVAR (current_buffer, case_fold_search)))
+ if (NILP (Vcase_fold_search))
return Qnil;
i1 = XFIXNAT (c1);
diff --git a/src/minibuf.c b/src/minibuf.c
index 58adde1bf66..0d5b375e450 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -2320,7 +2320,6 @@ syms_of_minibuf (void)
DEFSYM (Qcurrent_input_method, "current-input-method");
DEFSYM (Qactivate_input_method, "activate-input-method");
- DEFSYM (Qcase_fold_search, "case-fold-search");
DEFSYM (Qmetadata, "metadata");
DEFSYM (Qcycle_sort_function, "cycle-sort-function");
diff --git a/src/search.c b/src/search.c
index 2996d32fca1..75a67ff3223 100644
--- a/src/search.c
+++ b/src/search.c
@@ -281,7 +281,7 @@ looking_at_1 (Lisp_Object string, bool posix, bool modify_data)
struct regexp_cache *cache_entry = compile_pattern (
string,
modify_match_data ? &search_regs : NULL,
- (!NILP (BVAR (current_buffer, case_fold_search))
+ (!NILP (Vcase_fold_search)
? BVAR (current_buffer, case_canon_table) : Qnil),
posix,
!NILP (BVAR (current_buffer, enable_multibyte_characters)));
@@ -402,7 +402,7 @@ string_match_1 (Lisp_Object regexp, Lisp_Object string, Lisp_Object start,
struct regexp_cache *cache_entry
= compile_pattern (regexp,
modify_match_data ? &search_regs : NULL,
- (!NILP (BVAR (current_buffer, case_fold_search))
+ (!NILP (Vcase_fold_search)
? BVAR (current_buffer, case_canon_table)
: Qnil),
posix,
@@ -1068,10 +1068,10 @@ search_command (Lisp_Object string, Lisp_Object bound, Lisp_Object noerror,
BVAR (current_buffer, case_eqv_table));
np = search_buffer (string, PT, PT_BYTE, lim, lim_byte, n, RE,
- (!NILP (BVAR (current_buffer, case_fold_search))
+ (!NILP (Vcase_fold_search)
? BVAR (current_buffer, case_canon_table)
: Qnil),
- (!NILP (BVAR (current_buffer, case_fold_search))
+ (!NILP (Vcase_fold_search)
? BVAR (current_buffer, case_eqv_table)
: Qnil),
posix);
@@ -3402,7 +3402,7 @@ DEFUN ("re--describe-compiled", Fre__describe_compiled, Sre__describe_compiled,
{
struct regexp_cache *cache_entry
= compile_pattern (regexp, NULL,
- (!NILP (BVAR (current_buffer, case_fold_search))
+ (!NILP (Vcase_fold_search)
? BVAR (current_buffer, case_canon_table) : Qnil),
false,
!NILP (BVAR (current_buffer,
--
2.42.0
[profile-w-case-fold-search-patch (application/octet-stream, attachment)]
[profile-without-case-fold-search-patch (application/octet-stream, attachment)]
[Message part 5 (text/plain, inline)]
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 15 Dec 2023 13:48:01 GMT)
Full text and
rfc822 format available.
Message #304 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> See the attached patch.
LGTM,
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 06:06:01 GMT)
Full text and
rfc822 format available.
Message #307 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org,
> mattias.engdegard <at> gmail.com
> Date: Fri, 15 Dec 2023 11:01:53 +0000
>
> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
> >> Do I understand correctly that what is needed to implement your
> >> suggestion is
> >>
> >> 1. Remove case_fold_search_ slot from buffer objects
> >> 2. Use DEFVAR_LISP for case-fold-search
> >> 3. Declare case-fold-search buffer-local
> >> 4. Replace BVAR(...) references in C with Vcase_fold_search
> >
> > Yes (assuming that 3 refers to the use of `make-variable-buffer-local`).
>
> See the attached patch.
Thanks, but I wonder what, if anything, we should say about this
change in NEWS? Also, do any of the manuals require any changes wrt
this?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 06:12:01 GMT)
Full text and
rfc822 format available.
Message #310 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> Thanks, but I wonder what, if anything, we should say about this
> change in NEWS? Also, do any of the manuals require any changes wrt
> this?
AFAIK this is a purely internal change with no visible effect at the
ELisp level other than a slightly different performance profile when
accessing `case-fold-search` (barring subtle inconsistencies in our
implementation of let/setq/symbol-value, of course).
So, I think it doesn't require any change in the docs or the etc/NEWS.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 08:31:01 GMT)
Full text and
rfc822 format available.
Message #313 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Ihor Radchenko <yantar92 <at> posteo.net>, dmitry <at> gutov.dev,
> 66117 <at> debbugs.gnu.org, mattias.engdegard <at> gmail.com
> Date: Sun, 17 Dec 2023 01:11:46 -0500
>
> > Thanks, but I wonder what, if anything, we should say about this
> > change in NEWS? Also, do any of the manuals require any changes wrt
> > this?
>
> AFAIK this is a purely internal change with no visible effect at the
> ELisp level
??? Then what were these about:
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#199
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#217
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#283
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#295
It sounds like there will be some effect on some Lisp programs, at
least in some cases, and if so, we should mention those cases, even if
we think they are rare.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 10:29:02 GMT)
Full text and
rfc822 format available.
Message #316 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> AFAIK this is a purely internal change with no visible effect at the
>> ELisp level
> It sounds like there will be some effect on some Lisp programs, at
> least in some cases, and if so, we should mention those cases, even if
> we think they are rare.
We have discussed a number of approaches to handle the let-binding
performance and to handle sometimes-confusing behaviour of
`case-fold-search'. Most of them had problems with breaking the existing
behaviour or potentially causing unexpected side effects. Except the one
I used in the patch.
The messages you linked to discuss problems with _other_ proposed
approaches.
> ??? Then what were these about:
>
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#199
This is about an idea to make let-binding the `case-fold-search' affect
current buffer only, but never other buffers.
What the patch does is different - it turns `case-fold-search' into an
ordinary buffer-local variable, like the ones defined from Elisp.
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#217
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#283
These two are about creating a new case-fold-search-override that
ignores buffer-local bindings. Again, a different idea.
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#295
This was not about this patch, but about a more general problem of
case-fold-search serving two purposes - user setting for interactive
searches and internal regexp search switch.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 10:38:02 GMT)
Full text and
rfc822 format available.
Message #319 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, dmitry <at> gutov.dev,
> 66117 <at> debbugs.gnu.org, mattias.engdegard <at> gmail.com
> Date: Sun, 17 Dec 2023 10:31:50 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> AFAIK this is a purely internal change with no visible effect at the
> >> ELisp level
>
> > It sounds like there will be some effect on some Lisp programs, at
> > least in some cases, and if so, we should mention those cases, even if
> > we think they are rare.
>
> We have discussed a number of approaches to handle the let-binding
> performance and to handle sometimes-confusing behaviour of
> `case-fold-search'. Most of them had problems with breaking the existing
> behaviour or potentially causing unexpected side effects. Except the one
> I used in the patch.
>
> The messages you linked to discuss problems with _other_ proposed
> approaches.
Maybe I'm confused, but won't this change have at least _some_ effect
on Lisp programs? For example, what about this fragment from the
ELisp manual, which describes the effect of
make-variable-buffer-local:
A peculiar wrinkle of this feature is that binding the variable
(with ‘let’ or other binding constructs) does not create a
buffer-local binding for it. Only setting the variable (with ‘set’
or ‘setq’), while the variable does not have a ‘let’-style binding
that was made in the current buffer, does so.
Will this case work the same after the change as it did before?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 10:47:02 GMT)
Full text and
rfc822 format available.
Message #322 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org, mattias.engdegard <at> gmail.com,
> monnier <at> iro.umontreal.ca
> Date: Sun, 17 Dec 2023 12:36:59 +0200
> From: Eli Zaretskii <eliz <at> gnu.org>
>
> Maybe I'm confused, but won't this change have at least _some_ effect
> on Lisp programs? For example, what about this fragment from the
> ELisp manual, which describes the effect of
> make-variable-buffer-local:
>
> A peculiar wrinkle of this feature is that binding the variable
> (with ‘let’ or other binding constructs) does not create a
> buffer-local binding for it. Only setting the variable (with ‘set’
> or ‘setq’), while the variable does not have a ‘let’-style binding
> that was made in the current buffer, does so.
>
> Will this case work the same after the change as it did before?
Also, re this part of the patch:
> + DEFVAR_LISP ("case-fold-search", Vcase_fold_search,
> + doc: /* Non-nil if searches and matches should ignore case. */);
> + Vcase_fold_search = Qt;
> + DEFSYM (Qcase_fold_search, "case-fold-search");
> + Fmake_variable_buffer_local (Qcase_fold_search);
Which value does this set to t -- the global default? Isn't that
different from what we had before?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 10:50:01 GMT)
Full text and
rfc822 format available.
Message #325 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> The messages you linked to discuss problems with _other_ proposed
>> approaches.
>
> Maybe I'm confused, but won't this change have at least _some_ effect
> on Lisp programs? For example, what about this fragment from the
> ELisp manual, which describes the effect of
> make-variable-buffer-local:
>
> A peculiar wrinkle of this feature is that binding the variable
> (with ‘let’ or other binding constructs) does not create a
> buffer-local binding for it. Only setting the variable (with ‘set’
> or ‘setq’), while the variable does not have a ‘let’-style binding
> that was made in the current buffer, does so.
>
> Will this case work the same after the change as it did before?
I read this differently - when you have
(let ((var 'val))
(make-variable-buffer-local 'var))
it will not set buffer-local value to 'val.
You have to set the buffer value explicitly via set/setq outside let
context.
And the patch makes `case-fold-search' buffer-local internally. Nothing
needs to call (make-variable-buffer-local 'case-fold-search).
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 10:54:02 GMT)
Full text and
rfc822 format available.
Message #328 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> Also, re this part of the patch:
>
>> + DEFVAR_LISP ("case-fold-search", Vcase_fold_search,
>> + doc: /* Non-nil if searches and matches should ignore case. */);
>> + Vcase_fold_search = Qt;
>> + DEFSYM (Qcase_fold_search, "case-fold-search");
>> + Fmake_variable_buffer_local (Qcase_fold_search);
>
> Which value does this set to t -- the global default? Isn't that
> different from what we had before?
AFAIU, not different. Previously, we had
> @@ -4785,7 +4779,6 @@ init_buffer_once (void)
> bset_tab_line_format (&buffer_defaults, Qnil);
> bset_abbrev_mode (&buffer_defaults, Qnil);
> bset_overwrite_mode (&buffer_defaults, Qnil);
> - bset_case_fold_search (&buffer_defaults, Qt);
So, the default value was Qt previously as well.
The DEFVAR_LISP + V... + DEFSYM + Fmake_variable_buffer_local is taken
from other buffer-local variables. For example,
DEFVAR_LISP ("display-line-numbers-width", Vdisplay_line_numbers_width,
doc: /* Minimum width of space reserved for line number display.
A positive number means reserve that many columns for line numbers,
even if the actual number needs less space.
The default value of nil means compute the space dynamically.
Any other value is treated as nil. */);
Vdisplay_line_numbers_width = Qnil;
DEFSYM (Qdisplay_line_numbers_width, "display-line-numbers-width");
Fmake_variable_buffer_local (Qdisplay_line_numbers_width);
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 11:02:01 GMT)
Full text and
rfc822 format available.
Message #331 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: monnier <at> iro.umontreal.ca, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org,
> mattias.engdegard <at> gmail.com
> Date: Sun, 17 Dec 2023 10:52:33 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > Maybe I'm confused, but won't this change have at least _some_ effect
> > on Lisp programs? For example, what about this fragment from the
> > ELisp manual, which describes the effect of
> > make-variable-buffer-local:
> >
> > A peculiar wrinkle of this feature is that binding the variable
> > (with ‘let’ or other binding constructs) does not create a
> > buffer-local binding for it. Only setting the variable (with ‘set’
> > or ‘setq’), while the variable does not have a ‘let’-style binding
> > that was made in the current buffer, does so.
> >
> > Will this case work the same after the change as it did before?
>
> I read this differently
Differently from what?
> - when you have
>
> (let ((var 'val))
> (make-variable-buffer-local 'var))
>
> it will not set buffer-local value to 'val.
And before the patch what would have happened?
> And the patch makes `case-fold-search' buffer-local internally. Nothing
> needs to call (make-variable-buffer-local 'case-fold-search).
The patch itself calls make-variable-buffer-local.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 11:07:02 GMT)
Full text and
rfc822 format available.
Message #334 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org, mattias.engdegard <at> gmail.com,
> monnier <at> iro.umontreal.ca
> Date: Sun, 17 Dec 2023 10:56:45 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > Also, re this part of the patch:
> >
> >> + DEFVAR_LISP ("case-fold-search", Vcase_fold_search,
> >> + doc: /* Non-nil if searches and matches should ignore case. */);
> >> + Vcase_fold_search = Qt;
> >> + DEFSYM (Qcase_fold_search, "case-fold-search");
> >> + Fmake_variable_buffer_local (Qcase_fold_search);
> >
> > Which value does this set to t -- the global default? Isn't that
> > different from what we had before?
>
> AFAIU, not different. Previously, we had
>
> > @@ -4785,7 +4779,6 @@ init_buffer_once (void)
> > bset_tab_line_format (&buffer_defaults, Qnil);
> > bset_abbrev_mode (&buffer_defaults, Qnil);
> > bset_overwrite_mode (&buffer_defaults, Qnil);
> > - bset_case_fold_search (&buffer_defaults, Qt);
>
> So, the default value was Qt previously as well.
buffer_defaults is not the default value, AFAIU.
> The DEFVAR_LISP + V... + DEFSYM + Fmake_variable_buffer_local is taken
> from other buffer-local variables. For example,
I'm talking specifically about any changes from previous behavior
visible from Lisp. I think we should test all of the following:
default-value
default-boundp
setq-default
default-toplevel-value
set-default-toplevel-value
and make sure they all behave exactly the same, both in and out of a
let-binding.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 11:17:01 GMT)
Full text and
rfc822 format available.
Message #337 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> AFAIU, not different. Previously, we had
>>
>> > @@ -4785,7 +4779,6 @@ init_buffer_once (void)
>> > bset_tab_line_format (&buffer_defaults, Qnil);
>> > bset_abbrev_mode (&buffer_defaults, Qnil);
>> > bset_overwrite_mode (&buffer_defaults, Qnil);
>> > - bset_case_fold_search (&buffer_defaults, Qt);
>>
>> So, the default value was Qt previously as well.
>
> buffer_defaults is not the default value, AFAIU.
Unless I misunderstand, the comment says that buffer_defaults does hold
the default values.
/* This structure holds the default values of the buffer-local variables
that have special slots in each buffer.
The default value occupies the same slot in this structure
as an individual buffer's value occupies in that buffer.
Setting the default value also goes through the alist of buffers
and stores into each buffer that does not say it has a local value. */
extern struct buffer buffer_defaults;
>> The DEFVAR_LISP + V... + DEFSYM + Fmake_variable_buffer_local is taken
>> from other buffer-local variables. For example,
>
> I'm talking specifically about any changes from previous behavior
> visible from Lisp. I think we should test all of the following:
>
> default-value
> default-boundp
> setq-default
> default-toplevel-value
> set-default-toplevel-value
>
> and make sure they all behave exactly the same, both in and out of a
> let-binding.
The patch did not introduce test failures for make check on my side. Are
there existing tests for buffer-locals? Or do we need to add them?
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 11:24:02 GMT)
Full text and
rfc822 format available.
Message #340 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> > Maybe I'm confused, but won't this change have at least _some_ effect
>> > on Lisp programs? For example, what about this fragment from the
>> > ELisp manual, which describes the effect of
>> > make-variable-buffer-local:
>> >
>> > A peculiar wrinkle of this feature is that binding the variable
>> > (with ‘let’ or other binding constructs) does not create a
>> > buffer-local binding for it. Only setting the variable (with ‘set’
>> > or ‘setq’), while the variable does not have a ‘let’-style binding
>> > that was made in the current buffer, does so.
>> >
>> > Will this case work the same after the change as it did before?
>>
>> I read this differently
>
> Differently from what?
AFAIU, you are concerned that my call to Fmake_variable_buffer_local may
have unexpected side effects because make-variable-buffer-local can be
tricky, as stated in the quoted paragraph.
But it is not how I read this.
Or did you mean something else?
>> - when you have
>>
>> (let ((var 'val))
>> (make-variable-buffer-local 'var))
>>
>> it will not set buffer-local value to 'val.
>
> And before the patch what would have happened?
I did not make changes to `make-variable-buffer-local' in the patch.
Nothing changed there. I just wanted to make sure that we understand
that paragraph the same way - that `make-variable-buffer-local' has
"wrinkle" when called inside let context.
>> And the patch makes `case-fold-search' buffer-local internally. Nothing
>> needs to call (make-variable-buffer-local 'case-fold-search).
>
> The patch itself calls make-variable-buffer-local.
On top level, when Emacs defines all other variables. So, my call to
make-variable-buffer-local is not inside let and thus should not have
any problem with the paragraph you quoted.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 12:07:02 GMT)
Full text and
rfc822 format available.
Message #343 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org, mattias.engdegard <at> gmail.com,
> monnier <at> iro.umontreal.ca
> Date: Sun, 17 Dec 2023 11:19:57 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > I'm talking specifically about any changes from previous behavior
> > visible from Lisp. I think we should test all of the following:
> >
> > default-value
> > default-boundp
> > setq-default
> > default-toplevel-value
> > set-default-toplevel-value
> >
> > and make sure they all behave exactly the same, both in and out of a
> > let-binding.
>
> The patch did not introduce test failures for make check on my side. Are
> there existing tests for buffer-locals? Or do we need to add them?
I don't see any such tests. So yes, it would be good to add them.
But at the very least we should consider all of the above
theoretically and see if any of them are expected/supposed to change
as result of this.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 12:16:02 GMT)
Full text and
rfc822 format available.
Message #346 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: monnier <at> iro.umontreal.ca, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org,
> mattias.engdegard <at> gmail.com
> Date: Sun, 17 Dec 2023 11:26:28 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> > Maybe I'm confused, but won't this change have at least _some_ effect
> >> > on Lisp programs? For example, what about this fragment from the
> >> > ELisp manual, which describes the effect of
> >> > make-variable-buffer-local:
> >> >
> >> > A peculiar wrinkle of this feature is that binding the variable
> >> > (with ‘let’ or other binding constructs) does not create a
> >> > buffer-local binding for it. Only setting the variable (with ‘set’
> >> > or ‘setq’), while the variable does not have a ‘let’-style binding
> >> > that was made in the current buffer, does so.
> >> >
> >> > Will this case work the same after the change as it did before?
> >>
> >> I read this differently
> >
> > Differently from what?
>
> AFAIU, you are concerned that my call to Fmake_variable_buffer_local may
> have unexpected side effects because make-variable-buffer-local can be
> tricky, as stated in the quoted paragraph.
> But it is not how I read this.
>
> Or did you mean something else?
I meant to ask whether the behavior with case-fold-search before your
changes in the context of the above situation differs from its
behavior after the changes.
>
> >> - when you have
> >>
> >> (let ((var 'val))
> >> (make-variable-buffer-local 'var))
> >>
> >> it will not set buffer-local value to 'val.
> >
> > And before the patch what would have happened?
>
> I did not make changes to `make-variable-buffer-local' in the patch.
> Nothing changed there. I just wanted to make sure that we understand
> that paragraph the same way - that `make-variable-buffer-local' has
> "wrinkle" when called inside let context.
>
> >> And the patch makes `case-fold-search' buffer-local internally. Nothing
> >> needs to call (make-variable-buffer-local 'case-fold-search).
> >
> > The patch itself calls make-variable-buffer-local.
>
> On top level, when Emacs defines all other variables. So, my call to
> make-variable-buffer-local is not inside let and thus should not have
> any problem with the paragraph you quoted.
I think this is a misunderstanding: the quote from the ELisp manual
doesn't describe the situation where make-variable-buffer-local is
called inside a let-binding. It is describing the behavior of 'let'
when the variable was made buffer-local via make-variable-buffer-local.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 15:18:01 GMT)
Full text and
rfc822 format available.
Message #349 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> I'm talking specifically about any changes from previous behavior
> visible from Lisp. I think we should test all of the following:
>
> default-value
> default-boundp
> setq-default
> default-toplevel-value
> set-default-toplevel-value
>
> and make sure they all behave exactly the same, both in and out of a
> let-binding.
There should be absolutely no change visible using any combination of
the above function (including with the addition of `let`).
If there is, it's a bug in our C code (I don't know of such bugs, but
given the complexity of the code and its past history of a long litany
of bugs, I definitely won't vouch for it).
Some DEFVAR_PER_BUFFER variables *do* behave differently from normal
DEFVAR_LISP, but these are the "always buffer-local" ones, like
`mode-name`, `buffer-file-name`, etc...
Most of the DEFVAR_PER_BUFFER variables behave just like any
normal variable. They just use a different implementation strategy and
the C code works hard to hide that difference.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 15:25:02 GMT)
Full text and
rfc822 format available.
Message #352 received at 66117 <at> debbugs.gnu.org (full text, mbox):
>> Maybe I'm confused, but won't this change have at least _some_ effect
>> on Lisp programs? For example, what about this fragment from the
>> ELisp manual, which describes the effect of
>> make-variable-buffer-local:
>>
>> A peculiar wrinkle of this feature is that binding the variable
>> (with ‘let’ or other binding constructs) does not create a
>> buffer-local binding for it. Only setting the variable (with ‘set’
>> or ‘setq’), while the variable does not have a ‘let’-style binding
>> that was made in the current buffer, does so.
>>
>> Will this case work the same after the change as it did before?
>
> I read this differently - when you have
>
> (let ((var 'val))
> (make-variable-buffer-local 'var))
>
> it will not set buffer-local value to 'val.
So, what the paragraph above says is that
(setq var val)
will make `var` buffer-local, but
(let ((var val))
...)
will not and neither will
(let ((var val))
...
(setq var val2)
...)
IOW, `setq` makes the var buffer-local only when the assignment affects
the global binding.
The same holds for DEFVAR_PER_BUFFER variables like `case-fold-search`:
(with-temp-buffer
(let ((case-fold-search nil))
(list (local-variable-p 'case-fold-search)
(setq case-fold-search t)
(local-variable-p 'case-fold-search))))
==> (nil t nil)
-- Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sun, 17 Dec 2023 16:04:01 GMT)
Full text and
rfc822 format available.
Message #355 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> Most of the DEFVAR_PER_BUFFER variables behave just like any
> normal variable. They just use a different implementation strategy and
> the C code works hard to hide that difference.
BTW, one reason why the C code works hard to hide that difference is
that we already have enough different kinds of variables making life
complicated for the poor ELisp coders:
- lexical vars
- "normal" vars (`defvar`)
- buffer-local vars (`make-local-variable`)
- automatically buffer-local vars (`make-variable-buffer-local` or DEFVAR_PER_BUFFER)
- always buffer-local vars (DEFVAR_PER_BUFFER plus a -1 value in buffer_local_flags)
- integer-only vars (DEFVAR_INT)
- boolean-only vars (DEFVAR_BOOL)
- aliases (defvaralias)
- so-called "terminal-local" vars which are actually keyboard-local, where
several "terminals" can share the same keyboard (DEFVAR_KBOARD), tho
nowadays this virtually never happens, but makes for extra complexity/bugs.
[ And we used to also have frame-local vars, as well as simultaneously
frame-local and buffer-local vars, but we got rid of those. ]
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 19 Dec 2023 13:22:01 GMT)
Full text and
rfc822 format available.
Message #358 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> > I'm talking specifically about any changes from previous behavior
>> > visible from Lisp. I think we should test all of the following:
>> >
>> > default-value
>> > default-boundp
>> > setq-default
>> > default-toplevel-value
>> > set-default-toplevel-value
>> >
>> > and make sure they all behave exactly the same, both in and out of a
>> > let-binding.
>>
>> The patch did not introduce test failures for make check on my side. Are
>> there existing tests for buffer-locals? Or do we need to add them?
>
> I don't see any such tests. So yes, it would be good to add them.
> But at the very least we should consider all of the above
> theoretically and see if any of them are expected/supposed to change
> as result of this.
Do you still think that we need to discuss special cases further, after
Stefan's reply?
If yes, I will need help with various edge cases, because I am not
really sure about them. For example, I have no clue if below is expected
(defvar test/foo 'value)
(default-boundp 'test/foo)
(default-value 'test/foo) ;; -> value
(let ((test/foo t))
(default-value 'test/foo)) ;; -> t ???
(defvar-local test/foo2 'value)
(default-boundp 'test/foo2) ;; -> t
(default-value 'test/foo2) ;; -> value
(let ((test/foo2 t))
(default-value 'test/foo2)) ;; -> t ???
(setq-default test/foo2 'value2)
(default-boundp 'test/foo2) ;; -> t
(default-value 'test/foo2) ;; -> value2
(let ((test/foo2 t))
(default-value 'test/foo2)) ;; -> t ???
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 19 Dec 2023 13:40:02 GMT)
Full text and
rfc822 format available.
Message #361 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org, mattias.engdegard <at> gmail.com,
> monnier <at> iro.umontreal.ca
> Date: Tue, 19 Dec 2023 13:24:12 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > I don't see any such tests. So yes, it would be good to add them.
> > But at the very least we should consider all of the above
> > theoretically and see if any of them are expected/supposed to change
> > as result of this.
>
> Do you still think that we need to discuss special cases further, after
> Stefan's reply?
Stefan seems to say the problem shouldn't exist, barring bugs. So no,
discussions are not needed, but tests will be welcome
> For example, I have no clue if below is expected
>
> (defvar test/foo 'value)
> (default-boundp 'test/foo)
> (default-value 'test/foo) ;; -> value
> (let ((test/foo t))
> (default-value 'test/foo)) ;; -> t ???
>
> (defvar-local test/foo2 'value)
> (default-boundp 'test/foo2) ;; -> t
> (default-value 'test/foo2) ;; -> value
> (let ((test/foo2 t))
> (default-value 'test/foo2)) ;; -> t ???
> (setq-default test/foo2 'value2)
> (default-boundp 'test/foo2) ;; -> t
> (default-value 'test/foo2) ;; -> value2
> (let ((test/foo2 t))
> (default-value 'test/foo2)) ;; -> t ???
I'll let Stefan answer that.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 20 Dec 2023 20:35:01 GMT)
Full text and
rfc822 format available.
Message #364 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> If yes, I will need help with various edge cases, because I am not
> really sure about them. For example, I have no clue if below is expected
>
> (defvar test/foo 'value)
> (default-boundp 'test/foo)
> (default-value 'test/foo) ;; -> value
Yup.
> (let ((test/foo t))
> (default-value 'test/foo)) ;; -> t ???
Yup.
> (defvar-local test/foo2 'value)
> (default-boundp 'test/foo2) ;; -> t
Yup.
> (default-value 'test/foo2) ;; -> value
Yup.
> (let ((test/foo2 t))
> (default-value 'test/foo2)) ;; -> t ???
Yup.
> (setq-default test/foo2 'value2)
> (default-boundp 'test/foo2) ;; -> t
Yup.
> (default-value 'test/foo2) ;; -> value2
Yup.
> (let ((test/foo2 t))
> (default-value 'test/foo2)) ;; -> t ???
Yup.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 21 Dec 2023 05:57:02 GMT)
Full text and
rfc822 format available.
Message #367 received at submit <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs <at> gnu.org> writes:
>> If yes, I will need help with various edge cases, because I am not
>> really sure about them. For example, I have no clue if below is expected
>>
>> (defvar test/foo 'value)
>> (default-boundp 'test/foo)
>> (default-value 'test/foo) ;; -> value
>
> Yup.
...
BTW, not directly related to this ticket, (set-)default-toplevel-value
might also be interesting to test, with and without buffer-local stuff.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 21 Dec 2023 05:57:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 21 Dec 2023 13:23:02 GMT)
Full text and
rfc822 format available.
Message #373 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> (let ((test/foo2 t))
>> (default-value 'test/foo2)) ;; -> t ???
>
> Yup.
Hmm.. Then, what about
(defvar-local let-tests-buffer-local-var 'value)
(with-temp-buffer
(setq-local let-tests-buffer-local-var 'baz)
(let ((let-tests-buffer-local-var 'bar))
;; This is failing.
(should (eq 'bar (default-value 'let-tests-buffer-local-var)))
(should (eq 'bar let-tests-buffer-local-var))
(with-temp-buffer
(should (eq 'bar (default-value 'let-tests-buffer-local-var)))
(should (eq 'bar let-tests-buffer-local-var)))))
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Thu, 21 Dec 2023 13:41:02 GMT)
Full text and
rfc822 format available.
Message #376 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Ihor Radchenko <yantar92 <at> posteo.net> writes:
> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
>>> (let ((test/foo2 t))
>>> (default-value 'test/foo2)) ;; -> t ???
>>
>> Yup.
>
> Hmm.. Then, what about
>
> (defvar-local let-tests-buffer-local-var 'value)
> (with-temp-buffer
> (setq-local let-tests-buffer-local-var 'baz)
> (let ((let-tests-buffer-local-var 'bar))
> ;; This is failing.
> (should (eq 'bar (default-value 'let-tests-buffer-local-var)))
C-h f default-value explains this:
Return SYMBOL’s default value.
This is the value that is seen in buffers that do not have their own values
for this variable.
> (should (eq 'bar let-tests-buffer-local-var))
> (with-temp-buffer
> (should (eq 'bar (default-value 'let-tests-buffer-local-var)))
> (should (eq 'bar let-tests-buffer-local-var)))))
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 23 Dec 2023 08:55:02 GMT)
Full text and
rfc822 format available.
Message #379 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Sun, 08 Oct 2023 09:00:49 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > My advice is to have a prototype working, then time it on local
> > filesystems.
>
> See the attached patch.
What do we want to do with this? any reasons not to install this on
master (and close this bug then)?
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 23 Dec 2023 09:39:02 GMT)
Full text and
rfc822 format available.
Message #382 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> > My advice is to have a prototype working, then time it on local
>> > filesystems.
>>
>> See the attached patch.
>
> What do we want to do with this? any reasons not to install this on
> master (and close this bug then)?
I thought that it is already installed...
I do not think that we should close the bug even after installing the
patch for `find-buffer-visiting'.
What I had in mind wrt improving `find-buffer-visiting' performance is:
1. Improve the Elisp part (this patch)
2. Improve case-fold-search binding, which will give another speedup
(another patch I submitted)
3. Optionally add tests
4. Address the scenario Dmitry described in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#91
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 23 Dec 2023 11:18:01 GMT)
Full text and
rfc822 format available.
Message #385 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, dmitry <at> gutov.dev,
> 66117 <at> debbugs.gnu.org
> Date: Sat, 23 Dec 2023 09:41:44 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> > My advice is to have a prototype working, then time it on local
> >> > filesystems.
> >>
> >> See the attached patch.
> >
> > What do we want to do with this? any reasons not to install this on
> > master (and close this bug then)?
>
> I thought that it is already installed...
The find-buffer part? I don't see it, did I miss something?
> I do not think that we should close the bug even after installing the
> patch for `find-buffer-visiting'.
>
> What I had in mind wrt improving `find-buffer-visiting' performance is:
>
> 1. Improve the Elisp part (this patch)
Let's install it.
> 2. Improve case-fold-search binding, which will give another speedup
> (another patch I submitted)
It should be installed, after you add the tests we've been discussing.
> 3. Optionally add tests
I hope the tests will not be optional...
> 4. Address the scenario Dmitry described in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#91
That could be a separate bug report. This one is already too long,
and includes too many different issues.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 23 Dec 2023 11:26:01 GMT)
Full text and
rfc822 format available.
Message #388 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> I thought that it is already installed...
>
> The find-buffer part? I don't see it, did I miss something?
No. I just had a problem with my local git repo. git reset fixed it and
revealed that the patch is indeed not installed yet.
>> 2. Improve case-fold-search binding, which will give another speedup
>> (another patch I submitted)
>
> It should be installed, after you add the tests we've been discussing.
>
>> 3. Optionally add tests
>
> I hope the tests will not be optional...
Didn't you say earlier that tests are not strictly necessary in
<https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#361>?
I do not mind adding tests, but it will not be as fast - I need help
with them.
>> 4. Address the scenario Dmitry described in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#91
>
> That could be a separate bug report. This one is already too long,
> and includes too many different issues.
Ok. I will create a separate bug report after the two submitted patches
are handled.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 23 Dec 2023 11:52:01 GMT)
Full text and
rfc822 format available.
Message #391 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: monnier <at> iro.umontreal.ca, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Sat, 23 Dec 2023 11:28:33 +0000
>
> >> 3. Optionally add tests
> >
> > I hope the tests will not be optional...
>
> Didn't you say earlier that tests are not strictly necessary in
> <https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#361>?
>
> I do not mind adding tests, but it will not be as fast - I need help
> with them.
We could install just a few now, and the rest afterwards. You have
already presented enough non-trivial cases that can be easily
converted to tests, and I think it is a good idea to do that when we
change such low-level and veteran code.
So yes, it is not strictly necessary, but highly desirable.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 23 Dec 2023 14:28:02 GMT)
Full text and
rfc822 format available.
Message #394 received at 66117 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:
>> I do not mind adding tests, but it will not be as fast - I need help
>> with them.
>
> We could install just a few now, and the rest afterwards. You have
> already presented enough non-trivial cases that can be easily
> converted to tests, and I think it is a good idea to do that when we
> change such low-level and veteran code.
See the attached.
I am not sure if these few cases are enough to warrant a separate commit
though.
[0001-Add-tests-for-let-bug-66117.patch (text/x-patch, inline)]
From 147ae00df849f04cdb00e09d8079533fa5dea98a Mon Sep 17 00:00:00 2001
Message-ID: <147ae00df849f04cdb00e09d8079533fa5dea98a.1703341788.git.yantar92 <at> posteo.net>
From: Ihor Radchenko <yantar92 <at> posteo.net>
Date: Sat, 23 Dec 2023 15:29:36 +0100
Subject: [PATCH] Add tests for `let' (bug#66117)
* test/src/eval-tests.el (eval-tests/default-value): New test.
---
test/src/eval-tests.el | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/test/src/eval-tests.el b/test/src/eval-tests.el
index 4589763b2f5..c1219591e40 100644
--- a/test/src/eval-tests.el
+++ b/test/src/eval-tests.el
@@ -282,4 +282,25 @@ eval-tests-defvaralias
(should-error (defvaralias 'eval-tests--my-c 'eval-tests--my-d)
:type 'cyclic-variable-indirection))
+(defvar eval-tests/global-var 'value)
+(defvar-local eval-tests/buffer-local-var 'value)
+(ert-deftest eval-tests/default-value ()
+ ;; `let' overrides the default value for global variables.
+ (should (default-boundp 'eval-tests/global-var))
+ (should (eq 'value (default-value 'eval-tests/global-var)))
+ (should (eq 'value eval-tests/global-var))
+ (let ((eval-tests/global-var 'bar))
+ (should (eq 'bar (default-value 'eval-tests/global-var)))
+ (should (eq 'bar eval-tests/global-var)))
+ ;; `let' overrides the default value everywhere, but leaves
+ ;; buffer-local values unchanged in current buffer and in the
+ ;; buffers where there is no explicitly set buffer-local value.
+ (should (default-boundp 'eval-tests/buffer-local-var))
+ (should (eq 'value (default-value 'eval-tests/buffer-local-var)))
+ (should (eq 'value eval-tests/buffer-local-var))
+ (with-temp-buffer
+ (let ((eval-tests/buffer-local-var 'bar))
+ (should (eq 'bar (default-value 'eval-tests/buffer-local-var)))
+ (should (eq 'bar eval-tests/buffer-local-var)))))
+
;;; eval-tests.el ends here
--
2.42.0
[Message part 3 (text/plain, inline)]
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 29 Dec 2023 07:48:02 GMT)
Full text and
rfc822 format available.
Message #397 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: monnier <at> iro.umontreal.ca, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Sat, 23 Dec 2023 11:28:33 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> I thought that it is already installed...
> >
> > The find-buffer part? I don't see it, did I miss something?
>
> No. I just had a problem with my local git repo. git reset fixed it and
> revealed that the patch is indeed not installed yet.
Is that patch still valid, or do you need to rebase it? Would you
like to repost it?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Fri, 29 Dec 2023 13:53:02 GMT)
Full text and
rfc822 format available.
Message #400 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> No. I just had a problem with my local git repo. git reset fixed it and
>> revealed that the patch is indeed not installed yet.
>
> Is that patch still valid, or do you need to rebase it? Would you
> like to repost it?
All the three patches I submitted in this thread are cleanly applying
onto freshly cloned master.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 30 Dec 2023 07:40:02 GMT)
Full text and
rfc822 format available.
Message #403 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: monnier <at> iro.umontreal.ca, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Sat, 23 Dec 2023 14:30:42 +0000
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> I do not mind adding tests, but it will not be as fast - I need help
> >> with them.
> >
> > We could install just a few now, and the rest afterwards. You have
> > already presented enough non-trivial cases that can be easily
> > converted to tests, and I think it is a good idea to do that when we
> > change such low-level and veteran code.
>
> See the attached.
> I am not sure if these few cases are enough to warrant a separate commit
> though.
I installed it, thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 30 Dec 2023 08:10:01 GMT)
Full text and
rfc822 format available.
Message #406 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: monnier <at> iro.umontreal.ca, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Fri, 29 Dec 2023 13:55:45 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> No. I just had a problem with my local git repo. git reset fixed it and
> >> revealed that the patch is indeed not installed yet.
> >
> > Is that patch still valid, or do you need to rebase it? Would you
> > like to repost it?
>
> All the three patches I submitted in this thread are cleanly applying
> onto freshly cloned master.
Thanks, I installed them.
Should we now close this bug?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 30 Dec 2023 09:51:02 GMT)
Full text and
rfc822 format available.
Message #409 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
> Date: Sat, 30 Dec 2023 10:08:34 +0200
> From: Eli Zaretskii <eliz <at> gnu.org>
>
> > From: Ihor Radchenko <yantar92 <at> posteo.net>
> > Cc: monnier <at> iro.umontreal.ca, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> > Date: Fri, 29 Dec 2023 13:55:45 +0000
> >
> > Eli Zaretskii <eliz <at> gnu.org> writes:
> >
> > >> No. I just had a problem with my local git repo. git reset fixed it and
> > >> revealed that the patch is indeed not installed yet.
> > >
> > > Is that patch still valid, or do you need to rebase it? Would you
> > > like to repost it?
> >
> > All the three patches I submitted in this thread are cleanly applying
> > onto freshly cloned master.
>
> Thanks, I installed them.
>
> Should we now close this bug?
Ehmm... I see that test/src/buffer-tests now have 5 failures that
were not there before:
5 unexpected results:
FAILED test-buffer-modifications
FAILED test-kill-buffer-auto-save-default
FAILED test-kill-buffer-auto-save-delete-no
FAILED test-kill-buffer-auto-save-delete-yes
FAILED test-restore-buffer-modified-p
Also, 7 of the lisp/files-test fail:
7 unexpected results:
FAILED files-tests-bug-18141
FAILED files-tests-file-name-non-special-make-auto-save-file-name
FAILED files-tests-file-name-non-special-set-visited-file-modtime
FAILED files-tests-no-file-write-contents
FAILED files-tests-revert-buffer
FAILED files-tests-revert-buffer-with-fine-grain
FAILED files-tests-zzdont-rewrite-precious-files
Could you please look into these? (Interestingly, those 5+7 don't
fail for me on MS-Windows, only on GNU/Linux...)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 30 Dec 2023 10:11:01 GMT)
Full text and
rfc822 format available.
Message #412 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
> Date: Sat, 30 Dec 2023 11:50:02 +0200
> From: Eli Zaretskii <eliz <at> gnu.org>
>
> > Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
> > Date: Sat, 30 Dec 2023 10:08:34 +0200
> > From: Eli Zaretskii <eliz <at> gnu.org>
> >
> > > From: Ihor Radchenko <yantar92 <at> posteo.net>
> > > Cc: monnier <at> iro.umontreal.ca, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> > > Date: Fri, 29 Dec 2023 13:55:45 +0000
> > >
> > > Eli Zaretskii <eliz <at> gnu.org> writes:
> > >
> > > >> No. I just had a problem with my local git repo. git reset fixed it and
> > > >> revealed that the patch is indeed not installed yet.
> > > >
> > > > Is that patch still valid, or do you need to rebase it? Would you
> > > > like to repost it?
> > >
> > > All the three patches I submitted in this thread are cleanly applying
> > > onto freshly cloned master.
> >
> > Thanks, I installed them.
> >
> > Should we now close this bug?
>
> Ehmm... I see that test/src/buffer-tests now have 5 failures that
> were not there before:
>
> 5 unexpected results:
> FAILED test-buffer-modifications
> FAILED test-kill-buffer-auto-save-default
> FAILED test-kill-buffer-auto-save-delete-no
> FAILED test-kill-buffer-auto-save-delete-yes
> FAILED test-restore-buffer-modified-p
>
> Also, 7 of the lisp/files-test fail:
>
> 7 unexpected results:
> FAILED files-tests-bug-18141
> FAILED files-tests-file-name-non-special-make-auto-save-file-name
> FAILED files-tests-file-name-non-special-set-visited-file-modtime
> FAILED files-tests-no-file-write-contents
> FAILED files-tests-revert-buffer
> FAILED files-tests-revert-buffer-with-fine-grain
> FAILED files-tests-zzdont-rewrite-precious-files
>
> Could you please look into these? (Interestingly, those 5+7 don't
> fail for me on MS-Windows, only on GNU/Linux...)
I think at least part of the problem is in this fragment from
find-file-noselect:
;; Find any buffer for a file that has same truename.
(other (and (not buf)
(find-buffer-visiting
filename
;; We want to filter out buffers that we've
;; visited via symlinks and the like, where
;; the symlink no longer exists.
(lambda (buffer)
(let ((file (buffer-local-value
'buffer-file-name buffer)))
(and file (file-exists-p file))))))))
This call to find-buffer-visiting fails because file-exists-p is
called with a nil argument:
Test test-buffer-modifications backtrace:
file-exists-p(nil)
find-buffer-visiting("/tmp/emacs-test-MzitxT-buffer" #f(compiled-fun
find-file-noselect("/tmp/emacs-test-MzitxT-buffer" nil nil nil)
find-file("/tmp/emacs-test-MzitxT-buffer")
Please attend to this as soon as you can, because I think this same
problem will break many other use cases.
(I guess this doesn't happen on Windows because the involved
files/directories are not symlinks.)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 30 Dec 2023 10:37:02 GMT)
Full text and
rfc822 format available.
Message #415 received at 66117 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:
>> Ehmm... I see that test/src/buffer-tests now have 5 failures that
>> were not there before:
>>
>> 5 unexpected results:
>> FAILED test-buffer-modifications
>> FAILED test-kill-buffer-auto-save-default
>> FAILED test-kill-buffer-auto-save-delete-no
>> FAILED test-kill-buffer-auto-save-delete-yes
>> FAILED test-restore-buffer-modified-p
>>
>> Also, 7 of the lisp/files-test fail:
>>
>> 7 unexpected results:
>> FAILED files-tests-bug-18141
>> FAILED files-tests-file-name-non-special-make-auto-save-file-name
>> FAILED files-tests-file-name-non-special-set-visited-file-modtime
>> FAILED files-tests-no-file-write-contents
>> FAILED files-tests-revert-buffer
>> FAILED files-tests-revert-buffer-with-fine-grain
>> FAILED files-tests-zzdont-rewrite-precious-files
>>
>> Could you please look into these? (Interestingly, those 5+7 don't
>> fail for me on MS-Windows, only on GNU/Linux...)
FYI, all the tests are passing on my side (GNU/Linux):
cd test
make src/buffer-tests
Ran 406 tests, 406 results as expected, 0 unexpected (2023-12-30 11:24:04+0100, 0.464788 sec)
> I think at least part of the problem is in this fragment from
> find-file-noselect:
>
> ;; Find any buffer for a file that has same truename.
> (other (and (not buf)
> (find-buffer-visiting
> filename
> ;; We want to filter out buffers that we've
> ;; visited via symlinks and the like, where
> ;; the symlink no longer exists.
> (lambda (buffer)
> (let ((file (buffer-local-value
> 'buffer-file-name buffer)))
> (and file (file-exists-p file))))))))
Not this one - (and file ...) will never trigger when file is nil.
> Test test-buffer-modifications backtrace:
> file-exists-p(nil)
> find-buffer-visiting("/tmp/emacs-test-MzitxT-buffer" #f(compiled-fun
> find-file-noselect("/tmp/emacs-test-MzitxT-buffer" nil nil nil)
> find-file("/tmp/emacs-test-MzitxT-buffer")
>
> Please attend to this as soon as you can, because I think this same
> problem will break many other use cases.
I think I found the problem - it is rather silly.
See the attached patch.
[0001-find-buffer-visiting-Fix-test-breakage-introduced-in.patch (text/x-patch, inline)]
From 0edc966cd7e2b9c2ce3af44a4a72e6f467eaf713 Mon Sep 17 00:00:00 2001
Message-ID: <0edc966cd7e2b9c2ce3af44a4a72e6f467eaf713.1703932575.git.yantar92 <at> posteo.net>
From: Ihor Radchenko <yantar92 <at> posteo.net>
Date: Sat, 30 Dec 2023 11:31:51 +0100
Subject: [PATCH] find-buffer-visiting: Fix test breakage introduced in
b7a737ef49
* lisp/files.el (find-buffer-visiting): Fix code branch checking for
buffers referring to the same file number. We should check the found
buffer with the file number, not current.
Link: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#412
---
lisp/files.el | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/lisp/files.el b/lisp/files.el
index 315ba831e8..78e2bca3cf 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -2220,15 +2220,17 @@ find-buffer-visiting
(number (file-attribute-file-identifier attributes)))
(and buffer-file-numbers-unique
(car-safe number) ;Make sure the inode is not just nil.
- (let ((buf (find-buffer 'buffer-file-number number)))
- (when (and buf (buffer-local-value 'buffer-file-name buf)
+ (let* ((buf (find-buffer 'buffer-file-number number))
+ (buf-file-name (and buf (buffer-local-value 'buffer-file-name buf))))
+ (when (and buf-file-name
;; Verify this buffer's file number
;; still belongs to its file.
- (file-exists-p buffer-file-name)
- (equal (file-attributes buffer-file-truename)
- attributes)
+ (file-exists-p buf-file-name)
+ (equal
+ (file-attributes (buffer-local-value 'buffer-file-truename buf))
+ attributes)
(or (not predicate)
- (funcall predicate (current-buffer))))
+ (funcall predicate buf)))
buf))))))))
--
2.42.0
[Message part 3 (text/plain, inline)]
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 30 Dec 2023 11:08:01 GMT)
Full text and
rfc822 format available.
Message #418 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
> Date: Sat, 30 Dec 2023 10:39:49 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > I think at least part of the problem is in this fragment from
> > find-file-noselect:
> >
> > ;; Find any buffer for a file that has same truename.
> > (other (and (not buf)
> > (find-buffer-visiting
> > filename
> > ;; We want to filter out buffers that we've
> > ;; visited via symlinks and the like, where
> > ;; the symlink no longer exists.
> > (lambda (buffer)
> > (let ((file (buffer-local-value
> > 'buffer-file-name buffer)))
> > (and file (file-exists-p file))))))))
>
> Not this one - (and file ...) will never trigger when file is nil.
Granted, that call to file-exists-p was not what I meant.
> I think I found the problem - it is rather silly.
> See the attached patch.
Thanks, installed.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Sat, 30 Dec 2023 12:49:01 GMT)
Full text and
rfc822 format available.
Message #421 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> All the three patches I submitted in this thread are cleanly applying
>> onto freshly cloned master.
>
> Thanks, I installed them.
Thanks!
> Should we now close this bug?
Yes, I think. I hope that closing the bug will not make question to
Stefan (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#373) fall
through the cracks.
--
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>
Reply sent
to
Eli Zaretskii <eliz <at> gnu.org>:
You have taken responsibility.
(Sat, 30 Dec 2023 13:25:01 GMT)
Full text and
rfc822 format available.
Notification sent
to
Ihor Radchenko <yantar92 <at> posteo.net>:
bug acknowledged by developer.
(Sat, 30 Dec 2023 13:25:02 GMT)
Full text and
rfc822 format available.
Message #426 received at 66117-done <at> debbugs.gnu.org (full text, mbox):
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: monnier <at> iro.umontreal.ca, dmitry <at> gutov.dev, 66117 <at> debbugs.gnu.org
> Date: Sat, 30 Dec 2023 12:51:09 +0000
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > Should we now close this bug?
>
> Yes, I think. I hope that closing the bug will not make question to
> Stefan (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#373) fall
> through the cracks.
Thanks, closing.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Mon, 01 Jan 2024 17:12:01 GMT)
Full text and
rfc822 format available.
Message #429 received at 66117 <at> debbugs.gnu.org (full text, mbox):
> Yes, I think. I hope that closing the bug will not make question to
> Stefan (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66117#373) fall
> through the cracks.
I thought Gerd replied to that one.
If there's still something unclear, let me know.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Mon, 01 Jan 2024 18:00:02 GMT)
Full text and
rfc822 format available.
Message #432 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>> (defvar-local let-tests-buffer-local-var 'value)
>> (with-temp-buffer
>> (setq-local let-tests-buffer-local-var 'baz)
>> (let ((let-tests-buffer-local-var 'bar))
>> ;; This is failing.
>> (should (eq 'bar (default-value 'let-tests-buffer-local-var)))
>
> C-h f default-value explains this:
>
> Return SYMBOL’s default value.
> This is the value that is seen in buffers that do not have their own values
> for this variable.
Are you saying that `let' never changes `default-value' output?
Unfortunately, no.
I first expected (by reading the C sources) that the default value is
changed by `let', but it actually looks like `let' sometimes does change
the default value and sometimes does not.
See the following two almost identical snippets that yield completely
different results depending on the present of `setq-default':
(defvar-local let-tests-buffer-local-var 'value) ;; default value is 'value
(with-temp-buffer
(let ((existing-buffer (current-buffer)))
(with-temp-buffer
(setq-local let-tests-buffer-local-var 'baz)
(let ((let-tests-buffer-local-var 'bar))
;; Let does not change the default value for variable.
(should (eq 'value (default-value 'let-tests-buffer-local-var)))
(should (eq 'bar let-tests-buffer-local-var))
(with-temp-buffer
;; We are in a new buffer - `let-tests-buffer-local-var' has its global default value.
(should (eq 'value (default-value 'let-tests-buffer-local-var)))
(should (eq 'value let-tests-buffer-local-var)))
(with-current-buffer existing-buffer
;; The above let DOES NOT change the current value in all the buffer
;; _existing_ when `let' is invoked.
(should (eq 'value let-tests-buffer-local-var))
;; Global default value is still unchanged.
(should (eq 'value (default-value 'let-tests-buffer-local-var))))))))
(with-temp-buffer
(let ((existing-buffer (current-buffer)))
(with-temp-buffer
;; !!! After commenting out this line, the behaviour changes.
;; (setq-local let-tests-buffer-local-var 'baz)
(let ((let-tests-buffer-local-var 'bar))
;; Now, let DOES change the default value for variable.
(should (eq 'bar (default-value 'let-tests-buffer-local-var)))
(should (eq 'bar let-tests-buffer-local-var))
(with-temp-buffer
;; We are in a new buffer - `let-tests-buffer-local-var' still has
;; let-bound global and local values.
(should (eq 'bar (default-value 'let-tests-buffer-local-var)))
(should (eq 'bar let-tests-buffer-local-var)))
(with-current-buffer existing-buffer
;; The above let DOES change the current value in all the buffer
;; _existing_ when `let' is invoked.
(should (eq 'bar let-tests-buffer-local-var))
;; Global default value is still let-bound.
(should (eq 'bar (default-value 'let-tests-buffer-local-var))))))))
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Mon, 01 Jan 2024 19:40:02 GMT)
Full text and
rfc822 format available.
Message #435 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Ihor Radchenko <yantar92 <at> posteo.net> writes:
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>
>>> (defvar-local let-tests-buffer-local-var 'value)
>>> (with-temp-buffer
>>> (setq-local let-tests-buffer-local-var 'baz)
>>> (let ((let-tests-buffer-local-var 'bar))
>>> ;; This is failing.
>>> (should (eq 'bar (default-value 'let-tests-buffer-local-var)))
>>
>> C-h f default-value explains this:
>>
>> Return SYMBOL’s default value.
>> This is the value that is seen in buffers that do not have their own values
>> for this variable.
>
> Are you saying that `let' never changes `default-value' output?
Not really. I tried to explain why default-value, in this case, returns
what it returns.
> Unfortunately, no.
Maybe you are seeing something like bug#65209? Or the fix done by Stefan
Monnier in the course of that bug? (See the mails under the bug.)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Mon, 01 Jan 2024 20:37:01 GMT)
Full text and
rfc822 format available.
Message #438 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>>>> (defvar-local let-tests-buffer-local-var 'value)
>>>> (with-temp-buffer
>>>> (setq-local let-tests-buffer-local-var 'baz)
>>>> (let ((let-tests-buffer-local-var 'bar))
>>>> ;; This is failing.
>>>> (should (eq 'bar (default-value 'let-tests-buffer-local-var)))
>>>
>>> C-h f default-value explains this:
>>>
>>> Return SYMBOL’s default value.
>>> This is the value that is seen in buffers that do not have their own values
>>> for this variable.
>>
>> Are you saying that `let' never changes `default-value' output?
>
> Not really. I tried to explain why default-value, in this case, returns
> what it returns.
I am sorry, but I do not see how the docstring explains what is
happening there. Because it is not clear what `let' does with the
default value.
>> Unfortunately, no.
>
> Maybe you are seeing something like bug#65209? Or the fix done by Stefan
> Monnier in the course of that bug? (See the mails under the bug.)
I am on the latest master.
And I do not think that it is bug#65209.
Let me simplify my example to avoid `setq-local' inside `let'.
AFAIU, what `let' does to the default value depends on whether current
buffer has its own local value of the variable or not.
(defvar-local let-tests-buffer-local-var2 'value) ;; default value is 'value
(with-temp-buffer
(setq-local let-tests-buffer-local-var2 'baz)
(let ((let-tests-buffer-local-var2 'bar))
;; Let does not change the default value for variable.
(should (eq 'value (default-value 'let-tests-buffer-local-var2)))
(should (eq 'bar let-tests-buffer-local-var2))
(with-temp-buffer
;; We are in a new buffer - `let-tests-buffer-local-var2' has its global default value.
(should (eq 'value (default-value 'let-tests-buffer-local-var2)))
(should (eq 'value let-tests-buffer-local-var2)))))
(with-temp-buffer
;; !!! After commenting out this line, the behaviour changes.
;; (setq-local let-tests-buffer-local-var2 'baz)
(let ((let-tests-buffer-local-var2 'bar))
;; Now, let DOES change the default value for variable.
(should (eq 'bar (default-value 'let-tests-buffer-local-var2)))
(should (eq 'bar let-tests-buffer-local-var2))
(with-temp-buffer
;; We are in a new buffer - `let-tests-buffer-local-var2' still has
;; let-bound global and local values.
(should (eq 'bar (default-value 'let-tests-buffer-local-var2)))
(should (eq 'bar let-tests-buffer-local-var2)))))
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 01:31:01 GMT)
Full text and
rfc822 format available.
Message #441 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Ihor Radchenko [2024-01-01 18:02:30] wrote:
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>
>>> (defvar-local let-tests-buffer-local-var 'value)
>>> (with-temp-buffer
>>> (setq-local let-tests-buffer-local-var 'baz)
>>> (let ((let-tests-buffer-local-var 'bar))
>>> ;; This is failing.
>>> (should (eq 'bar (default-value 'let-tests-buffer-local-var)))
>>
>> C-h f default-value explains this:
>>
>> Return SYMBOL’s default value.
>> This is the value that is seen in buffers that do not have their own values
>> for this variable.
>
> Are you saying that `let' never changes `default-value' output?
> Unfortunately, no.
Indeed, see the doc that Eli quoted 10 posts "ago":
[...] ELisp manual, which describes the effect of
make-variable-buffer-local:
A peculiar wrinkle of this feature is that binding the variable
(with ‘let’ or other binding constructs) does not create
a buffer-local binding for it. Only setting the variable (with
‘set’ or ‘setq’), while the variable does not have a ‘let’-style
binding that was made in the current buffer, does so.
IOW, `let` changes the binding that is "current": if the variable is
buffer-local in the current buffer it changes that buffer-local value
and otherwise it changes the global value.
[ This for "automatically buffer-local variables". ]
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 04:44:02 GMT)
Full text and
rfc822 format available.
Message #444 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Ihor Radchenko <yantar92 <at> posteo.net> writes:
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>
>>>>> (defvar-local let-tests-buffer-local-var 'value)
>>>>> (with-temp-buffer
>>>>> (setq-local let-tests-buffer-local-var 'baz)
>>>>> (let ((let-tests-buffer-local-var 'bar))
>>>>> ;; This is failing.
>>>>> (should (eq 'bar (default-value 'let-tests-buffer-local-var)))
>>>>
>>>> C-h f default-value explains this:
>>>>
>>>> Return SYMBOL’s default value.
>>>> This is the value that is seen in buffers that do not have their own values
>>>> for this variable.
>>>
>>> Are you saying that `let' never changes `default-value' output?
>>
>> Not really. I tried to explain why default-value, in this case, returns
>> what it returns.
>
> I am sorry, but I do not see how the docstring explains what is
> happening there. Because it is not clear what `let' does with the
> default value.
Sorry for not being clear. What I wanted to hint at is the part of the
doc string that reads "that do not have their own values...", which
means that what default-value returns depends whether or not SYMBOL got
its own value in the buffer, and that depends on the presence of lets,
and so on.
It's (too) complicated, but it is what it is :-/.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 10:50:01 GMT)
Full text and
rfc822 format available.
Message #447 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>>>>> C-h f default-value explains this:
>>>>>
>>>>> Return SYMBOL’s default value.
>>>>> This is the value that is seen in buffers that do not have their own values
>>>>> for this variable.
>>>>
>> ...
>> I am sorry, but I do not see how the docstring explains what is
>> happening there. Because it is not clear what `let' does with the
>> default value.
>
> Sorry for not being clear. What I wanted to hint at is the part of the
> doc string that reads "that do not have their own values...", which
> means that what default-value returns depends whether or not SYMBOL got
> its own value in the buffer, and that depends on the presence of lets,
> and so on.
It is not how I read this. AFAIU, `default-value' returns the value that
is used for buffers where the value is not explicitly set. Calling
`default-value' from inside the buffer where the value is set does not
change that global default value and should not change what
`default-value' returns.
And indeed
(defvar-local yant/test 'default-value)
(with-temp-buffer
(setq yant/test 'local-value)
(default-value 'yant/test)) ; => 'default-value
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 10:58:02 GMT)
Full text and
rfc822 format available.
Message #450 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>>> (defvar-local let-tests-buffer-local-var 'value)
>>>> (with-temp-buffer
>>>> (setq-local let-tests-buffer-local-var 'baz)
>>>> (let ((let-tests-buffer-local-var 'bar))
>>>> ;; This is failing.
>>>> (should (eq 'bar (default-value 'let-tests-buffer-local-var)))
>> ...
>> Are you saying that `let' never changes `default-value' output?
>> Unfortunately, no.
>
> Indeed, see the doc that Eli quoted 10 posts "ago":
>
> [...] ELisp manual, which describes the effect of
> make-variable-buffer-local:
>
> A peculiar wrinkle of this feature is that binding the variable
> (with ‘let’ or other binding constructs) does not create
> a buffer-local binding for it. Only setting the variable (with
> ‘set’ or ‘setq’), while the variable does not have a ‘let’-style
> binding that was made in the current buffer, does so.
>
> IOW, `let` changes the binding that is "current": if the variable is
> buffer-local in the current buffer it changes that buffer-local value
> and otherwise it changes the global value.
> [ This for "automatically buffer-local variables". ]
What you say is indeed what I observe, but I do not see how it follows
from the quoted manual text. When I read the above part of the manual, I
see a warning about using `set'/`setq' inside `let' that binds the same
variable.
I think that the caveat about binding buffer-local variables should be
documented.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 11:06:02 GMT)
Full text and
rfc822 format available.
Message #453 received at 66117 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Ihor Radchenko <yantar92 <at> posteo.net> writes:
> See the following two almost identical snippets that yield completely
> different results depending on the present of `setq-default':
> ...
I believe that I now understand enough to add a new meaningful test.
See the attached. Please check the comments in the test to make sure
that they make sense.
[0001-test-src-eval-tests.el-eval-tests-default-value-Add-.patch (text/x-patch, inline)]
From dfc6dbcd844fbd8b8634b056fb441848b86311ca Mon Sep 17 00:00:00 2001
Message-ID: <dfc6dbcd844fbd8b8634b056fb441848b86311ca.1704193627.git.yantar92 <at> posteo.net>
From: Ihor Radchenko <yantar92 <at> posteo.net>
Date: Tue, 2 Jan 2024 12:06:16 +0100
Subject: [PATCH] test/src/eval-tests.el (eval-tests/default-value): Add new
test case
---
test/src/eval-tests.el | 37 +++++++++++++++++++++++++------------
1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/test/src/eval-tests.el b/test/src/eval-tests.el
index c1219591e40..5cf9d4e2188 100644
--- a/test/src/eval-tests.el
+++ b/test/src/eval-tests.el
@@ -282,25 +282,38 @@ eval-tests-defvaralias
(should-error (defvaralias 'eval-tests--my-c 'eval-tests--my-d)
:type 'cyclic-variable-indirection))
-(defvar eval-tests/global-var 'value)
-(defvar-local eval-tests/buffer-local-var 'value)
+(defvar eval-tests/global-var 'global-value)
+(defvar-local eval-tests/buffer-local-var 'default-value)
(ert-deftest eval-tests/default-value ()
;; `let' overrides the default value for global variables.
(should (default-boundp 'eval-tests/global-var))
- (should (eq 'value (default-value 'eval-tests/global-var)))
- (should (eq 'value eval-tests/global-var))
- (let ((eval-tests/global-var 'bar))
- (should (eq 'bar (default-value 'eval-tests/global-var)))
- (should (eq 'bar eval-tests/global-var)))
+ (should (eq 'global-value (default-value 'eval-tests/global-var)))
+ (should (eq 'global-value eval-tests/global-var))
+ (let ((eval-tests/global-var 'let-value))
+ (should (eq 'let-value (default-value 'eval-tests/global-var)))
+ (should (eq 'let-value eval-tests/global-var)))
;; `let' overrides the default value everywhere, but leaves
;; buffer-local values unchanged in current buffer and in the
;; buffers where there is no explicitly set buffer-local value.
(should (default-boundp 'eval-tests/buffer-local-var))
- (should (eq 'value (default-value 'eval-tests/buffer-local-var)))
- (should (eq 'value eval-tests/buffer-local-var))
+ (should (eq 'default-value (default-value 'eval-tests/buffer-local-var)))
+ (should (eq 'default-value eval-tests/buffer-local-var))
(with-temp-buffer
- (let ((eval-tests/buffer-local-var 'bar))
- (should (eq 'bar (default-value 'eval-tests/buffer-local-var)))
- (should (eq 'bar eval-tests/buffer-local-var)))))
+ (let ((eval-tests/buffer-local-var 'let-value))
+ (should (eq 'let-value (default-value 'eval-tests/buffer-local-var)))
+ (should (eq 'let-value eval-tests/buffer-local-var))))
+ ;; When current buffer has explicit buffer-local binding, `let' does
+ ;; not alter the default binding.
+ (with-temp-buffer
+ (setq-local eval-tests/buffer-local-var 'local-value)
+ (let ((eval-tests/buffer-local-var 'let-value))
+ ;; Let in a buffer with local binding does not change the
+ ;; default value for variable.
+ (should (eq 'default-value (default-value 'eval-tests/buffer-local-var)))
+ (should (eq 'let-value eval-tests/buffer-local-var))
+ (with-temp-buffer
+ ;; We are in a new buffer - `eval-tests/buffer-local-var' has its global default value.
+ (should (eq 'default-value (default-value 'eval-tests/buffer-local-var)))
+ (should (eq 'default-value eval-tests/buffer-local-var))))))
;;; eval-tests.el ends here
--
2.42.0
[Message part 3 (text/plain, inline)]
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 11:09:01 GMT)
Full text and
rfc822 format available.
Message #456 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Ihor Radchenko <yantar92 <at> posteo.net> writes:
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>
>>>>>> C-h f default-value explains this:
>>>>>>
>>>>>> Return SYMBOL’s default value.
>>>>>> This is the value that is seen in buffers that do not have their own values
>>>>>> for this variable.
>>>>>
>>> ...
>>> I am sorry, but I do not see how the docstring explains what is
>>> happening there. Because it is not clear what `let' does with the
>>> default value.
>>
>> Sorry for not being clear. What I wanted to hint at is the part of the
>> doc string that reads "that do not have their own values...", which
>> means that what default-value returns depends whether or not SYMBOL got
>> its own value in the buffer, and that depends on the presence of lets,
>> and so on.
>
> It is not how I read this. AFAIU, `default-value' returns the value that
> is used for buffers where the value is not explicitly set. Calling
> `default-value' from inside the buffer where the value is set does not
> change that global default value and should not change what
> `default-value' returns.
>
> And indeed
>
> (defvar-local yant/test 'default-value)
> (with-temp-buffer
> (setq yant/test 'local-value)
> (default-value 'yant/test)) ; => 'default-value
Maybe the documentation could be changed to be clearer, but I'm afraid I
don't know how, because I think it describes 100% what is happening: the
setq creates a buffer-local binding in the temp buffer, and
default-value returns the value that is used in the case that a buffer
does _not_ have its own value, which is the value set in the
defvar-local...
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 11:15:02 GMT)
Full text and
rfc822 format available.
Message #459 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>> (defvar-local yant/test 'default-value)
>> (with-temp-buffer
>> (setq yant/test 'local-value)
>> (default-value 'yant/test)) ; => 'default-value
>
> Maybe the documentation could be changed to be clearer, but I'm afraid I
> don't know how, because I think it describes 100% what is happening: the
> setq creates a buffer-local binding in the temp buffer, and
> default-value returns the value that is used in the case that a buffer
> does _not_ have its own value, which is the value set in the
> defvar-local...
We clearly have some misunderstanding.
Doesn't your statement contradict the earlier one:
>>>> doc string that reads "that do not have their own values...", which
>>>> means that what default-value returns depends whether or not SYMBOL got
>>>> its own value in the buffer
In my example, SYMBOL does get its own value in the buffer, but
`default-value' still returns 'default-value.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 11:50:02 GMT)
Full text and
rfc822 format available.
Message #462 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Ihor Radchenko <yantar92 <at> posteo.net> writes:
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>
>>> (defvar-local yant/test 'default-value)
>>> (with-temp-buffer
>>> (setq yant/test 'local-value)
>>> (default-value 'yant/test)) ; => 'default-value
>>
>> Maybe the documentation could be changed to be clearer, but I'm afraid I
>> don't know how, because I think it describes 100% what is happening: the
>> setq creates a buffer-local binding in the temp buffer, and
>> default-value returns the value that is used in the case that a buffer
>> does _not_ have its own value, which is the value set in the
>> defvar-local...
>
> We clearly have some misunderstanding.
I tend to agree :-).
> Doesn't your statement contradict the earlier one:
>
> >>>> doc string that reads "that do not have their own values...", which
> >>>> means that what default-value returns depends whether or not SYMBOL got
> >>>> its own value in the buffer
>
> In my example, SYMBOL does get its own value in the buffer, but
> `default-value' still returns 'default-value.
I assume you mean the example from above? This one:
>>> (defvar-local yant/test 'default-value)
>>> (with-temp-buffer
>>> (setq yant/test 'local-value)
>>> (default-value 'yant/test)) ; => 'default-value
If so, the "gets its own ... _but_ default-value still returns..." hints
at the misunderstanding, maybe. It should be something like "because it
gets its own..., default-value returns...".
Maybe one could reformulate the doc string of default-value like this?
Return SYMBOL’s default value.
This is the value that is seen in buffers that do not have their own values
for this variable.
=>
Return SYMBOL’s default value.
This is the value that would be seen in the buffer if it didn't have
its own value.
Although I find the original clearer...
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 11:58:01 GMT)
Full text and
rfc822 format available.
Message #465 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Ihor Radchenko <yantar92 <at> posteo.net> writes:
> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
>>>>> (defvar-local let-tests-buffer-local-var 'value)
>>>>> (with-temp-buffer
>>>>> (setq-local let-tests-buffer-local-var 'baz)
>>>>> (let ((let-tests-buffer-local-var 'bar))
>>>>> ;; This is failing.
>>>>> (should (eq 'bar (default-value 'let-tests-buffer-local-var)))
>>> ...
>>> Are you saying that `let' never changes `default-value' output?
>>> Unfortunately, no.
>>
>> Indeed, see the doc that Eli quoted 10 posts "ago":
>>
>> [...] ELisp manual, which describes the effect of
>> make-variable-buffer-local:
>>
>> A peculiar wrinkle of this feature is that binding the variable
>> (with ‘let’ or other binding constructs) does not create
>> a buffer-local binding for it. Only setting the variable (with
>> ‘set’ or ‘setq’), while the variable does not have a ‘let’-style
>> binding that was made in the current buffer, does so.
>>
>> IOW, `let` changes the binding that is "current": if the variable is
>> buffer-local in the current buffer it changes that buffer-local value
>> and otherwise it changes the global value.
>> [ This for "automatically buffer-local variables". ]
>
> What you say is indeed what I observe, but I do not see how it follows
> from the quoted manual text. When I read the above part of the manual, I
> see a warning about using `set'/`setq' inside `let' that binds the same
> variable.
But it is talking about make-variable-buffer-local, so "the variable" is
a local variable, IMHO.
> I think that the caveat about binding buffer-local variables should be
> documented.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 12:06:02 GMT)
Full text and
rfc822 format available.
Message #468 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>> In my example, SYMBOL does get its own value in the buffer, but
>> `default-value' still returns 'default-value.
>
> I assume you mean the example from above? This one:
Yes.
>>>> (defvar-local yant/test 'default-value)
>>>> (with-temp-buffer
>>>> (setq yant/test 'local-value)
>>>> (default-value 'yant/test)) ; => 'default-value
>
> If so, the "gets its own ... _but_ default-value still returns..." hints
> at the misunderstanding, maybe. It should be something like "because it
> gets its own..., default-value returns...".
>
> Maybe one could reformulate the doc string of default-value like this?
>
> Return SYMBOL’s default value.
> This is the value that is seen in buffers that do not have their own values
> for this variable.
>
> =>
>
> Return SYMBOL’s default value.
> This is the value that would be seen in the buffer if it didn't have
> its own value.
>
> Although I find the original clearer...
Both versions are actually clear for me. What was not clear for me is
how the docstring explains my `let' example.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 12:09:02 GMT)
Full text and
rfc822 format available.
Message #471 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Ihor Radchenko <yantar92 <at> posteo.net> writes:
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>
>>> In my example, SYMBOL does get its own value in the buffer, but
>>> `default-value' still returns 'default-value.
>>
>> I assume you mean the example from above? This one:
>
> Yes.
>
>>>>> (defvar-local yant/test 'default-value)
>>>>> (with-temp-buffer
>>>>> (setq yant/test 'local-value)
>>>>> (default-value 'yant/test)) ; => 'default-value
>>
>> If so, the "gets its own ... _but_ default-value still returns..." hints
>> at the misunderstanding, maybe. It should be something like "because it
>> gets its own..., default-value returns...".
>>
>> Maybe one could reformulate the doc string of default-value like this?
>>
>> Return SYMBOL’s default value.
>> This is the value that is seen in buffers that do not have their own values
>> for this variable.
>>
>> =>
>>
>> Return SYMBOL’s default value.
>> This is the value that would be seen in the buffer if it didn't have
>> its own value.
>>
>> Although I find the original clearer...
>
> Both versions are actually clear for me. What was not clear for me is
> how the docstring explains my `let' example.
Sorry, I've lost track what the let example is.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 12:09:02 GMT)
Full text and
rfc822 format available.
Message #474 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>> What you say is indeed what I observe, but I do not see how it follows
>> from the quoted manual text. When I read the above part of the manual, I
>> see a warning about using `set'/`setq' inside `let' that binds the same
>> variable.
>
> But it is talking about make-variable-buffer-local, so "the variable" is
> a local variable, IMHO.
Of course. But my question was about what `let' does to the default
value of buffer-local variables. While the manual only explains about
setq/set.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 12:13:01 GMT)
Full text and
rfc822 format available.
Message #477 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>> Both versions are actually clear for me. What was not clear for me is
>> how the docstring explains my `let' example.
>
> Sorry, I've lost track what the let example is.
(defvar-local eval-tests/buffer-local-var 'default-value)
(with-temp-buffer
(setq-local eval-tests/buffer-local-var 'local-value)
(let ((eval-tests/buffer-local-var 'let-value))
;; Let in a buffer with local binding does not change the
;; default value for variable.
(should (eq 'default-value (default-value 'eval-tests/buffer-local-var)))))
(with-temp-buffer
;; (setq-local eval-tests/buffer-local-var 'local-value)
(let ((eval-tests/buffer-local-var 'let-value))
;; Let in a buffer with local binding does not change the
;; default value for variable.
(should (eq 'let-value (default-value 'eval-tests/buffer-local-var)))))
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 12:58:02 GMT)
Full text and
rfc822 format available.
Message #480 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Ihor Radchenko <yantar92 <at> posteo.net> writes:
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>
>>> Both versions are actually clear for me. What was not clear for me is
>>> how the docstring explains my `let' example.
>>
>> Sorry, I've lost track what the let example is.
>
> (defvar-local eval-tests/buffer-local-var 'default-value)
>
Thanks.
> (with-temp-buffer
> (setq-local eval-tests/buffer-local-var 'local-value)
> (let ((eval-tests/buffer-local-var 'let-value))
> ;; Let in a buffer with local binding does not change the
> ;; default value for variable.
> (should (eq 'default-value (default-value
> 'eval-tests/buffer-local-var)))))
I assume this is clear. Temp buffer has local value, the let changes the
local (current) value, default-value determines the value as if the
local values weren't there, and so on.
>
> (with-temp-buffer
> ;; (setq-local eval-tests/buffer-local-var 'local-value)
> (let ((eval-tests/buffer-local-var 'let-value))
> ;; Let in a buffer with local binding does not change the
> ;; default value for variable.
> (should (eq 'let-value (default-value 'eval-tests/buffer-local-var)))))
Here, temp has no local value, so the current value is the global one.
Let changes the global (current) value, and default-value picks that up.
In both cases, I don't know if that's mentioned explicitly in some doc
string. It's the normal behavior of let with special variables (dynamic
variables). The current value that is set changes depending on
circumstances.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 13:07:02 GMT)
Full text and
rfc822 format available.
Message #483 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>> (with-temp-buffer
>> ;; (setq-local eval-tests/buffer-local-var 'local-value)
>> (let ((eval-tests/buffer-local-var 'let-value))
>> ;; Let in a buffer with local binding does not change the
>> ;; default value for variable.
>> (should (eq 'let-value (default-value 'eval-tests/buffer-local-var)))))
>
> Here, temp has no local value, so the current value is the global one.
> Let changes the global (current) value, and default-value picks that up.
>
> In both cases, I don't know if that's mentioned explicitly in some doc
> string. It's the normal behavior of let with special variables (dynamic
> variables). The current value that is set changes depending on
> circumstances.
AFAIK, this detail is not documented anywhere.
--
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>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 13:30:01 GMT)
Full text and
rfc822 format available.
Message #486 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Ihor Radchenko <yantar92 <at> posteo.net> writes:
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>
>>> (with-temp-buffer
>>> ;; (setq-local eval-tests/buffer-local-var 'local-value)
>>> (let ((eval-tests/buffer-local-var 'let-value))
>>> ;; Let in a buffer with local binding does not change the
>>> ;; default value for variable.
>>> (should (eq 'let-value (default-value 'eval-tests/buffer-local-var)))))
>>
>> Here, temp has no local value, so the current value is the global one.
>> Let changes the global (current) value, and default-value picks that up.
>>
>> In both cases, I don't know if that's mentioned explicitly in some doc
>> string. It's the normal behavior of let with special variables (dynamic
>> variables). The current value that is set changes depending on
>> circumstances.
>
> AFAIK, this detail is not documented anywhere.
elisp.info, 12.10.2 Dynamic Binding has a bit.
(And, BTW, the doc string of let says it's a generalized variable?
Is that right?)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Tue, 02 Jan 2024 14:09:01 GMT)
Full text and
rfc822 format available.
Message #489 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Ihor Radchenko <yantar92 <at> posteo.net> writes:
> Ihor Radchenko <yantar92 <at> posteo.net> writes:
>
>> See the following two almost identical snippets that yield completely
>> different results depending on the present of `setq-default':
>> ...
>
> I believe that I now understand enough to add a new meaningful test.
> See the attached. Please check the comments in the test to make sure
> that they make sense.
Looks good to me.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 03 Jan 2024 02:14:02 GMT)
Full text and
rfc822 format available.
Message #492 received at 66117 <at> debbugs.gnu.org (full text, mbox):
>> Indeed, see the doc that Eli quoted 10 posts "ago":
>>
>> [...] ELisp manual, which describes the effect of
>> make-variable-buffer-local:
>>
>> A peculiar wrinkle of this feature is that binding the variable
>> (with ‘let’ or other binding constructs) does not create
>> a buffer-local binding for it. Only setting the variable (with
>> ‘set’ or ‘setq’), while the variable does not have a ‘let’-style
>> binding that was made in the current buffer, does so.
>>
>> IOW, `let` changes the binding that is "current": if the variable is
>> buffer-local in the current buffer it changes that buffer-local value
>> and otherwise it changes the global value.
>> [ This for "automatically buffer-local variables". ]
>
> What you say is indeed what I observe, but I do not see how it follows
> from the quoted manual text.
It follows from the first sentence:
A peculiar wrinkle of this feature is that binding the variable
(with ‘let’ or other binding constructs) does not create
a buffer-local binding for it.
> When I read the above part of the manual, I see a warning about using
> `set'/`setq' inside `let' that binds the same variable.
That's in the second sentence.
> I think that the caveat about binding buffer-local variables should be
> documented.
Actually, let-binding works the same for all vars: it affects only the
binding that's currently active.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#66117; Package
emacs.
(Wed, 03 Jan 2024 14:29:01 GMT)
Full text and
rfc822 format available.
Message #495 received at 66117 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> (And, BTW, the doc string of let says it's a generalized variable?
> Is that right?)
To answer my own question: it is indeed
(defvar x '(1 . 2))
(setf (let ((a x)) (cdr a)) 4)
x
=> (1 . 4)
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Thu, 01 Feb 2024 12:24:09 GMT)
Full text and
rfc822 format available.
This bug report was last modified 1 year and 97 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.