GNU bug report logs -
#77924
31.0.50; [Feature branch] Change marker implementation
Previous Next
To reply to this bug, email your comments to 77924 AT debbugs.gnu.org.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sat, 19 Apr 2025 16:06:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Gerd Möllmann <gerd.moellmann <at> gmail.com>:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org.
(Sat, 19 Apr 2025 16:06:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
This is about the branch scratch/text-index on savannah, which is a port
of what I have in my Emacs.
The branch changes the implementation of Lisp markers.
- Markers no longer form a doubly-linked list, they are stored in a
marker vector instead which allows O(1) insertion and deletion of
markers. (The idea of a marker vector is from what I did in igc. In my
Emacs, both igc and old GC use the same marker vector implementation.
The new one is different from the one in feature/igc.)
- Lisp_Marker doesn't contain the character position directly. It is
stored in the marker vector instead. This leads to faster,
cache-friendly, marker position adjustments.
- Markers don't contain byte positions. Byte positions are computed when
needed. Bytepos <-> charpos conversion are done using a text-index
data structure. This removes the heuristics currently used in master,
and allows removing byte positions from markers.
In summary, I'd say performance is good, to say the least, in many cases
better, and it fixes corner cases leading the abysmal performance in
current master.
Please see the comments at the start of marker-vector.c and text-index.c
for more details. Also see the thread(s) on emacs-devel with Stef an me.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sat, 19 Apr 2025 16:33:02 GMT)
Full text and
rfc822 format available.
Message #8 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>
> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Date: Sat, 19 Apr 2025 18:05:38 +0200
>
> This is about the branch scratch/text-index on savannah, which is a port
> of what I have in my Emacs.
>
> The branch changes the implementation of Lisp markers.
>
> - Markers no longer form a doubly-linked list, they are stored in a
> marker vector instead which allows O(1) insertion and deletion of
> markers. (The idea of a marker vector is from what I did in igc. In my
> Emacs, both igc and old GC use the same marker vector implementation.
> The new one is different from the one in feature/igc.)
>
> - Lisp_Marker doesn't contain the character position directly. It is
> stored in the marker vector instead. This leads to faster,
> cache-friendly, marker position adjustments.
>
> - Markers don't contain byte positions. Byte positions are computed when
> needed. Bytepos <-> charpos conversion are done using a text-index
> data structure. This removes the heuristics currently used in master,
> and allows removing byte positions from markers.
>
> In summary, I'd say performance is good, to say the least, in many cases
> better, and it fixes corner cases leading the abysmal performance in
> current master.
>
> Please see the comments at the start of marker-vector.c and text-index.c
> for more details. Also see the thread(s) on emacs-devel with Stef an me.
Thanks.
Are there any backward-incompatible changes with this?
Do all the tests still pass as well as they did before these changes?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sat, 19 Apr 2025 16:59:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> Are there any backward-incompatible changes with this?
None I'm aware of.
> Do all the tests still pass as well as they did before these changes?
I got a SEGV in buffer-tests right now when I checked again that went
away an a second run. So I'll have to check that.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sat, 19 Apr 2025 20:57:02 GMT)
Full text and
rfc822 format available.
Message #14 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>> Are there any backward-incompatible changes with this?
>
> None I'm aware of.
>
>> Do all the tests still pass as well as they did before these changes?
>
> I got a SEGV in buffer-tests right now when I checked again that went
> away an a second run. So I'll have to check that.
SUMMARY OF TEST RESULTS
-----------------------
Files examined: 530
Ran 8041 tests, 7765 results as expected, 0 unexpected, 276 skipped
[scratch/text-index] gerd <at> mini 2025-04-19 22:51
Now fixed.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sun, 20 Apr 2025 08:35:01 GMT)
Full text and
rfc822 format available.
Message #17 received at 77924 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>
>>> Are there any backward-incompatible changes with this?
>>
>> None I'm aware of.
>>
>>> Do all the tests still pass as well as they did before these changes?
>>
>> I got a SEGV in buffer-tests right now when I checked again that went
>> away an a second run. So I'll have to check that.
>
> SUMMARY OF TEST RESULTS
> -----------------------
> Files examined: 530
> Ran 8041 tests, 7765 results as expected, 0 unexpected, 276 skipped
> [scratch/text-index] gerd <at> mini 2025-04-19 22:51
>
> Now fixed.
Commit message:
[text-index-change.log (application/octet-stream, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Mon, 21 Apr 2025 04:42:02 GMT)
Full text and
rfc822 format available.
Message #20 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>
>> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>>
>>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>>
>>>> Are there any backward-incompatible changes with this?
>>>
>>> None I'm aware of.
>>>
>>>> Do all the tests still pass as well as they did before these changes?
>>>
>>> I got a SEGV in buffer-tests right now when I checked again that went
>>> away an a second run. So I'll have to check that.
>>
>> SUMMARY OF TEST RESULTS
>> -----------------------
>> Files examined: 530
>> Ran 8041 tests, 7765 results as expected, 0 unexpected, 276 skipped
>> [scratch/text-index] gerd <at> mini 2025-04-19 22:51
>>
>> Now fixed.
>
> Commit message:
A warning: the branch currently contains a bug that can lead to a stack
overflow that looks like this:
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0x16b0ffff0)
frame #0: 0x000000010451bc38 emacs`ensure_charpos_indexed(b=0x0000000107280d28, charpos=4415031360) at text-index.c:356
frame #1: 0x000000010451bb10 emacs`text_index_charpos_to_bytepos(b=0x0000000107280840, charpos=25980) at text-index.c:621:3
frame #2: 0x00000001046cc1d0 emacs`marker_vector_bytepos(m=0x0000000107280d28) at marker-vector.c:323:10
frame #3: 0x0000000104529aec emacs`marker_byte_position(marker=(struct Lisp_Marker *) $52 = 0x0000000107280d28) at marker.c:376:29
frame #4: 0x000000010451cfb4 emacs`BUF_PT_BYTE(buf=0x0000000107280840) at buffer.h:909:6
!gud 909:6:/Users/gerd/emacs/github/cl-packages/src/buffer.h
* frame #5: 0x000000010451bdec emacs`narrow_charpos_bounds(b=0x0000000107280840, prev=0x000000016b100178, next=0x000000016b100168, charpos=25980) at text-index.c:549:42
frame #6: 0x000000010451bb78 emacs`text_index_charpos_to_bytepos(b=0x0000000107280840, charpos=25980) at text-index.c:628:23
frame #7: 0x00000001046cc1d0 emacs`marker_vector_bytepos(m=0x0000000107280d28) at marker-vector.c:323:10
frame #8: 0x0000000104529aec emacs`marker_byte_position(marker=(struct Lisp_Marker *) $52 = 0x0000000107280d28) at marker.c:376:29
frame #9: 0x000000010451cfb4 emacs`BUF_PT_BYTE(buf=0x0000000107280840) at buffer.h:909:6
frame #10: 0x000000010451bdec emacs`narrow_charpos_bounds(b=0x0000000107280840, prev=0x000000016b1002c8, next=0x000000016b1002b8, charpos=25980) at text-index.c:549:42
frame #11: 0x000000010451bb78 emacs`text_index_charpos_to_bytepos(b=0x0000000107280840, charpos=25980) at text-index.c:628:23
frame #12: 0x00000001046cc1d0 emacs`marker_vector_bytepos(m=0x0000000107280d28) at marker-vector.c:323:10
frame #13: 0x0000000104529aec emacs`marker_byte_position(marker=(struct Lisp_Marker *) $52 = 0x0000000107280d28) at marker.c:376:29
frame #14: 0x000000010451cfb4 emacs`BUF_PT_BYTE(buf=0x0000000107280840) at buffer.h:909:6
The reason for that is that functions like BUF_PT (and maybe others) are
"too smart", in the case of indirect buffers, for what I need in the
text index. I'll fix that a bit later today.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Mon, 21 Apr 2025 05:42:02 GMT)
Full text and
rfc822 format available.
Message #23 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> I'll fix that a bit later today.
Done.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Mon, 21 Apr 2025 18:38:03 GMT)
Full text and
rfc822 format available.
Message #26 received at 77924 <at> debbugs.gnu.org (full text, mbox):
[[[ To any NSA and FBI agents reading my email: please consider ]]]
[[[ whether defending the US Constitution against all enemies, ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]
The one thing I worry about, regarding this new data structure,
is that allocation of the vectors could get troblesome
wnet there are many markers in a buffer.
I don't know whether that theoretical problem makes a big difference
in practice. The new scheme may be better. I just suggest looking at
this question.
--
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Tue, 22 Apr 2025 02:30:02 GMT)
Full text and
rfc822 format available.
Message #29 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> This is about the branch scratch/text-index on savannah, which is a port
> of what I have in my Emacs.
>
> The branch changes the implementation of Lisp markers.
>
> - Markers no longer form a doubly-linked list, they are stored in a
> marker vector instead which allows O(1) insertion and deletion of
> markers. (The idea of a marker vector is from what I did in igc. In my
> Emacs, both igc and old GC use the same marker vector implementation.
> The new one is different from the one in feature/igc.)
>
> - Lisp_Marker doesn't contain the character position directly. It is
> stored in the marker vector instead. This leads to faster,
> cache-friendly, marker position adjustments.
>
> - Markers don't contain byte positions. Byte positions are computed when
> needed. Bytepos <-> charpos conversion are done using a text-index
> data structure. This removes the heuristics currently used in master,
> and allows removing byte positions from markers.
>
> In summary, I'd say performance is good, to say the least, in many cases
> better, and it fixes corner cases leading the abysmal performance in
> current master.
This sounds great. I pushed a couple of typo fixes while reading the
patches, please use them as you see fit (squash, etc.).
- Any chance you could add a comment explaining how you arrived at
text_index_interval=4*1024? If it's arbitrarily chosen, adding a
comment to that effect might be useful.
- Maybe rename `text-index-interval` to `text-index--interval` (to
indicate that it's internal)?
- I guess `use-text-index` can be removed?
> Please see the comments at the start of marker-vector.c and text-index.c
> for more details. Also see the thread(s) on emacs-devel with Stef an me.
For posterity, I guess that would be:
Re: Question about region caches
https://lists.gnu.org/r/emacs-devel/2025-03/msg01382.html
PS. BTW, a small procedural thing. Instead of merging master into a
scratch/ branch, I recommend deleting the branch, rebasing it on
master, and then pushing it again. This way, when we later merge it
into master, we avoid the merge commits, and the history is kept
clean and more easily reviewable. Not the end of the world either
way, but something to consider.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Tue, 22 Apr 2025 04:13:01 GMT)
Full text and
rfc822 format available.
Message #32 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Stefan Kangas <stefankangas <at> gmail.com> writes:
> This sounds great. I pushed a couple of typo fixes while reading the
> patches, please use them as you see fit (squash, etc.).
Thanks, I've ported that back to my side.
>
> - Any chance you could add a comment explaining how you arrived at
> text_index_interval=4*1024? If it's arbitrarily chosen, adding a
> comment to that effect might be useful.
It's neither completely arbitrary nor can I really say much about it.
INTERVAL / 2 is basically the worst-case distance one has to scan
through text. Don't really know what to say more, and the above is
pretty obvious.
>
> - Maybe rename `text-index-interval` to `text-index--interval` (to
> indicate that it's internal)?
>
> - I guess `use-text-index` can be removed?
I've removed these Lisp variables a few days ago or so. They were only
for experimenting with the whole thing.
>> Please see the comments at the start of marker-vector.c and text-index.c
>> for more details. Also see the thread(s) on emacs-devel with Stef an me.
>
> For posterity, I guess that would be:
>
> Re: Question about region caches
> https://lists.gnu.org/r/emacs-devel/2025-03/msg01382.html
>
> PS. BTW, a small procedural thing. Instead of merging master into a
> scratch/ branch, I recommend deleting the branch, rebasing it on
> master, and then pushing it again. This way, when we later merge it
> into master, we avoid the merge commits, and the history is kept
> clean and more easily reviewable. Not the end of the world either
> way, but something to consider.
Sorry, too much work :-).
(For reviewing a branch with merges from master I recommend to find the
latest merge commit, take the parent commit on the master side, and
range diff with that. (If you have the merge in the reflog, that
speeds up finding the latest merge, but that's only the case if you did
the merge in that repo.))
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Tue, 22 Apr 2025 08:36:02 GMT)
Full text and
rfc822 format available.
Message #35 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> Stefan Kangas <stefankangas <at> gmail.com> writes:
>
>> This sounds great. I pushed a couple of typo fixes while reading the
>> patches, please use them as you see fit (squash, etc.).
>
> Thanks, I've ported that back to my side.
>
>>
>> - Any chance you could add a comment explaining how you arrived at
>> text_index_interval=4*1024? If it's arbitrarily chosen, adding a
>> comment to that effect might be useful.
>
> It's neither completely arbitrary nor can I really say much about it.
> INTERVAL / 2 is basically the worst-case distance one has to scan
> through text. Don't really know what to say more, and the above is
> pretty obvious.
>
>>
>> - Maybe rename `text-index-interval` to `text-index--interval` (to
>> indicate that it's internal)?
>>
>> - I guess `use-text-index` can be removed?
>
> I've removed these Lisp variables a few days ago or so. They were only
> for experimenting with the whole thing.
>
>>> Please see the comments at the start of marker-vector.c and text-index.c
>>> for more details. Also see the thread(s) on emacs-devel with Stef an me.
>>
>> For posterity, I guess that would be:
>>
>> Re: Question about region caches
>> https://lists.gnu.org/r/emacs-devel/2025-03/msg01382.html
>>
>> PS. BTW, a small procedural thing. Instead of merging master into a
>> scratch/ branch, I recommend deleting the branch, rebasing it on
>> master, and then pushing it again. This way, when we later merge it
>> into master, we avoid the merge commits, and the history is kept
>> clean and more easily reviewable. Not the end of the world either
>> way, but something to consider.
>
> Sorry, too much work :-).
>
> (For reviewing a branch with merges from master I recommend to find the
> latest merge commit, take the parent commit on the master side, and
> range diff with that. (If you have the merge in the reflog, that
> speeds up finding the latest merge, but that's only the case if you did
> the merge in that repo.))
There was the open question left whether or not it pays off to avoid the
binary search in charpos -> bytepos conversion. Benchmarked it with
(defun elb-replace-region-contents-entry ()
(with-temp-buffer
(let ((step (apply #'concat (make-list 2000 "🙂été👶🏿 "))))
(dotimes (_ (/ 100000 (length step)))
(insert step)))
(dotimes (_ 100000)
(let* ((a (1+ (random (point-max))))
(b (1+ (random (point-max))))
(beg (min a b))
(end (max a b)))
(replace-region-contents beg end "🙂été👶🏿 🙂été👶🏿 ")))))
2000 runs on my idle Mac mini, M1 etc.
* Without optimization Results
| test | non-gc (s) | gc (s) | gcs | total (s) | err (s) |
|-------------------------+------------+--------+-----+-----------+---------|
| replace-region-contents | 0.93 | 0.00 | 0 | 0.93 | 0.33 |
|-------------------------+------------+--------+-----+-----------+---------|
| total | 0.93 | 0.00 | 0 | 0.93 | 0.33 |
* With opt. Results
| test | non-gc (s) | gc (s) | gcs | total (s) | err (s) |
|-------------------------+------------+--------+-----+-----------+---------|
| replace-region-contents | 0.89 | 0.00 | 0 | 0.89 | 0.32 |
|-------------------------+------------+--------+-----+-----------+---------|
| total | 0.89 | 0.00 | 0 | 0.89 | 0.32 |
I think the large err values are because of the random and the O(ND) in
replace-region-contents. Whatever, the optimization seems to pay off, so
I'll add that to scratch/text-index on savannah later.
Tests with the change:
SUMMARY OF TEST RESULTS
-----------------------
Files examined: 530
Ran 8041 tests, 7765 results as expected, 0 unexpected, 276 skipped
I consider this feature done when that is in, and play with arm64 NEON a
bit instead. In C++ ;-).
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Tue, 22 Apr 2025 12:36:01 GMT)
Full text and
rfc822 format available.
Message #38 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> * Without optimization Results
>
> | test | non-gc (s) | gc (s) | gcs | total (s) | err (s) |
> |-------------------------+------------+--------+-----+-----------+---------|
> | replace-region-contents | 0.93 | 0.00 | 0 | 0.93 | 0.33 |
> |-------------------------+------------+--------+-----+-----------+---------|
> | total | 0.93 | 0.00 | 0 | 0.93 | 0.33 |
>
> * With opt. Results
>
> | test | non-gc (s) | gc (s) | gcs | total (s) | err (s) |
> |-------------------------+------------+--------+-----+-----------+---------|
> | replace-region-contents | 0.89 | 0.00 | 0 | 0.89 | 0.32 |
> |-------------------------+------------+--------+-----+-----------+---------|
> | total | 0.89 | 0.00 | 0 | 0.89 | 0.32 |
** master Results
| test | non-gc (s) | gc (s) | gcs | total (s) | err (s) |
|-------------------------+------------+--------+-----+-----------+---------|
| replace-region-contents | 1.04 | 0.00 | 0 | 1.04 | 0.33 |
|-------------------------+------------+--------+-----+-----------+---------|
| total | 1.04 | 0.00 | 0 | 1.04 | 0.33 |
So, improvement, also here with text-index.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Tue, 22 Apr 2025 14:09:01 GMT)
Full text and
rfc822 format available.
Message #41 received at 77924 <at> debbugs.gnu.org (full text, mbox):
>> - Any chance you could add a comment explaining how you arrived at
>> text_index_interval=4*1024? If it's arbitrarily chosen, adding a
>> comment to that effect might be useful.
> It's neither completely arbitrary nor can I really say much about it.
> INTERVAL / 2 is basically the worst-case distance one has to scan
> through text. Don't really know what to say more, and the above is
> pretty obvious.
FWIW, the current code in `master` uses a threshold of 5k (sometimes 5k
bytes, sometimes 5k chars) as the minimum distance between
"cache-markers".
It's a tradeoff between the size of the text-index array (and the time
it takes to populate/update it) and the time it takes to scan the text
from one of the positions recorded in that array to the position we're
actually interested in.
We haven't really investigated which value would be ideal.
The code that does the scan can probably be improved significantly
(e.g. by processing the text several bytes at a time).
> (defun elb-replace-region-contents-entry ()
> (with-temp-buffer
> (let ((step (apply #'concat (make-list 2000 "🙂été👶🏿 "))))
> (dotimes (_ (/ 100000 (length step)))
> (insert step)))
>
> (dotimes (_ 100000)
> (let* ((a (1+ (random (point-max))))
> (b (1+ (random (point-max))))
> (beg (min a b))
> (end (max a b)))
> (replace-region-contents beg end "🙂été👶🏿 🙂été👶🏿 ")))))
As discussed in the other thread, this performs fairly few
charpos->bytepos conversions, actually. Instead it spends most of its
time inside the `diffseq.h` code. You could try something like:
(require 'subr-x)
(defun elb-replace-region-contents-entry ()
(with-temp-buffer
(let ((step (apply #'concat (make-list 2000 "🙂été👶🏿 "))))
(dotimes (_ (/ 500000 (length step)))
(insert step))
(dotimes (_ 100000)
(let* ((a (+ (point-min) (random (buffer-size))))
(b (min (+ a (length step) (random 512) -256) (point-max))))
(replace-region-contents a b (lambda () step)))))))
which should spend more time in Emacs's code (especially in
`buffer_chars_equal`) than in `diffseq.h`
>> PS. BTW, a small procedural thing. Instead of merging master into a
>> scratch/ branch, I recommend deleting the branch, rebasing it on
>> master, and then pushing it again. This way, when we later merge it
>> into master, we avoid the merge commits, and the history is kept
>> clean and more easily reviewable. Not the end of the world either
>> way, but something to consider.
>
> Sorry, too much work :-).
>
> (For reviewing a branch with merges from master I recommend to find the
> latest merge commit, take the parent commit on the master side, and
> range diff with that. (If you have the merge in the reflog, that
> speeds up finding the latest merge, but that's only the case if you did
> the merge in that repo.))
git diff origin/master...origin/scratch/text-index
should take care of it.
If there's interest, I could take care of rebasing (and improving the
commit messages on the rebased commits) before merging into `master`.
> I consider this feature done when that is in, and play with arm64 NEON a
> bit instead. In C++ ;-).
BTW, regarding processing several bytes at a time, we could try
something like
long unsigned eightbytes = BUF_FETCH_8BYTES (bytepos);
long unsigned eightbits = ((~eightbytes) & 0x8080808080808080) >> 7;
long unsigned fourtwobits = eightbits + (eightbits >> 32);
long unsigned twofourbits = fourtwobits + (fourtwobits >> 16);
nchars += (twofourbits + (twofourbits >> 8)) & 0xff;
Not sure it would be significantly faster than the current code, tho.
Maybe an even easier first change would be to replace
if (CHAR_HEAD_P (BUF_FETCH_BYTE (b, bytepos)))
++charpos;
with
charpos += (~BUF_FETCH_BYTE (b, bytepos)) >> 7;
so as to avoid a branch that can be sometimes hard to predict.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Tue, 22 Apr 2025 14:35:02 GMT)
Full text and
rfc822 format available.
Message #44 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>> - Any chance you could add a comment explaining how you arrived at
>>> text_index_interval=4*1024? If it's arbitrarily chosen, adding a
>>> comment to that effect might be useful.
>> It's neither completely arbitrary nor can I really say much about it.
>> INTERVAL / 2 is basically the worst-case distance one has to scan
>> through text. Don't really know what to say more, and the above is
>> pretty obvious.
>
> FWIW, the current code in `master` uses a threshold of 5k (sometimes 5k
> bytes, sometimes 5k chars) as the minimum distance between
> "cache-markers".
>
> It's a tradeoff between the size of the text-index array (and the time
> it takes to populate/update it) and the time it takes to scan the text
> from one of the positions recorded in that array to the position we're
> actually interested in.
>
> We haven't really investigated which value would be ideal.
True. It might even be different between machines, don't know. I once,
in the beginning, tried large values, don't remember, maybe 16K, and
performance seemed to drop, but not much. Difficult to find an ideal
value.
> The code that does the scan can probably be improved significantly
> (e.g. by processing the text several bytes at a time).
For sure.
>> (defun elb-replace-region-contents-entry ()
>> (with-temp-buffer
>> (let ((step (apply #'concat (make-list 2000 "🙂été👶🏿 "))))
>> (dotimes (_ (/ 100000 (length step)))
>> (insert step)))
>>
>> (dotimes (_ 100000)
>> (let* ((a (1+ (random (point-max))))
>> (b (1+ (random (point-max))))
>> (beg (min a b))
>> (end (max a b)))
>> (replace-region-contents beg end "🙂été👶🏿 🙂été👶🏿 ")))))
>
> As discussed in the other thread, this performs fairly few
> charpos->bytepos conversions, actually. Instead it spends most of its
> time inside the `diffseq.h` code. You could try something like:
>
> (require 'subr-x)
>
> (defun elb-replace-region-contents-entry ()
> (with-temp-buffer
> (let ((step (apply #'concat (make-list 2000 "🙂été👶🏿 "))))
> (dotimes (_ (/ 500000 (length step)))
> (insert step))
>
> (dotimes (_ 100000)
> (let* ((a (+ (point-min) (random (buffer-size))))
> (b (min (+ a (length step) (random 512) -256) (point-max))))
> (replace-region-contents a b (lambda () step)))))))
>
> which should spend more time in Emacs's code (especially in
> `buffer_chars_equal`) than in `diffseq.h`
>
>>> PS. BTW, a small procedural thing. Instead of merging master into a
>>> scratch/ branch, I recommend deleting the branch, rebasing it on
>>> master, and then pushing it again. This way, when we later merge it
>>> into master, we avoid the merge commits, and the history is kept
>>> clean and more easily reviewable. Not the end of the world either
>>> way, but something to consider.
>>
>> Sorry, too much work :-).
>>
>> (For reviewing a branch with merges from master I recommend to find the
>> latest merge commit, take the parent commit on the master side, and
>> range diff with that. (If you have the merge in the reflog, that
>> speeds up finding the latest merge, but that's only the case if you did
>> the merge in that repo.))
>
> git diff origin/master...origin/scratch/text-index
>
> should take care of it.
>
> If there's interest, I could take care of rebasing (and improving the
> commit messages on the rebased commits) before merging into `master`.
Please consider the branch yours, if you like, as far as I am concerned
:-). Because...
>> I consider this feature done when that is in, and play with arm64 NEON a
>> bit instead. In C++ ;-).
...of that-
>
> BTW, regarding processing several bytes at a time, we could try
> something like
>
> long unsigned eightbytes = BUF_FETCH_8BYTES (bytepos);
> long unsigned eightbits = ((~eightbytes) & 0x8080808080808080) >> 7;
> long unsigned fourtwobits = eightbits + (eightbits >> 32);
> long unsigned twofourbits = fourtwobits + (fourtwobits >> 16);
> nchars += (twofourbits + (twofourbits >> 8)) & 0xff;
>
> Not sure it would be significantly faster than the current code, tho.
> Maybe an even easier first change would be to replace
>
> if (CHAR_HEAD_P (BUF_FETCH_BYTE (b, bytepos)))
> ++charpos;
>
> with
>
> charpos += (~BUF_FETCH_BYTE (b, bytepos)) >> 7;
>
> so as to avoid a branch that can be sometimes hard to predict.
>
Hard to tell if that would be faster. Maybe if I know a bit more about
the current state of affairs of SIMD support, one could write something
using that.
That would certainly be faster. On my M1 chips it would mean processing
16 bytes at a time (128 bit registers), and I've seen a presentation
that talked about 256 and even 512 bits on some Intel CPUs (AMX).
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Tue, 22 Apr 2025 16:16:02 GMT)
Full text and
rfc822 format available.
Message #47 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> As discussed in the other thread, this performs fairly few
> charpos->bytepos conversions, actually. Instead it spends most of its
> time inside the `diffseq.h` code. You could try something like:
>
> (require 'subr-x)
>
> (defun elb-replace-region-contents-entry ()
> (with-temp-buffer
> (let ((step (apply #'concat (make-list 2000 "🙂été👶🏿 "))))
> (dotimes (_ (/ 500000 (length step)))
> (insert step))
>
> (dotimes (_ 100000)
> (let* ((a (+ (point-min) (random (buffer-size))))
> (b (min (+ a (length step) (random 512) -256) (point-max))))
> (replace-region-contents a b (lambda () step)))))))
>
> which should spend more time in Emacs's code (especially in
> `buffer_chars_equal`) than in `diffseq.h`
Tried to run that on my mini, but it didn't finish in ca. 2h. So I tried
to call that function on my MB, and had to kill my Emacs :-).
Anyway, the other benchmarks showed an improvement over master, so I'm
content.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Wed, 23 Apr 2025 04:48:02 GMT)
Full text and
rfc822 format available.
Message #50 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>>>> PS. BTW, a small procedural thing. Instead of merging master into a
>>>> scratch/ branch, I recommend deleting the branch, rebasing it on
>>>> master, and then pushing it again. This way, when we later merge it
>>>> into master, we avoid the merge commits, and the history is kept
>>>> clean and more easily reviewable. Not the end of the world either
>>>> way, but something to consider.
>>>
>>> Sorry, too much work :-).
>>>
>>> (For reviewing a branch with merges from master I recommend to find the
>>> latest merge commit, take the parent commit on the master side, and
>>> range diff with that. (If you have the merge in the reflog, that
>>> speeds up finding the latest merge, but that's only the case if you did
>>> the merge in that repo.))
>>
>> git diff origin/master...origin/scratch/text-index
>>
>> should take care of it.
>>
>> If there's interest, I could take care of rebasing (and improving the
>> commit messages on the rebased commits) before merging into `master`.
>
> Please consider the branch yours, if you like, as far as I am concerned
> :-). Because...
>
>>> I consider this feature done when that is in, and play with arm64 NEON a
>>> bit instead. In C++ ;-).
>
> ...of that-
It just appeared to me that I better ask instead of making assumptions:
When I landed tty child frames in master, there was a certain procedure
how to do that. See
https://yhetil.org/emacs-devel/m2ldwdrnds.fsf <at> gmail.com/
where I asked about this, and see Eli's reply. In essence: No rebasing,
squashing, and so on. Following that procedure, is why I posted the
change-log-style commit message for such a merge here.
My assumption up to a few minutes ago was that that procedure has
changed? Which I wouldn't know because I'm not following the lists for
some time. But, as I said, now I'm not really sure...
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Wed, 23 Apr 2025 13:16:02 GMT)
Full text and
rfc822 format available.
Message #53 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> Cc: Stefan Kangas <stefankangas <at> gmail.com>, 77924 <at> debbugs.gnu.org
> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Date: Wed, 23 Apr 2025 06:47:21 +0200
>
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>
> >>>> PS. BTW, a small procedural thing. Instead of merging master into a
> >>>> scratch/ branch, I recommend deleting the branch, rebasing it on
> >>>> master, and then pushing it again. This way, when we later merge it
> >>>> into master, we avoid the merge commits, and the history is kept
> >>>> clean and more easily reviewable. Not the end of the world either
> >>>> way, but something to consider.
> >>>
> >>> Sorry, too much work :-).
> >>>
> >>> (For reviewing a branch with merges from master I recommend to find the
> >>> latest merge commit, take the parent commit on the master side, and
> >>> range diff with that. (If you have the merge in the reflog, that
> >>> speeds up finding the latest merge, but that's only the case if you did
> >>> the merge in that repo.))
> >>
> >> git diff origin/master...origin/scratch/text-index
> >>
> >> should take care of it.
> >>
> >> If there's interest, I could take care of rebasing (and improving the
> >> commit messages on the rebased commits) before merging into `master`.
> >
> > Please consider the branch yours, if you like, as far as I am concerned
> > :-). Because...
> >
> >>> I consider this feature done when that is in, and play with arm64 NEON a
> >>> bit instead. In C++ ;-).
> >
> > ...of that-
>
> It just appeared to me that I better ask instead of making assumptions:
> When I landed tty child frames in master, there was a certain procedure
> how to do that. See
>
> https://yhetil.org/emacs-devel/m2ldwdrnds.fsf <at> gmail.com/
>
> where I asked about this, and see Eli's reply. In essence: No rebasing,
> squashing, and so on. Following that procedure, is why I posted the
> change-log-style commit message for such a merge here.
>
> My assumption up to a few minutes ago was that that procedure has
> changed
It hasn't changed.
Some people like to rebase, others dislike rebasing. (Full
disclosure: I'm in the latter camp.) Since we decided not to care
either way, we don't request contributors to do it one way or the
other: they can do it however they like, and we will accept that
regardless.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Wed, 23 Apr 2025 13:28:02 GMT)
Full text and
rfc822 format available.
Message #56 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> My assumption up to a few minutes ago was that that procedure has
>> changed
>
> It hasn't changed.
>
> Some people like to rebase, others dislike rebasing. (Full
> disclosure: I'm in the latter camp.)
(I'm in the same camp.)
> Since we decided not to care either way, we don't request contributors
> to do it one way or the other: they can do it however they like, and
> we will accept that regardless.
Thanks for the clarification!
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Wed, 23 Apr 2025 14:35:03 GMT)
Full text and
rfc822 format available.
Message #59 received at 77924 <at> debbugs.gnu.org (full text, mbox):
FWIW, I'm not a great fan of rebasing either. I did rebase the branch
last night not for rebasing's sake but because I felt there was a need
for more detailed commit messages.
In any case, any objection to merging the branch?
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Wed, 23 Apr 2025 14:42:07 GMT)
Full text and
rfc822 format available.
Message #62 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, stefankangas <at> gmail.com,
> 77924 <at> debbugs.gnu.org
> Date: Wed, 23 Apr 2025 10:34:11 -0400
>
> FWIW, I'm not a great fan of rebasing either. I did rebase the branch
> last night not for rebasing's sake but because I felt there was a need
> for more detailed commit messages.
>
> In any case, any objection to merging the branch?
I'd like to have a closer review of it first, so please wait for a
while. When I skimmed it, I remember having several, hopefully minor,
aspects that stood out, and I want to take a closer look.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Wed, 23 Apr 2025 14:48:09 GMT)
Full text and
rfc822 format available.
Message #65 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> Cc: gerd.moellmann <at> gmail.com, stefankangas <at> gmail.com, 77924 <at> debbugs.gnu.org
> Date: Wed, 23 Apr 2025 17:41:18 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
>
> > From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> > Cc: Eli Zaretskii <eliz <at> gnu.org>, stefankangas <at> gmail.com,
> > 77924 <at> debbugs.gnu.org
> > Date: Wed, 23 Apr 2025 10:34:11 -0400
> >
> > FWIW, I'm not a great fan of rebasing either. I did rebase the branch
> > last night not for rebasing's sake but because I felt there was a need
> > for more detailed commit messages.
> >
> > In any case, any objection to merging the branch?
>
> I'd like to have a closer review of it first, so please wait for a
> while. When I skimmed it, I remember having several, hopefully minor,
> aspects that stood out, and I want to take a closer look.
Btw, did I miss some benchmarks? I presume that the main motivation
for this branch is to speed up markers and charpos-to-bytepos
conversions, is that right? If so, the only benchmark I saw shows only
a very mild speed-up, but maybe I missed something?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Wed, 23 Apr 2025 15:37:05 GMT)
Full text and
rfc822 format available.
Message #68 received at 77924 <at> debbugs.gnu.org (full text, mbox):
[ Hi Martin, need your opinion on `marker-last-position`. ]
Eli wrote:
>> I'd like to have a closer review of it first, so please wait for a
>> while. When I skimmed it, I remember having several, hopefully minor,
>> aspects that stood out, and I want to take a closer look.
Please do. I hope my rebase makes it easier. 🙂
BTW, during my rebasing I noticed the following incompatibility:
commit 89ad510b2ebd5ed3ed8477e4a45ada3402bcedca
Author: Gerd Möllmann <gerd.moellmann <at> gmail.com>
Date: Thu Apr 17 08:42:52 2025 +0200
marker-vector.c: Move marker's position info to the array
makes it "impossible" to implement `marker-last-position`.
AFAICT this function is not used outside of Emacs at all (based on
a search of all *ELPA packages), but it *is* used in
`window--state-put-2`, since the following commit:
commit 912e37b811107768e0cb3bc95184177f817dbdb2
Author: Martin Rudalics <rudalics <at> gmx.at>
Date: Mon Mar 4 10:33:49 2024 +0100
Fix 'set-window-configuration' and 'window-state-put'
What's the impact of breaking `marker-last-position`?
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Wed, 23 Apr 2025 16:11:03 GMT)
Full text and
rfc822 format available.
Message #71 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> FWIW, I'm not a great fan of rebasing either. I did rebase the branch
> last night not for rebasing's sake but because I felt there was a need
> for more detailed commit messages.
>
> In any case, any objection to merging the branch?
BTW, while porting back your changes, I got
1 emacs-git … am --3way -- /Users/gerd/emacs/github/cl-packages/0003-marker_vector_adjust_for_delete-Delete-function.patch
Markdown-style quotes in commit message
Markdown-style quotes in commit message
Commit aborted; please see the file CONTRIBUTE
:-)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Wed, 23 Apr 2025 16:24:03 GMT)
Full text and
rfc822 format available.
Message #74 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> Gerd and I played with some micro benchmarks to confirm that the
> pathological behaviors are indeed solved, but other than that, the focus
> was on making sure the branch does not make the normal case slower
> (contrary to my sorted-array-of-markers-with-gap).
And, not to forget, it removed the cache markers which piled up a bit in
igc. That was actually my original motivation at the beginning.
Very nice, but so far only for. And, I don't know if that's wishful
thinking, but it feels a bit snappier in interactive use.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Wed, 23 Apr 2025 16:30:07 GMT)
Full text and
rfc822 format available.
Message #77 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> Btw, did I miss some benchmarks? I presume that the main motivation
> for this branch is to speed up markers and charpos-to-bytepos
> conversions, is that right? If so, the only benchmark I saw shows only
> a very mild speed-up, but maybe I missed something?
AFAICT we have tuned the current charpos<->bytepos conversion code such
that its worst case virtually never shows up in practice.
Supposedly Ihor has a use case where that conversion still can make
a noticeable difference, but I don't know if he tried the `text-index`
branch to confirm that it makes a measurable difference on his case.
Gerd and I played with some micro benchmarks to confirm that the
pathological behaviors are indeed solved, but other than that, the focus
was on making sure the branch does not make the normal case slower
(contrary to my sorted-array-of-markers-with-gap).
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 05:02:02 GMT)
Full text and
rfc822 format available.
Message #80 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: gerd.moellmann <at> gmail.com, stefankangas <at> gmail.com,
> 77924 <at> debbugs.gnu.org, Ihor Radchenko <yantar92 <at> posteo.net>
> Date: Wed, 23 Apr 2025 11:41:43 -0400
>
> > Btw, did I miss some benchmarks? I presume that the main motivation
> > for this branch is to speed up markers and charpos-to-bytepos
> > conversions, is that right? If so, the only benchmark I saw shows only
> > a very mild speed-up, but maybe I missed something?
>
> AFAICT we have tuned the current charpos<->bytepos conversion code such
> that its worst case virtually never shows up in practice.
>
> Supposedly Ihor has a use case where that conversion still can make
> a noticeable difference, but I don't know if he tried the `text-index`
> branch to confirm that it makes a measurable difference on his case.
>
> Gerd and I played with some micro benchmarks to confirm that the
> pathological behaviors are indeed solved, but other than that, the focus
> was on making sure the branch does not make the normal case slower
> (contrary to my sorted-array-of-markers-with-gap).
Still, for performance-oriented changes, it would be nice to see some
benchmarks. Scrolling through some large file with many non-ASCII
characters comes to mind (the Unicode UCD has quite a few of such
files).
Also, I believe there were cases where a lot of conversions
created gobs of markers which slowed down things; it would be good to
run those cases to see how they are affected.
And finally, how about running the whole benchmark suite we have, in
case it uncovers some unexpected results?
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 05:43:02 GMT)
Full text and
rfc822 format available.
Message #83 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
>> Cc: gerd.moellmann <at> gmail.com, stefankangas <at> gmail.com,
>> 77924 <at> debbugs.gnu.org, Ihor Radchenko <yantar92 <at> posteo.net>
>> Date: Wed, 23 Apr 2025 11:41:43 -0400
>>
>> > Btw, did I miss some benchmarks? I presume that the main motivation
>> > for this branch is to speed up markers and charpos-to-bytepos
>> > conversions, is that right? If so, the only benchmark I saw shows only
>> > a very mild speed-up, but maybe I missed something?
>>
>> AFAICT we have tuned the current charpos<->bytepos conversion code such
>> that its worst case virtually never shows up in practice.
>>
>> Supposedly Ihor has a use case where that conversion still can make
>> a noticeable difference, but I don't know if he tried the `text-index`
>> branch to confirm that it makes a measurable difference on his case.
>>
>> Gerd and I played with some micro benchmarks to confirm that the
>> pathological behaviors are indeed solved, but other than that, the focus
>> was on making sure the branch does not make the normal case slower
>> (contrary to my sorted-array-of-markers-with-gap).
>
> Still, for performance-oriented changes, it would be nice to see some
> benchmarks. Scrolling through some large file with many non-ASCII
> characters comes to mind (the Unicode UCD has quite a few of such
> files).
>
> Also, I believe there were cases where a lot of conversions
> created gobs of markers which slowed down things; it would be good to
> run those cases to see how they are affected.
>
> And finally, how about running the whole benchmark suite we have, in
> case it uncovers some unexpected results?
I can offer to run benchmarks on my Mac mini.
But we should agree upon what elisp-benchmark files I should use and on
the number of runs, because running them takes an eternity, and because
some of the benchmarks Stef (and me for elb-search) ran are not in the
"official" elisp-benchmarks package.
Don't know about the scrolling benchmark. Is what is in elisp-benchmarks
enough? If not, can you please give me a bencnhmark file for what you
want benchmarked?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 06:02:02 GMT)
Full text and
rfc822 format available.
Message #86 received at 77924 <at> debbugs.gnu.org (full text, mbox):
[வியாழன் ஏப்ரல் 24, 2025] Eli Zaretskii wrote:
>> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
>> Cc: gerd.moellmann <at> gmail.com, stefankangas <at> gmail.com,
>> 77924 <at> debbugs.gnu.org, Ihor Radchenko <yantar92 <at> posteo.net>
>> Date: Wed, 23 Apr 2025 11:41:43 -0400
>>
>> > Btw, did I miss some benchmarks? I presume that the main motivation
>> > for this branch is to speed up markers and charpos-to-bytepos
>> > conversions, is that right? If so, the only benchmark I saw shows only
>> > a very mild speed-up, but maybe I missed something?
>>
>> AFAICT we have tuned the current charpos<->bytepos conversion code such
>> that its worst case virtually never shows up in practice.
>>
>> Supposedly Ihor has a use case where that conversion still can make
>> a noticeable difference, but I don't know if he tried the `text-index`
>> branch to confirm that it makes a measurable difference on his case.
>>
>> Gerd and I played with some micro benchmarks to confirm that the
>> pathological behaviors are indeed solved, but other than that, the focus
>> was on making sure the branch does not make the normal case slower
>> (contrary to my sorted-array-of-markers-with-gap).
>
> Still, for performance-oriented changes, it would be nice to see some
> benchmarks. Scrolling through some large file with many non-ASCII
> characters comes to mind (the Unicode UCD has quite a few of such
> files).
Is there a standard benchmark code that I can try for this? I have such
a large HTML file with Tamil text, and I can scroll through the buffer
produced by shr-render-buffer.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 06:30:02 GMT)
Full text and
rfc822 format available.
Message #89 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Visuwesh <visuweshm <at> gmail.com> writes:
> [வியாழன் ஏப்ரல் 24, 2025] Eli Zaretskii wrote:
>
>>> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
>>> Cc: gerd.moellmann <at> gmail.com, stefankangas <at> gmail.com,
>>> 77924 <at> debbugs.gnu.org, Ihor Radchenko <yantar92 <at> posteo.net>
>>> Date: Wed, 23 Apr 2025 11:41:43 -0400
>>>
>>> > Btw, did I miss some benchmarks? I presume that the main motivation
>>> > for this branch is to speed up markers and charpos-to-bytepos
>>> > conversions, is that right? If so, the only benchmark I saw shows only
>>> > a very mild speed-up, but maybe I missed something?
>>>
>>> AFAICT we have tuned the current charpos<->bytepos conversion code such
>>> that its worst case virtually never shows up in practice.
>>>
>>> Supposedly Ihor has a use case where that conversion still can make
>>> a noticeable difference, but I don't know if he tried the `text-index`
>>> branch to confirm that it makes a measurable difference on his case.
>>>
>>> Gerd and I played with some micro benchmarks to confirm that the
>>> pathological behaviors are indeed solved, but other than that, the focus
>>> was on making sure the branch does not make the normal case slower
>>> (contrary to my sorted-array-of-markers-with-gap).
>>
>> Still, for performance-oriented changes, it would be nice to see some
>> benchmarks. Scrolling through some large file with many non-ASCII
>> characters comes to mind (the Unicode UCD has quite a few of such
>> files).
>
> Is there a standard benchmark code that I can try for this? I have such
> a large HTML file with Tamil text, and I can scroll through the buffer
> produced by shr-render-buffer.
The elisp-benchmarks package has an elb-scroll.el, in its benchmarks
directory. AFAICT, that loads an xmenu.c from the resources
sub-directory of the benchmarks.
You could copy that, or modify it.
P.S.
One can set the benchmarks directory to somewhere else, and run
benchmarks like this:
(setq elb-bench-directory "~/emacs/notes/code/benchmarks")
(elisp-benchmarks-run ".*replace-region-contents.*" t 100)
The 100 is the number of runs, which should be chosen high enough that
the "err" column in the result buffer is reasonably low. I'd start with
1 to see how long that takes, and then increase it.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 07:52:01 GMT)
Full text and
rfc822 format available.
Message #92 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, stefankangas <at> gmail.com,
> 77924 <at> debbugs.gnu.org, yantar92 <at> posteo.net
> Date: Thu, 24 Apr 2025 07:42:21 +0200
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > Still, for performance-oriented changes, it would be nice to see some
> > benchmarks. Scrolling through some large file with many non-ASCII
> > characters comes to mind (the Unicode UCD has quite a few of such
> > files).
> >
> > Also, I believe there were cases where a lot of conversions
> > created gobs of markers which slowed down things; it would be good to
> > run those cases to see how they are affected.
> >
> > And finally, how about running the whole benchmark suite we have, in
> > case it uncovers some unexpected results?
>
> I can offer to run benchmarks on my Mac mini.
Should be fine, I think. I very much doubt that there will be
significant differences between platforms for this kind of changes.
> But we should agree upon what elisp-benchmark files I should use and on
> the number of runs, because running them takes an eternity, and because
> some of the benchmarks Stef (and me for elb-search) ran are not in the
> "official" elisp-benchmarks package.
I trust you and Stefan to decide on that. I myself am not familiar
with the elisp-benchmarks suite well enough to have an opinion that is
of importance. From where I stand, the important part is comparing
the current master with the branch.
> Don't know about the scrolling benchmark. Is what is in elisp-benchmarks
> enough? If not, can you please give me a bencnhmark file for what you
> want benchmarked?
I don't know whether we have it in elisp-benchmarks, but if we don't,
then the following should be good enough:
(defun scroll-up-benchmark ()
(interactive)
(let ((oldgc gcs-done)
(oldtime (float-time)))
(condition-case nil (while t (scroll-up) (redisplay))
(error (message "GCs: %d Elapsed time: %f seconds"
(- gcs-done oldgc) (- (float-time) oldtime))))))
Evaluate this, then invoke "M-x scroll-up-benchmark" in a large buffer
with lots of non-ASCII characters. Compare the timings between the
two versions of Emacs.
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 07:52:02 GMT)
Full text and
rfc822 format available.
Message #95 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Visuwesh <visuweshm <at> gmail.com>
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, gerd.moellmann <at> gmail.com,
> yantar92 <at> posteo.net, stefankangas <at> gmail.com, 77924 <at> debbugs.gnu.org
> Date: Thu, 24 Apr 2025 11:31:33 +0530
>
> > Still, for performance-oriented changes, it would be nice to see some
> > benchmarks. Scrolling through some large file with many non-ASCII
> > characters comes to mind (the Unicode UCD has quite a few of such
> > files).
>
> Is there a standard benchmark code that I can try for this? I have such
> a large HTML file with Tamil text, and I can scroll through the buffer
> produced by shr-render-buffer.
See my response to Gerd a few minutes ago.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 07:58:02 GMT)
Full text and
rfc822 format available.
Message #98 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier
> <monnier <at> iro.umontreal.ca>, yantar92 <at> posteo.net, stefankangas <at> gmail.com,
> 77924 <at> debbugs.gnu.org
> Date: Thu, 24 Apr 2025 08:29:22 +0200
>
> > Is there a standard benchmark code that I can try for this? I have such
> > a large HTML file with Tamil text, and I can scroll through the buffer
> > produced by shr-render-buffer.
>
> The elisp-benchmarks package has an elb-scroll.el, in its benchmarks
> directory. AFAICT, that loads an xmenu.c from the resources
> sub-directory of the benchmarks.
Please test scrolling with non-ASCII characters, not with xmenu.c
(which is pure-ASCII), since charpos-to-bytepos conversions in
pure-ASCII buffers are trivial.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 08:02:02 GMT)
Full text and
rfc822 format available.
Message #101 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> BTW, during my rebasing I noticed the following incompatibility:
>
> commit 89ad510b2ebd5ed3ed8477e4a45ada3402bcedca
> Author: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Date: Thu Apr 17 08:42:52 2025 +0200
>
> marker-vector.c: Move marker's position info to the array
>
> makes it "impossible" to implement `marker-last-position`.
What would 'markerp' then return for a marker whose buffer has been
killed? The idea of 'marker-last-position' is that if some agent (like
a tab bar entry) keeps a reference to a killed buffer, that buffer
cannot be collected and neither its markers, although the latter have
been unchained already. ‘marker-position’ would always return nil for
such a marker but 'marker-last-position' would have worked in the past.
> AFAICT this function is not used outside of Emacs at all (based on
> a search of all *ELPA packages), but it *is* used in
> `window--state-put-2`, since the following commit:
>
> commit 912e37b811107768e0cb3bc95184177f817dbdb2
> Author: Martin Rudalics <rudalics <at> gmx.at>
> Date: Mon Mar 4 10:33:49 2024 +0100
>
> Fix 'set-window-configuration' and 'window-state-put'
>
> What's the impact of breaking `marker-last-position`?
If 'window-restore-killed-buffer-windows' is a function that tries to
resurrect a killed buffer, it could not derive the position of the start
and point position of the buffer in that window. So AFAICT the function
'tab-bar-select-restore-windows' would be affected. Juri can tell more.
martin
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 08:31:03 GMT)
Full text and
rfc822 format available.
Message #104 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> I trust you and Stefan to decide on that. I myself am not familiar
> with the elisp-benchmarks suite well enough to have an opinion that is
> of importance. From where I stand, the important part is comparing
> the current master with the branch.
>
>> Don't know about the scrolling benchmark. Is what is in elisp-benchmarks
>> enough? If not, can you please give me a bencnhmark file for what you
>> want benchmarked?
>
> I don't know whether we have it in elisp-benchmarks, but if we don't,
> then the following should be good enough:
>
> (defun scroll-up-benchmark ()
> (interactive)
> (let ((oldgc gcs-done)
> (oldtime (float-time)))
> (condition-case nil (while t (scroll-up) (redisplay))
> (error (message "GCs: %d Elapsed time: %f seconds"
> (- gcs-done oldgc) (- (float-time) oldtime))))))
>
> Evaluate this, then invoke "M-x scroll-up-benchmark" in a large buffer
> with lots of non-ASCII characters. Compare the timings between the
> two versions of Emacs.
elb-scroll from elisp-bechmarks is basically
(dotimes (_ 10)
(elb-smie-mode)
(goto-char (point-min))
(condition-case nil
(while t (scroll-up nil) (redisplay 'force))
(end-of-buffer nil))))))
looks similar, but I don't know what elb-smie-mode does.
Seems to have something to do with prog-mode, so maybe this depends on
the test file being a C file, which might be interesting for Visuwesh.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 09:11:01 GMT)
Full text and
rfc822 format available.
Message #107 received at 77924 <at> debbugs.gnu.org (full text, mbox):
martin rudalics <rudalics <at> gmx.at> writes:
>> BTW, during my rebasing I noticed the following incompatibility:
>>
>> commit 89ad510b2ebd5ed3ed8477e4a45ada3402bcedca
>> Author: Gerd Möllmann <gerd.moellmann <at> gmail.com>
>> Date: Thu Apr 17 08:42:52 2025 +0200
>>
>> marker-vector.c: Move marker's position info to the array
>>
>> makes it "impossible" to implement `marker-last-position`.
>
> What would 'markerp' then return for a marker whose buffer has been
> killed? The idea of 'marker-last-position' is that if some agent (like
> a tab bar entry) keeps a reference to a killed buffer, that buffer
> cannot be collected and neither its markers, although the latter have
> been unchained already. ‘marker-position’ would always return nil for
> such a marker but 'marker-last-position' would have worked in the past.
>
>> AFAICT this function is not used outside of Emacs at all (based on
>> a search of all *ELPA packages), but it *is* used in
>> `window--state-put-2`, since the following commit:
>>
>> commit 912e37b811107768e0cb3bc95184177f817dbdb2
>> Author: Martin Rudalics <rudalics <at> gmx.at>
>> Date: Mon Mar 4 10:33:49 2024 +0100
>>
>> Fix 'set-window-configuration' and 'window-state-put'
>>
>> What's the impact of breaking `marker-last-position`?
>
> If 'window-restore-killed-buffer-windows' is a function that tries to
> resurrect a killed buffer, it could not derive the position of the start
> and point position of the buffer in that window. So AFAICT the function
> 'tab-bar-select-restore-windows' would be affected. Juri can tell more.
>
> martin
Hm, that might be an unsolvable problem with igc, actually. At least I
don't know ATM how it could be solved.
Let me try to explain. The function below is what igc uses to scan a
marker vector and remove stale references (weak references). It's from
my Emacs, not from feature/igc, but that doesn't play a role. the
situation is the same here and there.
igc.c:
2136 static mps_res_t
2137 fix_marker_vector (mps_ss_t ss, struct Lisp_Vector *v)
2138 {
2139 MPS_SCAN_BEGIN (ss)
2140 {
2141 const ptrdiff_t max_entry = XFIXNUM (v->contents[MARKER_VECTOR_MAX_ENTRY]);
2142 for (ptrdiff_t e = MARKER_VECTOR_HEADER_SIZE;
2143 e <= max_entry; e += MARKER_VECTOR_ENTRY_SIZE)
2144 {
2145 /* Note that we cannot access anything of a marker here because
2146 that is not allowed by MPS while scanning an unrelated
2147 object. This includes MARKERP because that accesses the
2148 header of a marker. */
2149 Lisp_Object *p = &v->contents[e + MARKER_VECTOR_OFFSET_MARKER];
2150 if (!NILP (*p) && !FIXNUMP (*p))
2151 {
2152 IGC_FIX12_OBJ (ss, p);
2153 if (NILP (*p))
2154 {
2155 /* Changed to nil means the weak reference was to a dead
2156 marker. Put on free-list. */
2157 v->contents[e] = v->contents[MARKER_VECTOR_FREE];
2158 v->contents[MARKER_VECTOR_FREE] = make_fixnum (e);
2159 }
2160 }
2161 }
2162 }
2163 MPS_SCAN_END (ss);
2164 return MPS_RES_OK;
2165 }
The problem is that MPS doesn't allow accessing other MPS-managed
objects while working on the marker vector. See the comment at line 2145.
Markers are such MPS-managed objects, so we can't read or change members
of a marker here, not the buffer, not something else.
That restriction is very real, it suffices to add a MARKERP there to
make things literally fall apart when a marker object happens to be
behind a memory barrier, for example.
Like I said, no idea. I've added Helmut and Pip in CC.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 09:19:01 GMT)
Full text and
rfc822 format available.
Message #110 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> martin rudalics <rudalics <at> gmx.at> writes:
>
>>> BTW, during my rebasing I noticed the following incompatibility:
>>>
>>> commit 89ad510b2ebd5ed3ed8477e4a45ada3402bcedca
>>> Author: Gerd Möllmann <gerd.moellmann <at> gmail.com>
>>> Date: Thu Apr 17 08:42:52 2025 +0200
>>>
>>> marker-vector.c: Move marker's position info to the array
>>>
>>> makes it "impossible" to implement `marker-last-position`.
>>
>> What would 'markerp' then return for a marker whose buffer has been
>> killed? The idea of 'marker-last-position' is that if some agent (like
>> a tab bar entry) keeps a reference to a killed buffer, that buffer
>> cannot be collected and neither its markers, although the latter have
>> been unchained already. ‘marker-position’ would always return nil for
>> such a marker but 'marker-last-position' would have worked in the past.
>>
>>> AFAICT this function is not used outside of Emacs at all (based on
>>> a search of all *ELPA packages), but it *is* used in
>>> `window--state-put-2`, since the following commit:
>>>
>>> commit 912e37b811107768e0cb3bc95184177f817dbdb2
>>> Author: Martin Rudalics <rudalics <at> gmx.at>
>>> Date: Mon Mar 4 10:33:49 2024 +0100
>>>
>>> Fix 'set-window-configuration' and 'window-state-put'
>>>
>>> What's the impact of breaking `marker-last-position`?
>>
>> If 'window-restore-killed-buffer-windows' is a function that tries to
>> resurrect a killed buffer, it could not derive the position of the start
>> and point position of the buffer in that window. So AFAICT the function
>> 'tab-bar-select-restore-windows' would be affected. Juri can tell more.
>>
>> martin
>
> Hm, that might be an unsolvable problem with igc, actually. At least I
> don't know ATM how it could be solved.
>
> Let me try to explain. The function below is what igc uses to scan a
> marker vector and remove stale references (weak references). It's from
> my Emacs, not from feature/igc, but that doesn't play a role. the
> situation is the same here and there.
>
> igc.c:
> 2136 static mps_res_t
> 2137 fix_marker_vector (mps_ss_t ss, struct Lisp_Vector *v)
> 2138 {
> 2139 MPS_SCAN_BEGIN (ss)
> 2140 {
> 2141 const ptrdiff_t max_entry = XFIXNUM (v->contents[MARKER_VECTOR_MAX_ENTRY]);
> 2142 for (ptrdiff_t e = MARKER_VECTOR_HEADER_SIZE;
> 2143 e <= max_entry; e += MARKER_VECTOR_ENTRY_SIZE)
> 2144 {
> 2145 /* Note that we cannot access anything of a marker here because
> 2146 that is not allowed by MPS while scanning an unrelated
> 2147 object. This includes MARKERP because that accesses the
> 2148 header of a marker. */
> 2149 Lisp_Object *p = &v->contents[e + MARKER_VECTOR_OFFSET_MARKER];
> 2150 if (!NILP (*p) && !FIXNUMP (*p))
> 2151 {
> 2152 IGC_FIX12_OBJ (ss, p);
> 2153 if (NILP (*p))
> 2154 {
> 2155 /* Changed to nil means the weak reference was to a dead
> 2156 marker. Put on free-list. */
> 2157 v->contents[e] = v->contents[MARKER_VECTOR_FREE];
> 2158 v->contents[MARKER_VECTOR_FREE] = make_fixnum (e);
> 2159 }
> 2160 }
> 2161 }
> 2162 }
> 2163 MPS_SCAN_END (ss);
> 2164 return MPS_RES_OK;
> 2165 }
>
> The problem is that MPS doesn't allow accessing other MPS-managed
> objects while working on the marker vector. See the comment at line 2145.
>
> Markers are such MPS-managed objects, so we can't read or change members
> of a marker here, not the buffer, not something else.
>
> That restriction is very real, it suffices to add a MARKERP there to
> make things literally fall apart when a marker object happens to be
> behind a memory barrier, for example.
>
> Like I said, no idea. I've added Helmut and Pip in CC.
No wait, not a problem :-). If someone from Lisp has that marker in his
hands, it's of course not dead. So, sorry for the confusion.
So, one could, theoretically store the charpos in a field of a marker
when "unchaining" it. Ugly, but doable.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 09:39:01 GMT)
Full text and
rfc822 format available.
Message #113 received at 77924 <at> debbugs.gnu.org (full text, mbox):
[வியாழன் ஏப்ரல் 24, 2025] Gerd Möllmann wrote:
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>> I trust you and Stefan to decide on that. I myself am not familiar
>> with the elisp-benchmarks suite well enough to have an opinion that is
>> of importance. From where I stand, the important part is comparing
>> the current master with the branch.
>>
>>> Don't know about the scrolling benchmark. Is what is in elisp-benchmarks
>>> enough? If not, can you please give me a bencnhmark file for what you
>>> want benchmarked?
>>
>> I don't know whether we have it in elisp-benchmarks, but if we don't,
>> then the following should be good enough:
>>
>> (defun scroll-up-benchmark ()
>> (interactive)
>> (let ((oldgc gcs-done)
>> (oldtime (float-time)))
>> (condition-case nil (while t (scroll-up) (redisplay))
>> (error (message "GCs: %d Elapsed time: %f seconds"
>> (- gcs-done oldgc) (- (float-time) oldtime))))))
>>
>> Evaluate this, then invoke "M-x scroll-up-benchmark" in a large buffer
>> with lots of non-ASCII characters. Compare the timings between the
>> two versions of Emacs.
>
> elb-scroll from elisp-bechmarks is basically
>
> (dotimes (_ 10)
> (elb-smie-mode)
> (goto-char (point-min))
> (condition-case nil
> (while t (scroll-up nil) (redisplay 'force))
> (end-of-buffer nil))))))
>
> looks similar, but I don't know what elb-smie-mode does.
elb-smie-mode's is Stefan's SMIE demo as a C major-mode AFAIU.
> Seems to have something to do with prog-mode, so maybe this depends on
> the test file being a C file, which might be interesting for Visuwesh.
I commented out (elb-smie-mode) form in the above snippet and that
showed a backtrace which I could not explain. Replacing the form with
(text-mode) didn't help either. So I'm running the benchmark with that
form intact, even though the file I'm scrolling through is a text file.
I am running the benchmark currently, and will report the results once
they are done.
Debugger entered--Lisp error: (overflow-error)
round(-0.0e+NaN)
(let ((squares (apply #'+ (mapcar #'(lambda (x) (expt x 2)) errs)))) (round (/ (* 100 (sqrt squares)) elapsed)))
(format "|%2d%%\n" (let ((squares (apply #'+ (mapcar #'(lambda ... ...) errs)))) (round (/ (* 100 (sqrt squares)) elapsed))))
(insert (format "|%2d%%\n" (let ((squares (apply #'+ (mapcar #'... errs)))) (round (/ (* 100 (sqrt squares)) elapsed)))))
(let* ((--cl-var-- tests) (test nil) (l nil) (test-elapsed nil) (test-gcs nil) (test-gc-elapsed nil) (test-err nil) (elapsed 0) (gcs 0) (gc-elapsed 0) (errs nil) (--cl-var-- t)) (while (consp --cl-var--) (setq test (car --cl-var--)) (setq l (gethash test res)) (setq test-elapsed (let* ((--cl-var-- l) (x nil) (--cl-var-- 0)) (while (consp --cl-var--) (setq x (car --cl-var--)) (setq --cl-var-- (+ --cl-var-- (car x))) (setq --cl-var-- (cdr --cl-var--))) --cl-var--)) (setq test-gcs (let* ((--cl-var-- l) (x nil) (--cl-var-- 0)) (while (consp --cl-var--) (setq x (car --cl-var--)) (setq --cl-var-- (+ --cl-var-- (car ...))) (setq --cl-var-- (cdr --cl-var--))) --cl-var--)) (setq test-gc-elapsed (let* ((--cl-var-- l) (x nil) (--cl-var-- 0)) (while (consp --cl-var--) (setq x (car --cl-var--)) (setq --cl-var-- (+ --cl-var-- (car ...))) (setq --cl-var-- (cdr --cl-var--))) --cl-var--)) (setq test-err (if (cdr l) (elb-std-deviation (mapcar #'car l)))) (insert (apply #'format "|%s|%.2f|%.2f|%d|%.2f" test (mapcar #'(lambda (x) (/ x runs)) (list (- test-elapsed test-gc-elapsed) test-gc-elapsed test-gcs test-elapsed)))) (insert (format "|%2d%%\n" (if test-err (round (/ (* 100 test-err) test-elapsed)) -0.0))) (setq elapsed (+ elapsed test-elapsed)) (setq gcs (+ gcs test-gcs)) (setq gc-elapsed (+ gc-elapsed test-gc-elapsed)) (setq errs (nconc errs (list (or test-err -0.0)))) (setq --cl-var-- (cdr --cl-var--)) (setq --cl-var-- nil)) (insert "|-\n") (insert (apply #'format "|total|%.2f|%.2f|%d|%.2f" (mapcar #'(lambda (x) (/ x runs)) (list (- elapsed gc-elapsed) gc-elapsed gcs elapsed)))) (insert (format "|%2d%%\n" (let ((squares (apply #'+ (mapcar ... errs)))) (round (/ (* 100 (sqrt squares)) elapsed))))) nil)
elb--display(("scroll") #<hash-table equal 0/0 0x10e69c524987 ...> 1)
(while (>= (setq --cl-var-- (1- --cl-var--)) 0) (message "Iteration number: %d" i) (let* ((--cl-var-- tests) (test nil) (entry-point nil) (--cl-var-- t)) (while (consp --cl-var--) (setq test (car --cl-var--)) (setq entry-point (intern (concat "elb-" test "-entry"))) (garbage-collect) (message "Running %s..." test) (let ((time (condition-case err (condition-case nil ... ...) (... ... nil)))) (if time (progn (let* (... ...) (puthash v ... v))))) (setq --cl-var-- (cdr --cl-var--)) (setq --cl-var-- nil)) nil) (elb--display tests res i) (setq i (+ i 1)))
(let* ((runs (or runs elb-runs)) (--cl-var-- runs) (i 1)) (while (>= (setq --cl-var-- (1- --cl-var--)) 0) (message "Iteration number: %d" i) (let* ((--cl-var-- tests) (test nil) (entry-point nil) (--cl-var-- t)) (while (consp --cl-var--) (setq test (car --cl-var--)) (setq entry-point (intern (concat "elb-" test "-entry"))) (garbage-collect) (message "Running %s..." test) (let ((time (condition-case err ... ...))) (if time (progn (let* ... ...)))) (setq --cl-var-- (cdr --cl-var--)) (setq --cl-var-- nil)) nil) (elb--display tests res i) (setq i (+ i 1))) nil)
(let ((tests (let ((names 'nil)) (mapatoms #'(lambda (s) (let ... ...))) (sort names #'string-lessp)))) (let* ((runs (or runs elb-runs)) (--cl-var-- runs) (i 1)) (while (>= (setq --cl-var-- (1- --cl-var--)) 0) (message "Iteration number: %d" i) (let* ((--cl-var-- tests) (test nil) (entry-point nil) (--cl-var-- t)) (while (consp --cl-var--) (setq test (car --cl-var--)) (setq entry-point (intern (concat "elb-" test "-entry"))) (garbage-collect) (message "Running %s..." test) (let ((time ...)) (if time (progn ...))) (setq --cl-var-- (cdr --cl-var--)) (setq --cl-var-- nil)) nil) (elb--display tests res i) (setq i (+ i 1))) nil))
(let* ((native-comp-speed elb-speed) (compilation-safety elb-safety) (compile-function (if (featurep 'native-compile) #'native-compile #'byte-compile-file)) (res (make-hash-table :test #'equal)) (sources (directory-files elb-bench-directory t "\\.el\\'")) (test-sources (if selector (let* ((--cl-var-- sources) (f nil) (--cl-var-- nil)) (while (consp --cl-var--) (setq f (car --cl-var--)) (if (string-match selector f) (progn ...)) (setq --cl-var-- (cdr --cl-var--))) (nreverse --cl-var--)) sources))) (if recompile (progn (mapc #'(lambda (f) (message "Compiling... %s" f) (funcall compile-function f)) test-sources))) (mapc #'(lambda (file) (condition-case err (load file) ((debug error) (message "Error loading: %S" err) nil))) (mapcar (if (and (featurep 'native-compile) (fboundp 'comp-el-to-eln-filename)) #'comp-el-to-eln-filename #'file-name-sans-extension) test-sources)) (let ((tests (let ((names 'nil)) (mapatoms #'(lambda ... ...)) (sort names #'string-lessp)))) (let* ((runs (or runs elb-runs)) (--cl-var-- runs) (i 1)) (while (>= (setq --cl-var-- (1- --cl-var--)) 0) (message "Iteration number: %d" i) (let* ((--cl-var-- tests) (test nil) (entry-point nil) (--cl-var-- t)) (while (consp --cl-var--) (setq test (car --cl-var--)) (setq entry-point (intern ...)) (garbage-collect) (message "Running %s..." test) (let (...) (if time ...)) (setq --cl-var-- (cdr --cl-var--)) (setq --cl-var-- nil)) nil) (elb--display tests res i) (setq i (+ i 1))) nil)))
(progn (if --cl-rest-- (signal 'wrong-number-of-arguments (list 'elisp-benchmarks-run (+ 3 (length --cl-rest--))))) (let* ((native-comp-speed elb-speed) (compilation-safety elb-safety) (compile-function (if (featurep 'native-compile) #'native-compile #'byte-compile-file)) (res (make-hash-table :test #'equal)) (sources (directory-files elb-bench-directory t "\\.el\\'")) (test-sources (if selector (let* ((--cl-var-- sources) (f nil) (--cl-var-- nil)) (while (consp --cl-var--) (setq f ...) (if ... ...) (setq --cl-var-- ...)) (nreverse --cl-var--)) sources))) (if recompile (progn (mapc #'(lambda (f) (message "Compiling... %s" f) (funcall compile-function f)) test-sources))) (mapc #'(lambda (file) (condition-case err (load file) ((debug error) (message "Error loading: %S" err) nil))) (mapcar (if (and (featurep 'native-compile) (fboundp 'comp-el-to-eln-filename)) #'comp-el-to-eln-filename #'file-name-sans-extension) test-sources)) (let ((tests (let ((names ...)) (mapatoms #'...) (sort names #'string-lessp)))) (let* ((runs (or runs elb-runs)) (--cl-var-- runs) (i 1)) (while (>= (setq --cl-var-- (1- --cl-var--)) 0) (message "Iteration number: %d" i) (let* ((--cl-var-- tests) (test nil) (entry-point nil) (--cl-var-- t)) (while (consp --cl-var--) (setq test ...) (setq entry-point ...) (garbage-collect) (message "Running %s..." test) (let ... ...) (setq --cl-var-- ...) (setq --cl-var-- nil)) nil) (elb--display tests res i) (setq i (+ i 1))) nil))))
(let* ((recompile (if --cl-rest-- (car-safe (prog1 --cl-rest-- (setq --cl-rest-- (cdr --cl-rest--)))) t)) (runs (car-safe (prog1 --cl-rest-- (setq --cl-rest-- (cdr --cl-rest--)))))) (progn (if --cl-rest-- (signal 'wrong-number-of-arguments (list 'elisp-benchmarks-run (+ 3 (length --cl-rest--))))) (let* ((native-comp-speed elb-speed) (compilation-safety elb-safety) (compile-function (if (featurep 'native-compile) #'native-compile #'byte-compile-file)) (res (make-hash-table :test #'equal)) (sources (directory-files elb-bench-directory t "\\.el\\'")) (test-sources (if selector (let* (... ... ...) (while ... ... ... ...) (nreverse --cl-var--)) sources))) (if recompile (progn (mapc #'(lambda ... ... ...) test-sources))) (mapc #'(lambda (file) (condition-case err (load file) (... ... nil))) (mapcar (if (and (featurep ...) (fboundp ...)) #'comp-el-to-eln-filename #'file-name-sans-extension) test-sources)) (let ((tests (let (...) (mapatoms ...) (sort names ...)))) (let* ((runs (or runs elb-runs)) (--cl-var-- runs) (i 1)) (while (>= (setq --cl-var-- ...) 0) (message "Iteration number: %d" i) (let* (... ... ... ...) (while ... ... ... ... ... ... ... ...) nil) (elb--display tests res i) (setq i (+ i 1))) nil)))))
elisp-benchmarks-run("elb-scroll" t 100)
(progn (elisp-benchmarks-run "elb-scroll" t 100))
eval((progn (elisp-benchmarks-run "elb-scroll" t 100)) t)
elisp--eval-last-sexp(nil)
#f(compiled-function () #<bytecode 0xf3e77a510d12>)()
handler-bind-1(#f(compiled-function () #<bytecode 0xf3e77a510d12>) (error) eval-expression--debug)
eval-last-sexp(nil)
funcall-interactively(eval-last-sexp nil)
call-interactively(eval-last-sexp nil nil)
command-execute(eval-last-sexp)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 10:00:02 GMT)
Full text and
rfc822 format available.
Message #116 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> So, one could, theoretically store the charpos in a field of a marker
> when "unchaining" it. Ugly, but doable.
And one could do something practically, which I now pushed to savannah.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 10:07:06 GMT)
Full text and
rfc822 format available.
Message #119 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Visuwesh <visuweshm <at> gmail.com> writes:
> [வியாழன் ஏப்ரல் 24, 2025] Gerd Möllmann wrote:
>
>> looks similar, but I don't know what elb-smie-mode does.
>
> elb-smie-mode's is Stefan's SMIE demo as a C major-mode AFAIU.
Thanks, but I have no idea what that does.
>
>> Seems to have something to do with prog-mode, so maybe this depends on
>> the test file being a C file, which might be interesting for Visuwesh.
>
> I commented out (elb-smie-mode) form in the above snippet and that
> showed a backtrace which I could not explain. Replacing the form with
> (text-mode) didn't help either. So I'm running the benchmark with that
> form intact, even though the file I'm scrolling through is a text file.
> I am running the benchmark currently, and will report the results once
> they are done.
>
> Debugger entered--Lisp error: (overflow-error)
> round(-0.0e+NaN)
> (let ((squares (apply #'+ (mapcar #'(lambda (x) (expt x 2)) errs))))
> (round (/ (* 100 (sqrt squares)) elapsed)))
> (format "|%2d%%\n" (let ((squares (apply #'+ (mapcar #'(lambda ...
> ...) errs)))) (round (/ (* 100 (sqrt squares)) elapsed))))
> (insert (format "|%2d%%\n" (let ((squares (apply #'+ (mapcar #'...
> errs)))) (round (/ (* 100 (sqrt squares)) elapsed)))))
> (let* ((--cl-var-- tests) (test nil) (l nil) (test-elapsed nil)
> (test-gcs nil) (test-gc-elapsed nil) (test-err nil) (elapsed 0) (gcs
> 0) (gc-elapsed 0) (errs nil) (--cl-var-- t)) (while (consp --cl-var--)
> (setq test (car --cl-var--)) (setq l (gethash test res)) (setq
> test-elapsed (let* ((--cl-var-- l) (x nil) (--cl-var-- 0)) (while
> (consp --cl-var--) (setq x (car --cl-var--)) (setq --cl-var-- (+
> --cl-var-- (car x))) (setq --cl-var-- (cdr --cl-var--))) --cl-var--))
> (setq test-gcs (let* ((--cl-var-- l) (x nil) (--cl-var-- 0)) (while
> (consp --cl-var--) (setq x (car --cl-var--)) (setq --cl-var-- (+
> --cl-var-- (car ...))) (setq --cl-var-- (cdr --cl-var--)))
> --cl-var--)) (setq test-gc-elapsed (let* ((--cl-var-- l) (x nil)
> (--cl-var-- 0)) (while (consp --cl-var--) (setq x (car --cl-var--))
> (setq --cl-var-- (+ --cl-var-- (car ...))) (setq --cl-var-- (cdr
> --cl-var--))) --cl-var--)) (setq test-err (if (cdr l)
> (elb-std-deviation (mapcar #'car l)))) (insert (apply #'format
Something went wrong when computing the "err" values, apparently. Maybe
it'll work if you increase the number of runs the NaN goes away?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 10:34:03 GMT)
Full text and
rfc822 format available.
Message #122 received at 77924 <at> debbugs.gnu.org (full text, mbox):
[வியாழன் ஏப்ரல் 24, 2025] Gerd Möllmann wrote:
>>> Seems to have something to do with prog-mode, so maybe this depends on
>>> the test file being a C file, which might be interesting for Visuwesh.
>>
>> I commented out (elb-smie-mode) form in the above snippet and that
>> showed a backtrace which I could not explain. Replacing the form with
>> (text-mode) didn't help either. So I'm running the benchmark with that
>> form intact, even though the file I'm scrolling through is a text file.
>> I am running the benchmark currently, and will report the results once
>> they are done.
>>
>> Debugger entered--Lisp error: (overflow-error)
>> round(-0.0e+NaN)
>> (let ((squares (apply #'+ (mapcar #'(lambda (x) (expt x 2)) errs))))
>> (round (/ (* 100 (sqrt squares)) elapsed)))
>> (format "|%2d%%\n" (let ((squares (apply #'+ (mapcar #'(lambda ...
>> ...) errs)))) (round (/ (* 100 (sqrt squares)) elapsed))))
>> (insert (format "|%2d%%\n" (let ((squares (apply #'+ (mapcar #'...
>> errs)))) (round (/ (* 100 (sqrt squares)) elapsed)))))
>> (let* ((--cl-var-- tests) (test nil) (l nil) (test-elapsed nil)
>> (test-gcs nil) (test-gc-elapsed nil) (test-err nil) (elapsed 0) (gcs
>> 0) (gc-elapsed 0) (errs nil) (--cl-var-- t)) (while (consp --cl-var--)
>> (setq test (car --cl-var--)) (setq l (gethash test res)) (setq
>> test-elapsed (let* ((--cl-var-- l) (x nil) (--cl-var-- 0)) (while
>> (consp --cl-var--) (setq x (car --cl-var--)) (setq --cl-var-- (+
>> --cl-var-- (car x))) (setq --cl-var-- (cdr --cl-var--))) --cl-var--))
>> (setq test-gcs (let* ((--cl-var-- l) (x nil) (--cl-var-- 0)) (while
>> (consp --cl-var--) (setq x (car --cl-var--)) (setq --cl-var-- (+
>> --cl-var-- (car ...))) (setq --cl-var-- (cdr --cl-var--)))
>> --cl-var--)) (setq test-gc-elapsed (let* ((--cl-var-- l) (x nil)
>> (--cl-var-- 0)) (while (consp --cl-var--) (setq x (car --cl-var--))
>> (setq --cl-var-- (+ --cl-var-- (car ...))) (setq --cl-var-- (cdr
>> --cl-var--))) --cl-var--)) (setq test-err (if (cdr l)
>> (elb-std-deviation (mapcar #'car l)))) (insert (apply #'format
>
> Something went wrong when computing the "err" values, apparently. Maybe
> it'll work if you increase the number of runs the NaN goes away?
Oddly enough, it throws an error as soon as I start the benchmark. I
don't think even a single iteration in the loop of elb-scroll-entry is
complete, let alone a complete run.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 11:17:08 GMT)
Full text and
rfc822 format available.
Message #125 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>
>> So, one could, theoretically store the charpos in a field of a marker
>> when "unchaining" it. Ugly, but doable.
>
> And one could do something practically, which I now pushed to savannah.
Can you explain how that works in the non-indirect buffer case? AFAICT,
we call marker_vector_reset but not marker_vector_remove in that case,
and there is no code in marker_vector_reset to remember the last
charpos. And the comment in marker_vector_reset sounds like it's not
called at all in the IGC case?
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 11:25:09 GMT)
Full text and
rfc822 format available.
Message #128 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Visuwesh <visuweshm <at> gmail.com> writes:
> [வியாழன் ஏப்ரல் 24, 2025] Gerd Möllmann wrote:
>
>>>> Seems to have something to do with prog-mode, so maybe this depends on
>>>> the test file being a C file, which might be interesting for Visuwesh.
>>>
>>> I commented out (elb-smie-mode) form in the above snippet and that
>>> showed a backtrace which I could not explain. Replacing the form with
>>> (text-mode) didn't help either. So I'm running the benchmark with that
>>> form intact, even though the file I'm scrolling through is a text file.
>>> I am running the benchmark currently, and will report the results once
>>> they are done.
>>>
>>> Debugger entered--Lisp error: (overflow-error)
>>> round(-0.0e+NaN)
>>> (let ((squares (apply #'+ (mapcar #'(lambda (x) (expt x 2)) errs))))
>>> (round (/ (* 100 (sqrt squares)) elapsed)))
>>> (format "|%2d%%\n" (let ((squares (apply #'+ (mapcar #'(lambda ...
>>> ...) errs)))) (round (/ (* 100 (sqrt squares)) elapsed))))
>>> (insert (format "|%2d%%\n" (let ((squares (apply #'+ (mapcar #'...
>>> errs)))) (round (/ (* 100 (sqrt squares)) elapsed)))))
>>> (let* ((--cl-var-- tests) (test nil) (l nil) (test-elapsed nil)
>>> (test-gcs nil) (test-gc-elapsed nil) (test-err nil) (elapsed 0) (gcs
>>> 0) (gc-elapsed 0) (errs nil) (--cl-var-- t)) (while (consp --cl-var--)
>>> (setq test (car --cl-var--)) (setq l (gethash test res)) (setq
>>> test-elapsed (let* ((--cl-var-- l) (x nil) (--cl-var-- 0)) (while
>>> (consp --cl-var--) (setq x (car --cl-var--)) (setq --cl-var-- (+
>>> --cl-var-- (car x))) (setq --cl-var-- (cdr --cl-var--))) --cl-var--))
>>> (setq test-gcs (let* ((--cl-var-- l) (x nil) (--cl-var-- 0)) (while
>>> (consp --cl-var--) (setq x (car --cl-var--)) (setq --cl-var-- (+
>>> --cl-var-- (car ...))) (setq --cl-var-- (cdr --cl-var--)))
>>> --cl-var--)) (setq test-gc-elapsed (let* ((--cl-var-- l) (x nil)
>>> (--cl-var-- 0)) (while (consp --cl-var--) (setq x (car --cl-var--))
>>> (setq --cl-var-- (+ --cl-var-- (car ...))) (setq --cl-var-- (cdr
>>> --cl-var--))) --cl-var--)) (setq test-err (if (cdr l)
>>> (elb-std-deviation (mapcar #'car l)))) (insert (apply #'format
>>
>> Something went wrong when computing the "err" values, apparently. Maybe
>> it'll work if you increase the number of runs the NaN goes away?
>
> Oddly enough, it throws an error as soon as I start the benchmark. I
> don't think even a single iteration in the loop of elb-scroll-entry is
> complete, let alone a complete run.
Hm, I can't really get that to run really well either, but while trying
I found that
- it must be run with GUI emacs -Q because it tries to enlarge-window
which it can't on a terminal, and it assumes certain sizes
("stipulated sizes not whatever" error)
- the resources directory containing xmenu.c or whatever must be a
sibling to the benchmarks directory
- The pattern matching the test case doesn't seem to always work. I use
".*scroll.*" but the report at the end shows also "setq" and
"font-lock"???
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 11:30:12 GMT)
Full text and
rfc822 format available.
Message #131 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Pip Cet <pipcet <at> protonmail.com> writes:
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>
>> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>>
>>> So, one could, theoretically store the charpos in a field of a marker
>>> when "unchaining" it. Ugly, but doable.
>>
>> And one could do something practically, which I now pushed to savannah.
>
> Can you explain how that works in the non-indirect buffer case?
Yes, it doesn't :-).
> AFAICT, we call marker_vector_reset but not marker_vector_remove in
> that case, and there is no code in marker_vector_reset to remember the
> last charpos. And the comment in marker_vector_reset sounds like it's
> not called at all in the IGC case?
You mean this:
/* The old GC contains at least one assertion that unchaining markers
in kill-buffer resets the markers' buffers. IGC does not do this,
can't do this, and does not need it. */
What I meant there is what I described in my mistaken mail before, that
it can't do anything with the markers. But if they are dead, who cares.
CLassic thinko.
I'll remove these comments.
Thanks!
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 11:38:09 GMT)
Full text and
rfc822 format available.
Message #134 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> I'll remove these comments.
Show now be on savannah.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 11:45:02 GMT)
Full text and
rfc822 format available.
Message #137 received at 77924 <at> debbugs.gnu.org (full text, mbox):
[வியாழன் ஏப்ரல் 24, 2025] Gerd Möllmann wrote:
> Visuwesh <visuweshm <at> gmail.com> writes:
>
>> [வியாழன் ஏப்ரல் 24, 2025] Gerd Möllmann wrote:
>>
>>>>> Seems to have something to do with prog-mode, so maybe this depends on
>>>>> the test file being a C file, which might be interesting for Visuwesh.
>>>>
>>>> I commented out (elb-smie-mode) form in the above snippet and that
>>>> showed a backtrace which I could not explain. Replacing the form with
>>>> (text-mode) didn't help either. So I'm running the benchmark with that
>>>> form intact, even though the file I'm scrolling through is a text file.
>>>> I am running the benchmark currently, and will report the results once
>>>> they are done.
>>>>
>>>> Debugger entered--Lisp error: (overflow-error)
>>> [...]
>>>
>>> Something went wrong when computing the "err" values, apparently. Maybe
>>> it'll work if you increase the number of runs the NaN goes away?
>>
>> Oddly enough, it throws an error as soon as I start the benchmark. I
>> don't think even a single iteration in the loop of elb-scroll-entry is
>> complete, let alone a complete run.
>
> Hm, I can't really get that to run really well either, but while trying
> I found that
>
> - it must be run with GUI emacs -Q because it tries to enlarge-window
> which it can't on a terminal, and it assumes certain sizes
> ("stipulated sizes not whatever" error)
That's a shame. AFAIU, the non-ASCII benchmark would be relevant
outside composition as well.
> - the resources directory containing xmenu.c or whatever must be a
> sibling to the benchmarks directory
>
> - The pattern matching the test case doesn't seem to always work. I use
> ".*scroll.*" but the report at the end shows also "setq" and
> "font-lock"???
I am using "elb-scroll" as the pattern and it only picks up scroll.
(Though the time it takes to run 10 repeats (so 100 loops) is ~30 mins
here.)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 11:55:08 GMT)
Full text and
rfc822 format available.
Message #140 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Visuwesh <visuweshm <at> gmail.com> writes:
>> - it must be run with GUI emacs -Q because it tries to enlarge-window
>> which it can't on a terminal, and it assumes certain sizes
>> ("stipulated sizes not whatever" error)
>
> That's a shame. AFAIU, the non-ASCII benchmark would be relevant
> outside composition as well.
+1
>> - the resources directory containing xmenu.c or whatever must be a
>> sibling to the benchmarks directory
>>
>> - The pattern matching the test case doesn't seem to always work. I use
>> ".*scroll.*" but the report at the end shows also "setq" and
>> "font-lock"???
>
> I am using "elb-scroll" as the pattern and it only picks up scroll.
> (Though the time it takes to run 10 repeats (so 100 loops) is ~30 mins
> here.)
Is that with the smie mode?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 11:55:13 GMT)
Full text and
rfc822 format available.
Message #143 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> Cc: gerd.moellmann <at> gmail.com, stefankangas <at> gmail.com, 77924 <at> debbugs.gnu.org
> Date: Wed, 23 Apr 2025 17:41:18 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
>
> > From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> > Cc: Eli Zaretskii <eliz <at> gnu.org>, stefankangas <at> gmail.com,
> > 77924 <at> debbugs.gnu.org
> > Date: Wed, 23 Apr 2025 10:34:11 -0400
> >
> > FWIW, I'm not a great fan of rebasing either. I did rebase the branch
> > last night not for rebasing's sake but because I felt there was a need
> > for more detailed commit messages.
> >
> > In any case, any objection to merging the branch?
>
> I'd like to have a closer review of it first, so please wait for a
> while. When I skimmed it, I remember having several, hopefully minor,
> aspects that stood out, and I want to take a closer look.
See below:
> - m->next = BUF_MARKERS (buf);
> - BUF_MARKERS (buf) = m;
> + marker_vector_add (buf, m);
> + marker_vector_set_charpos (m, charpos);
Could we please name functions that manipulate and get marker
information marker_SOMETHING and not marker_vector_something?
marker_vector_add is semi-okay (although marker_add would have been
nicer, IMO), but marker_vector_set_charpos and marker_vector_charpos
are not, because AFAIU they manipulate the marker's position, not
marker-vector's position.
In general, wherever the fact that we have a vector of markers is
merely an implementation detail that is not important to the
programmer, can we just drop the "vector" part from the names of the
functions, for the same reason that the old functions and macros
weren't called marker_list_SOMETHING?
> + DO_MARKERS (b, m)
> + {
> + if (!vectorlike_marked_p (&m->header))
> + marker_vector_remove (XVECTOR (BUF_MARKERS (b)), m);
> + }
> + END_DO_MARKERS;
Would it be possible not to introduce macros that modify the C syntax?
These are problematic for more than one reason (one being that they
are not recognized by C modes we use). We have macros whose names
start with FOR_EACH_ (like FOR_EACH_FRAME); can we introduce
FOR_EACH_MARKER instead, and not hide opening/closing braces inside
macros?
> + marker_vector_reset (b);
I'd prefer marker_vector_clear. Or maybe even marker_delete_all.
> - set_marker_both (point_marker, Qnil, PT, PT_BYTE);
> + set_marker_both (point_marker, Qnil, PT);
It's confusing to keep calling this set_marker_both, when we no longer
pass the byte position.
> + DO_MARKERS (current_buffer, tail)
> + {
> + if (tail->need_adjustment)
> + {
> + tail->need_adjustment = 0;
> + if (tail->insertion_type)
> + {
> + marker_vector_set_charpos (tail, from);
> + }
We don't use braces around blocks of a single statement.
> INLINE ptrdiff_t
> +gc_vsize (const struct Lisp_Vector *v)
> +{
> + return v->header.size & ~ARRAY_MARK_FLAG;
> +}
> +
> +INLINE ptrdiff_t
> gc_asize (Lisp_Object array)
> {
> /* Like ASIZE, but also can be used in the garbage collector. */
> - return XVECTOR (array)->header.size & ~ARRAY_MARK_FLAG;
> + return gc_vsize (XVECTOR (array));
> }
Please add a comment to gc_vsize similar to what we say in gc_asize.
> +/* Push entry ENTRY of V to its free-list. This must set MARKER to not
^^^^^^
> + be a marker, which is done by using the MARKER field of entry
> + to form the free-list. */
> +
> +static void
> +push_free (struct Lisp_Vector *v, const ptrdiff_t entry)
> +{
> + check_is_entry (v, entry);
> + NEXT_FREE (v, entry) = FREE (v);
> + FREE (v) = make_fixnum (entry);
There's no argument named MARKER in this function.
> +Lisp_Object
> +alloc_marker_vector (ptrdiff_t len)
> +{
> +#ifdef HAVE_MPS
> + return igc_alloc_marker_vector (len, FREE_LIST_END);
> +#else
> + return make_vector (len, FREE_LIST_END);
> +#endif
> +}
We don't have any code on master that uses HAVE_MPS. This changeset
adds at least 2 of them. I'd prefer not to have these conditions on
master as long as the igc branch was not landed.
> +void
> +marker_vector_reset (struct buffer *b)
> +{
> + /* The old GC contains at least one assertion that unchaining markers
> + in kill-buffer resets the markers' buffers. IGC does not do this,
> + can't do this, and does not need it. */
Likewise here, wrt comments about igc.
> +/* Return marker M's byte position. */
> +
> +ptrdiff_t
> +marker_vector_bytepos (const struct Lisp_Marker *m)
> +{
> + const ptrdiff_t charpos = marker_vector_charpos (m);
> + return buf_charpos_to_bytepos (m->buffer, charpos);
Can m->buffer be NULL or a dead buffer? If so, what will happen here?
> +void
> +marker_vector_adjust_for_insert (struct buffer *b,
> + const ptrdiff_t from_charpos,
> + const ptrdiff_t to_charpos,
> + const bool before_markers)
> +{
> + const ptrdiff_t nchars = to_charpos - from_charpos;
> + struct Lisp_Vector *v = XVECTOR (BUF_MARKERS (b));
> + DO_MARKERS (b, m)
> + {
> + const ptrdiff_t charpos = XFIXNUM (CHARPOS (v, m->entry));
> + if (charpos == from_charpos)
> + {
> + if (m->insertion_type || before_markers)
> + CHARPOS (v, m->entry) = make_fixnum (to_charpos);
> + }
> + else if (charpos > from_charpos)
> + CHARPOS (v, m->entry) = make_fixnum (charpos + nchars);
Why does it use CHARPOS and not marker_vector_set_charpos?
> +void
> +marker_vector_adjust_for_replace (struct buffer *b,
> + const ptrdiff_t from_charpos,
> + const ptrdiff_t old_nchars,
> + const ptrdiff_t new_nchars)
> +{
> + const ptrdiff_t diff_nchars = new_nchars - old_nchars;
> + const ptrdiff_t old_to_charpos = from_charpos + old_nchars;
> + struct Lisp_Vector *v = XVECTOR (BUF_MARKERS (b));
> + DO_MARKERS (b, m)
> + {
> + const ptrdiff_t charpos = XFIXNUM (CHARPOS (v, m->entry));
> + if (charpos >= old_to_charpos)
> + CHARPOS (v, m->entry) = make_fixnum (charpos + diff_nchars);
> + else if (charpos > from_charpos)
> + CHARPOS (v, m->entry) = make_fixnum (from_charpos);
> + }
> + END_DO_MARKERS;
Likewise here.
> +Lisp_Object make_marker_vector (void);
> +Lisp_Object alloc_marker_vector (ptrdiff_t len);
> +void marker_vector_add (struct buffer *b, struct Lisp_Marker *m);
> +void marker_vector_remove (struct Lisp_Vector *v, struct Lisp_Marker *m);
> +void marker_vector_reset (struct buffer *b);
> +void marker_vector_set_charpos (struct Lisp_Marker *m, ptrdiff_t charpos);
> +ptrdiff_t marker_vector_charpos (const struct Lisp_Marker *m);
> +ptrdiff_t marker_vector_bytepos (const struct Lisp_Marker *m);
> +void marker_vector_adjust_for_insert (struct buffer *b, const ptrdiff_t from,
> + ptrdiff_t to, bool before_markers);
> +void marker_vector_adjust_for_replace (struct buffer *b, ptrdiff_t from,
> + ptrdiff_t old_chars, ptrdiff_t new_chars);
We use the 'extern' qualifier for prototypes of non-static functions
in header files.
> + Note that the byte positions at interval boundaries can be in the
> + middle of a multi-byte character.
> +
> + character start byte position
> + |
> + ------01234-------- bytes of a character (up to 5 in Emacs'
> + | internal encoding)
> + N * interval
> +
> + To find the character position corresponding to a byte position
> + BYTEPOS, we look up the character position in the index at BYTEPOS /
> + interval. From there, we can scan forward in the text until we reach
> + BYTEPOS, counting characters, or we scan backward from the interval
> + end, if that is closer.
> +
> + To find the byte position BYTEPOS corresponding to a given character
> + position CHARPOS, we search the index for the last entry ENTRY whose
> + character position is <= CHARPOS. That entry corresponds to a byte
> + position ENTRY * interval size. From there, we scan the text until
> + we reach BYTEPOS, counting characters until we reach CHARPOS. The
> + byte position reached at the end is BYTEPOS. We also scan backward
> + from the interval end, if that looks advantageous.
Doesn't this make charpos->bytepos conversion more expensive than in
the other direction? If not, why not? If the charpos->bytepos
conversion is indeed more expensive, shouldn't it be the other way
around, since AFAIR we need a byte position of a given character
position much more frequently than the other way around? (And now it
should be even more frequently, since markers record only the
character positions.)
> + Why divide the text into intervals of bytes instead of characters?
> + Dividing the text into intervals of characters makes scanning
> + overhead less uniform, since characters can be of different lengths
> + (1 to 5 bytes). */
AFAIU, this should mention the fact that the implementation wants to
use binary search to find the entry corresponding to a character
position.
> +// clang-format off
Do we need this,l and if we do, does it have to be a C++ style
comment?
> +
> +struct text_index
> +{
> + /* Value at index IDX is the character position of byte position IDX *
> + INTERVAL. Note that that byte position may be in the middle of a
> + character. The value at index 0 is BEG. */
> + ptrdiff_t *charpos;
What is IDX here? And what is INTERVAL?
> + /* Number of valid entries in the above array. This is always at least 1
> + because the first entry is for BEG. */
> + size_t nentries;
> +
> + /* Number of entries allocated. */
> + size_t capacity;
Why not ptrdiff_t? AFAIR, unsigned types should be avoided, to
prevent problems when mixing them with signed types.
> +/* Value is true is known position cache of TI is valid. */
^^
Typo: "if".
> +static bool
> +is_cache_valid (const struct text_index *ti)
> +{
> + return ti->cache.bytepos != TEXT_INDEX_INVALID_POSITION;
> +}
Shouldn't we also test against BEG_BYTE and Z_BYTE?
> +/* Return the byte position in index TI corresponding to index entry
> + ENTRY. Note that this position cab be in the middle of a multi-byte
^^^
"can"
> +/* Return the character position in index TI corresponding index entry
^^^^^^^^^^^^^
"corresponding to"
> + /* Start at the byte position of the last index entry. if TO_BYTEPOS
^^
Capitalize "If".
> + /* Loop over bytes, starting one after the index entry we start from
> + because we are only interested in yet unknown entries, and the
> + one at EMTRY can be assumed to stay unchanged. */
^^^^^
"ENTRY"
> + for (++bytepos; bytepos < z_byte; ++bytepos)
> + {
> + if (CHAR_HEAD_P (BUF_FETCH_BYTE (b, bytepos)))
> + ++charpos;
> +
> + if (bytepos == next_stop)
> + {
> + /* Add a new index entry. */
> + append_entry (ti, charpos);
> +
> + /* If we reached the one after the position we are interested
> + in, we're done since we can then scan forward and backward
> + to BYTEPOS. */
> + if ((to.bytepos != TEXT_INDEX_INVALID_POSITION
> + && bytepos > to.bytepos)
> + || (to.charpos != TEXT_INDEX_INVALID_POSITION
> + && charpos > to.charpos))
> + break;
> +
> + /* Compute next stop. We are done if no next entry
> + can be built. */
> + next_stop += TEXT_INDEX_INTERVAL;
> + if (next_stop >= z_byte)
> + break;
> + }
> + }
I think this loop could be optimized if it used BYTES_BY_CHAR_HEAD
instead of advancing one byte at a time.
> +static ptrdiff_t
> +charpos_forward_to_bytepos (struct buffer *b, const struct text_pos from,
> + const ptrdiff_t to_bytepos)
> +{
> + eassert (from.bytepos <= to_bytepos);
> + ptrdiff_t bytepos = from.bytepos;
> + ptrdiff_t charpos = from.charpos;
> + while (bytepos < to_bytepos)
> + {
> + ++bytepos;
> + if (CHAR_HEAD_P (BUF_FETCH_BYTE (b, bytepos)))
> + ++charpos;
> + }
> + return charpos;
> +}
Likewise here.
> + ptrdiff_t charpos = from.charpos;
Please use CHARPOS(from) instead of accessing the struct members
directly (here and elsewhere).
> + while (charpos < to_charpos)
> + {
> + ++bytepos;
> + if (CHAR_HEAD_P (BUF_FETCH_BYTE (b, bytepos)))
> + ++charpos;
> + }
Here we could also use BYTES_BY_CHAR_HEAD.
> +/* Improve the known bytepos bounds *PREV and *NEXT if KNOWN is closer
> + to BYTEPOS. */
> +
> +static void
> +narrow_charpos_bounds_1 (const struct text_pos known, struct text_pos *prev,
> + struct text_pos *next, const ptrdiff_t charpos)
The commentary mentions BYTEPOS, but the function doesn't have such an
argument.
> +/* Improve the known bytepos bounds *PREV and *NEXT of buffer B using
> + known positions in B. BYTEPOS is a byte position to convert to a
> + character position. */
> +
> +static void
> +narrow_charpos_bounds (struct buffer *b, struct text_pos *prev,
> + struct text_pos *next, const ptrdiff_t charpos)
Same here.
> +/* Return the character position in buffer B corresponding to
> + byte position BYTEPOS. */
> +
> +ptrdiff_t
> +buf_bytepos_to_charpos (struct buffer *b, const ptrdiff_t bytepos)
> +{
> + /* FIXME: Can BYTEPOS ever be outside of BEGV_BYTE..ZV_BYTE? */
> + /* If this buffer has as many characters as bytes, each character must
> + be one byte. This takes care of the case where
> + enable-multibyte-characters is nil. */
> + const struct text_pos z = z_pos (b);
> + if (z.charpos == z.bytepos)
> + return bytepos;
> +
> + /* Begin with the interval (BEG, Z), and improve on that by taking known
> + positions into account like PT, GPT and the cache. This might
> + already find the answer. */
> + struct text_index *ti = ensure_has_index (b);
> + struct text_pos prev = beg_pos (b);
> + struct text_pos next = z;
> +
> + narrow_bytepos_bounds (b, &prev, &next, bytepos);
> +
> + /* Z_BYTE does not have an index entry because we don't want
> + extra entries for (Z, Z_BYTE), so short-circuit *before* looking
> + up the index. Changing that would be possible but leads to more
> + code than this if-statement, so it's probably not worth it. */
> + if (next.bytepos == bytepos)
> + return next.charpos;
> +
> + ensure_bytepos_indexed (b, bytepos);
> + const ptrdiff_t entry = index_bytepos_entry (ti, bytepos);
> + narrow_bytepos_bounds_1 (index_text_pos (ti, entry), &prev, &next, bytepos);
> + narrow_bytepos_bounds_1 (next_known_text_pos (b, entry),
> + &prev, &next, bytepos);
> +
> + if (next.charpos - prev.charpos == next.bytepos - prev.bytepos
> + /* Beware: NEXT and PREV can be in the middle of multibyte chars! */
> + && CHAR_HEAD_P (BUF_FETCH_BYTE (b, prev.bytepos)))
> + return prev.charpos + (bytepos - prev.bytepos); /* ASCII-only! */
AFAICT, you already handled the ASCII-only case at the beginning, so
why is this needed?
> + /* Don't scan forward if CHARPOS is exactly on the previous know
^^^^
"known"
> +DEFUN ("text-index--charpos-to-bytepos", Ftext_index__charpos_to_bytepos,
> + Stext_index__charpos_to_bytepos, 1, 1, 0,
> + doc: /* Convert CHARPOS to a bytepos in current buffer.
> +If POSITION is out of range, the value is nil. */)
> + (Lisp_Object charpos)
> +{
> + const EMACS_INT pos = fix_position (charpos);
> + if (pos < BEG || pos > Z)
> + return Qnil;
> + ptrdiff_t bytepos = buf_charpos_to_bytepos (current_buffer, pos);
> + return make_fixnum (bytepos);
> +}
> +
> +DEFUN ("text-index--bytepos-to-charpos", Ftext_index__bytepos_to_charpos,
> + Stext_index__bytepos_to_charpos, 1, 1, 0,
> + doc: /* Convert BYTEPOS to a charpos in current buffer.
> +If BYTEPOS is out of range, the value is nil. */)
> + (Lisp_Object bytepos)
> +{
> + CHECK_FIXNUM (bytepos);
> + const ptrdiff_t pos_byte = XFIXNUM (bytepos);
> + if (pos_byte < BEG_BYTE || pos_byte > Z_BYTE)
> + return Qnil;
> + ptrdiff_t charpos = buf_bytepos_to_charpos (current_buffer, pos_byte);
> + return make_fixnum (charpos);
> +}
> +
> +DEFUN ("text-index--charpos-to-bytepos-brute",
> + Ftext_index__charpos_to_bytepos_brute,
> + Stext_index__charpos_to_bytepos_brute, 1, 1, 0,
> + doc: /* Convert CHARPOS to a bytepos in current buffer.
> +Compute with brute force. */)
> + (Lisp_Object pos)
> +{
> + const EMACS_INT to_charpos = fix_position (pos);
> + if (to_charpos < BEG || to_charpos > Z)
> + return Qnil;
> + ptrdiff_t charpos = BEG, bytepos = BEG_BYTE;
> + while (charpos < to_charpos)
> + {
> + ++bytepos;
> + if (CHAR_HEAD_P (FETCH_BYTE (bytepos)))
> + ++charpos;
> + }
> + return make_fixnum (bytepos);
> +}
Why are these functions needed? We already have byte-to-position and
position-bytes, so at least the first two seem redundant. When would
be the last two ones useful?
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 12:03:09 GMT)
Full text and
rfc822 format available.
Message #146 received at 77924 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
[வியாழன் ஏப்ரல் 24, 2025] Gerd Möllmann wrote:
>> Is there a standard benchmark code that I can try for this? I have such
>> a large HTML file with Tamil text, and I can scroll through the buffer
>> produced by shr-render-buffer.
>
> The elisp-benchmarks package has an elb-scroll.el, in its benchmarks
> directory. AFAICT, that loads an xmenu.c from the resources
> sub-directory of the benchmarks.
>
> You could copy that, or modify it.
>
> P.S.
>
> One can set the benchmarks directory to somewhere else, and run
> benchmarks like this:
>
> (setq elb-bench-directory "~/emacs/notes/code/benchmarks")
> (elisp-benchmarks-run ".*replace-region-contents.*" t 100)
>
> The 100 is the number of runs, which should be chosen high enough that
> the "err" column in the result buffer is reasonably low. I'd start with
> 1 to see how long that takes, and then increase it.
I ran the benchmark with the text file tamil.txt in place of xmenu.c,
but I didn't disable elb-smie-mode since I ran into weird issues. In
both master and text-index cases, I ran the benchmark with
% emacs -Q -l elisp-benchmarks-run --eval '(elisp-benchmarks-run "elb-scroll" t 10)'
and here are the results for the master branch:
* Results
| test | non-gc (s) | gc (s) | gcs | total (s) | err |
|--------+------------+--------+-----+-----------+-----|
| scroll | 155.73 | 49.29 | 835 | 205.02 | 0% |
|--------+------------+--------+-----+-----------+-----|
| total | 155.73 | 49.29 | 835 | 205.02 | 0% |
and for the scratch/text-index branch:
* Results
| test | non-gc (s) | gc (s) | gcs | total (s) | err |
|--------+------------+--------+-----+-----------+-----|
| scroll | 171.89 | 51.56 | 869 | 223.44 | 1% |
|--------+------------+--------+-----+-----------+-----|
| total | 171.89 | 51.56 | 869 | 223.44 | 1% |
Unfortunately, my laptop went to power saving mode during the last two
minutes or so for the scratch/text-index benchmark run so that might
play into the numbers a tiny bit. I can try repeating the test later
for the latter case if desired. Empirically speaking, both branches
seemed to scroll at the same speed.
This is how I configured both Emacses:
% ./configure --with-sound=alsa --with-x-toolkit=motif \
--without-xaw3d --without-gconf --without-libsystemd \
--with-cairo CFLAGS=-g3 CC=$(which gcc-13)
If anyone wants to repeat the test, I attach the text file (compressed)
I use here.
[tamil.txt.gz (application/gzip, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 12:03:14 GMT)
Full text and
rfc822 format available.
Message #149 received at 77924 <at> debbugs.gnu.org (full text, mbox):
[வியாழன் ஏப்ரல் 24, 2025] Gerd Möllmann wrote:
>>> - the resources directory containing xmenu.c or whatever must be a
>>> sibling to the benchmarks directory
>>>
>>> - The pattern matching the test case doesn't seem to always work. I use
>>> ".*scroll.*" but the report at the end shows also "setq" and
>>> "font-lock"???
>>
>> I am using "elb-scroll" as the pattern and it only picks up scroll.
>> (Though the time it takes to run 10 repeats (so 100 loops) is ~30 mins
>> here.)
>
> Is that with the smie mode?
Yep, I gave up on trying to load text-mode or otherwise, and use
elb-smie-mode in the test. See my other mail for the results.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 13:28:02 GMT)
Full text and
rfc822 format available.
Message #152 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Visuwesh <visuweshm <at> gmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier
> <monnier <at> iro.umontreal.ca>, yantar92 <at> posteo.net, stefankangas <at> gmail.com,
> 77924 <at> debbugs.gnu.org
> Date: Thu, 24 Apr 2025 17:31:49 +0530
>
> I ran the benchmark with the text file tamil.txt in place of xmenu.c,
> but I didn't disable elb-smie-mode since I ran into weird issues. In
> both master and text-index cases, I ran the benchmark with
>
> % emacs -Q -l elisp-benchmarks-run --eval '(elisp-benchmarks-run "elb-scroll" t 10)'
>
> and here are the results for the master branch:
>
> * Results
>
> | test | non-gc (s) | gc (s) | gcs | total (s) | err |
> |--------+------------+--------+-----+-----------+-----|
> | scroll | 155.73 | 49.29 | 835 | 205.02 | 0% |
> |--------+------------+--------+-----+-----------+-----|
> | total | 155.73 | 49.29 | 835 | 205.02 | 0% |
>
> and for the scratch/text-index branch:
>
> * Results
>
> | test | non-gc (s) | gc (s) | gcs | total (s) | err |
> |--------+------------+--------+-----+-----------+-----|
> | scroll | 171.89 | 51.56 | 869 | 223.44 | 1% |
> |--------+------------+--------+-----+-----------+-----|
> | total | 171.89 | 51.56 | 869 | 223.44 | 1% |
>
> Unfortunately, my laptop went to power saving mode during the last two
> minutes or so for the scratch/text-index benchmark run so that might
> play into the numbers a tiny bit. I can try repeating the test later
> for the latter case if desired. Empirically speaking, both branches
> seemed to scroll at the same speed.
Thanks.
Unless the power saving mode explains it, the results look a bit
disappointing to me: the branch is slower by about 10%. That's
unexpected, I think.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 13:36:01 GMT)
Full text and
rfc822 format available.
Message #155 received at 77924 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Visuwesh <visuweshm <at> gmail.com> writes:
> [வியாழன் ஏப்ரல் 24, 2025] Gerd Möllmann wrote:
>
>>> Is there a standard benchmark code that I can try for this? I have such
>>> a large HTML file with Tamil text, and I can scroll through the buffer
>>> produced by shr-render-buffer.
>>
>> The elisp-benchmarks package has an elb-scroll.el, in its benchmarks
>> directory. AFAICT, that loads an xmenu.c from the resources
>> sub-directory of the benchmarks.
>>
>> You could copy that, or modify it.
>>
>> P.S.
>>
>> One can set the benchmarks directory to somewhere else, and run
>> benchmarks like this:
>>
>> (setq elb-bench-directory "~/emacs/notes/code/benchmarks")
>> (elisp-benchmarks-run ".*replace-region-contents.*" t 100)
>>
>> The 100 is the number of runs, which should be chosen high enough that
>> the "err" column in the result buffer is reasonably low. I'd start with
>> 1 to see how long that takes, and then increase it.
>
> I ran the benchmark with the text file tamil.txt in place of xmenu.c,
> but I didn't disable elb-smie-mode since I ran into weird issues. In
> both master and text-index cases, I ran the benchmark with
>
> % emacs -Q -l elisp-benchmarks-run --eval '(elisp-benchmarks-run "elb-scroll" t 10)'
>
> and here are the results for the master branch:
>
> * Results
>
> | test | non-gc (s) | gc (s) | gcs | total (s) | err |
> |--------+------------+--------+-----+-----------+-----|
> | scroll | 155.73 | 49.29 | 835 | 205.02 | 0% |
> |--------+------------+--------+-----+-----------+-----|
> | total | 155.73 | 49.29 | 835 | 205.02 | 0% |
>
> and for the scratch/text-index branch:
>
> * Results
>
> | test | non-gc (s) | gc (s) | gcs | total (s) | err |
> |--------+------------+--------+-----+-----------+-----|
> | scroll | 171.89 | 51.56 | 869 | 223.44 | 1% |
> |--------+------------+--------+-----+-----------+-----|
> | total | 171.89 | 51.56 | 869 | 223.44 | 1% |
>
> Unfortunately, my laptop went to power saving mode during the last two
> minutes or so for the scratch/text-index benchmark run so that might
> play into the numbers a tiny bit. I can try repeating the test later
> for the latter case if desired. Empirically speaking, both branches
> seemed to scroll at the same speed.
>
> This is how I configured both Emacses:
>
> % ./configure --with-sound=alsa --with-x-toolkit=motif \
> --without-xaw3d --without-gconf --without-libsystemd \
> --with-cairo CFLAGS=-g3 CC=$(which gcc-13)
>
> If anyone wants to repeat the test, I attach the text file (compressed)
> I use here.
Same on Mac mini M1, idle, tamil.txt but without the elb-smie-mode, 10
runs.
* master Results
| test | non-gc (s) | gc (s) | gcs | total (s) | err (s) |
|--------+------------+--------+------+-----------+---------|
| scroll | 23.48 | 12.00 | 1271 | 35.48 | 0.23 |
|--------+------------+--------+------+-----------+---------|
| total | 23.48 | 12.00 | 1271 | 35.48 | 0.23 |
* text-index Results
| test | non-gc (s) | gc (s) | gcs | total (s) | err (s) |
|--------+------------+--------+------+-----------+---------|
| scroll | 23.03 | 12.06 | 1271 | 35.09 | 0.15 |
|--------+------------+--------+------+-----------+---------|
| total | 23.03 | 12.06 | 1271 | 35.09 | 0.15 |
Config
[config.log (application/octet-stream, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 16:14:02 GMT)
Full text and
rfc822 format available.
Message #158 received at 77924 <at> debbugs.gnu.org (full text, mbox):
>> Evaluate this, then invoke "M-x scroll-up-benchmark" in a large buffer
>> with lots of non-ASCII characters. Compare the timings between the
>> two versions of Emacs.
>
> elb-scroll from elisp-bechmarks is basically
>
> (dotimes (_ 10)
> (elb-smie-mode)
> (goto-char (point-min))
> (condition-case nil
> (while t (scroll-up nil) (redisplay 'force))
> (end-of-buffer nil))))))
>
> looks similar, but I don't know what elb-smie-mode does.
It's a major mode for the C language, separate from CC-mode.
[ It's basically "vendored" copy of the `sm-c-mode` that's on GNU ELPA. ]
So the benchmark tests scroll time, including jit/font-lock time.
It uses its own copy of a major mode, so that you can compare "scroll +
font-lock" performance between different Emacs releases without being
affected by improvements/regressions in CC-mode itself.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 16:29:02 GMT)
Full text and
rfc822 format available.
Message #161 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> Cc: gerd.moellmann <at> gmail.com, stefankangas <at> gmail.com, 77924 <at> debbugs.gnu.org
>> Date: Wed, 23 Apr 2025 17:41:18 +0300
>> From: Eli Zaretskii <eliz <at> gnu.org>
>>
>> > From: Stefan Monnier <monnier <at> iro.umontreal.ca>
>> > Cc: Eli Zaretskii <eliz <at> gnu.org>, stefankangas <at> gmail.com,
>> > 77924 <at> debbugs.gnu.org
>> > Date: Wed, 23 Apr 2025 10:34:11 -0400
>> >
>> > FWIW, I'm not a great fan of rebasing either. I did rebase the branch
>> > last night not for rebasing's sake but because I felt there was a need
>> > for more detailed commit messages.
>> >
>> > In any case, any objection to merging the branch?
>>
>> I'd like to have a closer review of it first, so please wait for a
>> while. When I skimmed it, I remember having several, hopefully minor,
>> aspects that stood out, and I want to take a closer look.
>
> See below:
>
>> - m->next = BUF_MARKERS (buf);
>> - BUF_MARKERS (buf) = m;
>> + marker_vector_add (buf, m);
>> + marker_vector_set_charpos (m, charpos);
>
> Could we please name functions that manipulate and get marker
> information marker_SOMETHING and not marker_vector_something?
> marker_vector_add is semi-okay (although marker_add would have been
> nicer, IMO), but marker_vector_set_charpos and marker_vector_charpos
> are not, because AFAIU they manipulate the marker's position, not
> marker-vector's position.
>
> In general, wherever the fact that we have a vector of markers is
> merely an implementation detail that is not important to the
> programmer, can we just drop the "vector" part from the names of the
> functions, for the same reason that the old functions and macros
> weren't called marker_list_SOMETHING?
Well. For one thing, the marker is now no longer the central data
structure. It's basically only fancy index into a marker vector + 2 bit
flags (which could/should by moved to the marker vector, maybe). The
marker vector is the central data structure holding a character position
which the marker refers to.
Secondly functions marker_position and marker_byte_position still exist,
basically one level higher. But below that level, the implementation
necessarily lurks.
That's why I'd prefer to keep the marker_vector prefix.
>> + DO_MARKERS (b, m)
>> + {
>> + if (!vectorlike_marked_p (&m->header))
>> + marker_vector_remove (XVECTOR (BUF_MARKERS (b)), m);
>> + }
>> + END_DO_MARKERS;
>
> Would it be possible not to introduce macros that modify the C syntax?
> These are problematic for more than one reason (one being that they
> are not recognized by C modes we use). We have macros whose names
> start with FOR_EACH_ (like FOR_EACH_FRAME); can we introduce
> FOR_EACH_MARKER instead, and not hide opening/closing braces inside
> macros?
I tried, already in igc, but failed. IOW, I don't now a way to get rid
of the END_DO_MARKERS.
[...]
I hope Stef can take care of some of the rest. Stef, please let me know
if should help. It could take a few days though, because of real-world
interference.
Thanks for the review.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 16:33:02 GMT)
Full text and
rfc822 format available.
Message #164 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>> Evaluate this, then invoke "M-x scroll-up-benchmark" in a large buffer
>>> with lots of non-ASCII characters. Compare the timings between the
>>> two versions of Emacs.
>>
>> elb-scroll from elisp-bechmarks is basically
>>
>> (dotimes (_ 10)
>> (elb-smie-mode)
>> (goto-char (point-min))
>> (condition-case nil
>> (while t (scroll-up nil) (redisplay 'force))
>> (end-of-buffer nil))))))
>>
>> looks similar, but I don't know what elb-smie-mode does.
>
> It's a major mode for the C language, separate from CC-mode.
> [ It's basically "vendored" copy of the `sm-c-mode` that's on GNU
> ELPA. ]
Ah, interesting.
> So the benchmark tests scroll time, including jit/font-lock time.
> It uses its own copy of a major mode, so that you can compare "scroll +
> font-lock" performance between different Emacs releases without being
> affected by improvements/regressions in CC-mode itself.
So maybe it would make sense to run both that benchmark as-is plus one
without smie but with tamil.txt, I guess. Or in other words, now only
the as-is benchmark, because the tamil.txt results I've already posted.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 18:26:02 GMT)
Full text and
rfc822 format available.
Message #167 received at 77924 <at> debbugs.gnu.org (full text, mbox):
[வியாழன் ஏப்ரல் 24, 2025] Gerd Möllmann wrote:
>> So the benchmark tests scroll time, including jit/font-lock time.
>> It uses its own copy of a major mode, so that you can compare "scroll +
>> font-lock" performance between different Emacs releases without being
>> affected by improvements/regressions in CC-mode itself.
>
> So maybe it would make sense to run both that benchmark as-is plus one
> without smie but with tamil.txt, I guess. Or in other words, now only
> the as-is benchmark, because the tamil.txt results I've already posted.
Before including tamil.txt, I would like to make clear that the file
contents is volume 1 of a (popular) novel which was released in the 50s.
I got the text itself from Project Madurai which states that [1]:
Acknowledgement:
Our Sincere thanks go to the following persons for their assistance in the preparation of this work.
Etext preparation : AU-KBC Research Center (Mr. Baskaran), Anna University, Chennai,India
Proof-reading: Mr. S. Anbumani, Mr. N.D. Logasundaram,Mr. Narayanan Govindarajan,
Ms. Pavithra Srinivasan, Mr. Ramachandran Mahadevan, Ms. Sathya, Mr. Sreeram Krishnamoorthy,
Dr. Sridhar Rathinam, Mrs. Srilatha Rajagopal, Mr. VinothJagannathan
Web version: Mr. S. Anbumani, Blacksburg, Virginia, USA
This webpage presents the Etxt in Tamil script in Unicode encoding.
This file was last revised on Apr. 12, 2003
© Project Madurai, 1998-2021.
Project Madurai is an open, voluntary, worldwide initiative devoted to preparation
of electronic texts of tamil literary works and to distribute them free on the Internet.
Details of Project Madurai are available at the website
https://www.projectmadurai.org/
You are welcome to freely distribute this file, provided this header page is kept intact.
So I assume there should no problem in distributing it in the repo. If
there could be, we could try composing all the text from any work in
freetamilebooks.com which publishes under the CC BY-NC-SA license.
1. https://www.projectmadurai.org/pm_etexts/utf8/pmuni0169_00.html
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 19:27:01 GMT)
Full text and
rfc822 format available.
Message #170 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>>> + DO_MARKERS (b, m)
>>> + {
>>> + if (!vectorlike_marked_p (&m->header))
>>> + marker_vector_remove (XVECTOR (BUF_MARKERS (b)), m);
>>> + }
>>> + END_DO_MARKERS;
>>
>> Would it be possible not to introduce macros that modify the C syntax?
>> These are problematic for more than one reason (one being that they
>> are not recognized by C modes we use). We have macros whose names
>> start with FOR_EACH_ (like FOR_EACH_FRAME); can we introduce
>> FOR_EACH_MARKER instead, and not hide opening/closing braces inside
>> macros?
>
> I tried, already in igc, but failed. IOW, I don't now a way to get rid
> of the END_DO_MARKERS.
Okay, I'll bite.
I think what we should do is mimic FOR_EACH_TAIL, and use
FOR_EACH_MARKER like this:
struct Lisp_Marker *m;
FOR_EACH_MARKER (b, m)
{
/* do something with m */
}
However, if we wanted to, we could do this (the "continued" thing is
overkill to make "break" work in a FOR_EACH_MARKER loop):
diff --git a/src/marker-vector.h b/src/marker-vector.h
index 141e79ac0ee..bfca02fc33d 100644
--- a/src/marker-vector.h
+++ b/src/marker-vector.h
@@ -39,27 +39,55 @@ #define EMACS_MARKER_VECTOR_H
MARKER_VECTOR_ENTRY_SIZE = 2,
};
-/* Iterate over markers in marker vector MV, binding a variable with
- name M to a pointer to Lisp_Marker. The loop must be ended
- with an END_DO_MARKERS. */
-
-# define DO_MARKERS_OF_VECTOR(mv, m) \
- for (ptrdiff_t e_ = MARKER_VECTOR_HEADER_SIZE, \
- end_ = XFIXNUM (AREF (mv, MARKER_VECTOR_MAX_ENTRY)); \
- e_ <= end_; \
- e_ += MARKER_VECTOR_ENTRY_SIZE) \
- { \
- Lisp_Object m_ = AREF (mv, e_ + MARKER_VECTOR_OFFSET_MARKER); \
- if (MARKERP (m_)) \
- { \
- struct Lisp_Marker *m = XMARKER (m_);
-
-/* Iterate over markers of buffer B, binding a variable with name M to a
- pointer to Lisp_Marker. The loop must be ended with an
- END_DO_MARKERS. */
-
-# define DO_MARKERS(b, m) DO_MARKERS_OF_VECTOR (BUF_MARKERS (b), m)
-# define END_DO_MARKERS }}
+struct for_each_marker_data
+{
+ ptrdiff_t e;
+ ptrdiff_t end;
+ Lisp_Object m;
+ Lisp_Object mv;
+ bool continued;
+ struct Lisp_Marker *marker;
+};
+
+INLINE struct for_each_marker_data
+build_for_each_marker_data(Lisp_Object mv)
+{
+ struct for_each_marker_data ret;
+ ret.e = MARKER_VECTOR_HEADER_SIZE;
+ ret.end = XFIXNUM (AREF (mv, MARKER_VECTOR_MAX_ENTRY));
+ ret.m = Qnil;
+ ret.mv = mv;
+ ret.marker = NULL;
+ ret.continued = true;
+ return ret;
+}
+
+INLINE bool
+next_marker_entry (struct for_each_marker_data *d)
+{
+ if (!d->continued)
+ return false;
+ while (d->e <= d->end)
+ {
+ d->m = AREF (d->mv, d->e + MARKER_VECTOR_OFFSET_MARKER);
+ d->e += MARKER_VECTOR_ENTRY_SIZE;
+ if (MARKERP (d->m))
+ {
+ d->marker = XMARKER (d->m);
+ d->continued = false;
+ return true;
+ }
+ }
+ return false;
+}
+
+#define FOR_EACH_MARKER_OF_VECTOR(v, m) \
+ for (struct for_each_marker_data d_ = build_for_each_marker_data (v); \
+ next_marker_entry (&d_);) \
+ for (struct Lisp_Marker *m = d_.marker; !d_.continued; d_.continued = true)
+
+#define FOR_EACH_MARKER(b, m) \
+ FOR_EACH_MARKER_OF_VECTOR(BUF_MARKERS (b), m)
Lisp_Object make_marker_vector (void);
Lisp_Object alloc_marker_vector (ptrdiff_t len);
diff --git a/src/alloc.c b/src/alloc.c
index c90f60ee518..30bdd1140b8 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -7114,12 +7114,11 @@ sweep_symbols (void)
static void
unchain_dead_markers (struct buffer *b)
{
- DO_MARKERS (b, m)
+ FOR_EACH_MARKER (b, m)
{
if (!vectorlike_marked_p (&m->header))
marker_vector_remove (XVECTOR (BUF_MARKERS (b)), m);
}
- END_DO_MARKERS;
}
NO_INLINE /* For better stack traces */
diff --git a/src/buffer.c b/src/buffer.c
index 93a2edb3693..21b6096517d 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -2073,12 +2073,11 @@ DEFUN ("kill-buffer", Fkill_buffer, Skill_buffer, 0, 1, "bKill buffer: ",
/* Unchain all markers that belong to this indirect buffer.
Don't unchain the markers that belong to the base buffer
or its other indirect buffers. */
- DO_MARKERS (b, m)
+ FOR_EACH_MARKER (b, m)
{
if (m->buffer == b)
marker_vector_remove (XVECTOR (BUF_MARKERS (b)), m);
}
- END_DO_MARKERS;
/* Intervals should be owned by the base buffer (Bug#16502). */
i = buffer_intervals (b);
@@ -2618,7 +2617,7 @@ #define swapfield_(field, type) \
other_buffer->text->end_unchanged = other_buffer->text->gpt;
swap_buffer_overlays (current_buffer, other_buffer);
{
- DO_MARKERS (current_buffer, m)
+ FOR_EACH_MARKER (current_buffer, m)
{
if (m->buffer == other_buffer)
m->buffer = current_buffer;
@@ -2627,8 +2626,7 @@ #define swapfield_(field, type) \
BUF_MARKERS(buf) should either be for `buf' or dead. */
eassert (!m->buffer);
}
- END_DO_MARKERS;
- DO_MARKERS (other_buffer, m)
+ FOR_EACH_MARKER (other_buffer, m)
{
if (m->buffer == current_buffer)
m->buffer = other_buffer;
@@ -2637,7 +2635,6 @@ #define swapfield_(field, type) \
BUF_MARKERS(buf) should either be for `buf' or dead. */
eassert (!m->buffer);
}
- END_DO_MARKERS;
}
{ /* Some of the C code expects that both window markers of a
@@ -2746,12 +2743,11 @@ DEFUN ("set-buffer-multibyte", Fset_buffer_multibyte, Sset_buffer_multibyte,
GPT = GPT_BYTE;
TEMP_SET_PT_BOTH (PT_BYTE, PT_BYTE);
- DO_MARKERS (current_buffer, tail)
+ FOR_EACH_MARKER (current_buffer, tail)
{
const ptrdiff_t bytepos = marker_vector_bytepos (tail);
marker_vector_set_charpos (tail, bytepos);
}
- END_DO_MARKERS;
/* Convert multibyte form of 8-bit characters to unibyte. */
pos = BEG;
@@ -2901,13 +2897,12 @@ DEFUN ("set-buffer-multibyte", Fset_buffer_multibyte, Sset_buffer_multibyte,
TEMP_SET_PT_BOTH (position, byte);
}
- DO_MARKERS (current_buffer, tail)
+ FOR_EACH_MARKER (current_buffer, tail)
{
ptrdiff_t bytepos = marker_vector_bytepos (tail);
bytepos = advance_to_char_boundary (bytepos);
marker_vector_set_charpos (tail, BYTE_TO_CHAR (bytepos));
}
- END_DO_MARKERS;
/* Do this last, so it can calculate the new correspondences
between chars and bytes. */
diff --git a/src/coding.c b/src/coding.c
index f3dd2639e73..51c87fb3161 100644
--- a/src/coding.c
+++ b/src/coding.c
@@ -8118,14 +8118,13 @@ decode_coding_object (struct coding_system *coding,
move_gap_both (from, from_byte);
if (BASE_EQ (src_object, dst_object))
{
- DO_MARKERS (current_buffer, tail)
+ FOR_EACH_MARKER (current_buffer, tail)
{
const ptrdiff_t charpos = marker_vector_charpos (tail);
tail->need_adjustment
= charpos == (tail->insertion_type ? from : to);
need_marker_adjustment |= tail->need_adjustment;
}
- END_DO_MARKERS;
saved_pt = PT, saved_pt_byte = PT_BYTE;
TEMP_SET_PT_BOTH (from, from_byte);
current_buffer->text->inhibit_shrinking = true;
@@ -8250,7 +8249,7 @@ decode_coding_object (struct coding_system *coding,
if (need_marker_adjustment)
{
- DO_MARKERS (current_buffer, tail)
+ FOR_EACH_MARKER (current_buffer, tail)
{
if (tail->need_adjustment)
{
@@ -8269,7 +8268,6 @@ decode_coding_object (struct coding_system *coding,
}
}
}
- END_DO_MARKERS;
}
}
@@ -8340,14 +8338,13 @@ encode_coding_object (struct coding_system *coding,
if (BASE_EQ (src_object, dst_object) && BUFFERP (src_object))
{
same_buffer = true;
- DO_MARKERS (XBUFFER (src_object), tail)
+ FOR_EACH_MARKER (XBUFFER (src_object), tail)
{
const ptrdiff_t charpos = marker_vector_charpos (tail);
tail->need_adjustment
= charpos == (tail->insertion_type ? from : to);
need_marker_adjustment |= tail->need_adjustment;
}
- END_DO_MARKERS;
}
if (! NILP (CODING_ATTR_PRE_WRITE (attrs)))
@@ -8505,7 +8502,7 @@ encode_coding_object (struct coding_system *coding,
if (need_marker_adjustment)
{
- DO_MARKERS (current_buffer, tail)
+ FOR_EACH_MARKER (current_buffer, tail)
{
if (tail->need_adjustment)
{
@@ -8524,7 +8521,6 @@ encode_coding_object (struct coding_system *coding,
}
}
}
- END_DO_MARKERS;
}
}
diff --git a/src/editfns.c b/src/editfns.c
index 61cb7bdd201..b96275dc0a3 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -4438,7 +4438,7 @@ transpose_markers (ptrdiff_t start1, ptrdiff_t end1,
amt1 = (end2 - start2) + (start2 - end1);
amt2 = (end1 - start1) + (start2 - end1);
- DO_MARKERS (current_buffer, marker)
+ FOR_EACH_MARKER (current_buffer, marker)
{
mpos = marker_vector_charpos (marker);
if (mpos >= start1 && mpos < end2)
@@ -4452,7 +4452,6 @@ transpose_markers (ptrdiff_t start1, ptrdiff_t end1,
}
marker_vector_set_charpos (marker, mpos);
}
- END_DO_MARKERS;
}
DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5,
diff --git a/src/marker-vector.c b/src/marker-vector.c
index b65bbe15845..cd2603f9561 100644
--- a/src/marker-vector.c
+++ b/src/marker-vector.c
@@ -164,7 +164,7 @@ check_marker_vector (struct Lisp_Vector *v, bool allocating)
size_t nused = 0;
Lisp_Object mv = make_lisp_ptr (v, Lisp_Vectorlike);
- DO_MARKERS_OF_VECTOR (mv, m)
+ FOR_EACH_MARKER_OF_VECTOR (mv, m)
{
eassert (m->entry == e_);
eassert (m->buffer != NULL);
@@ -175,7 +175,6 @@ check_marker_vector (struct Lisp_Vector *v, bool allocating)
}
++nused;
}
- END_DO_MARKERS;
eassert ((nused + nfree) * MARKER_VECTOR_ENTRY_SIZE
+ MARKER_VECTOR_HEADER_SIZE == gc_vsize (v));
@@ -286,11 +285,10 @@ marker_vector_reset (struct buffer *b)
/* The old GC contains at least one assertion that unchaining markers
in kill-buffer resets the markers' buffers. IGC does not do this,
can't do this, and does not need it. */
- DO_MARKERS (b, m)
+ FOR_EACH_MARKER (b, m)
{
m->buffer = NULL;
}
- END_DO_MARKERS;
BUF_MARKERS (b) = Qnil;
}
@@ -348,7 +346,7 @@ marker_vector_adjust_for_insert (struct buffer *b,
{
const ptrdiff_t nchars = to_charpos - from_charpos;
struct Lisp_Vector *v = XVECTOR (BUF_MARKERS (b));
- DO_MARKERS (b, m)
+ FOR_EACH_MARKER (b, m)
{
const ptrdiff_t charpos = XFIXNUM (CHARPOS (v, m->entry));
if (charpos == from_charpos)
@@ -359,7 +357,6 @@ marker_vector_adjust_for_insert (struct buffer *b,
else if (charpos > from_charpos)
CHARPOS (v, m->entry) = make_fixnum (charpos + nchars);
}
- END_DO_MARKERS;
}
/* Adjust marker positions of buffer Bs for a replacement of text at
@@ -375,7 +372,7 @@ marker_vector_adjust_for_replace (struct buffer *b,
const ptrdiff_t diff_nchars = new_nchars - old_nchars;
const ptrdiff_t old_to_charpos = from_charpos + old_nchars;
struct Lisp_Vector *v = XVECTOR (BUF_MARKERS (b));
- DO_MARKERS (b, m)
+ FOR_EACH_MARKER (b, m)
{
const ptrdiff_t charpos = XFIXNUM (CHARPOS (v, m->entry));
if (charpos >= old_to_charpos)
@@ -383,5 +380,4 @@ marker_vector_adjust_for_replace (struct buffer *b,
else if (charpos > from_charpos)
CHARPOS (v, m->entry) = make_fixnum (from_charpos);
}
- END_DO_MARKERS;
}
diff --git a/src/undo.c b/src/undo.c
index 65de0bd4e13..3e63c8af3f9 100644
--- a/src/undo.c
+++ b/src/undo.c
@@ -128,7 +128,7 @@ record_marker_adjustments (ptrdiff_t from, ptrdiff_t to)
{
prepare_record ();
- DO_MARKERS (current_buffer, m)
+ FOR_EACH_MARKER (current_buffer, m)
{
ptrdiff_t charpos = marker_vector_charpos (m);
eassert (charpos <= Z);
@@ -154,7 +154,6 @@ record_marker_adjustments (ptrdiff_t from, ptrdiff_t to)
}
}
}
- END_DO_MARKERS;
}
/* Record that a deletion is about to take place, of the characters in
Note that "m" appears only once, so there is another possibility:
FOR_EACH_MARKER (v, struct Lisp_Marker *m)
{
/* do something with m */
}
I'm not saying any of this is a good idea, but it appears possible.
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 19:54:01 GMT)
Full text and
rfc822 format available.
Message #173 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Pip Cet <pipcet <at> protonmail.com> writes:
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>
>>>> + DO_MARKERS (b, m)
>>>> + {
>>>> + if (!vectorlike_marked_p (&m->header))
>>>> + marker_vector_remove (XVECTOR (BUF_MARKERS (b)), m);
>>>> + }
>>>> + END_DO_MARKERS;
>>>
>>> Would it be possible not to introduce macros that modify the C syntax?
>>> These are problematic for more than one reason (one being that they
>>> are not recognized by C modes we use). We have macros whose names
>>> start with FOR_EACH_ (like FOR_EACH_FRAME); can we introduce
>>> FOR_EACH_MARKER instead, and not hide opening/closing braces inside
>>> macros?
>>
>> I tried, already in igc, but failed. IOW, I don't now a way to get rid
>> of the END_DO_MARKERS.
>
> Okay, I'll bite.
:-)
> I think what we should do is mimic FOR_EACH_TAIL, and use
> FOR_EACH_MARKER like this:
>
> struct Lisp_Marker *m;
> FOR_EACH_MARKER (b, m)
> {
> /* do something with m */
> }
We need an if somewhere for the MARKERP, don't we? So...
> However, if we wanted to, we could do this (the "continued" thing is
> overkill to make "break" work in a FOR_EACH_MARKER loop):
... we'll need this.
The continued thing is quite clever, didn't think of it. I dismissed the
two for loops because of the break/continue. So, since it's already
done, and can be pilfered, that's fine with me :-). Let's see if the
others find it too complicated.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Thu, 24 Apr 2025 21:03:01 GMT)
Full text and
rfc822 format available.
Message #176 received at 77924 <at> debbugs.gnu.org (full text, mbox):
>>> So the benchmark tests scroll time, including jit/font-lock time.
>>> It uses its own copy of a major mode, so that you can compare "scroll +
>>> font-lock" performance between different Emacs releases without being
>>> affected by improvements/regressions in CC-mode itself.
>>
>> So maybe it would make sense to run both that benchmark as-is plus one
>> without smie but with tamil.txt, I guess. Or in other words, now only
>> the as-is benchmark, because the tamil.txt results I've already posted.
>
> Before including tamil.txt, I would like to make clear that the file
> contents is volume 1 of a (popular) novel which was released in the 50s.
> I got the text itself from Project Madurai which states that [1]:
It'd be good to have a benchmark for multibyte "human text", indeed.
I'll let others figure out the copyright issues (hopefully none) and
then write the corresponding benchmark.
In the mean time I took that `xmenu.c` code used for the scroll and SMIE
benchmarks and made it multibyte by replacing all the ASCII letter with
arbitrary non-ASCII characters.
I tried to do it in such a way that the keywords are preserved so it's
still a "valid" C file that is still highlighted and indented in the
same way.
The resulting `scroll-nonascii` and `smie-nonascii` benchmarks are now
in `elisp-benchmark` in `elpa.git`.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 04:20:03 GMT)
Full text and
rfc822 format available.
Message #179 received at 77924 <at> debbugs.gnu.org (full text, mbox):
BTW, I did a run of the complete benchmark suite (plus some extra
micro-benchmarks). This compares the base of the branch to the tip of
the text-index branch:
| test | BASE (s) | gc (s) | TINDEX (s) | gc (s) | err |
|--------------------------+------------+--------+------------+--------+-----|
| bubble | 4.46 | 0.00 | 4.57 | 0.00 | 0% |
| bubble-no-cons | 15.04 | 0.00 | 14.71 | 0.00 | 0% |
| bytecomp | 3.67 | 0.00 | 3.72 | 0.00 | 0% |
| dhrystone | 10.30 | 0.00 | 10.13 | 0.00 | 0% |
| eieio | 4.06 | 0.00 | 4.10 | 0.00 | 0% |
| fibn | 3.22 | 0.00 | 3.34 | 0.00 | 0% |
| fibn-named-let | 4.11 | 0.00 | 4.08 | 0.00 | 0% |
| fibn-rec | 6.81 | 0.00 | 6.88 | 0.00 | 0% |
| fibn-tc | 5.34 | 0.00 | 5.12 | 0.00 | 0% |
| flet | 10.53 | 0.00 | 10.68 | 0.00 | 0% |
| font-lock | 1.30 | 0.00 | 1.29 | 0.00 | 0% |
| inclist | 24.65 | 0.00 | 16.21 | 0.00 | 0% |
| inclist-type-hints | 24.56 | 0.00 | 16.23 | 0.00 | 0% |
| listlen-tc | 5.19 | 0.00 | 5.16 | 0.00 | 0% |
| map-closure | 9.86 | 0.00 | 9.26 | 0.00 | 0% |
| markers-charbyte-1G/0*1 | 37.38 | 0.00 | 5.54 | 0.00 | 2% |
| markers-charbyte-1G/0*10 | 283.68 | 0.00 | 7.25 | 0.00 | 1% |
| markers-charbyte-1G/1*1 | 3.64 | 0.00 | 0.97 | 0.00 | 5% |
| markers-charbyte-1G/1*10 | 26.11 | 0.00 | 2.69 | 0.00 | 2% |
| markers-save-excursion-0 | 14.07 | 1.48 | 15.58 | 0.00 | 1% |
| markers-save-excursion-3 | 12.97 | 1.34 | 14.99 | 0.00 | 0% |
| nbody | 4.95 | 0.00 | 4.80 | 0.00 | 0% |
| pack-unpack | 0.85 | 0.00 | 0.84 | 0.00 | 0% |
| pack-unpack-old | 2.82 | 0.00 | 2.77 | 0.00 | 1% |
| pcase | 10.37 | 0.00 | 10.35 | 0.00 | 0% |
| pidigits | 10.43 | 0.00 | 9.09 | 0.00 | 2% |
| regexp-cubic-200 | 1.48 | 0.00 | 1.48 | 0.00 | 1% |
| regexp-cubic-400 | 2.31 | 0.00 | 2.32 | 0.00 | 1% |
| regexp-exponential | 3.68 | 0.00 | 3.69 | 0.00 | 0% |
| regexp-gnumsg | 0.92 | 0.00 | 0.92 | 0.00 | 0% |
| regexp-linear-100 | 1.07 | 0.00 | 1.08 | 0.00 | 0% |
| regexp-linear-1000 | 1.01 | 0.00 | 1.01 | 0.00 | 0% |
| regexp-linear-5000 | 1.01 | 0.00 | 1.01 | 0.00 | 0% |
| regexp-quadratic-1000 | 1.06 | 0.00 | 1.07 | 0.00 | 1% |
| regexp-quadratic-5000 | 2.65 | 0.00 | 2.66 | 0.00 | 1% |
| regexp-tuareg | 0.46 | 0.00 | 0.44 | 0.00 | 0% |
| scroll | 1.33 | 0.00 | 1.33 | 0.00 | 0% |
| scroll-nonascii | 2.68 | 0.00 | 2.60 | 0.00 | 0% |
| search | 29.42 | 0.00 | 29.55 | 0.00 | 0% |
| search/m50k | 29.02 | 0.00 | 29.55 | 0.00 | 0% |
| search/nolookup | 24.84 | 0.00 | 24.99 | 0.00 | 0% |
| search/p02 | 55.03 | 0.00 | 52.91 | 0.00 | 0% |
| search/p32 | 31.22 | 0.00 | 31.12 | 0.00 | 0% |
| smie | 2.98 | 0.00 | 3.03 | 0.00 | 0% |
| smie-nonascii | 5.82 | 0.00 | 15.96 | 0.00 | 0% |
|--------------------------+------------+--------+------------+--------+-----|
Most of the benchmarks are mostly unaffected. Comments:
- the `err` column shows the maximum of the std-dev for the BASE and the
TINDEX (both were run 10 times).
This is run on a fairly old server, because it's the machine with
least variability to which I have access (no DVFS effects, plenty of
idle cores and RAM not to be too affected by other processes, ...).
- The `markers-charbyte-*` benchmarks are micro-benchmarks testing the
charpos->bytepos conversion by making many calls of `char-after` to
random positions in the buffer. The fact that we go *much* faster there
is just normal: this is the poor behavior the patch is intended to fix.
IOW, not going much faster would be a big disappointment.
- The `markers-save-excursion-*` benchmarks are meant to test the
performance of a "cheap" save-excursion. Here, the BASE code is very
efficient and my previous "sorted-array-of-markers-with-gap" had trouble
staying competitive. Here we see that the new branch is a bit slower
but not by very much. Also note that it GCs less, presumably because
the marker objects are smaller (4words vs 6words) so GCs are a bit
less frequent.
- `inclist` is significantly faster. I have no clue what's up with that.
This micro-benchmark presumably doesn't get anywhere near markers or
charpos or bytepos.
FWIW, I ran those benchmarks on a slightly different pair of builds (I
honestly can't say what's different) and there was no difference in
that other case. AFAICT 16.2s is the "right" answer and the 24.5s
shown here is a weird corner case that's better ignored.
Welcome to the wonderful world of micro-benchmarks!
- `pidigits` is slightly faster, and I again have no clue why.
Probably some unrelated effect. In this case, my other pair of builds
showed approximately the same difference, so it seems to be "a bit
less of a fluke"? In any case, I'd ignore this one as well.
- the `search*` benchmarks are designed to test the bytepos->charpos
conversions done during regexp matching because the lookups for the
`syntax-table` text properties. I wrote them back when we had
a serious performance problem there, but nowadays the `master` branch
handles this very well, so we see the branch is not able to get any
benefit: the bytepos to convert is basically always right next to the
last one, so we never need to consult anything like markers or the
text-index to compute the charpos.
- `smie` is a test which re-indents `xmenu.c` (using the
SMIE-based `sm-c-mode` rather than CC-mode). Since `xmenu.c` is
an ASCII file, the text-index can't be very useful so it's no wonder
that we don't see any performance difference.
- Finally `smie-nonascii` is the same as `smie` except that it uses
a file that's identical to `xmenu.c` but where all the letters of
comments/strings/identifiers were replaced by non-ASCII ones.
Here, we see that TINDEX` is *much* slower than BASE.
This doesn't seem to be a fluke: I see the exact same performance
difference in my other pair of builds.
This last one is a serious problem that we need to address before we can
merge the branch.
FWIW, I also ran those benchmark on the same machine with Debian's
Emacs-30.1 and the results are basically identical to those of
BASE above.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 05:41:04 GMT)
Full text and
rfc822 format available.
Message #182 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> - Finally `smie-nonascii` is the same as `smie` except that it uses
> a file that's identical to `xmenu.c` but where all the letters of
> comments/strings/identifiers were replaced by non-ASCII ones.
> Here, we see that TINDEX` is *much* slower than BASE.
> This doesn't seem to be a fluke: I see the exact same performance
> difference in my other pair of builds.
>
> This last one is a serious problem that we need to address before we can
> merge the branch.
ASCII means that the text index is basically not used.
In the non-ASCII case it is used. Re-indentation means text modification
which in turns leads to index invalidation and re-building the index as
needed. Index invalidation discards the index from the modification
position to the end of the text. Re-building goes from the last valid
position the index contains to the next position that is needed to do a
position conversion.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 06:22:04 GMT)
Full text and
rfc822 format available.
Message #185 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Cc: monnier <at> iro.umontreal.ca, stefankangas <at> gmail.com, 77924 <at> debbugs.gnu.org
> Date: Thu, 24 Apr 2025 18:27:51 +0200
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> - m->next = BUF_MARKERS (buf);
> >> - BUF_MARKERS (buf) = m;
> >> + marker_vector_add (buf, m);
> >> + marker_vector_set_charpos (m, charpos);
> >
> > Could we please name functions that manipulate and get marker
> > information marker_SOMETHING and not marker_vector_something?
> > marker_vector_add is semi-okay (although marker_add would have been
> > nicer, IMO), but marker_vector_set_charpos and marker_vector_charpos
> > are not, because AFAIU they manipulate the marker's position, not
> > marker-vector's position.
> >
> > In general, wherever the fact that we have a vector of markers is
> > merely an implementation detail that is not important to the
> > programmer, can we just drop the "vector" part from the names of the
> > functions, for the same reason that the old functions and macros
> > weren't called marker_list_SOMETHING?
>
> Well. For one thing, the marker is now no longer the central data
> structure. It's basically only fancy index into a marker vector + 2 bit
> flags (which could/should by moved to the marker vector, maybe). The
> marker vector is the central data structure holding a character position
> which the marker refers to.
>
> Secondly functions marker_position and marker_byte_position still exist,
> basically one level higher. But below that level, the implementation
> necessarily lurks.
>
> That's why I'd prefer to keep the marker_vector prefix.
These implementation details are not important for someone who wants
to understand what the call does, as opposed to someone who studies
the implementation. So having marker_position etc. will make it
easier to read the code, let alone get us shorter names.
> >> + DO_MARKERS (b, m)
> >> + {
> >> + if (!vectorlike_marked_p (&m->header))
> >> + marker_vector_remove (XVECTOR (BUF_MARKERS (b)), m);
> >> + }
> >> + END_DO_MARKERS;
> >
> > Would it be possible not to introduce macros that modify the C syntax?
> > These are problematic for more than one reason (one being that they
> > are not recognized by C modes we use). We have macros whose names
> > start with FOR_EACH_ (like FOR_EACH_FRAME); can we introduce
> > FOR_EACH_MARKER instead, and not hide opening/closing braces inside
> > macros?
>
> I tried, already in igc, but failed. IOW, I don't now a way to get rid
> of the END_DO_MARKERS.
It would mean that each loop will need to reproduce some of the
boilerplate code you now hide behind DO_MARKERS, such that the opening
braces are not part of the macro, but are explicit in the source.
Like we do in the FOR_EACH_* macros. That would require some more
typing, but the benefits IMO far outweigh this downside.
> I hope Stef can take care of some of the rest. Stef, please let me know
> if should help. It could take a few days though, because of real-world
> interference.
>
> Thanks for the review.
Thanks for working on this in the first place.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 06:26:02 GMT)
Full text and
rfc822 format available.
Message #188 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, stefankangas <at> gmail.com,
> 77924 <at> debbugs.gnu.org, yantar92 <at> posteo.net
> Date: Thu, 24 Apr 2025 18:32:00 +0200
>
> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
> >>> Evaluate this, then invoke "M-x scroll-up-benchmark" in a large buffer
> >>> with lots of non-ASCII characters. Compare the timings between the
> >>> two versions of Emacs.
> >>
> >> elb-scroll from elisp-bechmarks is basically
> >>
> >> (dotimes (_ 10)
> >> (elb-smie-mode)
> >> (goto-char (point-min))
> >> (condition-case nil
> >> (while t (scroll-up nil) (redisplay 'force))
> >> (end-of-buffer nil))))))
> >>
> >> looks similar, but I don't know what elb-smie-mode does.
> >
> > It's a major mode for the C language, separate from CC-mode.
> > [ It's basically "vendored" copy of the `sm-c-mode` that's on GNU
> > ELPA. ]
>
> Ah, interesting.
>
> > So the benchmark tests scroll time, including jit/font-lock time.
> > It uses its own copy of a major mode, so that you can compare "scroll +
> > font-lock" performance between different Emacs releases without being
> > affected by improvements/regressions in CC-mode itself.
>
> So maybe it would make sense to run both that benchmark as-is plus one
> without smie but with tamil.txt, I guess. Or in other words, now only
> the as-is benchmark, because the tamil.txt results I've already posted.
Scrolling through tamil.txt and any other similar text file needs a
benchmark that doesn't require sm-c-mode, because these are usually
not C source files, and OTOH C source files rarely include many
non-ASCII characters, let alone Tamil or something like that.
And one more thing, because I'm not familiar with how the benchmark
suite is run: the scroll benchmarks must be run interactively, because
otherwise we are at best exercising the TTY redisplay, and at worst
don't exercise redisplay at all, since nothing is being displayed.
The charpos-to-bytepos conversions are used a lot in the display code,
so we must let redisplay work for this to be a meaningful benchmark.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 06:36:01 GMT)
Full text and
rfc822 format available.
Message #191 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Visuwesh <visuweshm <at> gmail.com>
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, Eli Zaretskii
> <eliz <at> gnu.org>, yantar92 <at> posteo.net, stefankangas <at> gmail.com,
> 77924 <at> debbugs.gnu.org
> Date: Thu, 24 Apr 2025 23:55:05 +0530
>
> [வியாழன் ஏப்ரல் 24, 2025] Gerd Möllmann wrote:
>
> >> So the benchmark tests scroll time, including jit/font-lock time.
> >> It uses its own copy of a major mode, so that you can compare "scroll +
> >> font-lock" performance between different Emacs releases without being
> >> affected by improvements/regressions in CC-mode itself.
> >
> > So maybe it would make sense to run both that benchmark as-is plus one
> > without smie but with tamil.txt, I guess. Or in other words, now only
> > the as-is benchmark, because the tamil.txt results I've already posted.
>
> Before including tamil.txt, I would like to make clear that the file
> contents is volume 1 of a (popular) novel which was released in the 50s.
> I got the text itself from Project Madurai which states that [1]:
>
> Acknowledgement:
> Our Sincere thanks go to the following persons for their assistance in the preparation of this work.
> Etext preparation : AU-KBC Research Center (Mr. Baskaran), Anna University, Chennai,India
> Proof-reading: Mr. S. Anbumani, Mr. N.D. Logasundaram,Mr. Narayanan Govindarajan,
> Ms. Pavithra Srinivasan, Mr. Ramachandran Mahadevan, Ms. Sathya, Mr. Sreeram Krishnamoorthy,
> Dr. Sridhar Rathinam, Mrs. Srilatha Rajagopal, Mr. VinothJagannathan
> Web version: Mr. S. Anbumani, Blacksburg, Virginia, USA
>
> This webpage presents the Etxt in Tamil script in Unicode encoding.
> This file was last revised on Apr. 12, 2003
>
> © Project Madurai, 1998-2021.
> Project Madurai is an open, voluntary, worldwide initiative devoted to preparation
> of electronic texts of tamil literary works and to distribute them free on the Internet.
> Details of Project Madurai are available at the website
> https://www.projectmadurai.org/
> You are welcome to freely distribute this file, provided this header page is kept intact.
>
> So I assume there should no problem in distributing it in the repo. If
> there could be, we could try composing all the text from any work in
> freetamilebooks.com which publishes under the CC BY-NC-SA license.
>
> 1. https://www.projectmadurai.org/pm_etexts/utf8/pmuni0169_00.html
First, we don't have to include the file in our benchmarks. We could
instead run the benchmarks on it and publish the results. It isn't
like the implementation of charpos<->bytepos conversions changes too
often.
Second, after looking at the site, I cannot figure out the copyright
issues. The only place where this is addressed is the following
question in their FAQ:
3.3 How about copyright issues ?
Project Madurai strictly adheres to copyright standards. All the
works in the collection are either from public-domain (after a certain
period of their creation, currently ca. 75 years after the lifespan of
the author) or with due consent from the respective authors.
I understand the PD part, but not the other part. What does it mean?
They also have this:
3.4 I have a suggestion. Whom can I contact ?
Feel free to send a mail to Dr. Kalyanasundaram . They will be glad
to hear your suggestion or opinions, if any.
So maybe we should ask this person, assuming we want to include the
file in our benchmark.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 06:46:02 GMT)
Full text and
rfc822 format available.
Message #194 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> Date: Thu, 24 Apr 2025 19:26:10 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, monnier <at> iro.umontreal.ca, 77924 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>
> > I tried, already in igc, but failed. IOW, I don't now a way to get rid
> > of the END_DO_MARKERS.
>
> Okay, I'll bite.
>
> I think what we should do is mimic FOR_EACH_TAIL, and use
> FOR_EACH_MARKER like this:
>
> struct Lisp_Marker *m;
> FOR_EACH_MARKER (b, m)
> {
> /* do something with m */
> }
Yes, that's what I basically had in mind.
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 07:02:02 GMT)
Full text and
rfc822 format available.
Message #197 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, monnier <at> iro.umontreal.ca,
> 77924 <at> debbugs.gnu.org, stefankangas <at> gmail.com
> Date: Thu, 24 Apr 2025 21:53:05 +0200
>
> Pip Cet <pipcet <at> protonmail.com> writes:
>
> > I think what we should do is mimic FOR_EACH_TAIL, and use
> > FOR_EACH_MARKER like this:
> >
> > struct Lisp_Marker *m;
> > FOR_EACH_MARKER (b, m)
> > {
> > /* do something with m */
> > }
>
> We need an if somewhere for the MARKERP, don't we?
Why is that needed, btw? Can't we change the representation and/or
the functions involved to avoid the need for such a test?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 07:04:01 GMT)
Full text and
rfc822 format available.
Message #200 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Gerd Möllmann <gerd.moellmann <at> gmail.com>, Eli
> Zaretskii <eliz <at> gnu.org>,
> yantar92 <at> posteo.net, stefankangas <at> gmail.com, 77924 <at> debbugs.gnu.org
> Date: Thu, 24 Apr 2025 17:02:26 -0400
>
> In the mean time I took that `xmenu.c` code used for the scroll and SMIE
> benchmarks and made it multibyte by replacing all the ASCII letter with
> arbitrary non-ASCII characters.
> I tried to do it in such a way that the keywords are preserved so it's
> still a "valid" C file that is still highlighted and indented in the
> same way.
>
> The resulting `scroll-nonascii` and `smie-nonascii` benchmarks are now
> in `elisp-benchmark` in `elpa.git`.
Would you mind sharing the results?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 07:22:01 GMT)
Full text and
rfc822 format available.
Message #203 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Sent from my iPhone
> On 25. Apr 2025, at 09:01, Eli Zaretskii <eliz <at> gnu.org> wrote:
>
>
>>
>> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
>> Cc: Eli Zaretskii <eliz <at> gnu.org>, monnier <at> iro.umontreal.ca,
>> 77924 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>> Date: Thu, 24 Apr 2025 21:53:05 +0200
>>
>> Pip Cet <pipcet <at> protonmail.com> writes:
>>
>>> I think what we should do is mimic FOR_EACH_TAIL, and use
>>> FOR_EACH_MARKER like this:
>>>
>>> struct Lisp_Marker *m;
>>> FOR_EACH_MARKER (b, m)
>>> {
>>> /* do something with m */
>>> }
>>
>> We need an if somewhere for the MARKERP, don't we?
>
> Why is that needed, btw? Can't we change the representation and/or
> the functions involved to avoid the need for such a test?
When markers are freed their entry in the market vector no longer contains a marker reference.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 07:40:02 GMT)
Full text and
rfc822 format available.
Message #206 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: gerd.moellmann <at> gmail.com, stefankangas <at> gmail.com, 77924 <at> debbugs.gnu.org
> Date: Fri, 25 Apr 2025 00:19:42 -0400
>
> BTW, I did a run of the complete benchmark suite (plus some extra
> micro-benchmarks). This compares the base of the branch to the tip of
> the text-index branch:
Thanks.
> - Finally `smie-nonascii` is the same as `smie` except that it uses
> a file that's identical to `xmenu.c` but where all the letters of
> comments/strings/identifiers were replaced by non-ASCII ones.
> Here, we see that TINDEX` is *much* slower than BASE.
> This doesn't seem to be a fluke: I see the exact same performance
> difference in my other pair of builds.
>
> This last one is a serious problem that we need to address before we can
> merge the branch.
Agreed.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 07:46:01 GMT)
Full text and
rfc822 format available.
Message #209 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Date: Fri, 25 Apr 2025 09:20:55 +0200
> Cc: pipcet <at> protonmail.com, monnier <at> iro.umontreal.ca, 77924 <at> debbugs.gnu.org,
> stefankangas <at> gmail.com
>
>
> > On 25. Apr 2025, at 09:01, Eli Zaretskii <eliz <at> gnu.org> wrote:
> >
> >
> >>
> >> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> >> Cc: Eli Zaretskii <eliz <at> gnu.org>, monnier <at> iro.umontreal.ca,
> >> 77924 <at> debbugs.gnu.org, stefankangas <at> gmail.com
> >> Date: Thu, 24 Apr 2025 21:53:05 +0200
> >>
> >> Pip Cet <pipcet <at> protonmail.com> writes:
> >>
> >>> I think what we should do is mimic FOR_EACH_TAIL, and use
> >>> FOR_EACH_MARKER like this:
> >>>
> >>> struct Lisp_Marker *m;
> >>> FOR_EACH_MARKER (b, m)
> >>> {
> >>> /* do something with m */
> >>> }
> >>
> >> We need an if somewhere for the MARKERP, don't we?
> >
> > Why is that needed, btw? Can't we change the representation and/or
> > the functions involved to avoid the need for such a test?
>
> When markers are freed their entry in the market vector no longer contains a marker reference.
I understand, but why does this need to be tested inside the loop?
Can't the loop itself know how many markers are in the vector, and
stop when they are exhausted?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 07:52:01 GMT)
Full text and
rfc822 format available.
Message #212 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Sent from my iPhone
> On 25. Apr 2025, at 09:45, Eli Zaretskii <eliz <at> gnu.org> wrote:
>
>
>>
>> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
>> Date: Fri, 25 Apr 2025 09:20:55 +0200
>> Cc: pipcet <at> protonmail.com, monnier <at> iro.umontreal.ca, 77924 <at> debbugs.gnu.org,
>> stefankangas <at> gmail.com
>>
>>
>>>> On 25. Apr 2025, at 09:01, Eli Zaretskii <eliz <at> gnu.org> wrote:
>>>
>>>
>>>>
>>>> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
>>>> Cc: Eli Zaretskii <eliz <at> gnu.org>, monnier <at> iro.umontreal.ca,
>>>> 77924 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>>>> Date: Thu, 24 Apr 2025 21:53:05 +0200
>>>>
>>>> Pip Cet <pipcet <at> protonmail.com> writes:
>>>>
>>>>> I think what we should do is mimic FOR_EACH_TAIL, and use
>>>>> FOR_EACH_MARKER like this:
>>>>>
>>>>> struct Lisp_Marker *m;
>>>>> FOR_EACH_MARKER (b, m)
>>>>> {
>>>>> /* do something with m */
>>>>> }
>>>>
>>>> We need an if somewhere for the MARKERP, don't we?
>>>
>>> Why is that needed, btw? Can't we change the representation and/or
>>> the functions involved to avoid the need for such a test?
>>
>> When markers are freed their entry in the market vector no longer contains a marker reference.
>
> I understand, but why does this need to be tested inside the loop?
> Can't the loop itself know how many markers are in the vector, and
> stop when they are exhausted?
You are thinking of compacting the vector when a marker is freed? Then we would lose the O(1) deletion. As it is the entry is simply pushed on a free list.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 10:19:01 GMT)
Full text and
rfc822 format available.
Message #215 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> BTW, I did a run of the complete benchmark suite (plus some extra
> micro-benchmarks). This compares the base of the branch to the tip of
> the text-index branch:
>
> | test | BASE (s) | gc (s) | TINDEX (s) | gc (s) | err |
> |--------------------------+------------+--------+------------+--------+-----|
> | bubble | 4.46 | 0.00 | 4.57 | 0.00 | 0% |
> | bubble-no-cons | 15.04 | 0.00 | 14.71 | 0.00 | 0% |
> | bytecomp | 3.67 | 0.00 | 3.72 | 0.00 | 0% |
> | dhrystone | 10.30 | 0.00 | 10.13 | 0.00 | 0% |
> | eieio | 4.06 | 0.00 | 4.10 | 0.00 | 0% |
> | fibn | 3.22 | 0.00 | 3.34 | 0.00 | 0% |
> | fibn-named-let | 4.11 | 0.00 | 4.08 | 0.00 | 0% |
> | fibn-rec | 6.81 | 0.00 | 6.88 | 0.00 | 0% |
> | fibn-tc | 5.34 | 0.00 | 5.12 | 0.00 | 0% |
> | flet | 10.53 | 0.00 | 10.68 | 0.00 | 0% |
> | font-lock | 1.30 | 0.00 | 1.29 | 0.00 | 0% |
> | inclist | 24.65 | 0.00 | 16.21 | 0.00 | 0% |
> | inclist-type-hints | 24.56 | 0.00 | 16.23 | 0.00 | 0% |
> | listlen-tc | 5.19 | 0.00 | 5.16 | 0.00 | 0% |
> | map-closure | 9.86 | 0.00 | 9.26 | 0.00 | 0% |
> | markers-charbyte-1G/0*1 | 37.38 | 0.00 | 5.54 | 0.00 | 2% |
> | markers-charbyte-1G/0*10 | 283.68 | 0.00 | 7.25 | 0.00 | 1% |
> | markers-charbyte-1G/1*1 | 3.64 | 0.00 | 0.97 | 0.00 | 5% |
> | markers-charbyte-1G/1*10 | 26.11 | 0.00 | 2.69 | 0.00 | 2% |
> | markers-save-excursion-0 | 14.07 | 1.48 | 15.58 | 0.00 | 1% |
> | markers-save-excursion-3 | 12.97 | 1.34 | 14.99 | 0.00 | 0% |
> | nbody | 4.95 | 0.00 | 4.80 | 0.00 | 0% |
> | pack-unpack | 0.85 | 0.00 | 0.84 | 0.00 | 0% |
> | pack-unpack-old | 2.82 | 0.00 | 2.77 | 0.00 | 1% |
> | pcase | 10.37 | 0.00 | 10.35 | 0.00 | 0% |
> | pidigits | 10.43 | 0.00 | 9.09 | 0.00 | 2% |
> | regexp-cubic-200 | 1.48 | 0.00 | 1.48 | 0.00 | 1% |
> | regexp-cubic-400 | 2.31 | 0.00 | 2.32 | 0.00 | 1% |
> | regexp-exponential | 3.68 | 0.00 | 3.69 | 0.00 | 0% |
> | regexp-gnumsg | 0.92 | 0.00 | 0.92 | 0.00 | 0% |
> | regexp-linear-100 | 1.07 | 0.00 | 1.08 | 0.00 | 0% |
> | regexp-linear-1000 | 1.01 | 0.00 | 1.01 | 0.00 | 0% |
> | regexp-linear-5000 | 1.01 | 0.00 | 1.01 | 0.00 | 0% |
> | regexp-quadratic-1000 | 1.06 | 0.00 | 1.07 | 0.00 | 1% |
> | regexp-quadratic-5000 | 2.65 | 0.00 | 2.66 | 0.00 | 1% |
> | regexp-tuareg | 0.46 | 0.00 | 0.44 | 0.00 | 0% |
> | scroll | 1.33 | 0.00 | 1.33 | 0.00 | 0% |
> | scroll-nonascii | 2.68 | 0.00 | 2.60 | 0.00 | 0% |
> | search | 29.42 | 0.00 | 29.55 | 0.00 | 0% |
> | search/m50k | 29.02 | 0.00 | 29.55 | 0.00 | 0% |
> | search/nolookup | 24.84 | 0.00 | 24.99 | 0.00 | 0% |
> | search/p02 | 55.03 | 0.00 | 52.91 | 0.00 | 0% |
> | search/p32 | 31.22 | 0.00 | 31.12 | 0.00 | 0% |
> | smie | 2.98 | 0.00 | 3.03 | 0.00 | 0% |
> | smie-nonascii | 5.82 | 0.00 | 15.96 | 0.00 | 0% |
> |--------------------------+------------+--------+------------+--------+-----|
>
> Most of the benchmarks are mostly unaffected. Comments:
>
> - the `err` column shows the maximum of the std-dev for the BASE and the
> TINDEX (both were run 10 times).
> This is run on a fairly old server, because it's the machine with
> least variability to which I have access (no DVFS effects, plenty of
> idle cores and RAM not to be too affected by other processes, ...).
>
> - The `markers-charbyte-*` benchmarks are micro-benchmarks testing the
> charpos->bytepos conversion by making many calls of `char-after` to
> random positions in the buffer. The fact that we go *much* faster there
> is just normal: this is the poor behavior the patch is intended to fix.
> IOW, not going much faster would be a big disappointment.
>
> - The `markers-save-excursion-*` benchmarks are meant to test the
> performance of a "cheap" save-excursion. Here, the BASE code is very
> efficient and my previous "sorted-array-of-markers-with-gap" had trouble
> staying competitive. Here we see that the new branch is a bit slower
> but not by very much. Also note that it GCs less, presumably because
> the marker objects are smaller (4words vs 6words) so GCs are a bit
> less frequent.
>
> - `inclist` is significantly faster. I have no clue what's up with that.
> This micro-benchmark presumably doesn't get anywhere near markers or
> charpos or bytepos.
> FWIW, I ran those benchmarks on a slightly different pair of builds (I
> honestly can't say what's different) and there was no difference in
> that other case. AFAICT 16.2s is the "right" answer and the 24.5s
> shown here is a weird corner case that's better ignored.
> Welcome to the wonderful world of micro-benchmarks!
>
> - `pidigits` is slightly faster, and I again have no clue why.
> Probably some unrelated effect. In this case, my other pair of builds
> showed approximately the same difference, so it seems to be "a bit
> less of a fluke"? In any case, I'd ignore this one as well.
>
> - the `search*` benchmarks are designed to test the bytepos->charpos
> conversions done during regexp matching because the lookups for the
> `syntax-table` text properties. I wrote them back when we had
> a serious performance problem there, but nowadays the `master` branch
> handles this very well, so we see the branch is not able to get any
> benefit: the bytepos to convert is basically always right next to the
> last one, so we never need to consult anything like markers or the
> text-index to compute the charpos.
>
> - `smie` is a test which re-indents `xmenu.c` (using the
> SMIE-based `sm-c-mode` rather than CC-mode). Since `xmenu.c` is
> an ASCII file, the text-index can't be very useful so it's no wonder
> that we don't see any performance difference.
>
> - Finally `smie-nonascii` is the same as `smie` except that it uses
> a file that's identical to `xmenu.c` but where all the letters of
> comments/strings/identifiers were replaced by non-ASCII ones.
> Here, we see that TINDEX` is *much* slower than BASE.
> This doesn't seem to be a fluke: I see the exact same performance
> difference in my other pair of builds.
>
> This last one is a serious problem that we need to address before we can
> merge the branch.
>
> FWIW, I also ran those benchmark on the same machine with Debian's
> Emacs-30.1 and the results are basically identical to those of
> BASE above.
I've got the new benchmarks from elpa.git, and ran the scroll and smie
becnmarks on my otherwise idle Mac mini M1, GUI Emacs,
Scroll:
* master Results
| test | non-gc (s) | gc (s) | gcs | total (s) | err |
|-----------------+------------+--------+-----+-----------+-----|
| scroll | 1.35 | 0.34 | 22 | 1.69 | 0% |
| scroll-nonascii | 3.24 | 0.77 | 50 | 4.01 | 0% |
|-----------------+------------+--------+-----+-----------+-----|
| total | 4.59 | 1.11 | 72 | 5.70 | 0% |
* text-index Results
| test | non-gc (s) | gc (s) | gcs | total (s) | err |
|-----------------+------------+--------+-----+-----------+-----|
| scroll | 1.34 | 0.33 | 22 | 1.68 | 0% |
| scroll-nonascii | 3.16 | 0.75 | 49 | 3.91 | 0% |
|-----------------+------------+--------+-----+-----------+-----|
| total | 4.51 | 1.08 | 71 | 5.59 | 0% |
My summary, together with what I got with tamil.txt yesterday:
indistinguishable, if not a tad better with text-index
Smie:
* master Results
| test | non-gc (s) | gc (s) | gcs | total (s) | err |
|---------------+------------+--------+-----+-----------+-----|
| font-lock | 0.52 | 0.32 | 24 | 0.84 | 0% |
| smie | 1.19 | 0.50 | 36 | 1.69 | 0% |
| smie-nonascii | 2.13 | 0.53 | 39 | 2.66 | 0% |
|---------------+------------+--------+-----+-----------+-----|
| total | 3.83 | 1.35 | 99 | 5.19 | 0% |
* text-index Results
| test | non-gc (s) | gc (s) | gcs | total (s) | err |
|---------------+------------+--------+-----+-----------+-----|
| font-lock | 0.51 | 0.31 | 23 | 0.82 | 0% |
| smie | 1.18 | 0.45 | 33 | 1.63 | 0% |
| smie-nonascii | 5.49 | 0.48 | 35 | 5.98 | 0% |
|---------------+------------+--------+-----+-----------+-----|
| total | 7.19 | 1.24 | 92 | 8.43 | 0% |
That shows also the difference in smie-nonascii, i.e. in a C file
containing multi-byte characters (one should suffice to make
Z != Z_BYTE, and the index is used).
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 10:36:02 GMT)
Full text and
rfc822 format available.
Message #218 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Date: Fri, 25 Apr 2025 09:51:05 +0200
> Cc: pipcet <at> protonmail.com, monnier <at> iro.umontreal.ca, 77924 <at> debbugs.gnu.org,
> stefankangas <at> gmail.com
>
>
> Sent from my iPhone
>
> > On 25. Apr 2025, at 09:45, Eli Zaretskii <eliz <at> gnu.org> wrote:
> >
> >
> >>
> >> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> >> Date: Fri, 25 Apr 2025 09:20:55 +0200
> >> Cc: pipcet <at> protonmail.com, monnier <at> iro.umontreal.ca, 77924 <at> debbugs.gnu.org,
> >> stefankangas <at> gmail.com
> >>
> >>
> >>>> On 25. Apr 2025, at 09:01, Eli Zaretskii <eliz <at> gnu.org> wrote:
> >>>
> >>>
> >>>>
> >>>> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> >>>> Cc: Eli Zaretskii <eliz <at> gnu.org>, monnier <at> iro.umontreal.ca,
> >>>> 77924 <at> debbugs.gnu.org, stefankangas <at> gmail.com
> >>>> Date: Thu, 24 Apr 2025 21:53:05 +0200
> >>>>
> >>>> Pip Cet <pipcet <at> protonmail.com> writes:
> >>>>
> >>>>> I think what we should do is mimic FOR_EACH_TAIL, and use
> >>>>> FOR_EACH_MARKER like this:
> >>>>>
> >>>>> struct Lisp_Marker *m;
> >>>>> FOR_EACH_MARKER (b, m)
> >>>>> {
> >>>>> /* do something with m */
> >>>>> }
> >>>>
> >>>> We need an if somewhere for the MARKERP, don't we?
> >>>
> >>> Why is that needed, btw? Can't we change the representation and/or
> >>> the functions involved to avoid the need for such a test?
> >>
> >> When markers are freed their entry in the market vector no longer contains a marker reference.
> >
> > I understand, but why does this need to be tested inside the loop?
> > Can't the loop itself know how many markers are in the vector, and
> > stop when they are exhausted?
>
> You are thinking of compacting the vector when a marker is freed? Then we would lose the O(1) deletion. As it is the entry is simply pushed on a free list.
Actually, I thought about maintaining the number of markers in the
vector, so there would be no need to detect that by the MARKERP test.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 11:42:02 GMT)
Full text and
rfc822 format available.
Message #221 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
>> Date: Fri, 25 Apr 2025 09:51:05 +0200
>> Cc: pipcet <at> protonmail.com, monnier <at> iro.umontreal.ca, 77924 <at> debbugs.gnu.org,
>> stefankangas <at> gmail.com
>>
>>
>> Sent from my iPhone
>>
>> > On 25. Apr 2025, at 09:45, Eli Zaretskii <eliz <at> gnu.org> wrote:
>> >
>> >
>> >>
>> >> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
>> >> Date: Fri, 25 Apr 2025 09:20:55 +0200
>> >> Cc: pipcet <at> protonmail.com, monnier <at> iro.umontreal.ca, 77924 <at> debbugs.gnu.org,
>> >> stefankangas <at> gmail.com
>> >>
>> >>
>> >>>> On 25. Apr 2025, at 09:01, Eli Zaretskii <eliz <at> gnu.org> wrote:
>> >>>
>> >>>
>> >>>>
>> >>>> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
>> >>>> Cc: Eli Zaretskii <eliz <at> gnu.org>, monnier <at> iro.umontreal.ca,
>> >>>> 77924 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>> >>>> Date: Thu, 24 Apr 2025 21:53:05 +0200
>> >>>>
>> >>>> Pip Cet <pipcet <at> protonmail.com> writes:
>> >>>>
>> >>>>> I think what we should do is mimic FOR_EACH_TAIL, and use
>> >>>>> FOR_EACH_MARKER like this:
>> >>>>>
>> >>>>> struct Lisp_Marker *m;
>> >>>>> FOR_EACH_MARKER (b, m)
>> >>>>> {
>> >>>>> /* do something with m */
>> >>>>> }
>> >>>>
>> >>>> We need an if somewhere for the MARKERP, don't we?
>> >>>
>> >>> Why is that needed, btw? Can't we change the representation and/or
>> >>> the functions involved to avoid the need for such a test?
>> >>
>> >> When markers are freed their entry in the market vector no longer contains a marker reference.
>> >
>> > I understand, but why does this need to be tested inside the loop?
>> > Can't the loop itself know how many markers are in the vector, and
>> > stop when they are exhausted?
>>
>> You are thinking of compacting the vector when a marker is freed?
>> Then we would lose the O(1) deletion. As it is the entry is simply
>> pushed on a free list.
>
> Actually, I thought about maintaining the number of markers in the
> vector, so there would be no need to detect that by the MARKERP test.
Not sure how that would work, but maybe you are thinking of the LRU list
in igc.
Let's say we have a marker vector like [__M___M___] where _ means free,
and M stands for an entry of a live marker.
In feature/igc, the M entries are kept in a doubly-linked LRU list.
Iteration can then be done following the that list. The order in that
list is last recently used first, and the whole thing was needed to work
with the cache marker heuristic in charpos<->bytepos conversion. That
visits slots of the vector in unpredictable order.
The heuristics and cache markers are no longer used in
scratch/text-index, so the LRU list is no longer needed (= 2 words per
entry = doubling the entry size), and I removed it.
Iteration is done by stepping over all _ and M in order (= fast), and
only giving out at the M entries. And as a small optimization for the
case of over-allocation, I remember the max index of an M that was ever
used.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 12:13:02 GMT)
Full text and
rfc822 format available.
Message #224 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Cc: pipcet <at> protonmail.com, monnier <at> iro.umontreal.ca,
> 77924 <at> debbugs.gnu.org, stefankangas <at> gmail.com
> Date: Fri, 25 Apr 2025 13:40:47 +0200
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > Actually, I thought about maintaining the number of markers in the
> > vector, so there would be no need to detect that by the MARKERP test.
>
> Not sure how that would work, but maybe you are thinking of the LRU list
> in igc.
We were talking about different aspects, sorry.
Regarding the issue with a marker vector where some slots are free, we
could simply compare with NULL. Or we could keep a pointer to the
next non-free slot. Not sure what should be cheaper, since iteration
over all the markers is quite frequent, whereas adding and removing
markers is probably less so.
> Iteration is done by stepping over all _ and M in order (= fast), and
> only giving out at the M entries. And as a small optimization for the
> case of over-allocation, I remember the max index of an M that was ever
> used.
Compacting the vector would also be possible, and I'm not sure it will
be too expensive, since memcpy is quite fast on modern machines.
All of this is not very important, except that we are talking about
simplifications of FOR_EACH_MARKER macro, so if any of this
facilitates a simpler macro without hampering performance, I'd suggest
to consider it seriously.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 12:23:02 GMT)
Full text and
rfc822 format available.
Message #227 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
>> Cc: pipcet <at> protonmail.com, monnier <at> iro.umontreal.ca,
>> 77924 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>> Date: Fri, 25 Apr 2025 13:40:47 +0200
>>
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>
>> > Actually, I thought about maintaining the number of markers in the
>> > vector, so there would be no need to detect that by the MARKERP test.
>>
>> Not sure how that would work, but maybe you are thinking of the LRU list
>> in igc.
>
> We were talking about different aspects, sorry.
>
> Regarding the issue with a marker vector where some slots are free, we
> could simply compare with NULL. Or we could keep a pointer to the
> next non-free slot. Not sure what should be cheaper, since iteration
> over all the markers is quite frequent, whereas adding and removing
> markers is probably less so.
>
>> Iteration is done by stepping over all _ and M in order (= fast), and
>> only giving out at the M entries. And as a small optimization for the
>> case of over-allocation, I remember the max index of an M that was ever
>> used.
>
> Compacting the vector would also be possible, and I'm not sure it will
> be too expensive, since memcpy is quite fast on modern machines.
>
> All of this is not very important, except that we are talking about
> simplifications of FOR_EACH_MARKER macro, so if any of this
> facilitates a simpler macro without hampering performance, I'd suggest
> to consider it seriously.
(Just to mention an important aspect to keep in mind:
Some iteration over markers in master are for adjusting marker
positions, and these positions are held in the marker objects
themselves. These loops touch memory memory "all over the place".
In scratch/text-index, the marker positions are kept in the marker
vector, and adjusting positions can be done by accessing only the vector
from left to right. I think that is very CPU-cache-friendly. I would
expect that to be *much* faster than in master.)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 14:09:01 GMT)
Full text and
rfc822 format available.
Message #230 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
>> BTW, I did a run of the complete benchmark suite (plus some extra
>> micro-benchmarks). This compares the base of the branch to the tip of
>> the text-index branch:
>>
>> | test | BASE (s) | gc (s) | TINDEX (s) | gc (s) | err |
>> |--------------------------+------------+--------+------------+--------+-----|
>> | bubble | 4.46 | 0.00 | 4.57 | 0.00 | 0% |
>> | bubble-no-cons | 15.04 | 0.00 | 14.71 | 0.00 | 0% |
>> | bytecomp | 3.67 | 0.00 | 3.72 | 0.00 | 0% |
>> | dhrystone | 10.30 | 0.00 | 10.13 | 0.00 | 0% |
>> | eieio | 4.06 | 0.00 | 4.10 | 0.00 | 0% |
>> | fibn | 3.22 | 0.00 | 3.34 | 0.00 | 0% |
>> | fibn-named-let | 4.11 | 0.00 | 4.08 | 0.00 | 0% |
>> | fibn-rec | 6.81 | 0.00 | 6.88 | 0.00 | 0% |
>> | fibn-tc | 5.34 | 0.00 | 5.12 | 0.00 | 0% |
>> | flet | 10.53 | 0.00 | 10.68 | 0.00 | 0% |
>> | font-lock | 1.30 | 0.00 | 1.29 | 0.00 | 0% |
>> | inclist | 24.65 | 0.00 | 16.21 | 0.00 | 0% |
>> | inclist-type-hints | 24.56 | 0.00 | 16.23 | 0.00 | 0% |
>> | listlen-tc | 5.19 | 0.00 | 5.16 | 0.00 | 0% |
>> | map-closure | 9.86 | 0.00 | 9.26 | 0.00 | 0% |
>> | markers-charbyte-1G/0*1 | 37.38 | 0.00 | 5.54 | 0.00 | 2% |
>> | markers-charbyte-1G/0*10 | 283.68 | 0.00 | 7.25 | 0.00 | 1% |
>> | markers-charbyte-1G/1*1 | 3.64 | 0.00 | 0.97 | 0.00 | 5% |
>> | markers-charbyte-1G/1*10 | 26.11 | 0.00 | 2.69 | 0.00 | 2% |
>> | markers-save-excursion-0 | 14.07 | 1.48 | 15.58 | 0.00 | 1% |
>> | markers-save-excursion-3 | 12.97 | 1.34 | 14.99 | 0.00 | 0% |
>> | nbody | 4.95 | 0.00 | 4.80 | 0.00 | 0% |
>> | pack-unpack | 0.85 | 0.00 | 0.84 | 0.00 | 0% |
>> | pack-unpack-old | 2.82 | 0.00 | 2.77 | 0.00 | 1% |
>> | pcase | 10.37 | 0.00 | 10.35 | 0.00 | 0% |
>> | pidigits | 10.43 | 0.00 | 9.09 | 0.00 | 2% |
>> | regexp-cubic-200 | 1.48 | 0.00 | 1.48 | 0.00 | 1% |
>> | regexp-cubic-400 | 2.31 | 0.00 | 2.32 | 0.00 | 1% |
>> | regexp-exponential | 3.68 | 0.00 | 3.69 | 0.00 | 0% |
>> | regexp-gnumsg | 0.92 | 0.00 | 0.92 | 0.00 | 0% |
>> | regexp-linear-100 | 1.07 | 0.00 | 1.08 | 0.00 | 0% |
>> | regexp-linear-1000 | 1.01 | 0.00 | 1.01 | 0.00 | 0% |
>> | regexp-linear-5000 | 1.01 | 0.00 | 1.01 | 0.00 | 0% |
>> | regexp-quadratic-1000 | 1.06 | 0.00 | 1.07 | 0.00 | 1% |
>> | regexp-quadratic-5000 | 2.65 | 0.00 | 2.66 | 0.00 | 1% |
>> | regexp-tuareg | 0.46 | 0.00 | 0.44 | 0.00 | 0% |
>> | scroll | 1.33 | 0.00 | 1.33 | 0.00 | 0% |
>> | scroll-nonascii | 2.68 | 0.00 | 2.60 | 0.00 | 0% |
>> | search | 29.42 | 0.00 | 29.55 | 0.00 | 0% |
>> | search/m50k | 29.02 | 0.00 | 29.55 | 0.00 | 0% |
>> | search/nolookup | 24.84 | 0.00 | 24.99 | 0.00 | 0% |
>> | search/p02 | 55.03 | 0.00 | 52.91 | 0.00 | 0% |
>> | search/p32 | 31.22 | 0.00 | 31.12 | 0.00 | 0% |
>> | smie | 2.98 | 0.00 | 3.03 | 0.00 | 0% |
>> | smie-nonascii | 5.82 | 0.00 | 15.96 | 0.00 | 0% |
>> |--------------------------+------------+--------+------------+--------+-----|
>>
>> Most of the benchmarks are mostly unaffected. Comments:
>>
>> - the `err` column shows the maximum of the std-dev for the BASE and the
>> TINDEX (both were run 10 times).
>> This is run on a fairly old server, because it's the machine with
>> least variability to which I have access (no DVFS effects, plenty of
>> idle cores and RAM not to be too affected by other processes, ...).
>>
>> - The `markers-charbyte-*` benchmarks are micro-benchmarks testing the
>> charpos->bytepos conversion by making many calls of `char-after` to
>> random positions in the buffer. The fact that we go *much* faster there
>> is just normal: this is the poor behavior the patch is intended to fix.
>> IOW, not going much faster would be a big disappointment.
>>
>> - The `markers-save-excursion-*` benchmarks are meant to test the
>> performance of a "cheap" save-excursion. Here, the BASE code is very
>> efficient and my previous "sorted-array-of-markers-with-gap" had trouble
>> staying competitive. Here we see that the new branch is a bit slower
>> but not by very much. Also note that it GCs less, presumably because
>> the marker objects are smaller (4words vs 6words) so GCs are a bit
>> less frequent.
>>
>> - `inclist` is significantly faster. I have no clue what's up with that.
>> This micro-benchmark presumably doesn't get anywhere near markers or
>> charpos or bytepos.
>> FWIW, I ran those benchmarks on a slightly different pair of builds (I
>> honestly can't say what's different) and there was no difference in
>> that other case. AFAICT 16.2s is the "right" answer and the 24.5s
>> shown here is a weird corner case that's better ignored.
>> Welcome to the wonderful world of micro-benchmarks!
>>
>> - `pidigits` is slightly faster, and I again have no clue why.
>> Probably some unrelated effect. In this case, my other pair of builds
>> showed approximately the same difference, so it seems to be "a bit
>> less of a fluke"? In any case, I'd ignore this one as well.
>>
>> - the `search*` benchmarks are designed to test the bytepos->charpos
>> conversions done during regexp matching because the lookups for the
>> `syntax-table` text properties. I wrote them back when we had
>> a serious performance problem there, but nowadays the `master` branch
>> handles this very well, so we see the branch is not able to get any
>> benefit: the bytepos to convert is basically always right next to the
>> last one, so we never need to consult anything like markers or the
>> text-index to compute the charpos.
>>
>> - `smie` is a test which re-indents `xmenu.c` (using the
>> SMIE-based `sm-c-mode` rather than CC-mode). Since `xmenu.c` is
>> an ASCII file, the text-index can't be very useful so it's no wonder
>> that we don't see any performance difference.
>>
>> - Finally `smie-nonascii` is the same as `smie` except that it uses
>> a file that's identical to `xmenu.c` but where all the letters of
>> comments/strings/identifiers were replaced by non-ASCII ones.
>> Here, we see that TINDEX` is *much* slower than BASE.
>> This doesn't seem to be a fluke: I see the exact same performance
>> difference in my other pair of builds.
>>
>> This last one is a serious problem that we need to address before we can
>> merge the branch.
>>
>> FWIW, I also ran those benchmark on the same machine with Debian's
>> Emacs-30.1 and the results are basically identical to those of
>> BASE above.
>
> I've got the new benchmarks from elpa.git, and ran the scroll and smie
> becnmarks on my otherwise idle Mac mini M1, GUI Emacs,
>
> Scroll:
>
> * master Results
>
> | test | non-gc (s) | gc (s) | gcs | total (s) | err |
> |-----------------+------------+--------+-----+-----------+-----|
> | scroll | 1.35 | 0.34 | 22 | 1.69 | 0% |
> | scroll-nonascii | 3.24 | 0.77 | 50 | 4.01 | 0% |
> |-----------------+------------+--------+-----+-----------+-----|
> | total | 4.59 | 1.11 | 72 | 5.70 | 0% |
>
> * text-index Results
>
> | test | non-gc (s) | gc (s) | gcs | total (s) | err |
> |-----------------+------------+--------+-----+-----------+-----|
> | scroll | 1.34 | 0.33 | 22 | 1.68 | 0% |
> | scroll-nonascii | 3.16 | 0.75 | 49 | 3.91 | 0% |
> |-----------------+------------+--------+-----+-----------+-----|
> | total | 4.51 | 1.08 | 71 | 5.59 | 0% |
>
> My summary, together with what I got with tamil.txt yesterday:
> indistinguishable, if not a tad better with text-index
>
> Smie:
>
> * master Results
>
> | test | non-gc (s) | gc (s) | gcs | total (s) | err |
> |---------------+------------+--------+-----+-----------+-----|
> | font-lock | 0.52 | 0.32 | 24 | 0.84 | 0% |
> | smie | 1.19 | 0.50 | 36 | 1.69 | 0% |
> | smie-nonascii | 2.13 | 0.53 | 39 | 2.66 | 0% |
> |---------------+------------+--------+-----+-----------+-----|
> | total | 3.83 | 1.35 | 99 | 5.19 | 0% |
>
> * text-index Results
>
> | test | non-gc (s) | gc (s) | gcs | total (s) | err |
> |---------------+------------+--------+-----+-----------+-----|
> | font-lock | 0.51 | 0.31 | 23 | 0.82 | 0% |
> | smie | 1.18 | 0.45 | 33 | 1.63 | 0% |
> | smie-nonascii | 5.49 | 0.48 | 35 | 5.98 | 0% |
> |---------------+------------+--------+-----+-----------+-----|
> | total | 7.19 | 1.24 | 92 | 8.43 | 0% |
>
> That shows also the difference in smie-nonascii, i.e. in a C file
> containing multi-byte characters (one should suffice to make
> Z != Z_BYTE, and the index is used).
I've been reading smie a bit, to get an impression what the smie
slowdown might mean in daily use. As far as I can see elb-smie is basically
5 times
(indent-region (point-min) (point-max)
-> indent-region-line-by-line for all lines
-> indent-region-according-to-mode
...
xmenu.c has ca. 2500 lines, so 12500 line-by-line indentations.
in 5.49s -> ca. 0.44ms per line. Or, taking 50ms as something a user might
perceive, 113 lines.
TBH, I feel somewhat under-motivated to try and optimize that.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 15:56:01 GMT)
Full text and
rfc822 format available.
Message #233 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, stefankangas <at> gmail.com,
> 77924 <at> debbugs.gnu.org
> Date: Fri, 25 Apr 2025 16:08:14 +0200
>
> > * master Results
> >
> > | test | non-gc (s) | gc (s) | gcs | total (s) | err |
> > |---------------+------------+--------+-----+-----------+-----|
> > | font-lock | 0.52 | 0.32 | 24 | 0.84 | 0% |
> > | smie | 1.19 | 0.50 | 36 | 1.69 | 0% |
> > | smie-nonascii | 2.13 | 0.53 | 39 | 2.66 | 0% |
> > |---------------+------------+--------+-----+-----------+-----|
> > | total | 3.83 | 1.35 | 99 | 5.19 | 0% |
> >
> > * text-index Results
> >
> > | test | non-gc (s) | gc (s) | gcs | total (s) | err |
> > |---------------+------------+--------+-----+-----------+-----|
> > | font-lock | 0.51 | 0.31 | 23 | 0.82 | 0% |
> > | smie | 1.18 | 0.45 | 33 | 1.63 | 0% |
> > | smie-nonascii | 5.49 | 0.48 | 35 | 5.98 | 0% |
> > |---------------+------------+--------+-----+-----------+-----|
> > | total | 7.19 | 1.24 | 92 | 8.43 | 0% |
> >
> > That shows also the difference in smie-nonascii, i.e. in a C file
> > containing multi-byte characters (one should suffice to make
> > Z != Z_BYTE, and the index is used).
>
> I've been reading smie a bit, to get an impression what the smie
> slowdown might mean in daily use. As far as I can see elb-smie is basically
> 5 times
>
> (indent-region (point-min) (point-max)
> -> indent-region-line-by-line for all lines
> -> indent-region-according-to-mode
> ...
>
> xmenu.c has ca. 2500 lines, so 12500 line-by-line indentations.
> in 5.49s -> ca. 0.44ms per line. Or, taking 50ms as something a user might
> perceive, 113 lines.
>
> TBH, I feel somewhat under-motivated to try and optimize that.
Do you understand why the branch is slower here? If so, can you
describe the reasons, so we could then think if thye same reasons
could slow down other cases?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 16:20:01 GMT)
Full text and
rfc822 format available.
Message #236 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
>> Cc: Eli Zaretskii <eliz <at> gnu.org>, stefankangas <at> gmail.com,
>> 77924 <at> debbugs.gnu.org
>> Date: Fri, 25 Apr 2025 16:08:14 +0200
>>
>> > * master Results
>> >
>> > | test | non-gc (s) | gc (s) | gcs | total (s) | err |
>> > |---------------+------------+--------+-----+-----------+-----|
>> > | font-lock | 0.52 | 0.32 | 24 | 0.84 | 0% |
>> > | smie | 1.19 | 0.50 | 36 | 1.69 | 0% |
>> > | smie-nonascii | 2.13 | 0.53 | 39 | 2.66 | 0% |
>> > |---------------+------------+--------+-----+-----------+-----|
>> > | total | 3.83 | 1.35 | 99 | 5.19 | 0% |
>> >
>> > * text-index Results
>> >
>> > | test | non-gc (s) | gc (s) | gcs | total (s) | err |
>> > |---------------+------------+--------+-----+-----------+-----|
>> > | font-lock | 0.51 | 0.31 | 23 | 0.82 | 0% |
>> > | smie | 1.18 | 0.45 | 33 | 1.63 | 0% |
>> > | smie-nonascii | 5.49 | 0.48 | 35 | 5.98 | 0% |
>> > |---------------+------------+--------+-----+-----------+-----|
>> > | total | 7.19 | 1.24 | 92 | 8.43 | 0% |
>> >
>> > That shows also the difference in smie-nonascii, i.e. in a C file
>> > containing multi-byte characters (one should suffice to make
>> > Z != Z_BYTE, and the index is used).
>>
>> I've been reading smie a bit, to get an impression what the smie
>> slowdown might mean in daily use. As far as I can see elb-smie is basically
>> 5 times
>>
>> (indent-region (point-min) (point-max)
>> -> indent-region-line-by-line for all lines
>> -> indent-region-according-to-mode
>> ...
>>
>> xmenu.c has ca. 2500 lines, so 12500 line-by-line indentations.
>> in 5.49s -> ca. 0.44ms per line. Or, taking 50ms as something a user might
>> perceive, 113 lines.
>>
>> TBH, I feel somewhat under-motivated to try and optimize that.
>
> Do you understand why the branch is slower here? If so, can you
> describe the reasons, so we could then think if thye same reasons
> could slow down other cases?
I haven't profiled anything .but I'd bet a small amount that it is what
I wrote in another mail:
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> - Finally `smie-nonascii` is the same as `smie` except that it uses
> a file that's identical to `xmenu.c` but where all the letters of
> comments/strings/identifiers were replaced by non-ASCII ones.
> Here, we see that TINDEX` is *much* slower than BASE.
> This doesn't seem to be a fluke: I see the exact same performance
> difference in my other pair of builds.
>
> This last one is a serious problem that we need to address before we can
> merge the branch.
ASCII means that the text index is basically not used.
In the non-ASCII case it is used. Re-indentation means text modification
which in turns leads to index invalidation and re-building the index as
needed. Index invalidation discards the index from the modification
position to the end of the text. Re-building goes from the last valid
position the index contains to the next position that is needed to do a
position conversion.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 16:35:01 GMT)
Full text and
rfc822 format available.
Message #239 received at 77924 <at> debbugs.gnu.org (full text, mbox):
>> * master Results
>>
>> | test | non-gc (s) | gc (s) | gcs | total (s) | err |
>> |---------------+------------+--------+-----+-----------+-----|
>> | font-lock | 0.52 | 0.32 | 24 | 0.84 | 0% |
>> | smie | 1.19 | 0.50 | 36 | 1.69 | 0% |
>> | smie-nonascii | 2.13 | 0.53 | 39 | 2.66 | 0% |
>> |---------------+------------+--------+-----+-----------+-----|
>> | total | 3.83 | 1.35 | 99 | 5.19 | 0% |
>>
>> * text-index Results
>>
>> | test | non-gc (s) | gc (s) | gcs | total (s) | err |
>> |---------------+------------+--------+-----+-----------+-----|
>> | font-lock | 0.51 | 0.31 | 23 | 0.82 | 0% |
>> | smie | 1.18 | 0.45 | 33 | 1.63 | 0% |
>> | smie-nonascii | 5.49 | 0.48 | 35 | 5.98 | 0% |
>> |---------------+------------+--------+-----+-----------+-----|
>> | total | 7.19 | 1.24 | 92 | 8.43 | 0% |
[ I'm surprised that I get 0 GCs in my tests whereas you get about 30.
My crystal ball suggests it's because I ran them in batch whereas you
ran them in a running Emacs, but even so: the difference is between
a `gc-cons-percentage` of 1.0 vs 0.1 so it would explain a factor of
10 difference, but you have much more than 10 runs of GC. ]
>> That shows also the difference in smie-nonascii, i.e. in a C file
>> containing multi-byte characters (one should suffice to make
>> Z != Z_BYTE, and the index is used).
>
> I've been reading smie a bit, to get an impression what the smie
> slowdown might mean in daily use. As far as I can see elb-smie is basically
> 5 times
>
> (indent-region (point-min) (point-max)
> -> indent-region-line-by-line for all lines
> -> indent-region-according-to-mode
> ...
>
> xmenu.c has ca. 2500 lines, so 12500 line-by-line indentations.
> in 5.49s -> ca. 0.44ms per line. Or, taking 50ms as something a user might
> perceive, 113 lines.
As you know, that is not really the question. The question is rather:
how/why does the new code slow down this benchmark by more than 2x even
though this benchmark is not expected to spend its time in marker
manipulation and charpos<->bytepos conversion.
I hope something like `gprof` might give useful hints.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 16:46:04 GMT)
Full text and
rfc822 format available.
Message #242 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>> * master Results
>>>
>>> | test | non-gc (s) | gc (s) | gcs | total (s) | err |
>>> |---------------+------------+--------+-----+-----------+-----|
>>> | font-lock | 0.52 | 0.32 | 24 | 0.84 | 0% |
>>> | smie | 1.19 | 0.50 | 36 | 1.69 | 0% |
>>> | smie-nonascii | 2.13 | 0.53 | 39 | 2.66 | 0% |
>>> |---------------+------------+--------+-----+-----------+-----|
>>> | total | 3.83 | 1.35 | 99 | 5.19 | 0% |
>>>
>>> * text-index Results
>>>
>>> | test | non-gc (s) | gc (s) | gcs | total (s) | err |
>>> |---------------+------------+--------+-----+-----------+-----|
>>> | font-lock | 0.51 | 0.31 | 23 | 0.82 | 0% |
>>> | smie | 1.18 | 0.45 | 33 | 1.63 | 0% |
>>> | smie-nonascii | 5.49 | 0.48 | 35 | 5.98 | 0% |
>>> |---------------+------------+--------+-----+-----------+-----|
>>> | total | 7.19 | 1.24 | 92 | 8.43 | 0% |
>
> [ I'm surprised that I get 0 GCs in my tests whereas you get about 30.
> My crystal ball suggests it's because I ran them in batch whereas you
> ran them in a running Emacs, but even so: the difference is between
> a `gc-cons-percentage` of 1.0 vs 0.1 so it would explain a factor of
> 10 difference, but you have much more than 10 runs of GC. ]
Maybe it's redisplay. I'm not sure if --batch does a redisplay at all.
If it does that would be on the initial frame and probably a terminal
redisplay. But that's just a guess.
>>> That shows also the difference in smie-nonascii, i.e. in a C file
>>> containing multi-byte characters (one should suffice to make
>>> Z != Z_BYTE, and the index is used).
>>
>> I've been reading smie a bit, to get an impression what the smie
>> slowdown might mean in daily use. As far as I can see elb-smie is basically
>> 5 times
>>
>> (indent-region (point-min) (point-max)
>> -> indent-region-line-by-line for all lines
>> -> indent-region-according-to-mode
>> ...
>>
>> xmenu.c has ca. 2500 lines, so 12500 line-by-line indentations.
>> in 5.49s -> ca. 0.44ms per line. Or, taking 50ms as something a user might
>> perceive, 113 lines.
>
> As you know, that is not really the question. The question is rather:
> how/why does the new code slow down this benchmark by more than 2x even
> though this benchmark is not expected to spend its time in marker
> manipulation and charpos<->bytepos conversion.
>
> I hope something like `gprof` might give useful hints.
See my other reply talking about building the index. I think the fast
one doesn't build_index and the slower one does. Also just gut feeling.
Profiling on macOS is a big PITA, BTW :-)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 17:32:01 GMT)
Full text and
rfc822 format available.
Message #245 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> In the non-ASCII case it is used. Re-indentation means text modification
> which in turns leads to index invalidation and re-building the index as
> needed. Index invalidation discards the index from the modification
> position to the end of the text. Re-building goes from the last valid
> position the index contains to the next position that is needed to do a
> position conversion.
I don't see how that would explain such a large slowdown: re-indenting
the whole buffer means reindenting each line, one by one starting from
the first, where indentation of a line modifies only that line and
usually looks only at that line and preceding text.
So it should cause an O(1) rebuilding of the index: the index will be
flushed when we start indenting the first line, and after that it
will get progressively re-filled and there shouldn't be much more
flushing going on during that time because we rarely look much beyond
the point where we last flushed (and hence beyond the point where we
will next flush).
Rebuilding the index once should take a very small amount of time
compared to performing the indentation.
Also, such modifications patterns are not specific to reindentation, so...
"think if the same reasons could slow down other cases?"
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 17:50:03 GMT)
Full text and
rfc822 format available.
Message #248 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> In the non-ASCII case it is used. Re-indentation means text modification
>> which in turns leads to index invalidation and re-building the index as
>> needed. Index invalidation discards the index from the modification
>> position to the end of the text. Re-building goes from the last valid
>> position the index contains to the next position that is needed to do a
>> position conversion.
>
> I don't see how that would explain such a large slowdown: re-indenting
> the whole buffer means reindenting each line, one by one starting from
> the first, where indentation of a line modifies only that line and
> usually looks only at that line and preceding text.
>
> So it should cause an O(1) rebuilding of the index: the index will be
> flushed when we start indenting the first line, and after that it
> will get progressively re-filled and there shouldn't be much more
> flushing going on during that time because we rarely look much beyond
> the point where we last flushed (and hence beyond the point where we
> will next flush).
>
> Rebuilding the index once should take a very small amount of time
> compared to performing the indentation.
>
> Also, such modifications patterns are not specific to reindentation, so...
> "think if the same reasons could slow down other cases?"
IÄm thinking of something like this:
Assume we initially are position P, and the next index
entry > P is I0 and so on.
-----P---------I0-----------I1.....
After insertion at P, the index is invalidated
-----P------------------------.....
Someone asks for a position P2 < I0, index is built up to
I0. If P2 > I0 then more.
-----P--P2-----I0-------------.....
We insert at P2, index is invalidated
-----P--P2--------------------.....
and so on. Maybe a save-excursion?
Hm, and the effect of TEXT_INDEX_INTERVAL might be interesting, if it's
somethings like that.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 17:58:02 GMT)
Full text and
rfc822 format available.
Message #251 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
>>> In the non-ASCII case it is used. Re-indentation means text modification
>>> which in turns leads to index invalidation and re-building the index as
>>> needed. Index invalidation discards the index from the modification
>>> position to the end of the text. Re-building goes from the last valid
>>> position the index contains to the next position that is needed to do a
>>> position conversion.
>>
>> I don't see how that would explain such a large slowdown: re-indenting
>> the whole buffer means reindenting each line, one by one starting from
>> the first, where indentation of a line modifies only that line and
>> usually looks only at that line and preceding text.
>>
>> So it should cause an O(1) rebuilding of the index: the index will be
>> flushed when we start indenting the first line, and after that it
>> will get progressively re-filled and there shouldn't be much more
>> flushing going on during that time because we rarely look much beyond
>> the point where we last flushed (and hence beyond the point where we
>> will next flush).
>>
>> Rebuilding the index once should take a very small amount of time
>> compared to performing the indentation.
>>
>> Also, such modifications patterns are not specific to reindentation, so...
>> "think if the same reasons could slow down other cases?"
>
> IÄm thinking of something like this:
>
> Assume we initially are position P, and the next index
> entry > P is I0 and so on.
>
> -----P---------I0-----------I1.....
>
> After insertion at P, the index is invalidated
>
> -----P------------------------.....
>
> Someone asks for a position P2 < I0, index is built up to
> I0. If P2 > I0 then more.
>
> -----P--P2-----I0-------------.....
>
> We insert at P2, index is invalidated
>
> -----P--P2--------------------.....
>
> and so on. Maybe a save-excursion?
>
> Hm, and the effect of TEXT_INDEX_INTERVAL might be interesting, if it's
> somethings like that.
Not well explained. What I simply mean is that we again and again
might be invalidating the index and need it right after, throw it away,
need it again.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 18:22:02 GMT)
Full text and
rfc822 format available.
Message #254 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>
>> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>
>>>> In the non-ASCII case it is used. Re-indentation means text modification
>>>> which in turns leads to index invalidation and re-building the index as
>>>> needed. Index invalidation discards the index from the modification
>>>> position to the end of the text. Re-building goes from the last valid
>>>> position the index contains to the next position that is needed to do a
>>>> position conversion.
>>>
>>> I don't see how that would explain such a large slowdown: re-indenting
>>> the whole buffer means reindenting each line, one by one starting from
>>> the first, where indentation of a line modifies only that line and
>>> usually looks only at that line and preceding text.
>>>
>>> So it should cause an O(1) rebuilding of the index: the index will be
>>> flushed when we start indenting the first line, and after that it
>>> will get progressively re-filled and there shouldn't be much more
>>> flushing going on during that time because we rarely look much beyond
>>> the point where we last flushed (and hence beyond the point where we
>>> will next flush).
>>>
>>> Rebuilding the index once should take a very small amount of time
>>> compared to performing the indentation.
>>>
>>> Also, such modifications patterns are not specific to reindentation, so...
>>> "think if the same reasons could slow down other cases?"
>>
>> IÄm thinking of something like this:
>>
>> Assume we initially are position P, and the next index
>> entry > P is I0 and so on.
>>
>> -----P---------I0-----------I1.....
>>
>> After insertion at P, the index is invalidated
>>
>> -----P------------------------.....
>>
>> Someone asks for a position P2 < I0, index is built up to
>> I0. If P2 > I0 then more.
>>
>> -----P--P2-----I0-------------.....
>>
>> We insert at P2, index is invalidated
>>
>> -----P--P2--------------------.....
>>
>> and so on. Maybe a save-excursion?
>>
>> Hm, and the effect of TEXT_INDEX_INTERVAL might be interesting, if it's
>> somethings like that.
>
> Not well explained. What I simply mean is that we again and again
> might be invalidating the index and need it right after, throw it away,
> need it again.
With interval 4096:
>>> * text-index Results
>>>
>>> | test | non-gc (s) | gc (s) | gcs | total (s) | err |
>>> |---------------+------------+--------+-----+-----------+-----|
>>> | font-lock | 0.51 | 0.31 | 23 | 0.82 | 0% |
>>> | smie | 1.18 | 0.45 | 33 | 1.63 | 0% |
>>> | smie-nonascii | 5.49 | 0.48 | 35 | 5.98 | 0% |
>>> |---------------+------------+--------+-----+-----------+-----|
>>> | total | 7.19 | 1.24 | 92 | 8.43 | 0% |
With interval 512
* Results
| test | non-gc (s) | gc (s) | gcs | total (s) | err |
|---------------+------------+--------+-----+-----------+-----|
| font-lock | 0.51 | 0.32 | 23 | 0.83 | 0% |
| smie | 1.18 | 0.47 | 33 | 1.65 | 0% |
| smie-nonascii | 5.48 | 0.50 | 35 | 5.98 | 0% |
|---------------+------------+--------+-----+-----------+-----|
| total | 7.18 | 1.29 | 92 | 8.47 | 0% |
Kind of interesting, and a bit surprising for me. So maybe my hypothesis
is wrong. Damn benchmarks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 18:36:01 GMT)
Full text and
rfc822 format available.
Message #257 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Cc: monnier <at> iro.umontreal.ca, stefankangas <at> gmail.com, 77924 <at> debbugs.gnu.org
> Date: Fri, 25 Apr 2025 18:19:12 +0200
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> >> Cc: Eli Zaretskii <eliz <at> gnu.org>, stefankangas <at> gmail.com,
> >> 77924 <at> debbugs.gnu.org
> >> Date: Fri, 25 Apr 2025 16:08:14 +0200
> >>
> >> > * master Results
> >> >
> >> > | test | non-gc (s) | gc (s) | gcs | total (s) | err |
> >> > |---------------+------------+--------+-----+-----------+-----|
> >> > | font-lock | 0.52 | 0.32 | 24 | 0.84 | 0% |
> >> > | smie | 1.19 | 0.50 | 36 | 1.69 | 0% |
> >> > | smie-nonascii | 2.13 | 0.53 | 39 | 2.66 | 0% |
> >> > |---------------+------------+--------+-----+-----------+-----|
> >> > | total | 3.83 | 1.35 | 99 | 5.19 | 0% |
> >> >
> >> > * text-index Results
> >> >
> >> > | test | non-gc (s) | gc (s) | gcs | total (s) | err |
> >> > |---------------+------------+--------+-----+-----------+-----|
> >> > | font-lock | 0.51 | 0.31 | 23 | 0.82 | 0% |
> >> > | smie | 1.18 | 0.45 | 33 | 1.63 | 0% |
> >> > | smie-nonascii | 5.49 | 0.48 | 35 | 5.98 | 0% |
> >> > |---------------+------------+--------+-----+-----------+-----|
> >> > | total | 7.19 | 1.24 | 92 | 8.43 | 0% |
> >> >
> >> > That shows also the difference in smie-nonascii, i.e. in a C file
> >> > containing multi-byte characters (one should suffice to make
> >> > Z != Z_BYTE, and the index is used).
> >>
> >> I've been reading smie a bit, to get an impression what the smie
> >> slowdown might mean in daily use. As far as I can see elb-smie is basically
> >> 5 times
> >>
> >> (indent-region (point-min) (point-max)
> >> -> indent-region-line-by-line for all lines
> >> -> indent-region-according-to-mode
> >> ...
> >>
> >> xmenu.c has ca. 2500 lines, so 12500 line-by-line indentations.
> >> in 5.49s -> ca. 0.44ms per line. Or, taking 50ms as something a user might
> >> perceive, 113 lines.
> >>
> >> TBH, I feel somewhat under-motivated to try and optimize that.
> >
> > Do you understand why the branch is slower here? If so, can you
> > describe the reasons, so we could then think if thye same reasons
> > could slow down other cases?
>
> I haven't profiled anything .but I'd bet a small amount that it is what
> I wrote in another mail:
>
> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
> > - Finally `smie-nonascii` is the same as `smie` except that it uses
> > a file that's identical to `xmenu.c` but where all the letters of
> > comments/strings/identifiers were replaced by non-ASCII ones.
> > Here, we see that TINDEX` is *much* slower than BASE.
> > This doesn't seem to be a fluke: I see the exact same performance
> > difference in my other pair of builds.
> >
> > This last one is a serious problem that we need to address before we can
> > merge the branch.
>
> ASCII means that the text index is basically not used.
>
> In the non-ASCII case it is used. Re-indentation means text modification
> which in turns leads to index invalidation and re-building the index as
> needed. Index invalidation discards the index from the modification
> position to the end of the text. Re-building goes from the last valid
> position the index contains to the next position that is needed to do a
> position conversion.
If code that causes invalidation and rebuilding of the index makes the
branch much slower, we should perhaps consider how to avoid such
slowdown. Because there will be more cases which cause something
similar.
Is it possible to invalidate only part(s) of the index? Surely, at
least some parts can remain intact?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 18:59:01 GMT)
Full text and
rfc822 format available.
Message #260 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> Is it possible to invalidate only part(s) of the index? Surely, at
> least some parts can remain intact?
The part before of changes remains unchanged. The part after could maybe
be reused, but I don't have developed an algorithm. And would like to
avoid that because it certainly will be fiddly.
But like I said, I'm not yet convinced the complexity is necessary.
Or maybe it's something else, and we are looking at this the wrong way.
I mean, in the Z == Z_BYTE case, all that is done in the conversions is
to immediately
if (Z == ZBYTE)
return pos;
while _everything_ else is done only in the multi-byte case. Maybe it's
the ASCII case that is the outlier.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Fri, 25 Apr 2025 19:03:01 GMT)
Full text and
rfc822 format available.
Message #263 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> Assume we initially are position P, and the next index
> entry > P is I0 and so on.
>
> -----P---------I0-----------I1.....
>
> After insertion at P, the index is invalidated
>
> -----P------------------------.....
>
> Someone asks for a position P2 < I0, index is built up to
> I0. If P2 > I0 then more.
Yes, but in the worst case we're still talking about
invalidating+recomputing one interval per line. So my O(1) would be
around 4096B / ~50B/line so less than 100.
I think refilling the whole index 100 times should still take
significantly less time than the rest of the indentation code.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sat, 26 Apr 2025 05:32:01 GMT)
Full text and
rfc822 format available.
Message #266 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> - Finally `smie-nonascii` is the same as `smie` except that it uses
> a file that's identical to `xmenu.c` but where all the letters of
> comments/strings/identifiers were replaced by non-ASCII ones.
> Here, we see that TINDEX` is *much* slower than BASE.
> This doesn't seem to be a fluke: I see the exact same performance
> difference in my other pair of builds.
>
> This last one is a serious problem that we need to address before we can
> merge the branch.
I have pushed a patch which fixes this performance bug.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sat, 26 Apr 2025 05:47:02 GMT)
Full text and
rfc822 format available.
Message #269 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> - Finally `smie-nonascii` is the same as `smie` except that it uses
>> a file that's identical to `xmenu.c` but where all the letters of
>> comments/strings/identifiers were replaced by non-ASCII ones.
>> Here, we see that TINDEX` is *much* slower than BASE.
>> This doesn't seem to be a fluke: I see the exact same performance
>> difference in my other pair of builds.
>>
>> This last one is a serious problem that we need to address before we can
>> merge the branch.
>
> I have pushed a patch which fixes this performance bug.
* Results
| test | non-gc (s) | gc (s) | gcs | total (s) | err |
|---------------+------------+--------+-----+-----------+-----|
| font-lock | 0.52 | 0.32 | 23 | 0.83 | 0% |
| smie | 1.18 | 0.46 | 33 | 1.64 | 0% |
| smie-nonascii | 2.10 | 0.49 | 35 | 2.59 | 0% |
|---------------+------------+--------+-----+-----------+-----|
| total | 3.79 | 1.27 | 92 | 5.07 | 0% |
Very nice! Can you tell the story a bit? For entertainment? :-).
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sat, 26 Apr 2025 06:55:02 GMT)
Full text and
rfc822 format available.
Message #272 received at 77924 <at> debbugs.gnu.org (full text, mbox):
[வெள்ளி ஏப்ரல் 25, 2025] Eli Zaretskii wrote:
>> From: Visuwesh <visuweshm <at> gmail.com>
>> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, Eli Zaretskii
>> <eliz <at> gnu.org>, yantar92 <at> posteo.net, stefankangas <at> gmail.com,
>> 77924 <at> debbugs.gnu.org
>> Date: Thu, 24 Apr 2025 23:55:05 +0530
>>
>> [வியாழன் ஏப்ரல் 24, 2025] Gerd Möllmann wrote:
>>
>> >> So the benchmark tests scroll time, including jit/font-lock time.
>> >> It uses its own copy of a major mode, so that you can compare "scroll +
>> >> font-lock" performance between different Emacs releases without being
>> >> affected by improvements/regressions in CC-mode itself.
>> >
>> > So maybe it would make sense to run both that benchmark as-is plus one
>> > without smie but with tamil.txt, I guess. Or in other words, now only
>> > the as-is benchmark, because the tamil.txt results I've already posted.
>>
>> Before including tamil.txt, I would like to make clear that the file
>> contents is volume 1 of a (popular) novel which was released in the 50s.
>> I got the text itself from Project Madurai which states that [1]:
>>
>> Acknowledgement:
>> Our Sincere thanks go to the following persons for their assistance in the preparation of this work.
>> Etext preparation : AU-KBC Research Center (Mr. Baskaran), Anna University, Chennai,India
>> Proof-reading: Mr. S. Anbumani, Mr. N.D. Logasundaram,Mr. Narayanan Govindarajan,
>> Ms. Pavithra Srinivasan, Mr. Ramachandran Mahadevan, Ms. Sathya, Mr. Sreeram Krishnamoorthy,
>> Dr. Sridhar Rathinam, Mrs. Srilatha Rajagopal, Mr. VinothJagannathan
>> Web version: Mr. S. Anbumani, Blacksburg, Virginia, USA
>>
>> This webpage presents the Etxt in Tamil script in Unicode encoding.
>> This file was last revised on Apr. 12, 2003
>>
>> © Project Madurai, 1998-2021.
>> Project Madurai is an open, voluntary, worldwide initiative devoted to preparation
>> of electronic texts of tamil literary works and to distribute them free on the Internet.
>> Details of Project Madurai are available at the website
>> https://www.projectmadurai.org/
>> You are welcome to freely distribute this file, provided this header page is kept intact.
>>
>> So I assume there should no problem in distributing it in the repo. If
>> there could be, we could try composing all the text from any work in
>> freetamilebooks.com which publishes under the CC BY-NC-SA license.
>>
>> 1. https://www.projectmadurai.org/pm_etexts/utf8/pmuni0169_00.html
>
> First, we don't have to include the file in our benchmarks. We could
> instead run the benchmarks on it and publish the results. It isn't
> like the implementation of charpos<->bytepos conversions changes too
> often.
That makes sense to me.
> Second, after looking at the site, I cannot figure out the copyright
> issues. The only place where this is addressed is the following
> question in their FAQ:
>
> 3.3 How about copyright issues ?
>
> Project Madurai strictly adheres to copyright standards. All the
> works in the collection are either from public-domain (after a certain
> period of their creation, currently ca. 75 years after the lifespan of
> the author) or with due consent from the respective authors.
>
> I understand the PD part, but not the other part. What does it mean?
AFAIU, it says that some works in the website are published after
getting permission from the author of the text. For example, refer to
the notice in https://projectmadurai.org/pm_etexts/utf8/pmuni0148.html.
The author of the work himself submitted the text to Project Madurai,
and agreed to distribute it.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sat, 26 Apr 2025 07:31:02 GMT)
Full text and
rfc822 format available.
Message #275 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> From: Visuwesh <visuweshm <at> gmail.com>
> Cc: gerd.moellmann <at> gmail.com, monnier <at> iro.umontreal.ca,
> yantar92 <at> posteo.net, stefankangas <at> gmail.com, 77924 <at> debbugs.gnu.org
> Date: Sat, 26 Apr 2025 12:24:02 +0530
>
> [வெள்ளி ஏப்ரல் 25, 2025] Eli Zaretskii wrote:
>
> > Second, after looking at the site, I cannot figure out the copyright
> > issues. The only place where this is addressed is the following
> > question in their FAQ:
> >
> > 3.3 How about copyright issues ?
> >
> > Project Madurai strictly adheres to copyright standards. All the
> > works in the collection are either from public-domain (after a certain
> > period of their creation, currently ca. 75 years after the lifespan of
> > the author) or with due consent from the respective authors.
> >
> > I understand the PD part, but not the other part. What does it mean?
>
> AFAIU, it says that some works in the website are published after
> getting permission from the author of the text. For example, refer to
> the notice in https://projectmadurai.org/pm_etexts/utf8/pmuni0148.html.
> The author of the work himself submitted the text to Project Madurai,
> and agreed to distribute it.
But that tells us nothing about the original permissions of the files
for which their authors gave distribution permissions to Project
Madurai. Thus, whether we can use these files is still unclear.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sat, 26 Apr 2025 16:24:01 GMT)
Full text and
rfc822 format available.
Message #278 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> * Results
>
> | test | non-gc (s) | gc (s) | gcs | total (s) | err |
> |---------------+------------+--------+-----+-----------+-----|
> | font-lock | 0.52 | 0.32 | 23 | 0.83 | 0% |
> | smie | 1.18 | 0.46 | 33 | 1.64 | 0% |
> | smie-nonascii | 2.10 | 0.49 | 35 | 2.59 | 0% |
> |---------------+------------+--------+-----+-----------+-----|
> | total | 3.79 | 1.27 | 92 | 5.07 | 0% |
>
> Very nice! Can you tell the story a bit? For entertainment? :-).
I didn't have time to get to the bottom of it, but here's what happened:
- rebuilt with `--enable-profiling`.
- ran the test then gprof.
- told me >50% of the time was spent in `buf_charpos_to_bytepos`.
- tweaked the source code to prevent inlining of the other functions
used in that code, to get more info from the profiler.
- the new profile showed that a lot more time was spent in
`bytepos_backward_to_charpos` than in `bytepos_forward_to_charpos`.
- I looked at the possible source of asymmetry and figured it might
be because of that corner case where we (needlessly) scan
backward from `next` when charpos is right at `prev`.
- tried the patch which confirmed my suspicion.
- called it a day.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sat, 26 Apr 2025 16:35:03 GMT)
Full text and
rfc822 format available.
Message #281 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> * Results
>>
>> | test | non-gc (s) | gc (s) | gcs | total (s) | err |
>> |---------------+------------+--------+-----+-----------+-----|
>> | font-lock | 0.52 | 0.32 | 23 | 0.83 | 0% |
>> | smie | 1.18 | 0.46 | 33 | 1.64 | 0% |
>> | smie-nonascii | 2.10 | 0.49 | 35 | 2.59 | 0% |
>> |---------------+------------+--------+-----+-----------+-----|
>> | total | 3.79 | 1.27 | 92 | 5.07 | 0% |
>>
>> Very nice! Can you tell the story a bit? For entertainment? :-).
>
> I didn't have time to get to the bottom of it, but here's what happened:
>
> - rebuilt with `--enable-profiling`.
> - ran the test then gprof.
> - told me >50% of the time was spent in `buf_charpos_to_bytepos`.
> - tweaked the source code to prevent inlining of the other functions
> used in that code, to get more info from the profiler.
> - the new profile showed that a lot more time was spent in
> `bytepos_backward_to_charpos` than in `bytepos_forward_to_charpos`.
> - I looked at the possible source of asymmetry and figured it might
> be because of that corner case where we (needlessly) scan
> backward from `next` when charpos is right at `prev`.
> - tried the patch which confirmed my suspicion.
> - called it a day.
Thanks!
And ouch, I hope that thing I tried there doesn't count as a premature
optimization (looks like one). That would betoo embarrassing :-).
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sat, 26 Apr 2025 19:26:01 GMT)
Full text and
rfc822 format available.
Message #284 received at 77924 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
>>> * Results
>>>
>>> | test | non-gc (s) | gc (s) | gcs | total (s) | err |
>>> |---------------+------------+--------+-----+-----------+-----|
>>> | font-lock | 0.52 | 0.32 | 23 | 0.83 | 0% |
>>> | smie | 1.18 | 0.46 | 33 | 1.64 | 0% |
>>> | smie-nonascii | 2.10 | 0.49 | 35 | 2.59 | 0% |
>>> |---------------+------------+--------+-----+-----------+-----|
>>> | total | 3.79 | 1.27 | 92 | 5.07 | 0% |
>>>
>>> Very nice! Can you tell the story a bit? For entertainment? :-).
>>
>> I didn't have time to get to the bottom of it, but here's what happened:
>>
>> - rebuilt with `--enable-profiling`.
>> - ran the test then gprof.
>> - told me >50% of the time was spent in `buf_charpos_to_bytepos`.
>> - tweaked the source code to prevent inlining of the other functions
>> used in that code, to get more info from the profiler.
>> - the new profile showed that a lot more time was spent in
>> `bytepos_backward_to_charpos` than in `bytepos_forward_to_charpos`.
>> - I looked at the possible source of asymmetry and figured it might
>> be because of that corner case where we (needlessly) scan
>> backward from `next` when charpos is right at `prev`.
>> - tried the patch which confirmed my suspicion.
>> - called it a day.
>
> Thanks!
>
> And ouch, I hope that thing I tried there doesn't count as a premature
> optimization (looks like one). That would betoo embarrassing :-).
To fix this, I would propose
[0001-Don-t-optimize-prematurely-in-text_index_charpos_to_.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sat, 26 Apr 2025 23:21:02 GMT)
Full text and
rfc822 format available.
Message #287 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> - /* Don't scan forward if CHARPOS is exactly on the previous known
> - position because the index bytepos can be in the middle of a
> - character, which is found by scanning backwards. */
> - ptrdiff_t bytepos
> - = (charpos == prev.charpos ? bytepos_of_head (b, prev.bytepos)
> - : (charpos - prev.charpos < next.charpos - charpos
> - ? bytepos_forward_to_charpos (b, prev, charpos)
> - : bytepos_backward_to_charpos (b, next, charpos)));
> + /* Scan forward if the distance to the previous known position is
> + smaller than the distance to the next known position. */
> + const ptrdiff_t bytepos
> + = (charpos - prev.charpos < next.charpos - charpos
> + ? bytepos_forward_to_charpos (b, prev, charpos)
> + : bytepos_backward_to_charpos (b, next, charpos));
I don't understand: `prev` can point into the middle of a char, so if
`charpos == prev.charpos`, `bytepos_forward_to_charpos` may return an
incorrect `bytepos` (it'll return `prev.bytepos` which can be in the
middle of that char).
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sun, 27 Apr 2025 02:32:02 GMT)
Full text and
rfc822 format available.
Message #290 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> - /* Don't scan forward if CHARPOS is exactly on the previous known
>> - position because the index bytepos can be in the middle of a
>> - character, which is found by scanning backwards. */
>> - ptrdiff_t bytepos
>> - = (charpos == prev.charpos ? bytepos_of_head (b, prev.bytepos)
>> - : (charpos - prev.charpos < next.charpos - charpos
>> - ? bytepos_forward_to_charpos (b, prev, charpos)
>> - : bytepos_backward_to_charpos (b, next, charpos)));
>> + /* Scan forward if the distance to the previous known position is
>> + smaller than the distance to the next known position. */
>> + const ptrdiff_t bytepos
>> + = (charpos - prev.charpos < next.charpos - charpos
>> + ? bytepos_forward_to_charpos (b, prev, charpos)
>> + : bytepos_backward_to_charpos (b, next, charpos));
>
> I don't understand: `prev` can point into the middle of a char, so if
> `charpos == prev.charpos`, `bytepos_forward_to_charpos` may return an
> incorrect `bytepos` (it'll return `prev.bytepos` which can be in the
> middle of that char).
>
>
> Stefan
Oh, shit, I forgot that I didn't make the forward/backward case
symmetrical. Add this to the mix.
1 file changed, 1 insertion(+), 1 deletion(-)
src/text-index.c | 2 +-
modified src/text-index.c
@@ -457,7 +457,7 @@ bytepos_forward_to_charpos (struct buffer *b, const struct text_pos from,
ptrdiff_t to_charpos)
{
eassert (from.charpos < to_charpos);
- ptrdiff_t bytepos = from.bytepos;
+ ptrdiff_t bytepos = char_start_bytepos (b, from.bytepos);
ptrdiff_t charpos = from.charpos;
while (charpos < to_charpos)
{
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sun, 27 Apr 2025 02:42:01 GMT)
Full text and
rfc822 format available.
Message #293 received at 77924 <at> debbugs.gnu.org (full text, mbox):
> Oh, shit, I forgot that I didn't make the forward/backward case
> symmetrical. Add this to the mix.
>
> 1 file changed, 1 insertion(+), 1 deletion(-)
> src/text-index.c | 2 +-
>
> modified src/text-index.c
> @@ -457,7 +457,7 @@ bytepos_forward_to_charpos (struct buffer *b, const struct text_pos from,
> ptrdiff_t to_charpos)
> {
> eassert (from.charpos < to_charpos);
> - ptrdiff_t bytepos = from.bytepos;
> + ptrdiff_t bytepos = char_start_bytepos (b, from.bytepos);
> ptrdiff_t charpos = from.charpos;
> while (charpos < to_charpos)
> {
Makes more sense. LGTM,
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sun, 27 Apr 2025 02:54:02 GMT)
Full text and
rfc822 format available.
Message #296 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> Makes more sense. LGTM,
Thanks, pushed.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#77924; Package
emacs.
(Sun, 27 Apr 2025 18:25:01 GMT)
Full text and
rfc822 format available.
Message #299 received at 77924 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> Could you try out the performance of the `scratch/text-index` branch on
> your famous Org test case(s) that has historically suffered from poor
> markers performance?
> [ And maybe report the result to bug#77924? ]
I did re-search-forward benchmark on a large multi-language Org file.
;; master
;; (23.0212253 0 0.0)
;; (22.53031908 0 0.0)
;; scratch/text-index branch
;; (22.993521733999998 0 0.0)
;; (25.580392903 0 0.0)
Overall, I see little difference in performance.
The benchmark:
(setq yant/re "\\(?:\\(?:\\<DEADLINE: *\\(\\(?:<\\(?:[[:digit:]]\\{4\\}-[[:digit:]]\\{2\\}-[[:digit:]]\\{2\\}\\(?: [[:alpha:]]+\\)?\\)\\(?: [[:digit:]]\\{1,2\\}:[[:digit:]]\\{2\\}\\(?:-[[:digit:]]\\{1,2\\}:[[:digit:]]\\{2\\}\\)?\\)?\\(?:\\(?: [+.:-]\\{1,2\\}[[:digit:]]+[dhmwy]\\(?:/[[:digit:]]+[dhmwy]\\)?\\)\\{1,2\\}\\)?>\\)\\)\\)\\|\\(?:\\(?:<\\(?:[[:digit:]]\\{4\\}-[[:digit:]]\\{2\\}-[[:digit:]]\\{2\\}\\(?: [[:alpha:]]+\\)?\\)\\(?: [[:digit:]]\\{1,2\\}:[[:digit:]]\\{2\\}\\(?:-[[:digit:]]\\{1,2\\}:[[:digit:]]\\{2\\}\\)?\\)?\\(?:\\(?: [+.:-]\\{1,2\\}[[:digit:]]+[dhmwy]\\(?:/[[:digit:]]+[dhmwy]\\)?\\)\\{1,2\\}\\)?>\\)\\|^\\*+[[:blank:]]+\\(?:[[:upper:]]+[[:blank:]]+\\)?\\[#A]\\|^[[:space:]]*:STYLE:[[:space:]]+habit[[:space:]]*$\\)\\)")
(benchmark-run 20 (goto-char (point-min)) (while (re-search-forward yant/re nil t)))
--
Ihor Radchenko // yantar92,
Org mode maintainer,
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#77924; Package
emacs.
(Wed, 30 Apr 2025 13:10:02 GMT)
Full text and
rfc822 format available.
Message #302 received at 77924 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
> On 24. Apr 2025, at 18:27, Gerd Möllmann <gerd.moellmann <at> gmail.com> wrote:
>
> I hope Stef can take care of some of the rest. Stef, please let me know
> if should help. It could take a few days though, because of real-world
> interference.
>
It doesn't look like I'll work on this again any time soon, so please don't rely on me.
[Message part 2 (text/html, inline)]
This bug report was last modified 101 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.