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.