bug#57266: Maintaining the base_line_number cache

bug#57266: Maintaining the base_line_number cache

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]

Tags: patch

Based on the recent discussion around counting lines, I proposed the
patch below.  Part of it aims to just document the way in which
the `w->base_line_number` cache is maintained, but it goes further,
fixing the problem with format-mode-line and narrowing I discovered
and simplifying some of the code based on the new understanding of how
the cache is supposed to work.


        Stefan


2022-08-17  Stefan Monnier  <monnier@iro.umontreal.ca>

    * src/xdisp.c (BASE_LINE_NUMBER_VALID_P): New macro.
    (try_scrolling): Use it.  Ignore `just_this_one_p` because it was
    too pessimistic.
    (redisplay_window): Remove `buffer_unchanged_p`, not used any more.
    Check BASE_LINE_NUMBER_VALID_P once and for all at the beginning and
    don't rely on other side-information that is only correlated with
    BASE_LINE_NUMBER_VALID_P to decide when to flush this cache.
    (decode_mode_spec): Check `BASE_LINE_NUMBER_VALID_P` before using
    the cache.

    * src/window.c (set_window_buffer): Flush the `base_line_number` cache.


In GNU Emacs 29.0.50 (build 1, i686-pc-linux-gnu, GTK+ Version 3.24.34, cairo version 1.16.0)
 of 2022-08-15 built on lechazo
Repository revision: 5564f72d5b3e2f162f52d935c077f6ea15fb60b7
Repository branch: work
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux bookworm/sid

Configured using:
 'configure -C --enable-checking --enable-check-lisp-object-type --with-modules --with-cairo --with-tiff=ifavailable
 'CFLAGS=-Wall -g3 -Og -Wno-pointer-sign'
 PKG_CONFIG_PATH=/home/monnier/lib/pkgconfig'


[-- Attachment #2: base_line.patch --]
[-- Type: text/patch, Size: 7051 bytes --]

diff --git a/src/window.c b/src/window.c
index c8fcb3a607f..436d4e342d7 100644
--- a/src/window.c
+++ b/src/window.c
@@ -4093,6 +4093,8 @@ set_window_buffer (Lisp_Object window, Lisp_Object buffer,
 			     buffer);
       w->start_at_line_beg = false;
       w->force_start = false;
+      /* Flush the base_line cache since it applied to another buffer.  */
+      w->base_line_number = 0;
     }
 
   wset_redisplay (w);
diff --git a/src/xdisp.c b/src/xdisp.c
index 03c43be5bc0..ec7cc904e78 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -18341,6 +18341,14 @@ cursor_row_fully_visible_p (struct window *w, bool force_p,
    `scroll-conservatively' and the Emacs manual.  */
 #define SCROLL_LIMIT 100
 
+/* The freshness of the w->base_line_number cache is only ensured at every
+   redisplay cycle, so the cache can be used only if there's been
+   no relevant changes to the buffer since the last redisplay.  */
+#define BASE_LINE_NUMBER_VALID_P(w)                      \
+   (eassert (current_buffer == XBUFFER ((w)->contents)), \
+    !current_buffer->clip_changed                        \
+    && BEG_UNCHANGED < (w)->base_line_number)
+
 static int
 try_scrolling (Lisp_Object window, bool just_this_one_p,
 	       intmax_t arg_scroll_conservatively, intmax_t scroll_step,
@@ -18640,9 +18648,10 @@ try_scrolling (Lisp_Object window, bool just_this_one_p,
   else
     {
       /* Maybe forget recorded base line for line number display.  */
-      if (!just_this_one_p
-	  || current_buffer->clip_changed
-	  || BEG_UNCHANGED < CHARPOS (startp))
+      /* FIXME: Why do we need this?  `try_scrolling` can only be called from
+         `redisplay_window` which should have flushed this cache already when
+         eeded.  */
+      if (!BASE_LINE_NUMBER_VALID_P (w))
 	w->base_line_number = 0;
 
       /* If cursor ends up on a partially visible line,
@@ -19404,9 +19413,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
   /* Record it now because it's overwritten.  */
   bool current_matrix_up_to_date_p = false;
   bool used_current_matrix_p = false;
-  /* This is less strict than current_matrix_up_to_date_p.
-     It indicates that the buffer contents and narrowing are unchanged.  */
-  bool buffer_unchanged_p = false;
   bool temp_scroll_step = false;
   specpdl_ref count = SPECPDL_INDEX ();
   int rc;
@@ -19505,11 +19511,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
 
   specbind (Qinhibit_point_motion_hooks, Qt);
 
-  buffer_unchanged_p
-    = (w->window_end_valid
-       && !current_buffer->clip_changed
-       && !window_outdated (w));
-
   /* When windows_or_buffers_changed is non-zero, we can't rely
      on the window end being valid, so set it to zero there.  */
   if (windows_or_buffers_changed)
@@ -19648,6 +19649,10 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
 	}
     }
 
+  if (!BASE_LINE_NUMBER_VALID_P (w))
+    /* Forget any recorded base line for line number display.  */
+    w->base_line_number = 0;
+
  force_start:
 
   /* Handle case where place to start displaying has been specified,
@@ -19668,10 +19673,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
       w->preserve_vscroll_p = false;
       w->window_end_valid = false;
 
-      /* Forget any recorded base line for line number display.  */
-      if (!buffer_unchanged_p)
-	w->base_line_number = 0;
-
       /* Redisplay the mode line.  Select the buffer properly for that.
 	 Also, run the hook window-scroll-functions
 	 because we have scrolled.  */
@@ -20000,12 +20001,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
 
       if (w->cursor.vpos >= 0)
 	{
-	  if (!just_this_one_p
-	      || current_buffer->clip_changed
-	      || BEG_UNCHANGED < CHARPOS (startp))
-	    /* Forget any recorded base line for line number display.  */
-	    w->base_line_number = 0;
-
 	  if (!cursor_row_fully_visible_p (w, true, false, false))
 	    {
 	      clear_glyph_matrix (w->desired_matrix);
@@ -20076,10 +20071,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
   debug_method_add (w, "recenter");
 #endif
 
-  /* Forget any previously recorded base line for line number display.  */
-  if (!buffer_unchanged_p)
-    w->base_line_number = 0;
-
   /* Determine the window start relative to point.  */
   init_iterator (&it, w, PT, PT_BYTE, NULL, DEFAULT_FACE_ID);
   it.current_y = it.last_visible_y;
@@ -24211,6 +24202,13 @@ maybe_produce_line_number (struct it *it)
       if (!last_line)
 	{
 	  /* If possible, reuse data cached by line-number-mode.  */
+	  /* NOTE: We use `base_line_number` without checking
+	     BASE_LINE_NUMBER_VALID_P because we assume that `redisplay_window`
+	     has already flushed this cache for us when needed.
+	     NOTE²: IIUC checking BASE_LINE_NUMBER_VALID_P here would be
+	     overly pessimistic because it might say that the cache
+	     was invalid before entering `redisplay_window` yet the
+	     value has just been refreshed.  */
 	  if (it->w->base_line_number > 0
 	      && it->w->base_line_pos > 0
 	      && it->w->base_line_pos <= IT_CHARPOS (*it)
@@ -27949,6 +27947,17 @@ decode_mode_spec (struct window *w, register int c, int field_width,
 	startpos = marker_position (w->start);
 	startpos_byte = marker_byte_position (w->start);
 	height = WINDOW_TOTAL_LINES (w);
+
+ 	if (!BASE_LINE_NUMBER_VALID_P (w))
+ 	  /* NOTE: This check is necessary when we're called from
+ 	     `format-mode-line` but not when we're called from
+ 	     `redisplay_window`.
+ 	     FIXME: It's usually harmless in that case, except if you have
+ 	     several `%l` in your modeline, in which case it will cause the
+ 	     cache to to be recomputed as many times when it's out of
+ 	     date :-(  */
+ 	  w->base_line_number = 0;
+
 	/* We cannot cope with w->start being outside of the
 	   accessible portion of the buffer; in particular,
 	   display_count_lines call below might infloop if called with
@@ -27962,8 +27971,6 @@ decode_mode_spec (struct window *w, register int c, int field_width,
 	  {
 	    startpos = BUF_BEGV (b);
 	    startpos_byte = BUF_BEGV_BYTE (b);
-	    w->base_line_pos = 0;
-	    w->base_line_number = 0;
 	  }
 
 	/* If we decided that this buffer isn't suitable for line numbers,
@@ -28044,6 +28051,12 @@ decode_mode_spec (struct window *w, register int c, int field_width,
 		goto no_value;
 	      }
 
+	    /* NOTE: if current_buffer->clip_changed is set or
+               if BEG_UNCHANGED is < position, this new cached value
+               may be unusable, because we can't reset BEG_UNCHANGED
+               or `clip_changed` from here (since they reflect the
+               changes since the last redisplay do they can only be reset
+               from `mark_window_display_accurate_1`).  */
 	    w->base_line_number = topline - nlines;
 	    w->base_line_pos = BYTE_TO_CHAR (position);
 	  }

bug#57266: Acknowledgement (Maintaining the base_line_number cache)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 108 bytes --]

Hmm... there was some serious thinko in my BASE_LINE_NUMBER_VALID_P.
Adjusted patch below,


        Stefan

[-- Attachment #2: base_line.patch --]
[-- Type: text/x-diff, Size: 7049 bytes --]

diff --git a/src/window.c b/src/window.c
index c8fcb3a607f..436d4e342d7 100644
--- a/src/window.c
+++ b/src/window.c
@@ -4093,6 +4093,8 @@ set_window_buffer (Lisp_Object window, Lisp_Object buffer,
 			     buffer);
       w->start_at_line_beg = false;
       w->force_start = false;
+      /* Flush the base_line cache since it applied to another buffer.  */
+      w->base_line_number = 0;
     }
 
   wset_redisplay (w);
diff --git a/src/xdisp.c b/src/xdisp.c
index 03c43be5bc0..902cd276a43 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -18341,6 +18341,14 @@ cursor_row_fully_visible_p (struct window *w, bool force_p,
    `scroll-conservatively' and the Emacs manual.  */
 #define SCROLL_LIMIT 100
 
+/* The freshness of the w->base_line_number cache is only ensured at every
+   redisplay cycle, so the cache can be used only if there's been
+   no relevant changes to the buffer since the last redisplay.  */
+#define BASE_LINE_NUMBER_VALID_P(w)                      \
+   (eassert (current_buffer == XBUFFER ((w)->contents)), \
+    !current_buffer->clip_changed                        \
+    && BEG_UNCHANGED >= (w)->base_line_pos)
+
 static int
 try_scrolling (Lisp_Object window, bool just_this_one_p,
 	       intmax_t arg_scroll_conservatively, intmax_t scroll_step,
@@ -18640,9 +18648,10 @@ try_scrolling (Lisp_Object window, bool just_this_one_p,
   else
     {
       /* Maybe forget recorded base line for line number display.  */
-      if (!just_this_one_p
-	  || current_buffer->clip_changed
-	  || BEG_UNCHANGED < CHARPOS (startp))
+      /* FIXME: Why do we need this?  `try_scrolling` can only be called from
+         `redisplay_window` which should have flushed this cache already when
+         eeded.  */
+      if (!BASE_LINE_NUMBER_VALID_P (w))
 	w->base_line_number = 0;
 
       /* If cursor ends up on a partially visible line,
@@ -19404,9 +19413,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
   /* Record it now because it's overwritten.  */
   bool current_matrix_up_to_date_p = false;
   bool used_current_matrix_p = false;
-  /* This is less strict than current_matrix_up_to_date_p.
-     It indicates that the buffer contents and narrowing are unchanged.  */
-  bool buffer_unchanged_p = false;
   bool temp_scroll_step = false;
   specpdl_ref count = SPECPDL_INDEX ();
   int rc;
@@ -19505,11 +19511,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
 
   specbind (Qinhibit_point_motion_hooks, Qt);
 
-  buffer_unchanged_p
-    = (w->window_end_valid
-       && !current_buffer->clip_changed
-       && !window_outdated (w));
-
   /* When windows_or_buffers_changed is non-zero, we can't rely
      on the window end being valid, so set it to zero there.  */
   if (windows_or_buffers_changed)
@@ -19648,6 +19649,10 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
 	}
     }
 
+  if (!BASE_LINE_NUMBER_VALID_P (w))
+    /* Forget any recorded base line for line number display.  */
+    w->base_line_number = 0;
+
  force_start:
 
   /* Handle case where place to start displaying has been specified,
@@ -19668,10 +19673,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
       w->preserve_vscroll_p = false;
       w->window_end_valid = false;
 
-      /* Forget any recorded base line for line number display.  */
-      if (!buffer_unchanged_p)
-	w->base_line_number = 0;
-
       /* Redisplay the mode line.  Select the buffer properly for that.
 	 Also, run the hook window-scroll-functions
 	 because we have scrolled.  */
@@ -20000,12 +20001,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
 
       if (w->cursor.vpos >= 0)
 	{
-	  if (!just_this_one_p
-	      || current_buffer->clip_changed
-	      || BEG_UNCHANGED < CHARPOS (startp))
-	    /* Forget any recorded base line for line number display.  */
-	    w->base_line_number = 0;
-
 	  if (!cursor_row_fully_visible_p (w, true, false, false))
 	    {
 	      clear_glyph_matrix (w->desired_matrix);
@@ -20076,10 +20071,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
   debug_method_add (w, "recenter");
 #endif
 
-  /* Forget any previously recorded base line for line number display.  */
-  if (!buffer_unchanged_p)
-    w->base_line_number = 0;
-
   /* Determine the window start relative to point.  */
   init_iterator (&it, w, PT, PT_BYTE, NULL, DEFAULT_FACE_ID);
   it.current_y = it.last_visible_y;
@@ -24211,6 +24202,13 @@ maybe_produce_line_number (struct it *it)
       if (!last_line)
 	{
 	  /* If possible, reuse data cached by line-number-mode.  */
+	  /* NOTE: We use `base_line_number` without checking
+	     BASE_LINE_NUMBER_VALID_P because we assume that `redisplay_window`
+	     has already flushed this cache for us when needed.
+	     NOTE²: IIUC checking BASE_LINE_NUMBER_VALID_P here would be
+	     overly pessimistic because it might say that the cache
+	     was invalid before entering `redisplay_window` yet the
+	     value has just been refreshed.  */
 	  if (it->w->base_line_number > 0
 	      && it->w->base_line_pos > 0
 	      && it->w->base_line_pos <= IT_CHARPOS (*it)
@@ -27949,6 +27947,17 @@ decode_mode_spec (struct window *w, register int c, int field_width,
 	startpos = marker_position (w->start);
 	startpos_byte = marker_byte_position (w->start);
 	height = WINDOW_TOTAL_LINES (w);
+
+ 	if (!BASE_LINE_NUMBER_VALID_P (w))
+ 	  /* NOTE: This check is necessary when we're called from
+ 	     `format-mode-line` but not when we're called from
+ 	     `redisplay_window`.
+ 	     FIXME: It's usually harmless in that case, except if you have
+ 	     several `%l` in your modeline, in which case it will cause the
+ 	     cache to to be recomputed as many times when it's out of
+ 	     date :-(  */
+ 	  w->base_line_number = 0;
+
 	/* We cannot cope with w->start being outside of the
 	   accessible portion of the buffer; in particular,
 	   display_count_lines call below might infloop if called with
@@ -27962,8 +27971,6 @@ decode_mode_spec (struct window *w, register int c, int field_width,
 	  {
 	    startpos = BUF_BEGV (b);
 	    startpos_byte = BUF_BEGV_BYTE (b);
-	    w->base_line_pos = 0;
-	    w->base_line_number = 0;
 	  }
 
 	/* If we decided that this buffer isn't suitable for line numbers,
@@ -28044,6 +28051,12 @@ decode_mode_spec (struct window *w, register int c, int field_width,
 		goto no_value;
 	      }
 
+	    /* NOTE: if current_buffer->clip_changed is set or
+               if BEG_UNCHANGED is < position, this new cached value
+               may be unusable, because we can't reset BEG_UNCHANGED
+               or `clip_changed` from here (since they reflect the
+               changes since the last redisplay do they can only be reset
+               from `mark_window_display_accurate_1`).  */
 	    w->base_line_number = topline - nlines;
 	    w->base_line_pos = BYTE_TO_CHAR (position);
 	  }

bug#57266: Maintaining the base_line_number cache

[Eli Zaretskii]

> Date: Wed, 17 Aug 2022 17:18:44 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Based on the recent discussion around counting lines, I proposed the
> patch below.  Part of it aims to just document the way in which
> the `w->base_line_number` cache is maintained, but it goes further,
> fixing the problem with format-mode-line and narrowing I discovered
> and simplifying some of the code based on the new understanding of how
> the cache is supposed to work.

Thanks for working on this, but I think this is sometimes insufficient
and sometimes too bold for my palate: I'm not thrilled by the
perspective of investigating hard-to-debug bug reports about this
tricky issue, and will only agree to absolutely 110% safe changes.

All in all, I'd be much happier if you didn't attempt any "cleanups"
whose value is questionable in this area, only added the missing
invalidations.  (It is okay to replace identical or similar tests with
a macro, of course.)

Specifically:

> --- a/src/window.c
> +++ b/src/window.c
> @@ -4093,6 +4093,8 @@ set_window_buffer (Lisp_Object window, Lisp_Object buffer,
>  			     buffer);
>        w->start_at_line_beg = false;
>        w->force_start = false;
> +      /* Flush the base_line cache since it applied to another buffer.  */
> +      w->base_line_number = 0;
>      }

This is harmless, but IMO unnecessary, because wset_buffer already
takes care of that, via adjust_window_count.

> +/* The freshness of the w->base_line_number cache is only ensured at every
> +   redisplay cycle, so the cache can be used only if there's been
> +   no relevant changes to the buffer since the last redisplay.  */
> +#define BASE_LINE_NUMBER_VALID_P(w)                      \
> +   (eassert (current_buffer == XBUFFER ((w)->contents)), \
> +    !current_buffer->clip_changed                        \
> +    && BEG_UNCHANGED < (w)->base_line_number)

The assertion there is unnecessary: instead of using current_buffer,
you can use XBUFFER ((w)->contents).  It is also safer. Recently I
learned that in some corners of redisplay_window, the condition

  current_buffer == XBUFFER ((w)->contents)

is not necessarily tru.

Also, at least one place where you want to use this macro check
window_outdated, which this macro doesn't do.  If you know the reason
why that call is unnecessary, please explain it.

>        /* Maybe forget recorded base line for line number display.  */
> -      if (!just_this_one_p
> -	  || current_buffer->clip_changed
> -	  || BEG_UNCHANGED < CHARPOS (startp))
> +      /* FIXME: Why do we need this?  `try_scrolling` can only be called from
> +         `redisplay_window` which should have flushed this cache already when
> +         eeded.  */
> +      if (!BASE_LINE_NUMBER_VALID_P (w))
>  	w->base_line_number = 0;

About the FIXME: please analyze the control flow inside
redisplay_window and post your conclusions here, if not include them
in the log message.  Some of the optimizations that redisplay_window
attempts early on, if they succeed, do "goto done;", bypassing the
rest of the code, including try_scrolling and whatnot.  You need to
convince yourself and us (at least me) that deleting those other
places which invalidate the cache is indeed justified, because there's
no chance we will bypass the ones you leave in the code, and OTOH that
we don't unnecessarily invalidate the cache where we shouldn't.  The
control flow in redisplay_window is quite tricky.

> @@ -19404,9 +19413,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
>    /* Record it now because it's overwritten.  */
>    bool current_matrix_up_to_date_p = false;
>    bool used_current_matrix_p = false;
> -  /* This is less strict than current_matrix_up_to_date_p.
> -     It indicates that the buffer contents and narrowing are unchanged.  */
> -  bool buffer_unchanged_p = false;

Before you can delete this and its uses, please perform the
above-mentioned analysis.

> @@ -19505,11 +19511,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
>  
>    specbind (Qinhibit_point_motion_hooks, Qt);
>  
> -  buffer_unchanged_p
> -    = (w->window_end_valid
> -       && !current_buffer->clip_changed
> -       && !window_outdated (w));

Here you remove window_outdated without replacing it with anything.
Why?

> @@ -20000,12 +20001,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
>  
>        if (w->cursor.vpos >= 0)
>  	{
> -	  if (!just_this_one_p
> -	      || current_buffer->clip_changed
> -	      || BEG_UNCHANGED < CHARPOS (startp))
> -	    /* Forget any recorded base line for line number display.  */
> -	    w->base_line_number = 0;

Sorry, I don't buy your "reason" for removing just_this_one_p.  And
the deletion itself needs that analysis I mentioned.

> +	     NOTE²: IIUC checking BASE_LINE_NUMBER_VALID_P here would be

Please avoid using non-ASCII characters in C sources, as they aren't
visited with UTF-8 encoding by default.

> +	     overly pessimistic because it might say that the cache
> +	     was invalid before entering `redisplay_window` yet the
> +	     value has just been refreshed.  */
>  	  if (it->w->base_line_number > 0
>  	      && it->w->base_line_pos > 0
>  	      && it->w->base_line_pos <= IT_CHARPOS (*it)
> @@ -27949,6 +27947,17 @@ decode_mode_spec (struct window *w, register int c, int field_width,
>  	startpos = marker_position (w->start);
>  	startpos_byte = marker_byte_position (w->start);
>  	height = WINDOW_TOTAL_LINES (w);
> +
> + 	if (!BASE_LINE_NUMBER_VALID_P (w))

Here your macro assumes the buffer is current_buffer, whereas the rest
of the code doesn't.  See my comment about that assertion in the macro.

> @@ -27962,8 +27971,6 @@ decode_mode_spec (struct window *w, register int c, int field_width,
>  	  {
>  	    startpos = BUF_BEGV (b);
>  	    startpos_byte = BUF_BEGV_BYTE (b);
> -	    w->base_line_pos = 0;
> -	    w->base_line_number = 0;

Here you assume that current_buffer->clip_changed detects the
condition

	if (!(BUF_BEGV_BYTE (b) <= startpos_byte
	      && startpos_byte <= BUF_ZV_BYTE (b)))

But I see no particular reason to think that assumption is always
valid.

> +	    /* NOTE: if current_buffer->clip_changed is set or
> +               if BEG_UNCHANGED is < position, this new cached value
> +               may be unusable, because we can't reset BEG_UNCHANGED
> +               or `clip_changed` from here (since they reflect the
> +               changes since the last redisplay do they can only be reset
> +               from `mark_window_display_accurate_1`).  */

The part of the comment in parentheses has some sort of typo, because
it doesn't parse.

bug#57266: Maintaining the base_line_number cache

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> All in all, I'd be much happier if you didn't attempt any "cleanups"
> whose value is questionable in this area, only added the missing
> invalidations.  (It is okay to replace identical or similar tests with
> a macro, of course.)

I think the `!just_this_one_p` is important enough since the test isn't
needed, and it otherwise prevents caching the base_line when a buffer is
displayed in more than 1 window, which can slow down redisplay noticeably
in such a case.

I added more comments to explain how the cache works.  It doesn't
explain why we tested `just_this_one_p`, tho, so maybe I'm still
missing something.  The Git history shows this test has been there ever
since 1993 but I haven't seen (nor figured) any explanation for it :-(
My best guess is that the original code basically only tried to be
"clever" when a single window needed refresh and otherwise redid
everything from scratch and that design also applied to the
base_line cache, without thinking too hard about whether it's
truly needed.

>> @@ -4093,6 +4093,8 @@ set_window_buffer (Lisp_Object window, Lisp_Object buffer,
>>  			     buffer);
>>        w->start_at_line_beg = false;
>>        w->force_start = false;
>> +      /* Flush the base_line cache since it applied to another buffer.  */
>> +      w->base_line_number = 0;
>>      }
>
> This is harmless, but IMO unnecessary, because wset_buffer already
> takes care of that, via adjust_window_count.

Indeed, thanks.  In the new patch I moved this from
`adjust_window_count` to `wset_buffer` (otherwise it's always executed
twice in `wset_buffer` and even three times in the other function that
calls `adjust_window_count`, since that one also calls `wset_buffer`).

>> +/* The freshness of the w->base_line_number cache is only ensured at every
>> +   redisplay cycle, so the cache can be used only if there's been
>> +   no relevant changes to the buffer since the last redisplay.  */
>> +#define BASE_LINE_NUMBER_VALID_P(w)                      \
>> +   (eassert (current_buffer == XBUFFER ((w)->contents)), \
>> +    !current_buffer->clip_changed                        \
>> +    && BEG_UNCHANGED < (w)->base_line_number)
>
> The assertion there is unnecessary: instead of using current_buffer,
> you can use XBUFFER ((w)->contents).  It is also safer.

Duh, indeed, thanks.

> Recently I learned that in some corners of redisplay_window, the
> condition
>
>   current_buffer == XBUFFER ((w)->contents)
>
> is not necessarily tru.

Indeed, this happens for example when calling `format-mode-line` from
a buffer different from the window's buffer.

> Also, at least one place where you want to use this macro check
> window_outdated, which this macro doesn't do.  If you know the reason
> why that call is unnecessary, please explain it.

It's not necessary because the only way the cache can be out of date
if when either clipping or buffer text or w->contents is changed and
that is already taken into account.

>>        /* Maybe forget recorded base line for line number display.  */
>> -      if (!just_this_one_p
>> -	  || current_buffer->clip_changed
>> -	  || BEG_UNCHANGED < CHARPOS (startp))
>> +      /* FIXME: Why do we need this?  `try_scrolling` can only be called from
>> +         `redisplay_window` which should have flushed this cache already when
>> +         eeded.  */
>> +      if (!BASE_LINE_NUMBER_VALID_P (w))
>>  	w->base_line_number = 0;
>
> About the FIXME: please analyze the control flow inside
> redisplay_window and post your conclusions here, if not include them
> in the log message.

The above code is in `try_scrolling` which is only called by
`redisplay_window` and my patch changes `redisplay_window` so it always
flushes the cache if needed, right from the start, so by the time we get
to `try_scrolling` we know the cache has already been flushed if needed
so this code is redundant.

> Some of the optimizations that redisplay_window attempts early on, if
> they succeed, do "goto done;", bypassing the rest of the code,
> including try_scrolling and whatnot.

The dependency goes the other way around.

>> @@ -20000,12 +20001,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
>>  
>>        if (w->cursor.vpos >= 0)
>>  	{
>> -	  if (!just_this_one_p
>> -	      || current_buffer->clip_changed
>> -	      || BEG_UNCHANGED < CHARPOS (startp))
>> -	    /* Forget any recorded base line for line number display.  */
>> -	    w->base_line_number = 0;
>
> Sorry, I don't buy your "reason" for removing just_this_one_p.  And
> the deletion itself needs that analysis I mentioned.

Maybe the !just_this_one_p was needed at some point in the past.
Now it's not any more, but I don't know when/where/how that changed.

>> @@ -27949,6 +27947,17 @@ decode_mode_spec (struct window *w, register int c, int field_width,
>>  	startpos = marker_position (w->start);
>>  	startpos_byte = marker_byte_position (w->start);
>>  	height = WINDOW_TOTAL_LINES (w);
>> +
>> + 	if (!BASE_LINE_NUMBER_VALID_P (w))
>
> Here your macro assumes the buffer is current_buffer, whereas the rest
> of the code doesn't.  See my comment about that assertion in the macro.

Indeed, thanks.

>> @@ -27962,8 +27971,6 @@ decode_mode_spec (struct window *w, register int c, int field_width,
>>  	  {
>>  	    startpos = BUF_BEGV (b);
>>  	    startpos_byte = BUF_BEGV_BYTE (b);
>> -	    w->base_line_pos = 0;
>> -	    w->base_line_number = 0;
>
> Here you assume that current_buffer->clip_changed detects the
> condition
>
> 	if (!(BUF_BEGV_BYTE (b) <= startpos_byte
> 	      && startpos_byte <= BUF_ZV_BYTE (b)))
>
> But I see no particular reason to think that assumption is always
> valid.

No, the reason those lines are redundant is that the base_line cache is
not affected by startpos: the value stored there is does not depend on startpos.
This said, in my patch I reverted this change because setting `w->base_line_pos = 0;`
has another side effect which is to reconsider whether to display line
numbers or not.

>> +	    /* NOTE: if current_buffer->clip_changed is set or
>> +               if BEG_UNCHANGED is < position, this new cached value
>> +               may be unusable, because we can't reset BEG_UNCHANGED
>> +               or `clip_changed` from here (since they reflect the
>> +               changes since the last redisplay do they can only be reset
>> +               from `mark_window_display_accurate_1`).  */
>
> The part of the comment in parentheses has some sort of typo, because
> it doesn't parse.

Indeed.  I think it's fixed now.


        Stefan


diff --git a/src/window.c b/src/window.c
index 2bce4c9723d..a6e3ad904f1 100644
--- a/src/window.c
+++ b/src/window.c
@@ -282,9 +282,6 @@ adjust_window_count (struct window *w, int arg)
 	b = b->base_buffer;
       b->window_count += arg;
       eassert (b->window_count >= 0);
-      /* These should be recalculated by redisplay code.  */
-      w->window_end_valid = false;
-      w->base_line_pos = 0;
     }
 }
 
@@ -301,6 +298,9 @@ wset_buffer (struct window *w, Lisp_Object val)
     eassert (MARKERP (w->start) && MARKERP (w->pointm));
   w->contents = val;
   adjust_window_count (w, 1);
+  /* These should be recalculated by redisplay code.  */
+  w->window_end_valid = false;
+  w->base_line_pos = 0;
 }
 
 static void
diff --git a/src/xdisp.c b/src/xdisp.c
index 03c43be5bc0..d9052c067f0 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -18341,6 +18341,39 @@ cursor_row_fully_visible_p (struct window *w, bool force_p,
    `scroll-conservatively' and the Emacs manual.  */
 #define SCROLL_LIMIT 100
 
+/* The freshness of the w->base_line_number cache is only ensured at every
+   redisplay cycle, so the cache can be used only if there's been
+   no relevant changes to the buffer since the last redisplay.  */
+static bool
+base_line_number_valid_p (struct window *w)
+{
+  /* The base_line cache stores a value which depends on:
+       A. w->contents (AKA `b`)
+       B. BUF_BEGV (b);
+       C. the contents of b->text
+     We flush the cache (in `wset_buffer`) when we set `w->contents`.
+     But for (B) and (C) we don't eagerly flush the cache when those get
+     changed, instead we rely on the `b->clip_changed` bit for (B) and on
+     BUF_BEG_UNCHANGED (b) for (C), and we check them here before making use
+     of the cache.
+     Both of those pieces of info are (re)set by redisplay.  This means that
+     after a modification which invalidates the cache, the cache will stay
+     invalid until the next redisplay, even if we recompute the value anew.
+     Worse: clip_changed and BUF_BEG_UNCHANGED only get reset at the end of a
+     redisplay cycle (after all the windows have been redisplayed), so the
+     base_lines we computed during redisplay are put into the cache
+     at a time when `base_line_number_valid_p` can return false.
+     In order to avoid throwing away this useful info, the cache is flushed
+     as needed at the beginning of redisplay (in `redisplay_window`) so that
+     during redisplay we can presume that the cache is valid (even if
+     `base_line_number_valid_p` is false).
+     It also means we can have a problem if the clipping or the b->text is
+     modified *during* redisplay.  */
+  struct buffer *b = XBUFFER ((w)->contents);
+  return !b->clip_changed
+         && BUF_BEG_UNCHANGED (b) >= (w)->base_line_pos;
+}
+
 static int
 try_scrolling (Lisp_Object window, bool just_this_one_p,
 	       intmax_t arg_scroll_conservatively, intmax_t scroll_step,
@@ -18639,12 +18672,6 @@ try_scrolling (Lisp_Object window, bool just_this_one_p,
     }
   else
     {
-      /* Maybe forget recorded base line for line number display.  */
-      if (!just_this_one_p
-	  || current_buffer->clip_changed
-	  || BEG_UNCHANGED < CHARPOS (startp))
-	w->base_line_number = 0;
-
       /* If cursor ends up on a partially visible line,
 	 treat that as being off the bottom of the screen.  */
       if (! cursor_row_fully_visible_p (w, extra_scroll_margin_lines <= 1,
@@ -19404,9 +19431,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
   /* Record it now because it's overwritten.  */
   bool current_matrix_up_to_date_p = false;
   bool used_current_matrix_p = false;
-  /* This is less strict than current_matrix_up_to_date_p.
-     It indicates that the buffer contents and narrowing are unchanged.  */
-  bool buffer_unchanged_p = false;
   bool temp_scroll_step = false;
   specpdl_ref count = SPECPDL_INDEX ();
   int rc;
@@ -19447,6 +19471,15 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
        cleverly elsewhere.  */
     w->must_be_updated_p = true;
 
+  if (!base_line_number_valid_p (w))
+    /* Forget any recorded base line for line number display.
+       We do it:
+       - after calling `reconsider_clip_changes` which may discover
+         that the clipping hasn't changed after all.
+       - early enough that we're sure it's done (the code below has
+         various `goto` which could otherwise skip this operation).  */
+    w->base_line_number = 0;
+
   if (MINI_WINDOW_P (w))
     {
       if (w == XWINDOW (echo_area_window)
@@ -19505,11 +19538,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
 
   specbind (Qinhibit_point_motion_hooks, Qt);
 
-  buffer_unchanged_p
-    = (w->window_end_valid
-       && !current_buffer->clip_changed
-       && !window_outdated (w));
-
   /* When windows_or_buffers_changed is non-zero, we can't rely
      on the window end being valid, so set it to zero there.  */
   if (windows_or_buffers_changed)
@@ -19668,10 +19696,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
       w->preserve_vscroll_p = false;
       w->window_end_valid = false;
 
-      /* Forget any recorded base line for line number display.  */
-      if (!buffer_unchanged_p)
-	w->base_line_number = 0;
-
       /* Redisplay the mode line.  Select the buffer properly for that.
 	 Also, run the hook window-scroll-functions
 	 because we have scrolled.  */
@@ -20000,12 +20024,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
 
       if (w->cursor.vpos >= 0)
 	{
-	  if (!just_this_one_p
-	      || current_buffer->clip_changed
-	      || BEG_UNCHANGED < CHARPOS (startp))
-	    /* Forget any recorded base line for line number display.  */
-	    w->base_line_number = 0;
-
 	  if (!cursor_row_fully_visible_p (w, true, false, false))
 	    {
 	      clear_glyph_matrix (w->desired_matrix);
@@ -20076,10 +20094,6 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
   debug_method_add (w, "recenter");
 #endif
 
-  /* Forget any previously recorded base line for line number display.  */
-  if (!buffer_unchanged_p)
-    w->base_line_number = 0;
-
   /* Determine the window start relative to point.  */
   init_iterator (&it, w, PT, PT_BYTE, NULL, DEFAULT_FACE_ID);
   it.current_y = it.last_visible_y;
@@ -24211,6 +24225,14 @@ maybe_produce_line_number (struct it *it)
       if (!last_line)
 	{
 	  /* If possible, reuse data cached by line-number-mode.  */
+	  /* NOTE: We use `base_line_number` without checking
+	     `base_line_number_valid_p` because we assume that
+             `redisplay_window` has already flushed this cache for us when
+             needed.
+	     Checking `base_line_number_valid_p` here would be
+	     overly pessimistic because it might say that the cache
+	     was invalid before entering `redisplay_window` yet the
+	     value has just been refreshed.  */
 	  if (it->w->base_line_number > 0
 	      && it->w->base_line_pos > 0
 	      && it->w->base_line_pos <= IT_CHARPOS (*it)
@@ -27523,10 +27545,24 @@ DEFUN ("format-mode-line", Fformat_mode_line, Sformat_mode_line,
 	= NILP (face) ? Qnil : list2 (Qface, face);
     }
 
+  if (!EQ (buffer, w->contents) || !base_line_number_valid_p (w))
+    /* decode_mode_spec/display_mode_element presume that the base_line
+       cache has been flushed if needed.  They don't flush it themselves,
+       because that could flush a value they just computed, e.g. when
+       %l appears twice in the mode line (or once in the mode line and
+       once in the frame title, ...).  */
+    w->base_line_number = 0;
+
   push_kboard (FRAME_KBOARD (it.f));
   display_mode_element (&it, 0, 0, 0, format, Qnil, false);
   pop_kboard ();
 
+  if (!EQ (buffer, w->contents) || !base_line_number_valid_p (w))
+    /* Flush any new base_line we may have computed while the clipping was
+       changed (or while operating on another buffer), since
+       `reconsider_clip_changes` may reset clip_changes to false later on.  */
+    w->base_line_number = 0;
+
   if (no_props)
     {
       len = MODE_LINE_NOPROP_LEN (string_start);
@@ -28044,6 +28080,12 @@ decode_mode_spec (struct window *w, register int c, int field_width,
 		goto no_value;
 	      }
 
+	    /* NOTE: if current_buffer->clip_changed is set or
+               if BEG_UNCHANGED is < position, this new cached value
+               may be unusable, because we can't reset BEG_UNCHANGED
+               or `clip_changed` from here (since they reflect the
+               changes since the last redisplay, they can only be reset
+               from `mark_window_display_accurate_1`).  */
 	    w->base_line_number = topline - nlines;
 	    w->base_line_pos = BYTE_TO_CHAR (position);
 	  }

bug#57266: Maintaining the base_line_number cache

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 57266@debbugs.gnu.org
> Date: Mon, 22 Aug 2022 00:34:42 -0400
> 
> > All in all, I'd be much happier if you didn't attempt any "cleanups"
> > whose value is questionable in this area, only added the missing
> > invalidations.  (It is okay to replace identical or similar tests with
> > a macro, of course.)
> 
> I think the `!just_this_one_p` is important enough since the test isn't
> needed, and it otherwise prevents caching the base_line when a buffer is
> displayed in more than 1 window, which can slow down redisplay noticeably
> in such a case.

I didn't just mean just_this_one_p thingy, I meant all the changes
that attempt to somehow "clean up" the use of the line-number cache.
Why do we have to do that at all?  This stuff is not too complex, and
it works for ages!  Isn't the fact that we find more and more small
issues with the changes telling you something?

Please, just leave it alone, and only fix the actual problems.

> > Also, at least one place where you want to use this macro check
> > window_outdated, which this macro doesn't do.  If you know the reason
> > why that call is unnecessary, please explain it.
> 
> It's not necessary because the only way the cache can be out of date
> if when either clipping or buffer text or w->contents is changed and
> that is already taken into account.

What about the w->last_overlay_modified part, which window_outdated
takes into account?

> >>        /* Maybe forget recorded base line for line number display.  */
> >> -      if (!just_this_one_p
> >> -	  || current_buffer->clip_changed
> >> -	  || BEG_UNCHANGED < CHARPOS (startp))
> >> +      /* FIXME: Why do we need this?  `try_scrolling` can only be called from
> >> +         `redisplay_window` which should have flushed this cache already when
> >> +         eeded.  */
> >> +      if (!BASE_LINE_NUMBER_VALID_P (w))
> >>  	w->base_line_number = 0;
> >
> > About the FIXME: please analyze the control flow inside
> > redisplay_window and post your conclusions here, if not include them
> > in the log message.
> 
> The above code is in `try_scrolling` which is only called by
> `redisplay_window` and my patch changes `redisplay_window` so it always
> flushes the cache if needed, right from the start, so by the time we get
> to `try_scrolling` we know the cache has already been flushed if needed
> so this code is redundant.

The "if needed" part is the one I don't buy.  You moved it towards the
beginning of redisplay_window, which might mean it is sometimes done
prematurely and/or unnecessarily.  And the explanation is too
"hand-wavy" for my palate, sorry.

Once again, I'd prefer that we didn't touch working code, which
nowadays supports at least 3e features: the mode-line display, the
display-line-numbers mode, and the line-number-at-pos function and its
callers.

bug#57266: Maintaining the base_line_number cache

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I think the `!just_this_one_p` is important enough since the test isn't
>> needed, and it otherwise prevents caching the base_line when a buffer is
>> displayed in more than 1 window, which can slow down redisplay noticeably
>> in such a case.
>
> I didn't just mean just_this_one_p thingy, I meant all the changes
> that attempt to somehow "clean up" the use of the line-number cache.
> Why do we have to do that at all?  This stuff is not too complex, and
> it works for ages!  Isn't the fact that we find more and more small
> issues with the changes telling you something?

Yes, it's telling me that the code is a mess and I'm trying to fix it by
documenting what is actually needed.

>> It's not necessary because the only way the cache can be out of date
>> if when either clipping or buffer text or w->contents is changed and
>> that is already taken into account.
> What about the w->last_overlay_modified part, which window_outdated
> takes into account?

Overlays have no influence over the line number count, because the
base_line cache only counts real \n characters, not display lines.

> Once again, I'd prefer that we didn't touch working code, which

Code we don't dare to change is a problem we should fix.

> nowadays supports at least 3e features: the mode-line display, the
> display-line-numbers mode, and the line-number-at-pos function and its
> callers.

AFAIK it's not used (yet) in `line-number-at-pos`.


        Stefan

bug#57266: Maintaining the base_line_number cache

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 57266@debbugs.gnu.org
> Date: Mon, 22 Aug 2022 08:41:41 -0400
> 
> > I didn't just mean just_this_one_p thingy, I meant all the changes
> > that attempt to somehow "clean up" the use of the line-number cache.
> > Why do we have to do that at all?  This stuff is not too complex, and
> > it works for ages!  Isn't the fact that we find more and more small
> > issues with the changes telling you something?
> 
> Yes, it's telling me that the code is a mess and I'm trying to fix it by
> documenting what is actually needed.

It's fine to fix and enhance the documentation, but please don't fix
the code that works.

bug#57266: Maintaining the base_line_number cache

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> > I didn't just mean just_this_one_p thingy, I meant all the changes
>> > that attempt to somehow "clean up" the use of the line-number cache.
>> > Why do we have to do that at all?  This stuff is not too complex, and
>> > it works for ages!  Isn't the fact that we find more and more small
>> > issues with the changes telling you something?
>> 
>> Yes, it's telling me that the code is a mess and I'm trying to fix it by
>> documenting what is actually needed.
>
> It's fine to fix and enhance the documentation, but please don't fix
> the code that works.

If the code doesn't match the doc, then we'll never know if the doc
is wrong.


        Stefan

bug#57266: Maintaining the base_line_number cache

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 57266@debbugs.gnu.org
> Date: Mon, 22 Aug 2022 09:32:50 -0400
> 
> >> > I didn't just mean just_this_one_p thingy, I meant all the changes
> >> > that attempt to somehow "clean up" the use of the line-number cache.
> >> > Why do we have to do that at all?  This stuff is not too complex, and
> >> > it works for ages!  Isn't the fact that we find more and more small
> >> > issues with the changes telling you something?
> >> 
> >> Yes, it's telling me that the code is a mess and I'm trying to fix it by
> >> documenting what is actually needed.
> >
> > It's fine to fix and enhance the documentation, but please don't fix
> > the code that works.
> 
> If the code doesn't match the doc, then we'll never know if the doc
> is wrong.

Which is why I'm perfectly OK with documenting what the code does.

I'm also OK with adding a single function that makes the tests for
invalidating the cache, and then calling that function everywhere
where we now do such tests, not all of them identical.  And in that
function we can document everything we know about the cases where the
cache should be invalidated.

But please leave the tests themselves where they are done now, I see
no special reason to move them.  Because nothing is really wrong,
except the one case you found (and which we should fix, I agree).

Please!  I hope my requests mean something, given that I'm the only
one who works on bugs in these areas, and probably will keep doing
that for the observable future.

bug#57266: Maintaining the base_line_number cache

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> >> > I didn't just mean just_this_one_p thingy, I meant all the changes
>> >> > that attempt to somehow "clean up" the use of the line-number cache.
>> >> > Why do we have to do that at all?  This stuff is not too complex, and
>> >> > it works for ages!  Isn't the fact that we find more and more small
>> >> > issues with the changes telling you something?
>> >> 
>> >> Yes, it's telling me that the code is a mess and I'm trying to fix it by
>> >> documenting what is actually needed.
>> >
>> > It's fine to fix and enhance the documentation, but please don't fix
>> > the code that works.
>> 
>> If the code doesn't match the doc, then we'll never know if the doc
>> is wrong.
>
> Which is why I'm perfectly OK with documenting what the code does.

But I'm not.  Because I don't really know what the code does.
The doc I wrote documents the design I came up with based on my
understanding of what the code does.

I went through the trouble of figuring out a design of how things
*should* work, document it, and adjust the code to match the design.
So I'm not interested in pushing some half-assed version of it that
documents a design that doesn't match the reality, nor pushing partial
fixes/workarounds (for borderline cases that noone ever bumped into
anyway) while keeping the mess of unexplained hacks.

> Please!  I hope my requests mean something, given that I'm the only
> one who works on bugs in these areas, and probably will keep doing
> that for the observable future.

Indeed, my proposed patch is not a bug fix.
It's a code-maintenance patch.  It's meant to improve the code, not
the behavior.
[ I do think it will improve the behavior in some cases (by fixing a few
  corner case bugs, and more importantly by improving performance when
  displaying a buffer is several windows), but that is not the
  motivation, and there is undoubtedly the risk that it will introduce
  regressions (which will help us better understand the code).  ]


        Stefan

bug#57266: Maintaining the base_line_number cache

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 57266@debbugs.gnu.org
> Date: Mon, 22 Aug 2022 14:02:44 -0400
> 
> > Which is why I'm perfectly OK with documenting what the code does.
> 
> But I'm not.  Because I don't really know what the code does.
> The doc I wrote documents the design I came up with based on my
> understanding of what the code does.

IME, our "understanding" what the display code does is frequently
flawed, and we discover that too late.

> I went through the trouble of figuring out a design of how things
> *should* work, document it, and adjust the code to match the design.
> So I'm not interested in pushing some half-assed version of it that
> documents a design that doesn't match the reality, nor pushing partial
> fixes/workarounds (for borderline cases that noone ever bumped into
> anyway) while keeping the mess of unexplained hacks.

I agree that if we were writing the display engine today, we'd do many
things differently and more cleanly.  But we are not at that place,
and the (marginal at best) advantages of having the code match your
understanding do not justify the risk of subtle and hard-to-debug
display glitches.

> > Please!  I hope my requests mean something, given that I'm the only
> > one who works on bugs in these areas, and probably will keep doing
> > that for the observable future.
> 
> Indeed, my proposed patch is not a bug fix.
> It's a code-maintenance patch.  It's meant to improve the code, not
> the behavior.

Sorry, I had (and still have from time to time) enough fallout from
similar experiments of yours around Emacs 25, which also aimed at
improving the code (by removing various reasons to perform more
thorough redisplay).  I'd rather not repeat that.

bug#57266: Maintaining the base_line_number cache

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Sorry, I had (and still have from time to time) enough fallout from
> similar experiments of yours around Emacs 25, which also aimed at
> improving the code (by removing various reasons to perform more
> thorough redisplay).  I'd rather not repeat that.

I guess you're talking about the `<foo>->redisplay` bits.
FWIW, the main motivation for those was not maintenance but:
- speed (in my daily use I often have a hundred frames and reducing the
  cases where the redisplay has to refresh them all made a significant
  difference to the overall responsiveness of Emacs).
- the addition of `pre-redisplay-functions` so that highlighting of a
  rectangular region could be implemented in ELisp (and at the same
  occasion, remove the ad-hoc handling of the `region` highlighting
  from the C code).


        Stefan

bug#57266: Maintaining the base_line_number cache

[Lars Ingebrigtsen]

Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> Indeed, my proposed patch is not a bug fix.
> It's a code-maintenance patch.  It's meant to improve the code, not
> the behavior.

My two cents: I was reading through parts of the newline (cache) code
last week while I was pondering whether to do the `pos-eol' functions or
not, and I have to say that I found it pretty impregnable.  So in a way
I'm relieved to learn that it's (partly) because the code doesn't make
as much sense as it should.  😀

Apparently Eli is the only person that understands the code at present,
and I understand his reluctance to change something that works.  But
making the code easier to understand would enable more people to
actually handle the code.  So I'm in favour of improving the code.

bug#57266: Maintaining the base_line_number cache

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> My two cents: I was reading through parts of the newline (cache) code
> last week while I was pondering whether to do the `pos-eol' functions or
> not, and I have to say that I found it pretty impregnable.  So in a way
> I'm relieved to learn that it's (partly) because the code doesn't make
> as much sense as it should.  😀

Side note, just for clarification: the base_line cache code is
independent/unrelated to the newline cache code (tho they deal with
things which are related, obviously).


        Stefan

bug#57266: Maintaining the base_line_number cache

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  57266@debbugs.gnu.org
> Date: Tue, 23 Aug 2022 16:21:55 +0200
> 
> Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of
> text editors" <bug-gnu-emacs@gnu.org> writes:
> 
> > Indeed, my proposed patch is not a bug fix.
> > It's a code-maintenance patch.  It's meant to improve the code, not
> > the behavior.
> 
> My two cents: I was reading through parts of the newline (cache) code
> last week while I was pondering whether to do the `pos-eol' functions or
> not, and I have to say that I found it pretty impregnable.  So in a way
> I'm relieved to learn that it's (partly) because the code doesn't make
> as much sense as it should.  😀
> 
> Apparently Eli is the only person that understands the code at present,
> and I understand his reluctance to change something that works.  But
> making the code easier to understand would enable more people to
> actually handle the code.  So I'm in favour of improving the code.

I'm also in favor of improving the code.  I just disagree with Stefan
about what constitutes "improvement" in this case.

I had my share of "TIL" experiences, where I was positive I knew how
the code should work and wrote or changed something accordingly --
only to discover that some of my concepts were completely wrong or
missed some important aspects which wound up invalidating some of the
code I wrote.  I don't think I'm alone in that.

Breaking things while fixing real bugs is justified.  But this isn't
that case.