From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
To: 57266@debbugs.gnu.org
Subject: bug#57266: Maintaining the base_line_number cache
Date: Wed, 17 Aug 2022 17:18:44 -0400 [thread overview]
Message-ID: <jwvzgg2zhzf.fsf@iro.umontreal.ca> (raw)
[-- 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);
}
next reply other threads:[~2022-08-17 21:18 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-17 21:18 Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors [this message]
[not found] ` <handler.57266.B.166077115022973.ack@debbugs.gnu.org>
2022-08-17 21:39 ` bug#57266: Acknowledgement (Maintaining the base_line_number cache) Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-08-18 6:03 ` bug#57266: Maintaining the base_line_number cache Eli Zaretskii
2022-08-22 4:34 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-08-22 12:11 ` Eli Zaretskii
2022-08-22 12:41 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-08-22 13:07 ` Eli Zaretskii
2022-08-22 13:32 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-08-22 16:21 ` Eli Zaretskii
2022-08-22 18:02 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-08-22 18:35 ` Eli Zaretskii
2022-08-22 19:57 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-08-23 14:21 ` Lars Ingebrigtsen
2022-08-23 14:40 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-08-23 15:56 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=jwvzgg2zhzf.fsf@iro.umontreal.ca \
--to=bug-gnu-emacs@gnu.org \
--cc=57266@debbugs.gnu.org \
--cc=monnier@iro.umontreal.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).