Re: emacs-27 d7a4cea: ; Add a new item to TODO

Re: emacs-27 d7a4cea: ; Add a new item to TODO

[Stefan Monnier]

> +** Make redisplay smarter about which parts to redraw
> +Currently, redisplay has only 2 levels of redrawing: either it
> +redisplays only the selected window on the selected frame, or it
> +redisplays all the windows on all the frames.  This doesn't scale well
> +when the number of visible frames is large.

I don't think that's true: the change I installed some years back (along
with the `pre-redisplay-function` and its use for the region highlight)
does specifically try and select a particular subset of frames/windows.
That's what the `wset_redisplay`, `fset_redisplay` and `bset_redisplay`
are about.

And indeed `pre-redisplay-function` is passed the specific set of
windows that have been found to need a redisplay and this set can be
much more varied than what you describe above.

> +Currently, two variables are used to make the decision what to
> +redisplay: update_mode_lines and windows_or_buffers_changed.

There's also a `redisplay` "bit" on frames, windows, and buffer
text objects.

> +One way of making this change is to go through all the places that set
> +update_mode_lines and windows_or_buffers_changed, figure out which
> +portions of the Emacs display could be affected by each change, and
> +then implement the bitmap which will record each of these affected
> +display portions.  The logic in redisplay_internal will then need to
> +be restructured so as to support this fine-grained redisplay.

Indeed, that's very useful and I did use exactly this approach back then
(and as the rest of your text describes, there are more opportunities).
In order to make this easier I changed the code such that every
assignment to those two variable sets it to a different value, and then
in at every redisplay cycle are record which value is found by
increasing the counter in the corresponding slot of
`redisplay--all-windows-cause` and `redisplay--mode-lines-cause`.


        Stefan

Re: emacs-27 d7a4cea: ; Add a new item to TODO

[Stefan Monnier]

> in at every redisplay cycle are record which value is found by
> increasing the counter in the corresponding slot of
> `redisplay--all-windows-cause` and `redisplay--mode-lines-cause`.

For example, in my currently running Emacs session, I see:

redisplay--all-windows-cause:
#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (31 631 0 205211 2 35619 52 388 19 178))
redisplay--mode-lines-cause:
#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (12 789 15 29 0 204962 23 644 2 34019 17 18 18 13 14 302 42 563 11 738 32 410 10 26))

So, in about 200k cases, the "cause" for redisplay was 0 meaning that
Emacs only redisplayed the selected window.  In about 35k cases, the
"cause" was 2, meaning that Emacs considered more than just the selected
frame, but did not consider all windows&frames (instead it only
considered those windows&frames with the `redisplay` bit set).

Of the remaining cases (where it did consider all windows&frames), 631
were due to "windows_or_buffers_changed == 31" which

    grep windows_or_buffers_changed.\*31 src/*.c

shows is due to a call to `set_window_scroll_bars`.
And 789 were due to update_mode_lines == 12 which comes from
kill-all-local-variables.

Both of those cases seem easy to fix with the patch below which I just
pushed to master.

Of course, as more and more code uses `[fbw]set_redisplay` instead of
just setting `windows_or_buffers_changed`, those two above
`redisplay--*-cause` variables lose some of their usefulness, because
when the cause is 2 we may still be redisplaying too much.


        Stefan


diff --git a/src/buffer.c b/src/buffer.c
index 4215acbf1d..dfc34faf6e 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -2814,7 +2814,7 @@ DEFUN ("kill-all-local-variables", Fkill_all_local_variables,
 
   /* Force mode-line redisplay.  Useful here because all major mode
      commands call this function.  */
-  update_mode_lines = 12;
+  bset_update_mode_line (current_buffer);
 
   return Qnil;
 }
diff --git a/src/window.c b/src/window.c
index 4eab786958..bcc989b5a7 100644
--- a/src/window.c
+++ b/src/window.c
@@ -7822,7 +7822,7 @@ set_window_scroll_bars (struct window *w, Lisp_Object width,
 	 if more than a single window needs to be considered, see
 	 redisplay_internal.  */
       if (changed)
-	windows_or_buffers_changed = 31;
+	wset_redisplay (w);
 
       return changed ? w : NULL;
     }

Re: emacs-27 d7a4cea: ; Add a new item to TODO

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Thu, 17 Dec 2020 11:22:49 -0500
> 
> > +** Make redisplay smarter about which parts to redraw
> > +Currently, redisplay has only 2 levels of redrawing: either it
> > +redisplays only the selected window on the selected frame, or it
> > +redisplays all the windows on all the frames.  This doesn't scale well
> > +when the number of visible frames is large.
> 
> I don't think that's true: the change I installed some years back (along
> with the `pre-redisplay-function` and its use for the region highlight)
> does specifically try and select a particular subset of frames/windows.
> That's what the `wset_redisplay`, `fset_redisplay` and `bset_redisplay`
> are about.

See bug#42406.

My personal experience is that fset_redisplay and wset_redisplay are
not "strong" enough to cause redisplay in many situations, so we need
to use those 2 variables to enforce more thorough redisplay.  In
particular, there are the cases where elements of display may need to
be redrawn that don't belong to any window.

> > +Currently, two variables are used to make the decision what to
> > +redisplay: update_mode_lines and windows_or_buffers_changed.
> 
> There's also a `redisplay` "bit" on frames, windows, and buffer
> text objects.

They are not very useful, and the problematic cases don't involve them
anyway.

> In order to make this easier I changed the code such that every
> assignment to those two variable sets it to a different value, and then
> in at every redisplay cycle are record which value is found by
> increasing the counter in the corresponding slot of
> `redisplay--all-windows-cause` and `redisplay--mode-lines-cause`.

That's useful when debugging excess redisplays, and can also help in
analyzing the use cases to decide on the bits.  But we need to go
further than that, IMO, because the current logic is still quite
simplistic.

Re: emacs-27 d7a4cea: ; Add a new item to TODO

[Stefan Monnier]

>> > +** Make redisplay smarter about which parts to redraw
>> > +Currently, redisplay has only 2 levels of redrawing: either it
>> > +redisplays only the selected window on the selected frame, or it
>> > +redisplays all the windows on all the frames.  This doesn't scale well
>> > +when the number of visible frames is large.
>> 
>> I don't think that's true: the change I installed some years back (along
>> with the `pre-redisplay-function` and its use for the region highlight)
>> does specifically try and select a particular subset of frames/windows.
>> That's what the `wset_redisplay`, `fset_redisplay` and `bset_redisplay`
>> are about.
>
> See bug#42406.

Thanks.

> My personal experience is that fset_redisplay and wset_redisplay are
> not "strong" enough to cause redisplay in many situations, so we need
> to use those 2 variables to enforce more thorough redisplay.  In
> particular, there are the cases where elements of display may need to
> be redrawn that don't belong to any window.

I fully agree.  But the above paragraph you installed is factually
incorrect, so anyone who reads this TODO and wants to act on it will
start with an incorrect understanding of the problem at hand and may
even give up right away seeing code like `bset_redisplay` and thinking
that the problem has already been solved.

> That's useful when debugging excess redisplays, and can also help in
> analyzing the use cases to decide on the bits.  But we need to go
> further than that, IMO, because the current logic is still quite
> simplistic.

No argument from me on that ;-)


        Stefan

Re: emacs-27 d7a4cea: ; Add a new item to TODO

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Thu, 17 Dec 2020 12:51:09 -0500
> 
> I fully agree.  But the above paragraph you installed is factually
> incorrect, so anyone who reads this TODO and wants to act on it will
> start with an incorrect understanding of the problem at hand and may
> even give up right away seeing code like `bset_redisplay` and thinking
> that the problem has already been solved.

Feel free to make that text more accurate, sure.  As long as we agree
that the problem exists, I see no need to argue.  The relevant code is
complex enough for me to welcome any insights from anyone, certainly
from you.  I don't claim to have a 100% clear vision of everything
that's going on with these flags.  I write that text so that we don't
forget the problem, and so that maybe someone will want to work on
this, and make the maintenance easier.