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

[Patrick Palka <patrick at parcs dot ath dot cx>]

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