Re: [PATCH] Speed up "gdb -tui" output

[Doug Evans <dje at google dot com>]

On Tue, Jan 6, 2015 at 8:12 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> In the TUI mode, we call wrefresh after outputting every single
> character.  Since fputs_* functions go through fputc_*, i.e. break
> their writes into single characters as well, and tui_putc makes a
> 1-character string out of every character and passes it to tui_puts,
> every time GDB wants to display more than a few characters, the I/O
> becomes very slow.
>
> I guess TUI relies on the curses library or the underlying stdio to do
> the buffering, but the Windows build of ncurses doesn't buffer and
> doesn't use stdio.
>
> The simple patch below speeds up GDB output in TUI mode several orders
> of magnitude, especially on Windows XP (but there's a very prominent
> speedup on Windows 7 as well).
>
> Any objections (ChangeLog entry will be added, of course)?
>
>
> *** gdb/tui/tui-io.c~1  2015-01-03 11:12:52.187500000 +0200
> --- gdb/tui/tui-io.c    2015-01-06 17:59:55.098500000 +0200
> *************** tui_puts (const char *string)
> *** 201,209 ****
>     TUI_CMD_WIN->detail.command_info.start_line
>       = TUI_CMD_WIN->detail.command_info.cur_line;
>
> !   /* We could defer the following.  */
> !   wrefresh (w);
> !   fflush (stdout);
>   }
>
>   /* Readline callback.
> --- 201,211 ----
>     TUI_CMD_WIN->detail.command_info.start_line
>       = TUI_CMD_WIN->detail.command_info.cur_line;
>
> !   if (c == '\n')
> !     {
> !       wrefresh (w);
> !       fflush (stdout);
> !     }
>   }
>
>   /* Readline callback.

I don't know TUI well enough to know if there are any gotchas here.
[I know curses well enough, but memory fades ...]

This is ok with me if it doesn't break anything :-),
but I think a comment explaining "why things are the way they are"
is needed here.

It's too bad the original author didn't elaborate on
"We could defer the following."
I can guess at was was meant.  I suspect it was
just an offhand wish-list item.  Normally one lets
curses do the refresh when we get to keyboard input
unless something needs to go out before then.
But tui_puts can't know which is which.

I could invest more time seeing if there's an answer in the
historical records.  There's nothing here:
https://sourceware.org/ml/gdb-patches/2001-07/msg00490.html
and I hesitate to spend too much time on this.

So while I'm ok with deleting the current comment,
I think it would help future readers to replace it with a new one.

Maybe something like:

  /* Since strings are generally output a character at a time
     (e.g., fputs_* break it up and send us a character at a time
     that's converted back to a string), blindly calling wrefresh here
     is really expensive on Windows.  PR 12345
     Assume that if the caller wants to output a string without a
     trailing newline that's not a prompt (curses will refresh the screen
     for us at keyboard input), then the caller knows what s/he is
     doing and will, if necessary, call wrefresh on his/her own.  */

[Yeah, I think this is deserving of a bug report.]

Having written all that, asking the caller to know to do
wrefresh at the needed times could be onerous.
One thing that occurs to me is that gdb does do things like:

Reading symbols from foo ... (work work work) done.
And if I do that on my standard monster benchmark (fortunately
I keep a ready-to-use copy lying around for experiments like this :-))
I see a long pause before "done." is printed.

And lo and behold, if I apply your patch I see this:

bash$ gdb -tui
(gdb) file foo<ret>

At this point I've hit return but I don't see anything printed.

pause pause pause

and then finally I see all the output:

Reading symbols from foo...done.
mumble ...
(gdb)

So the patch is not ok, at least as is.  Sorry!

One could put it in an appropriate #ifdef,
but maybe there's a better solution.

Another possibility would be to do the string -> char -> string
processing differently.  String printing utilities could accumulate
what they want to print and send the output in chunks instead
of characters.
Then tui_puts could get real strings instead of always getting
{ char, '\0' }, and maybe that would be enough.

I don't have a preference on which way to go yet.
I'm just tossing out ideas.