bug#57266: Maintaining the base_line_number cache

[Eli Zaretskii <eliz@gnu.org>]

> 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.