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#64596: 30.0.50; On FIXME: in src/buffer.c:1481 (force-mode-line-update) Date: Fri, 14 Jul 2023 17:20:30 +0300 Message-ID: <838rbifuc1.fsf@gnu.org> References: <877cr4nez9.fsf@localhost> <83v8eo3pfv.fsf@gnu.org> <87bkgeiuap.fsf@localhost> <83fs5qfznv.fsf@gnu.org> <87351qioxw.fsf@localhost> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="30980"; mail-complaints-to="usenet@ciao.gmane.io" Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org To: Ihor Radchenko Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Jul 14 16:21:39 2023 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 1qKJfv-0007oH-Ej for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 14 Jul 2023 16:21:39 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qKJfO-0003if-CE; Fri, 14 Jul 2023 10:21:06 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qKJfM-0003gc-DE for bug-gnu-emacs@gnu.org; Fri, 14 Jul 2023 10:21:04 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qKJfL-00040V-GN for bug-gnu-emacs@gnu.org; Fri, 14 Jul 2023 10:21:04 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qKJfL-0002yb-2c for bug-gnu-emacs@gnu.org; Fri, 14 Jul 2023 10:21:03 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 14 Jul 2023 14:21:03 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 64596 X-GNU-PR-Package: emacs Original-Received: via spool by 64596-submit@debbugs.gnu.org id=B64596.168934443211340 (code B ref 64596); Fri, 14 Jul 2023 14:21:03 +0000 Original-Received: (at 64596) by debbugs.gnu.org; 14 Jul 2023 14:20:32 +0000 Original-Received: from localhost ([127.0.0.1]:43136 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qKJep-0002wq-A0 for submit@debbugs.gnu.org; Fri, 14 Jul 2023 10:20:31 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:41400) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qKJem-0002wU-Fb for 64596@debbugs.gnu.org; Fri, 14 Jul 2023 10:20:29 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qKJec-0003uV-Rb; Fri, 14 Jul 2023 10:20:20 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=uKZp7mLtK9v/yC6q7BDLfNgrdR6wOAQkpDbEOKgOMec=; b=pPePg8hxrND0 m3O5kUdyFP3qpgNWZLSE/pxqRwjR5A4TevevawcIbEuy8JAFtW0TdTDPzd9dCcEPMWAOU4dHOj6lA UPyirjRxWsjSkQPzZTu/ZHezVsQWP0bJ6WJpIwojEgBRuJeQTU56Fkon+J7vzvk4UUMvYYYPwNgnE zm8rbWDvNPEbAjDt7WGdt8Z3YJrZPm9jKk6Rr8MLhOl8ZRNehrMMmvF8QJiL8vG6kbjzWz2loluX8 KWeiNESjMDJsO+jHhB7qCyiNt6Kz5XCejjFRRc8/XS4mbJllq24k2P52dwdW/wY2Ngq9RNqueflyK xYXgeJ6LTxtwgBBbuT+SUg==; Original-Received: from [87.69.77.57] (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 1qKJeX-0001bG-BI; Fri, 14 Jul 2023 10:20:18 -0400 In-Reply-To: <87351qioxw.fsf@localhost> (message from Ihor Radchenko on Fri, 14 Jul 2023 13:48:43 +0000) 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-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:265098 Archived-At: > From: Ihor Radchenko > Cc: monnier@iro.umontreal.ca, 64596@debbugs.gnu.org > Date: Fri, 14 Jul 2023 13:48:43 +0000 > > Eli Zaretskii writes: > > >> I'm afraid that the very existence of prevent_redisplay_optimizations_p > >> flag is a mistake hiding bugs with redisplay optimizations logic. > > > > I think you should avoid making such comments until you have a better > > understanding of the redisplay logic, or else I will stop taking your > > posts in this matter seriously. > > Your answer indicates that I missed something important after reading > the top comment in xdisp.c, redisplay_internal, redisplay_window, and > skimmed through a couple of other functions. But I still cannot figure > out what. Keep studying the code, is all I can suggest. There's much more there than meets the eye, IME. > Note that I did not mean that previous committers had bad intentions. I > just think that the idea of having "disable all optimizations" flag is > causing the code quality that could be otherwise somewhat better (at > least, more explicit about why redisplay should take long path). I agree that the underlying logic is somewhat subtle, and sometimes even mysterious, and I suggested a way to make this logic more explicit and better documented. But removing parts of display code because we don't always understand it is a bad idea, IME, and usually leads to bugs. So it is a non-starter, as far as I'm concerned. > I think that replacing all the instances of setting this flag to more > explicit alternatives would improve the display performance and also > readability. It's possible. But to consider such suggestions, I'd need to see a detailed proposal backed up by serious analysis. In any case, whether it will make performance better is not yet clear; the original motivation for this was not a performance issue at all. > > /* Optimize the case that only the line containing the cursor in the > > selected window has changed. > > > > The prevent_redisplay_optimizations_p flag says, among other things, > > that we are not in that case, and it is tested early enough in order > > to prevent us from examining additional conditions which are just > > waste of CPU cycles (and some of them are relatively expensive). > > Sure, but it is a very generic way to say this. It provides no detailed > reason what exactly changed in the buffer/window/frame that makes us > bypass this optimization. What detailed reasons would you like to see? > Is it not always possible to use the other, more specific, existing > flags for redisplay to indicate that something important has been changed? Which other flags? And why do you think they are more specific? At least some of them are actually _less_ specific. > >> So, I see not why calling bset_update_mode_line is not sufficient to > >> force mode line update in all windows associated with a single buffer. > > > > The mode line displays quite a lot of information. It could also > > display information we don't know about (on the level of the redisplay > > code), because mode-line-format supports :eval, which can execute any > > Lisp. Therefore, there could be changes to the buffer that affect the > > mode line indirectly. > > But isn't :eval processed by display_mode_lines? Once we take care to call > bset_update_mode_line, display_mode_lines is guaranteed to be executed, > AFAIU. Executed, but with what data? redisplay_window is a large function, and employs quite a few of shortcuts. If one of those shortcuts is taken when it shouldn't be, the data shown on the mode line might be inaccurate, because some of the variables affecting it were not updated. But let me turn the table and ask you: suppose that the prevent_redisplay_optimizations_p flag is not needed for updating the mode line per se -- what damage could it possibly do to redisplaying the mode line? why are you so eager to remove that setting from force-mode-line-update? > > One problem of this kind which we had relatively recently is when the > > changes in mode-line-format or some of its :eval forms yields a mode > > line that is significantly taller or smaller than the previously > > displayed one. Such changes in the mode-line height generally affect > > the window(s) as well, not just the mode line itself. > > Sure, and redisplay_window accounts for this when calling display_mode_lines: > > display_mode_lines (w); > unbind_to (count1, Qnil); > > /* If mode line height has changed, arrange for a thorough > immediate redisplay using the correct mode line height. */ > if (window_wants_mode_line (w) > && CURRENT_MODE_LINE_HEIGHT (w) != DESIRED_MODE_LINE_HEIGHT (w)) > { > f->fonts_changed = true; > w->mode_line_height = -1; > MATRIX_MODE_LINE_ROW (w->current_matrix)->height > = DESIRED_MODE_LINE_HEIGHT (w); > } And if you've read the relevant bug reports, this doesn't always work. But this is just an example, so even if you consider it to be solved, it tells us that there can be other issues like that. We are trying, slowly, to go from general flags to more specific ones, but we are not there yet, and removing code in this situation will just produce more buggy redisplay. I cannot agree to that. > > Moreover, you will see in the wild that force-mode-line-update is used > > not just to update the mode line, but also to force a more thorough > > redisplay of one or more windows. > > Why not `force-window-update'? Why does it matter? they both do almost the same (when the argument to force-window-update is a window). > And I now looked deeper into the code, and see no obvious downsides of > removing current_buffer->prevent_redisplay_optimizations_p = true; from > force-mode-line-update specifically. > `set-buffer-modified-p' may need to be re-considered though as it is the > original place where setting prevent_redisplay_optimizations_p was done > (for different reasons). Again, removing this is a non-starter, except if you want to do it locally. If we want to make any progress in this area in general, we should take one of the two approaches I described in my previous message. Anything else is simply irresponsible, and won't happen on my watch, sorry.