bug#61779: 30.0.50; excessive redisplay when the mini window is active

bug#61779: 30.0.50; excessive redisplay when the mini window is active

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

For some reason redisplay_internal will always update the mini window
even if its buffer contents have not changed.  I guess there is probably
a missing optimization wrt to updating the echo area contents, and
window_outdated does not take the echo area message into account:

	  && (CHARPOS (tlbufpos) == ZV
	      || FETCH_BYTE (BYTEPOS (tlbufpos)) == '\n'))
	/* Former continuation line has disappeared by becoming empty.  */
	goto cancel;
      else if (window_outdated (w) || MINI_WINDOW_P (w)) <==============
	{
	  /* We have to handle the case of continuation around a
	     wide-column character (see the comment in indent.c around
	     line 1340).

I am not sure that is why, however, so I can't just change that.
Anyway, update_window_line does not draw anything since the contents of
the glyph row do not change after redisplay, so that part is fine.  The
problem is that the redisplay will lead to update_window being called,
and gui_update_window_end will always call display_and_set_cursor
afterwards to display the cursor, which is not okay, as it generates a
lot of X protocol traffic when redisplay is called and redisplays the
mini-window.  Now add to that a process generating a lot of output, and
by doing so, calls to redisplay_preserve_echo_area, and you have an
absurd number of X requests, enough to saturate any reasonable network
connection.

Here is a patch to resolve the problem by not redrawing the cursor when
Emacs knows it is already there.  I've not seen any ill effects so far,
but the comment originally there seems to know better, so what are the
cases where ``phys_cursor_on_p is 1 but the cursor has been erased''?

Thanks.

diff --git a/src/xdisp.c b/src/xdisp.c
index c5c4321af77..39568bb6b1f 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -33333,12 +33333,21 @@ display_and_set_cursor (struct window *w, bool on,
 	      && new_cursor_width != w->phys_cursor_width)))
     erase_phys_cursor (w);
 
-  /* Don't check phys_cursor_on_p here because that flag is only set
-     to false in some cases where we know that the cursor has been
-     completely erased, to avoid the extra work of erasing the cursor
-     twice.  In other words, phys_cursor_on_p can be true and the cursor
-     still not be visible, or it has only been partly erased.  */
-  if (on)
+  /* XXX: this previously read:
+
+       Don't check phys_cursor_on_p here because that flag is only set
+       to false in some cases where we know that the cursor has been
+       completely erased, to avoid the extra work of erasing the
+       cursor twice.  In other words, phys_cursor_on_p can be true and
+       the cursor still not be visible, or it has only been partly
+       erased.
+
+     However, not checking this flag results in too much cursor redraw
+     if the mini-window is being redisplayed, since process output
+     results in calls to redisplay_internal, which always redisplays
+     the mini window regardless of whether or not its contents have
+     changed.  */
+  if (on && !w->phys_cursor_on_p)
     {
       w->phys_cursor_ascent = glyph_row->ascent;
       w->phys_cursor_height = glyph_row->height;

bug#61779: 30.0.50; excessive redisplay when the mini window is active

[Eli Zaretskii]

> Date: Sat, 25 Feb 2023 14:16:37 +0800
> From:  Po Lu via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> For some reason redisplay_internal will always update the mini window
> even if its buffer contents have not changed.  I guess there is probably
> a missing optimization wrt to updating the echo area contents, and
> window_outdated does not take the echo area message into account:
> 
> 	  && (CHARPOS (tlbufpos) == ZV
> 	      || FETCH_BYTE (BYTEPOS (tlbufpos)) == '\n'))
> 	/* Former continuation line has disappeared by becoming empty.  */
> 	goto cancel;
>       else if (window_outdated (w) || MINI_WINDOW_P (w)) <==============
> 	{
> 	  /* We have to handle the case of continuation around a
> 	     wide-column character (see the comment in indent.c around
> 	     line 1340).
> 
> I am not sure that is why, however, so I can't just change that.
> Anyway, update_window_line does not draw anything since the contents of
> the glyph row do not change after redisplay, so that part is fine.  The
> problem is that the redisplay will lead to update_window being called,
> and gui_update_window_end will always call display_and_set_cursor
> afterwards to display the cursor, which is not okay, as it generates a
> lot of X protocol traffic when redisplay is called and redisplays the
> mini-window.  Now add to that a process generating a lot of output, and
> by doing so, calls to redisplay_preserve_echo_area, and you have an
> absurd number of X requests, enough to saturate any reasonable network
> connection.
> 
> Here is a patch to resolve the problem by not redrawing the cursor when
> Emacs knows it is already there.  I've not seen any ill effects so far,
> but the comment originally there seems to know better, so what are the
> cases where ``phys_cursor_on_p is 1 but the cursor has been erased''?

This is okay for master, thanks.

Btw, the code which tests MINI_WINDOW_P here:

> 	  && (CHARPOS (tlbufpos) == ZV
> 	      || FETCH_BYTE (BYTEPOS (tlbufpos)) == '\n'))
> 	/* Former continuation line has disappeared by becoming empty.  */
> 	goto cancel;
>       else if (window_outdated (w) || MINI_WINDOW_P (w)) <==============
> 	{
> 	  /* We have to handle the case of continuation around a
> 	     wide-column character (see the comment in indent.c around
> 	     line 1340).

is very old, dating back to the initial revision of xdisp.c before the
Emacs 21 display engine changes, so maybe we should try removing that
instead (assuming that it is related to the more general change
regarding the cursor you want to install).

bug#61779: 30.0.50; excessive redisplay when the mini window is active

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sat, 25 Feb 2023 14:16:37 +0800
>> From:  Po Lu via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> For some reason redisplay_internal will always update the mini window
>> even if its buffer contents have not changed.  I guess there is probably
>> a missing optimization wrt to updating the echo area contents, and
>> window_outdated does not take the echo area message into account:
>> 
>> 	  && (CHARPOS (tlbufpos) == ZV
>> 	      || FETCH_BYTE (BYTEPOS (tlbufpos)) == '\n'))
>> 	/* Former continuation line has disappeared by becoming empty.  */
>> 	goto cancel;
>>       else if (window_outdated (w) || MINI_WINDOW_P (w)) <==============
>> 	{
>> 	  /* We have to handle the case of continuation around a
>> 	     wide-column character (see the comment in indent.c around
>> 	     line 1340).
>> 
>> I am not sure that is why, however, so I can't just change that.
>> Anyway, update_window_line does not draw anything since the contents of
>> the glyph row do not change after redisplay, so that part is fine.  The
>> problem is that the redisplay will lead to update_window being called,
>> and gui_update_window_end will always call display_and_set_cursor
>> afterwards to display the cursor, which is not okay, as it generates a
>> lot of X protocol traffic when redisplay is called and redisplays the
>> mini-window.  Now add to that a process generating a lot of output, and
>> by doing so, calls to redisplay_preserve_echo_area, and you have an
>> absurd number of X requests, enough to saturate any reasonable network
>> connection.
>> 
>> Here is a patch to resolve the problem by not redrawing the cursor when
>> Emacs knows it is already there.  I've not seen any ill effects so far,
>> but the comment originally there seems to know better, so what are the
>> cases where ``phys_cursor_on_p is 1 but the cursor has been erased''?
>
> This is okay for master, thanks.

The comment that was there still makes me uncomfortable, so:

> Btw, the code which tests MINI_WINDOW_P here:
>
>> 	  && (CHARPOS (tlbufpos) == ZV
>> 	      || FETCH_BYTE (BYTEPOS (tlbufpos)) == '\n'))
>> 	/* Former continuation line has disappeared by becoming empty.  */
>> 	goto cancel;
>>       else if (window_outdated (w) || MINI_WINDOW_P (w)) <==============
>> 	{
>> 	  /* We have to handle the case of continuation around a
>> 	     wide-column character (see the comment in indent.c around
>> 	     line 1340).
>
> is very old, dating back to the initial revision of xdisp.c before the
> Emacs 21 display engine changes, so maybe we should try removing that
> instead (assuming that it is related to the more general change
> regarding the cursor you want to install).

I guess that's the better idea, since it's the only reasonable way I
found to make update_window be called extremely often without any
changes actually happening.