From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#57266: Maintaining the base_line_number cache Date: Thu, 18 Aug 2022 09:03:37 +0300 Message-ID: <831qteccli.fsf@gnu.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="33677"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 57266@debbugs.gnu.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Aug 18 08:04:21 2022 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oOYdg-0008ab-RD for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 18 Aug 2022 08:04:21 +0200 Original-Received: from localhost ([::1]:47548 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oOYdf-0001F3-3n for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 18 Aug 2022 02:04:19 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:54550) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oOYdO-0001EY-5o for bug-gnu-emacs@gnu.org; Thu, 18 Aug 2022 02:04:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:35508) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1oOYdN-0005mS-TC for bug-gnu-emacs@gnu.org; Thu, 18 Aug 2022 02:04:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1oOYdN-0002cu-NU for bug-gnu-emacs@gnu.org; Thu, 18 Aug 2022 02:04:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 18 Aug 2022 06:04:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 57266 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 57266-submit@debbugs.gnu.org id=B57266.166080263210079 (code B ref 57266); Thu, 18 Aug 2022 06:04:01 +0000 Original-Received: (at 57266) by debbugs.gnu.org; 18 Aug 2022 06:03:52 +0000 Original-Received: from localhost ([127.0.0.1]:53490 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oOYdD-0002cU-LP for submit@debbugs.gnu.org; Thu, 18 Aug 2022 02:03:52 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:45828) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oOYdC-0002cH-7m for 57266@debbugs.gnu.org; Thu, 18 Aug 2022 02:03:50 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:36128) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oOYd6-0005kE-SD; Thu, 18 Aug 2022 02:03:44 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=MIME-version:References:Subject:In-Reply-To:To:From: Date; bh=WuNDSH5COSqZsarXtl6UGvpH3yJHSePDR6mImyvogHM=; b=Vzm0kDxiVsuUSsfwVcF+ kDCGwqFv6D7K8PlojXj3OuMJcsT6rfBzDXovaPy2Xj9tjK9znsfsxeUal+yCMJ7HUQ2VrB3YgNg9R YIdWQLwwEGkBqzVpSAq/jls+R8iUXp0+VWdUNA9SGWM9oWJslRuPDgRpm/w/FBefPMmmvbdgiLBiW lrG89zKPZsEoubji1vEnpWkXbF5Z+F/qjbKBCW32qpWfTWb7I+QOkhOAdt4JPX/XNs/0kG9aITnr0 RX/sw27x/xNdIkU5t0khViuUkZHeOyHwFViD+wTfm+HNPINgxJlz72jsVmviEBkxW45jphAqlA7dg fwLfp5SwffSd8w==; Original-Received: from [87.69.77.57] (port=4077 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oOYd6-0003ac-Ah; Thu, 18 Aug 2022 02:03:44 -0400 In-Reply-To: (bug-gnu-emacs@gnu.org) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:240126 Archived-At: > 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" > > 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.