Subject: Re: bug#77924: 31.0.50; [Feature branch] Change marker implementation

From: Gerd Möllmann <[email-masked]>
Date: Sat, 26 Apr 2025 21:25:23 +0200

Gerd Möllmann <[email-masked]> writes:

> Stefan Monnier <[email-masked]> 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

>From beaf4ab5a017e4339f90723c2a28a99baa2b43bc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Gerd=20M=C3=B6llmann?= <[email-masked]>
Date: Sat, 26 Apr 2025 21:20:22 +0200
Subject: [PATCH] Don't optimize prematurely in text_index_charpos_to_bytepos

* src/text-index.c (text_index_bytepos_to_charpos): Add a const.
(bytepos_of_head): Removed.
(text_index_charpos_to_bytepos): Don't be clever about choosing
to scan forward or backward.
---
 src/text-index.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/src/text-index.c b/src/text-index.c
index fcfa1a133b6..b9fad89ef4b 100644
--- a/src/text-index.c
+++ b/src/text-index.c
@@ -620,7 +620,7 @@ text_index_bytepos_to_charpos (struct buffer *b, const ptrdiff_t bytepos)
 
   /* Scan forward if the distance to the previous known position is
      smaller than the distance to the next known position.  */
-  ptrdiff_t charpos
+  const ptrdiff_t charpos
     = (bytepos - prev.bytepos < next.bytepos - bytepos)
       ? charpos_forward_to_bytepos (b, prev, bytepos)
       : charpos_backward_to_bytepos (b, next, bytepos);
@@ -629,14 +629,6 @@ text_index_bytepos_to_charpos (struct buffer *b, const ptrdiff_t bytepos)
   return charpos;
 }
 
-static ptrdiff_t
-bytepos_of_head (struct buffer *b, ptrdiff_t bytepos)
-{
-  while (!CHAR_HEAD_P (BUF_FETCH_BYTE (b, bytepos)))
-    --bytepos;
-  return bytepos;
-}
-
 /* Return the byte position in buffer B corresponding to character
    position CHARPOS.  */
 
@@ -684,14 +676,12 @@ text_index_charpos_to_bytepos (struct buffer *b, const ptrdiff_t charpos)
 	return prev.bytepos + (charpos - prev.charpos); /* ASCII-only!  */
     }
 
-  /* 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));
 
   cache (ti, charpos, bytepos);
   return bytepos;
-- 
2.49.0