Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

[Stefan Monnier]

>     Disable some display optimizations when frames need redisplay
>     These optimizations were previously disabled by the
>     windows_or_buffers_changed flag, which now is not set
>     when only some frames need to be redrawn.

Hmm... I only replaced things like

   windows_or_buffers_changed = 45;

with

   fset_redisplay (f);

which internally will set windows_or_buffers_changed to 2, so as long as
windows_or_buffers_changed is treated as a boolean, the code should not
be affected.

> +  /* True means we need to redraw frames whose 'redisplay' bit is set.  */
> +  bool consider_some_frames_p = false;

Why not use "windows_or_buffers_changed == REDISPLAY_SOME" as an
equivalent condition?

>    /* Build desired matrices, and update the display.  If
> -     consider_all_windows_p, do it for all windows on all frames.
> -     Otherwise do it for selected_window, only.  */
> +     consider_all_windows_p, do it for all windows on all frames.  If
> +     a frame's 'redisplay' flag is set, do it for all windows on each
> +     such frame.  Otherwise do it for selected_window, only.  */
> 
> -  if (consider_all_windows_p)
> +  if (!consider_all_windows_p)
>      {
>        FOR_EACH_FRAME (tail, frame)
> -	XFRAME (frame)->updated_p = false;
> +	{
> +	  if (XFRAME (frame)->redisplay && XFRAME (frame) != sf)
> +	    {
> +	      consider_some_frames_p = true;
> +	      break;
> +	    }
> +	}
> +    }
> +
> +  if (consider_all_windows_p || consider_some_frames_p)
> +    {
> +      FOR_EACH_FRAME (tail, frame)
> +	{
> +	  if (XFRAME (frame)->redisplay || consider_all_windows_p)
> +	    XFRAME (frame)->updated_p = false;
> +	}

I don't understand in which kind of scenario this could make
a difference.

>        && !windows_or_buffers_changed
>        && !f->cursor_type_changed
> +      && !f->redisplay

Same here: IIUC this change should make no difference, since
setting f->redisplay also sets windows_or_buffers_changed to
a non-nil value.

>        || windows_or_buffers_changed
> +      || f->redisplay

And same again here.

> +  if (windows_or_buffers_changed || f->cursor_type_changed || f->redisplay)

And here.

So, what am I missing?


        Stefan

Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Fri, 02 Oct 2015 08:44:34 -0400
> 
> So, what am I missing?

The setting of windows_or_buffers_changed in fset_redisplay can now be
removed.

Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

[Stefan Monnier]

>> So, what am I missing?
> The setting of windows_or_buffers_changed in fset_redisplay can now be
> removed.

Why would we want to do that?


        Stefan

Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

[Eli Zaretskii]

> X-Spam-Status: No, score=-0.7 required=5.0 tests=BAYES_40,RCVD_IN_DNSWL_LOW
> 	autolearn=disabled version=3.3.2
> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Fri, 02 Oct 2015 11:46:29 -0400
> 
> >> So, what am I missing?
> > The setting of windows_or_buffers_changed in fset_redisplay can now be
> > removed.
> 
> Why would we want to do that?

Because a global variable cannot possibly convey the information that
only frames F1 and F2 need to be redisplayed.  Only frame-specific
variables can do that.

IOW, when 2 out of 200 frames need to be redisplayed, there's no
reason for us to attempt redisplaying all the 200.

Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

[Stefan Monnier]

>> >> So, what am I missing?
>> > The setting of windows_or_buffers_changed in fset_redisplay can now be
>> > removed.
>> Why would we want to do that?

> Because a global variable cannot possibly convey the information that
> only frames F1 and F2 need to be redisplayed.  Only frame-specific
> variables can do that.

I know.  But the way windows_or_buffers_changed works, currently it has
3 possible values:
- 0: only the selected-window needs to be redisplayed.
- 2: all frames/windows/buffers which have the `redisplay' bit set need
  to be redisplayed
- >2: all frames&windows need to be redisplayed.

So, the value 2 (aka REDISPLAY_SOME) already gives you the information
that "only frames F1 and F2 need to be redisplayed".

> IOW, when 2 out of 200 frames need to be redisplayed, there's no
> reason for us to attempt redisplaying all the 200.

100% agreement, and that's why I introduced those `redisplay' bits and
the corresponding REDISPLAY_SOME value for windows_or_buffers_changed.

And AFAIK it already has the effect that when we call fset_redisplay only
those frames get redisplayed.

Also rather than "don't set windows_or_buffers_changed in
fset_redisplay", it seems like it would be safer to try and eliminate
(one by one) the places where we check the value of
windows_or_buffers_changed instead of checking f->redisplay or something
like that.


        Stefan

Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Fri, 02 Oct 2015 16:55:00 -0400
> 
> >> >> So, what am I missing?
> >> > The setting of windows_or_buffers_changed in fset_redisplay can now be
> >> > removed.
> >> Why would we want to do that?
> 
> > Because a global variable cannot possibly convey the information that
> > only frames F1 and F2 need to be redisplayed.  Only frame-specific
> > variables can do that.
> 
> I know.  But the way windows_or_buffers_changed works, currently it has
> 3 possible values:
> - 0: only the selected-window needs to be redisplayed.
> - 2: all frames/windows/buffers which have the `redisplay' bit set need
>   to be redisplayed
> - >2: all frames&windows need to be redisplayed.
> 
> So, the value 2 (aka REDISPLAY_SOME) already gives you the information
> that "only frames F1 and F2 need to be redisplayed".

But you need to examine the frame's 'redisplay' flag anyway.  So that
special value of windows_or_buffers_changed just adds management (you
need to reset it, and it can acquire higher values depending on what
redisplay_internal discovers), without adding any value.  The loop
over all frames checking whether any have the 'redisplay' flag set is
fast, so there's no need for a global flag to convey that.

> 100% agreement, and that's why I introduced those `redisplay' bits and
> the corresponding REDISPLAY_SOME value for windows_or_buffers_changed.
> 
> And AFAIK it already has the effect that when we call fset_redisplay only
> those frames get redisplayed.

Maybe so, but the effect of this on frames is completely undocumented,
and the name REDISPLAY_SOME has no mnemonic value that helps realize
its meaning.  Also, the fact that fset_redisplay, a setter function,
also sets windows_or_buffers_changed (and then does some more) is IMO
not a good idea, as it gets in the way of understanding the logic
without reading all the functions that are called, just by looking at
their names.

Anyway, I wish all this was written somewhere, rather than divulged in
a discussion of some commit.  The fact that you never documented these
parts in the code, and I never until now knew them, is regrettable, to
say the least.

> Also rather than "don't set windows_or_buffers_changed in
> fset_redisplay", it seems like it would be safer to try and eliminate
> (one by one) the places where we check the value of
> windows_or_buffers_changed instead of checking f->redisplay or something
> like that.

You cannot eliminate windows_or_buffers_changed without replacing them
with something.  They are there for a reason.  It could be that the
effect of having that variable non-zero is too radical, and causes too
thorough redisplay, but then the solution is surely not elimination.

Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

[Stefan Monnier]

>> So, the value 2 (aka REDISPLAY_SOME) already gives you the information
>> that "only frames F1 and F2 need to be redisplayed".
> But you need to examine the frame's 'redisplay' flag anyway.

Which frame(s)?  If windows_or_buffers_changed is 0, you shouldn't need
to examine any frame's `redisplay' flag, AFAIK.

> So that special value of windows_or_buffers_changed just adds
> management (you need to reset it, and it can acquire higher values
> depending on what redisplay_internal discovers), without adding any
> value.

I'm not sure how much there is to gain from removing it, since it's also
used for wset_redisplay and bset_redisplay.  But it sounds like you have
a plan.

> The loop over all frames checking whether any have the 'redisplay'
> flag set is fast, so there's no need for a global flag to convey that.

Indeed, such a loop is cheap.  But as long as windows_or_buffers_changed
is used for the wset_redisplay and bset_redisplay, I don't see much
benefit in removing it from fset_redisplay.

But of course, looping over all windows is also fairly cheap, so I guess
you're planning to remove it also from wset_redisplay and bset_redisplay?

>> 100% agreement, and that's why I introduced those `redisplay' bits and
>> the corresponding REDISPLAY_SOME value for windows_or_buffers_changed.
>> 
>> And AFAIK it already has the effect that when we call fset_redisplay only
>> those frames get redisplayed.

> Maybe so, but the effect of this on frames is completely undocumented,

I tried to document it in those var's comment:

   /* Nonzero if we should redraw the mode lines on the next redisplay.
      If it has value REDISPLAY_SOME, then only redisplay the mode lines where
      the `redisplay' bit has been set.  Otherwise, redisplay all mode lines
      (the number used is then only used to track down the cause for this
      full-redisplay).  */

   int update_mode_lines;

   /* Nonzero if window sizes or contents other than selected-window have
      changed since last redisplay that finished.
      If it has value REDISPLAY_SOME, then only redisplay the windows where
      the `redisplay' bit has been set.  Otherwise, redisplay all windows
      (the number used is then only used to track down the cause for this
      full-redisplay).  */

   int windows_or_buffers_changed;

I see it's not very specific about frame/window/buffer's `redisplay'
flag, tho.

>> Also rather than "don't set windows_or_buffers_changed in
>> fset_redisplay", it seems like it would be safer to try and eliminate
>> (one by one) the places where we check the value of
>> windows_or_buffers_changed instead of checking f->redisplay or something
>> like that.
> You cannot eliminate windows_or_buffers_changed without replacing them
> with something.

I lost you here.  I thought you're suggesting to get rid of this
variable.  I'm only suggesting to remove (step by step) some of those
places where we check its value and I'm not suggesting to remove them
without replacing them with something, but rather I suggest to replace
those checks by checks of things like f->redisplay.


        Stefan

Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Fri, 02 Oct 2015 21:05:13 -0400
> 
> > So that special value of windows_or_buffers_changed just adds
> > management (you need to reset it, and it can acquire higher values
> > depending on what redisplay_internal discovers), without adding any
> > value.
> 
> I'm not sure how much there is to gain from removing it, since it's also
> used for wset_redisplay and bset_redisplay.  But it sounds like you have
> a plan.
> 
> > The loop over all frames checking whether any have the 'redisplay'
> > flag set is fast, so there's no need for a global flag to convey that.
> 
> Indeed, such a loop is cheap.  But as long as windows_or_buffers_changed
> is used for the wset_redisplay and bset_redisplay, I don't see much
> benefit in removing it from fset_redisplay.
> 
> But of course, looping over all windows is also fairly cheap, so I guess
> you're planning to remove it also from wset_redisplay and bset_redisplay?

Maybe, I don't know yet.  I intended to, but now I think I need to
digest the new knowledge first.

> >> 100% agreement, and that's why I introduced those `redisplay' bits and
> >> the corresponding REDISPLAY_SOME value for windows_or_buffers_changed.
> >> 
> >> And AFAIK it already has the effect that when we call fset_redisplay only
> >> those frames get redisplayed.
> 
> > Maybe so, but the effect of this on frames is completely undocumented,
> 
> I tried to document it in those var's comment:
> 
>    /* Nonzero if we should redraw the mode lines on the next redisplay.
>       If it has value REDISPLAY_SOME, then only redisplay the mode lines where
>       the `redisplay' bit has been set.  Otherwise, redisplay all mode lines
>       (the number used is then only used to track down the cause for this
>       full-redisplay).  */
> 
>    int update_mode_lines;
> 
>    /* Nonzero if window sizes or contents other than selected-window have
>       changed since last redisplay that finished.
>       If it has value REDISPLAY_SOME, then only redisplay the windows where
>       the `redisplay' bit has been set.  Otherwise, redisplay all windows
>       (the number used is then only used to track down the cause for this
>       full-redisplay).  */
> 
>    int windows_or_buffers_changed;
> 
> I see it's not very specific about frame/window/buffer's `redisplay'
> flag, tho.

Indeed.

Tell you what: if you add commentary that describes the logic of using
these two variables, the window's and frame's 'redisplay' flags, and
how all those are used to decide which parts need to be redisplayed,
and if that commentary makes it clear that the code I added is
redundant, I will remove the code I added, and see if some alternative
design would be better.  Deal?

Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

[Stefan Monnier]

> Tell you what: if you add commentary that describes the logic of using
> these two variables, the window's and frame's 'redisplay' flags, and
> how all those are used to decide which parts need to be redisplayed,
> and if that commentary makes it clear that the code I added is
> redundant, I will remove the code I added, and see if some alternative
> design would be better.  Deal?

How do you like the patch below?


        Stefan


diff --git a/src/xdisp.c b/src/xdisp.c
index 1524783..439d9cb 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -434,22 +434,47 @@ static Lisp_Object Vmessage_stack;
 
 static bool message_enable_multibyte;
 
-/* Nonzero if we should redraw the mode lines on the next redisplay.
-   If it has value REDISPLAY_SOME, then only redisplay the mode lines where
-   the `redisplay' bit has been set.  Otherwise, redisplay all mode lines
-   (the number used is then only used to track down the cause for this
-   full-redisplay).  */
+/* At each redisplay cycle, we should refresh everything there is to refresh.
+   To do that efficiently, we use many optimizations that try to make sure we
+   don't waste too much time updating things that haven't changed.
+   The coarsest such optimization is that, in the most common cases, we only
+   look at the selected-window.
+
+   To know whether other windows should be considered for redisplay, we use the
+   variable windows_or_buffers_changed: as long as it is 0, it means that we
+   have not noticed anything that should require updating anything else than
+   the selected-window.  If it is set to REDISPLAY_SOME, it means that since
+   last redisplay, some changes have been made which could impact other
+   windows.  To know which ones need redisplay, every buffer, window, and frame
+   has a `redisplay' bit, which (if true) means that this object needs to be
+   redisplayed.  If windows_or_buffers_changed is 0, we know there's no point
+   looking for those `redisplay' bits (actually, there might be some such bits
+   set, but then only on objects which aren't displayed anyway).
+
+   Mostly for historical reasons, windows_or_buffers_changed can also take
+   other non-zero values.  In that case, the precise value doesn't matter (it
+   encodes the cause of the setting but is only used for debugging purposes),
+   and what it means is that we shouldn't pay attention to any `redisplay' bits
+   and we should simply try and redisplay every since window out there.  */
 
-int update_mode_lines;
+int windows_or_buffers_changed;
 
-/* Nonzero if window sizes or contents other than selected-window have changed
-   since last redisplay that finished.
-   If it has value REDISPLAY_SOME, then only redisplay the windows where
-   the `redisplay' bit has been set.  Otherwise, redisplay all windows
-   (the number used is then only used to track down the cause for this
-   full-redisplay).  */
+/* Nonzero if we should redraw the mode lines on the next redisplay.
+   Similarly to `windows_or_buffers_changed', If it has value REDISPLAY_SOME,
+   then only redisplay the mode lines in those buffers/windows/frames where the
+   `redisplay' bit has been set.
+   For any other value, redisplay all mode lines (the number used is then only
+   used to track down the cause for this full-redisplay).
+
+   The `redisplay' bits are the same as those used for
+   windows_or_buffers_changed, and setting windows_or_buffers_changed also
+   causes recomputation of the mode lines of all those windows.  IOW this
+   variable only has an effect if windows_or_buffers_changed is zero, in which
+   case we should only need to redisplay the mode-line of those objects with
+   a `redisplay' bit set but not the window's text content (tho we may still
+   need to refresh the text content of the selected-window).  */
 
-int windows_or_buffers_changed;
+int update_mode_lines;
 
 /* True after display_mode_line if %l was used and it displayed a
    line number.  */

Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Sat, 03 Oct 2015 22:33:06 -0400
> 
> > Tell you what: if you add commentary that describes the logic of using
> > these two variables, the window's and frame's 'redisplay' flags, and
> > how all those are used to decide which parts need to be redisplayed,
> > and if that commentary makes it clear that the code I added is
> > redundant, I will remove the code I added, and see if some alternative
> > design would be better.  Deal?
> 
> How do you like the patch below?

I like it very much, thanks!  Please commit.

Bonus points for adding something that describes where these variables
and the corresponding 'redisplay' flags are checked, because the
FOR_EACH_FRAME loop in redisplay_internal doesn't look at the
'redisplay' flags, so a large part of the logic you describe above
isn't (easily) visible at that level.

Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

[Stefan Monnier]

> I like it very much, thanks!  Please commit.

Thanks, installed.

> Bonus points for adding something that describes where these variables
> and the corresponding 'redisplay' flags are checked, because the
> FOR_EACH_FRAME loop in redisplay_internal doesn't look at the
> 'redisplay' flags, so a large part of the logic you describe above
> isn't (easily) visible at that level.

I added a note about the fact that the frame's redisplay flag is not
sufficient to exclude a frame.

Fundamentally, the only redisplay flags needed are the window ones.
The frame and buffer's redisplay flags could be dispensed with, at the
cost of making [bf]set_redisplay more expensive since they'd have to
loop through all the windows of that frame/buffer.


        Stefan

Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Mon, 05 Oct 2015 11:50:03 -0400
> 
> > Bonus points for adding something that describes where these variables
> > and the corresponding 'redisplay' flags are checked, because the
> > FOR_EACH_FRAME loop in redisplay_internal doesn't look at the
> > 'redisplay' flags, so a large part of the logic you describe above
> > isn't (easily) visible at that level.
> 
> I added a note about the fact that the frame's redisplay flag is not
> sufficient to exclude a frame.

Thanks.

> Fundamentally, the only redisplay flags needed are the window ones.
> The frame and buffer's redisplay flags could be dispensed with, at the
> cost of making [bf]set_redisplay more expensive since they'd have to
> loop through all the windows of that frame/buffer.

Yes, I agree.  But is that justified?  At least for the frame's flag,
a test for each window of the frame's window tree has the same cost as
setting the window-specific flag in fset_redisplay.  However, the test
is made only once per redisplay cycle, whereas the setting could be
done several times, depending on what Lisp is being run.  Right?

Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

[Stefan Monnier]

>> Fundamentally, the only redisplay flags needed are the window ones.
>> The frame and buffer's redisplay flags could be dispensed with, at the
>> cost of making [bf]set_redisplay more expensive since they'd have to
>> loop through all the windows of that frame/buffer.
> Yes, I agree.  But is that justified?

I did not make any measurements, I only went with my intuition.

> At least for the frame's flag, a test for each window of the frame's
> window tree has the same cost as setting the window-specific flag in
> fset_redisplay.

Right.  Tho we loop through all the frame's windows in the redisplay
anyway (IOW, I reused pre-existing loops).

> However, the test is made only once per redisplay cycle, whereas the
> setting could be done several times, depending on what Lisp is being
> run.  Right?

I think for fset_frame either way would work about as well: it should be
relatively rarely used anyway.  OTOH for bset_redisplay, I expect that the
cost of finding all the buffer's windows would be clearly too costly.


        Stefan

Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

[Stefan Monnier]

> cost of finding all the buffer's windows would be clearly too costly.

Well, it's not really that the cost would too costly, but that it would
be too high.


        Stefan "sorry"

Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

[martin rudalics]

 > I think for fset_frame either way would work about as well: it should be
 > relatively rarely used anyway.  OTOH for bset_redisplay, I expect that the
 > cost of finding all the buffer's windows would be clearly too costly.

Nothing should prevent us from replacing window_count with a list of the
windows showing the buffer.

martin

Re: [Emacs-diffs] master 5c9304e: Disable some display optimizations when frames need redisplay

[Stefan Monnier]

>> I think for fset_frame either way would work about as well: it should be
>> relatively rarely used anyway.  OTOH for bset_redisplay, I expect that the
>> cost of finding all the buffer's windows would be clearly too costly.
> Nothing should prevent us from replacing window_count with a list of the
> windows showing the buffer.

Sure, but I suspect calls to bset_redisplay can be much more numerous,
so I think it's much cheaper to have redisplay check b->text->redisplay
once per redisplay, than to have bset_redisplay loop through the
corresponding windows every time (even if this looping can indeed be
made more efficient if needed).

In any case, these choices don't seem particularly important, and
changing them wouldn't make much difference to the code.  So feel free
to try alternatives if you want, but personally, I think it'd be a waste
of time since the current solution is very cheap.


        Stefan