Re: [PATCH] Fix TUI flicker resulting from frequent frame changes (PR tui/13378)

[Pedro Alves <palves at redhat dot com>]

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.  */
>