Fix TUI flicker resulting from frequent frame changes (PR tui/13378)

  This patch fixes the perceived flicker of the TUI screen (and subsequent
slowdown) that most apparent when running an inferior while a
conditional breakpoint is active.  The cause of the flicker is that each
internal event GDB responds to is accompanied by a multitude of calls to
select_frame() and each such call forces the TUI screen to be refreshed.
We would like to not update the TUI screen after each such frame change.

The fix for this issue is pretty straightforward: do not update the TUI
screen when select_frame() gets called while synchronous execution of
the inferior is enabled.  This works because synchronous execution
remains enabled during the processing of internal events.  And since
during synchronous execution the user has no control of the TUI anyway,
it does not hurt to avoid updating the screen then.

The select_frame hook is still undesirable and should be removed, but
in the meantime this fix is seemingly an effective approximation of a
more disciplined approach of updating the TUI screen.

[ When the inferior is running while sync_execution is disabled, e.g.
  via "continue&" it looks like GDB lacks access to frame information
  thorughout -- and the hook never gets called -- so seemingly no
  worries in that case.  ]

[ up/down etc still work properl of course  ]

[ I am not sure that the change in tui_on_sync_execution_done is
  necessary/desirable.  It seems normal_stop already calls
  select_frame (get_current_frame ()) after sync_execution is toggled
  off.  ]

gdb/ChangeLog:

	* tui/tui-hooks.c (tui_selected_frame_level_changed_hook): Don't
	update the screen while synchronous execution is active.
	* tui/tui-interp.c (tui_on_sync_execution_done): Make sure that
	TUI's frame information is refreshed.
---
 gdb/tui/tui-hooks.c  | 8 ++++++++
 gdb/tui/tui-interp.c | 5 +++++
 2 files changed, 13 insertions(+)
  

On Fri, Jun 19, 2015 at 12:36 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> This patch fixes the perceived flicker of the TUI screen (and subsequent
> slowdown) that most apparent when running an inferior while a
> conditional breakpoint is active.  The cause of the flicker is that each
> internal event GDB responds to is accompanied by a multitude of calls to
> select_frame() and each such call forces the TUI screen to be refreshed.
> We would like to not update the TUI screen after each such frame change.
>
> The fix for this issue is pretty straightforward: do not update the TUI
> screen when select_frame() gets called while synchronous execution of
> the inferior is enabled.  This works because synchronous execution
> remains enabled during the processing of internal events.  And since
> during synchronous execution the user has no control of the TUI anyway,
> it does not hurt to avoid updating the screen then.
>
> The select_frame hook is still undesirable and should be removed, but
> in the meantime this fix is seemingly an effective approximation of a
> more disciplined approach of updating the TUI screen.
>
> [ When the inferior is running while sync_execution is disabled, e.g.
>   via "continue&" it looks like GDB lacks access to frame information
>   thorughout -- and the hook never gets called -- so seemingly no
>   worries in that case.  ]
>
> [ up/down etc still work properl of course  ]
>
> [ I am not sure that the change in tui_on_sync_execution_done is
>   necessary/desirable.  It seems normal_stop already calls
>   select_frame (get_current_frame ()) after sync_execution is toggled
>   off.  ]
>
> gdb/ChangeLog:
>
>         * tui/tui-hooks.c (tui_selected_frame_level_changed_hook): Don't
>         update the screen while synchronous execution is active.
>         * tui/tui-interp.c (tui_on_sync_execution_done): Make sure that
>         TUI's frame information is refreshed.
> ---
>  gdb/tui/tui-hooks.c  | 8 ++++++++
>  gdb/tui/tui-interp.c | 5 +++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
> index 8d84551..ca8358d 100644
> --- a/gdb/tui/tui-hooks.c
> +++ b/gdb/tui/tui-hooks.c
> @@ -133,6 +133,14 @@ tui_selected_frame_level_changed_hook (int level)
>    if (level < 0)
>      return;
>
> +  /* Do not respond to frame changes occurring while synchronous execution is
> +     enabled.  Updating the screen in response to each such frame change just
> +     results in pointless flicker and slowdown.  Once synchronous execution is
> +     done this hook will get called again to ensure that our frame information
> +     is refreshed.  */
> +  if (sync_execution)
> +    return;
> +
>    old_chain = make_cleanup_restore_target_terminal ();
>    target_terminal_ours_for_output ();
>
> diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
> index 1a5639d..2477536 100644
> --- a/gdb/tui/tui-interp.c
> +++ b/gdb/tui/tui-interp.c
> @@ -107,6 +107,11 @@ tui_on_sync_execution_done (void)
>  {
>    if (!interp_quiet_p (tui_interp))
>      display_gdb_prompt (NULL);
> +
> +  /* Make sure our frame information is refreshed now that synchronous
> +     execution is done.  */
> +  if (tui_active)
> +    deprecated_selected_frame_level_changed_hook (0);

This condition ought to be if (tui_active && has_stack_frames ()) so
that we don't call the hook (and thus, try to lookup frame
information) after the inferior has exited (which would result in a
confusing "No stack." error).

>  }
>
>  /* Observer for the command_error notification.  */
> --
> 2.4.4.410.g43ed522.dirty
>
  

On Fri, Jun 19, 2015 at 1:13 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Fri, Jun 19, 2015 at 12:36 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> This patch fixes the perceived flicker of the TUI screen (and subsequent
>> slowdown) that most apparent when running an inferior while a
>> conditional breakpoint is active.  The cause of the flicker is that each
>> internal event GDB responds to is accompanied by a multitude of calls to
>> select_frame() and each such call forces the TUI screen to be refreshed.
>> We would like to not update the TUI screen after each such frame change.
>>
>> The fix for this issue is pretty straightforward: do not update the TUI
>> screen when select_frame() gets called while synchronous execution of
>> the inferior is enabled.  This works because synchronous execution
>> remains enabled during the processing of internal events.  And since
>> during synchronous execution the user has no control of the TUI anyway,
>> it does not hurt to avoid updating the screen then.
>>
>> The select_frame hook is still undesirable and should be removed, but
>> in the meantime this fix is seemingly an effective approximation of a
>> more disciplined approach of updating the TUI screen.
>>
>> [ When the inferior is running while sync_execution is disabled, e.g.
>>   via "continue&" it looks like GDB lacks access to frame information
>>   thorughout -- and the hook never gets called -- so seemingly no
>>   worries in that case.  ]
>>
>> [ up/down etc still work properl of course  ]
>>
>> [ I am not sure that the change in tui_on_sync_execution_done is
>>   necessary/desirable.  It seems normal_stop already calls
>>   select_frame (get_current_frame ()) after sync_execution is toggled
>>   off.  ]
>>
>> gdb/ChangeLog:
>>
>>         * tui/tui-hooks.c (tui_selected_frame_level_changed_hook): Don't
>>         update the screen while synchronous execution is active.
>>         * tui/tui-interp.c (tui_on_sync_execution_done): Make sure that
>>         TUI's frame information is refreshed.
>> ---
>>  gdb/tui/tui-hooks.c  | 8 ++++++++
>>  gdb/tui/tui-interp.c | 5 +++++
>>  2 files changed, 13 insertions(+)
>>
>> diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
>> index 8d84551..ca8358d 100644
>> --- a/gdb/tui/tui-hooks.c
>> +++ b/gdb/tui/tui-hooks.c
>> @@ -133,6 +133,14 @@ tui_selected_frame_level_changed_hook (int level)
>>    if (level < 0)
>>      return;
>>
>> +  /* Do not respond to frame changes occurring while synchronous execution is
>> +     enabled.  Updating the screen in response to each such frame change just
>> +     results in pointless flicker and slowdown.  Once synchronous execution is
>> +     done this hook will get called again to ensure that our frame information
>> +     is refreshed.  */
>> +  if (sync_execution)
>> +    return;
>> +
>>    old_chain = make_cleanup_restore_target_terminal ();
>>    target_terminal_ours_for_output ();
>>
>> diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
>> index 1a5639d..2477536 100644
>> --- a/gdb/tui/tui-interp.c
>> +++ b/gdb/tui/tui-interp.c
>> @@ -107,6 +107,11 @@ tui_on_sync_execution_done (void)
>>  {
>>    if (!interp_quiet_p (tui_interp))
>>      display_gdb_prompt (NULL);
>> +
>> +  /* Make sure our frame information is refreshed now that synchronous
>> +     execution is done.  */
>> +  if (tui_active)
>> +    deprecated_selected_frame_level_changed_hook (0);
>
> This condition ought to be if (tui_active && has_stack_frames ()) so
> that we don't call the hook (and thus, try to lookup frame
> information) after the inferior has exited (which would result in a
> confusing "No stack." error).
>
>>  }
>>
>>  /* Observer for the command_error notification.  */
>> --
>> 2.4.4.410.g43ed522.dirty
>>

Ping.
  

On 06/19/2015 05:36 AM, Patrick Palka wrote:
> This patch fixes the perceived flicker of the TUI screen (and subsequent
> slowdown) that most apparent when running an inferior while a
> conditional breakpoint is active.  The cause of the flicker is that each
> internal event GDB responds to is accompanied by a multitude of calls to
> select_frame() and each such call forces the TUI screen to be refreshed.
> We would like to not update the TUI screen after each such frame change.
> 
> The fix for this issue is pretty straightforward: do not update the TUI
> screen when select_frame() gets called while synchronous execution of
> the inferior is enabled.  This works because synchronous execution
> remains enabled during the processing of internal events.  And since
> during synchronous execution the user has no control of the TUI anyway,
> it does not hurt to avoid updating the screen then.
> 
> The select_frame hook is still undesirable and should be removed, but
> in the meantime this fix is seemingly an effective approximation of a
> more disciplined approach of updating the TUI screen.
> 
> [ When the inferior is running while sync_execution is disabled, e.g.
>   via "continue&" it looks like GDB lacks access to frame information
>   thorughout -- and the hook never gets called -- so seemingly no
>   worries in that case.  ]

That can't be right.  Whenever GDB stops for any (thread) event, it'll
select the current frame.  So try putting a conditional breakpoint (whose
condition fails) on a tight loop, and then continuing, like:

(gdb) b inside_loop if 0
(gdb) c&

I see lots of TUI flicker doing this.  E.g.:

(top-gdb) bt
#0  tui_refresh_win (win_info=0xdbb3e0 <exec_info>) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-wingeneral.c:38
#1  0x000000000052fcfa in tui_show_exec_info_content (win_info=0x1a2e0e0) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-winsource.c:569
#2  0x000000000052fd8a in tui_update_exec_info (win_info=0x1a2e0e0) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-winsource.c:598
#3  0x000000000052bc62 in tui_show_frame_info (fi=0x1dbc2a0) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-stack.c:429
#4  0x0000000000526656 in tui_selected_frame_level_changed_hook (level=0) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-hooks.c:158
#5  0x000000000076a5b9 in select_frame (fi=0x1dbc2a0) at /home/pedro/gdb/mygit/build/../src/gdb/frame.c:1580
#6  0x00000000005ac80d in bpstat_check_breakpoint_conditions (bs=0x3d42a30, ptid=...) at /home/pedro/gdb/mygit/build/../src/gdb/breakpoint.c:5414
#7  0x00000000005acc05 in bpstat_stop_status (aspace=0x1034a40, bp_addr=4196369, ptid=..., ws=0x7fffffffd210) at /home/pedro/gdb/mygit/build/../src/gdb/breakpoint.c:5616
#8  0x000000000062e6bc in handle_signal_stop (ecs=0x7fffffffd1f0) at /home/pedro/gdb/mygit/build/../src/gdb/infrun.c:4529
#9  0x000000000062dbce in handle_inferior_event_1 (ecs=0x7fffffffd1f0) at /home/pedro/gdb/mygit/build/../src/gdb/infrun.c:4195
#10 0x000000000062dc70 in handle_inferior_event (ecs=0x7fffffffd1f0) at /home/pedro/gdb/mygit/build/../src/gdb/infrun.c:4220
#11 0x000000000062bfb9 in fetch_inferior_event (client_data=0x0) at /home/pedro/gdb/mygit/build/../src/gdb/infrun.c:3328

How about we instead find some more higher level place
to refresh?  I'm thinking that maybe whenever we display the prompt
might be a good place (before_prompt observer).  With both the prompt
and normal_stop, we cover every case that needs a refresh, I think.

If we don't want that to always refresh at the prompt, we could still
have the tui_selected_frame_level_changed_hook hook, but instead
of having that cause an immediate refresh, it'd just set a
flag to indicate that the next time we display the prompt, we should
refresh everything.  That way, multiple frame changes until we
reach the prompt cause only one refresh.  And if no frame changes,
the flag is not set, so no refresh is done at the prompt.
But if a simpler unconditional refresh at the prompt works, I'd
go for that.

Thanks,
Pedro Alves


> 
> [ up/down etc still work properl of course  ]
> 
> [ I am not sure that the change in tui_on_sync_execution_done is
>   necessary/desirable.  It seems normal_stop already calls
>   select_frame (get_current_frame ()) after sync_execution is toggled
>   off.  ]
> 
> gdb/ChangeLog:
> 
> 	* tui/tui-hooks.c (tui_selected_frame_level_changed_hook): Don't
> 	update the screen while synchronous execution is active.
> 	* tui/tui-interp.c (tui_on_sync_execution_done): Make sure that
> 	TUI's frame information is refreshed.
> ---
>  gdb/tui/tui-hooks.c  | 8 ++++++++
>  gdb/tui/tui-interp.c | 5 +++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
> index 8d84551..ca8358d 100644
> --- a/gdb/tui/tui-hooks.c
> +++ b/gdb/tui/tui-hooks.c
> @@ -133,6 +133,14 @@ tui_selected_frame_level_changed_hook (int level)
>    if (level < 0)
>      return;
>  
> +  /* Do not respond to frame changes occurring while synchronous execution is
> +     enabled.  Updating the screen in response to each such frame change just
> +     results in pointless flicker and slowdown.  Once synchronous execution is
> +     done this hook will get called again to ensure that our frame information
> +     is refreshed.  */
> +  if (sync_execution)
> +    return;
> +
>    old_chain = make_cleanup_restore_target_terminal ();
>    target_terminal_ours_for_output ();
>  
> diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
> index 1a5639d..2477536 100644
> --- a/gdb/tui/tui-interp.c
> +++ b/gdb/tui/tui-interp.c
> @@ -107,6 +107,11 @@ tui_on_sync_execution_done (void)
>  {
>    if (!interp_quiet_p (tui_interp))
>      display_gdb_prompt (NULL);
> +
> +  /* Make sure our frame information is refreshed now that synchronous
> +     execution is done.  */
> +  if (tui_active)
> +    deprecated_selected_frame_level_changed_hook (0);
>  }
>  
>  /* Observer for the command_error notification.  */
>
  

On Fri, Jun 26, 2015 at 9:36 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/19/2015 05:36 AM, Patrick Palka wrote:
>> This patch fixes the perceived flicker of the TUI screen (and subsequent
>> slowdown) that most apparent when running an inferior while a
>> conditional breakpoint is active.  The cause of the flicker is that each
>> internal event GDB responds to is accompanied by a multitude of calls to
>> select_frame() and each such call forces the TUI screen to be refreshed.
>> We would like to not update the TUI screen after each such frame change.
>>
>> The fix for this issue is pretty straightforward: do not update the TUI
>> screen when select_frame() gets called while synchronous execution of
>> the inferior is enabled.  This works because synchronous execution
>> remains enabled during the processing of internal events.  And since
>> during synchronous execution the user has no control of the TUI anyway,
>> it does not hurt to avoid updating the screen then.
>>
>> The select_frame hook is still undesirable and should be removed, but
>> in the meantime this fix is seemingly an effective approximation of a
>> more disciplined approach of updating the TUI screen.
>>
>> [ When the inferior is running while sync_execution is disabled, e.g.
>>   via "continue&" it looks like GDB lacks access to frame information
>>   thorughout -- and the hook never gets called -- so seemingly no
>>   worries in that case.  ]
>
> That can't be right.  Whenever GDB stops for any (thread) event, it'll
> select the current frame.  So try putting a conditional breakpoint (whose
> condition fails) on a tight loop, and then continuing, like:
>
> (gdb) b inside_loop if 0
> (gdb) c&
>
> I see lots of TUI flicker doing this.  E.g.:
>
> (top-gdb) bt
> #0  tui_refresh_win (win_info=0xdbb3e0 <exec_info>) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-wingeneral.c:38
> #1  0x000000000052fcfa in tui_show_exec_info_content (win_info=0x1a2e0e0) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-winsource.c:569
> #2  0x000000000052fd8a in tui_update_exec_info (win_info=0x1a2e0e0) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-winsource.c:598
> #3  0x000000000052bc62 in tui_show_frame_info (fi=0x1dbc2a0) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-stack.c:429
> #4  0x0000000000526656 in tui_selected_frame_level_changed_hook (level=0) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-hooks.c:158
> #5  0x000000000076a5b9 in select_frame (fi=0x1dbc2a0) at /home/pedro/gdb/mygit/build/../src/gdb/frame.c:1580
> #6  0x00000000005ac80d in bpstat_check_breakpoint_conditions (bs=0x3d42a30, ptid=...) at /home/pedro/gdb/mygit/build/../src/gdb/breakpoint.c:5414
> #7  0x00000000005acc05 in bpstat_stop_status (aspace=0x1034a40, bp_addr=4196369, ptid=..., ws=0x7fffffffd210) at /home/pedro/gdb/mygit/build/../src/gdb/breakpoint.c:5616
> #8  0x000000000062e6bc in handle_signal_stop (ecs=0x7fffffffd1f0) at /home/pedro/gdb/mygit/build/../src/gdb/infrun.c:4529
> #9  0x000000000062dbce in handle_inferior_event_1 (ecs=0x7fffffffd1f0) at /home/pedro/gdb/mygit/build/../src/gdb/infrun.c:4195
> #10 0x000000000062dc70 in handle_inferior_event (ecs=0x7fffffffd1f0) at /home/pedro/gdb/mygit/build/../src/gdb/infrun.c:4220
> #11 0x000000000062bfb9 in fetch_inferior_event (client_data=0x0) at /home/pedro/gdb/mygit/build/../src/gdb/infrun.c:3328

I see.. I did not catch that..

>
> How about we instead find some more higher level place
> to refresh?  I'm thinking that maybe whenever we display the prompt
> might be a good place (before_prompt observer).  With both the prompt
> and normal_stop, we cover every case that needs a refresh, I think.

I can imagine a problem with this.  It seems that when the screen gets
refreshed following a frame change, any scrolling that the user did in
the source/asm windows would get undone because the screen gets
re-centered on the currently executing line.  You can see this by
doing "frame 0", scrolling the window a bit and doing "frame 0" again:
the scrolling gets undone.  So by naively refreshing the source/asm
windows before each prompt, we would undo scrolling for benign
commands such as "print 1 + 2", "bt", I think...  This could be fixed
by being smarter about refreshing, by only refreshing the screen in
the before_prompt observer if the frame information/PC has changed.

>
> If we don't want that to always refresh at the prompt, we could still
> have the tui_selected_frame_level_changed_hook hook, but instead
> of having that cause an immediate refresh, it'd just set a
> flag to indicate that the next time we display the prompt, we should
> refresh everything.  That way, multiple frame changes until we
> reach the prompt cause only one refresh.  And if no frame changes,
> the flag is not set, so no refresh is done at the prompt.
> But if a simpler unconditional refresh at the prompt works, I'd
> go for that.

Setting a flag inside the hook would not be perfect anyway since it
seems that many times select_frame() is called on the already-selected
frame.  It seems it would be better to only refresh the screen if the
frame/PC actually changes as mentioned above.  This can be checked in
the observer itself -- no need for the hook, right?

>
> Thanks,
> Pedro Alves
>
>
>>
>> [ up/down etc still work properl of course  ]
>>
>> [ I am not sure that the change in tui_on_sync_execution_done is
>>   necessary/desirable.  It seems normal_stop already calls
>>   select_frame (get_current_frame ()) after sync_execution is toggled
>>   off.  ]
>>
>> gdb/ChangeLog:
>>
>>       * tui/tui-hooks.c (tui_selected_frame_level_changed_hook): Don't
>>       update the screen while synchronous execution is active.
>>       * tui/tui-interp.c (tui_on_sync_execution_done): Make sure that
>>       TUI's frame information is refreshed.
>> ---
>>  gdb/tui/tui-hooks.c  | 8 ++++++++
>>  gdb/tui/tui-interp.c | 5 +++++
>>  2 files changed, 13 insertions(+)
>>
>> diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
>> index 8d84551..ca8358d 100644
>> --- a/gdb/tui/tui-hooks.c
>> +++ b/gdb/tui/tui-hooks.c
>> @@ -133,6 +133,14 @@ tui_selected_frame_level_changed_hook (int level)
>>    if (level < 0)
>>      return;
>>
>> +  /* Do not respond to frame changes occurring while synchronous execution is
>> +     enabled.  Updating the screen in response to each such frame change just
>> +     results in pointless flicker and slowdown.  Once synchronous execution is
>> +     done this hook will get called again to ensure that our frame information
>> +     is refreshed.  */
>> +  if (sync_execution)
>> +    return;
>> +
>>    old_chain = make_cleanup_restore_target_terminal ();
>>    target_terminal_ours_for_output ();
>>
>> diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
>> index 1a5639d..2477536 100644
>> --- a/gdb/tui/tui-interp.c
>> +++ b/gdb/tui/tui-interp.c
>> @@ -107,6 +107,11 @@ tui_on_sync_execution_done (void)
>>  {
>>    if (!interp_quiet_p (tui_interp))
>>      display_gdb_prompt (NULL);
>> +
>> +  /* Make sure our frame information is refreshed now that synchronous
>> +     execution is done.  */
>> +  if (tui_active)
>> +    deprecated_selected_frame_level_changed_hook (0);
>>  }
>>
>>  /* Observer for the command_error notification.  */
>>
>
>
  

On 06/26/2015 03:09 PM, Patrick Palka wrote:
> On Fri, Jun 26, 2015 at 9:36 AM, Pedro Alves <palves@redhat.com> wrote:

>> How about we instead find some more higher level place
>> to refresh?  I'm thinking that maybe whenever we display the prompt
>> might be a good place (before_prompt observer).  With both the prompt
>> and normal_stop, we cover every case that needs a refresh, I think.
> 
> I can imagine a problem with this.  It seems that when the screen gets
> refreshed following a frame change, any scrolling that the user did in
> the source/asm windows would get undone because the screen gets
> re-centered on the currently executing line.  You can see this by
> doing "frame 0", scrolling the window a bit and doing "frame 0" again:
> the scrolling gets undone.  So by naively refreshing the source/asm
> windows before each prompt, we would undo scrolling for benign
> commands such as "print 1 + 2", "bt", I think...

Indeed.

> This could be fixed
> by being smarter about refreshing, by only refreshing the screen in
> the before_prompt observer if the frame information/PC has changed.

...

> It seems it would be better to only refresh the screen if the
> frame/PC actually changes as mentioned above.  This can be checked in
> the observer itself -- no need for the hook, right?

Yes, I think so.

Thanks,
Pedro Alves