public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Speed up "gdb -tui" output
@ 2015-01-06 16:12 Eli Zaretskii
  2015-01-06 18:37 ` Doug Evans
  0 siblings, 1 reply; 34+ messages in thread
From: Eli Zaretskii @ 2015-01-06 16:12 UTC (permalink / raw)
  To: gdb-patches

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.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-06 16:12 [PATCH] Speed up "gdb -tui" output Eli Zaretskii
@ 2015-01-06 18:37 ` Doug Evans
  2015-01-06 19:06   ` Eli Zaretskii
  2015-01-07 15:18   ` Pedro Alves
  0 siblings, 2 replies; 34+ messages in thread
From: Doug Evans @ 2015-01-06 18:37 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: gdb-patches

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.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-06 18:37 ` Doug Evans
@ 2015-01-06 19:06   ` Eli Zaretskii
  2015-01-06 20:54     ` Doug Evans
  2015-01-07 15:18   ` Pedro Alves
  1 sibling, 1 reply; 34+ messages in thread
From: Eli Zaretskii @ 2015-01-06 19:06 UTC (permalink / raw)
  To: Doug Evans; +Cc: gdb-patches

> Date: Tue, 6 Jan 2015 10:37:13 -0800
> From: Doug Evans <dje@google.com>
> Cc: gdb-patches <gdb-patches@sourceware.org>
> 
> 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)

If this is the only place where this matters, we could break that line
in two:

  Reading symbols from foo...
  Done reading symbols from foo.

Or maybe we could also call wrefresh when we see 3 consecutive dots,
assuming that these are the cases where a prolonged operation is under
way.

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

This will hit the same problem: how to know when to stop accumulating
and flush it out.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-06 19:06   ` Eli Zaretskii
@ 2015-01-06 20:54     ` Doug Evans
  2015-01-07 18:30       ` Eli Zaretskii
  0 siblings, 1 reply; 34+ messages in thread
From: Doug Evans @ 2015-01-06 20:54 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: gdb-patches

On Tue, Jan 6, 2015 at 11:06 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Tue, 6 Jan 2015 10:37:13 -0800
>> From: Doug Evans <dje@google.com>
>> Cc: gdb-patches <gdb-patches@sourceware.org>
>>
>> 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)
>
> If this is the only place where this matters, we could break that line
> in two:
>
>   Reading symbols from foo...
>   Done reading symbols from foo.
>
> Or maybe we could also call wrefresh when we see 3 consecutive dots,
> assuming that these are the cases where a prolonged operation is under
> way.

Not adding a newline after the three dots does lead to other problems:
the code is assuming other code won't output a newline.  It may be uncommon
but it still happens. Thus one could make the case that outputting anything
like this without a newline is something to be avoided.
OTOH, I wouldn't want to preclude it.

Keying off of "..." is a possibility, we could make it a formal convention.
One could make the case that it's too hacky though, I don't have a
strong opinion.
tui_puts would have to maintain global state to track what it's been previously
sent, which one would like to avoid if possible (though I see tui_puts already
does that to handle annotations).
I don't know if there are other places in gdb that output something
(sans newline),
do some work, and then output the rest of the line (setting aside
debugging output).
They would all have to be audited and maybe changed.

>
>> 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.
>
> This will hit the same problem: how to know when to stop accumulating
> and flush it out.

What I'm saying is tui_puts would still call wrefresh unconditionally.
This is no different than your patch, conceptually, except that
it'd be up to higher layers to not send tui_puts a character at a time for the
common case of outputting strings.

We send output a character at a time in part to support
lines_per_page, chars_per_line, and wrap_column,
relying on character-at-a-time buffering to save us.
Alas for windows that doesn't apparently doesn't work (yay windows).
So one way to go, and again, this is just a possibility,
is to not send tui_puts a character at a time.

Is it possible to fix windows?
If the bug really is there ...

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-06 18:37 ` Doug Evans
  2015-01-06 19:06   ` Eli Zaretskii
@ 2015-01-07 15:18   ` Pedro Alves
  2015-01-07 17:57     ` Eli Zaretskii
  2015-01-07 18:00     ` Doug Evans
  1 sibling, 2 replies; 34+ messages in thread
From: Pedro Alves @ 2015-01-07 15:18 UTC (permalink / raw)
  To: Doug Evans, Eli Zaretskii; +Cc: gdb-patches

On 01/06/2015 06:37 PM, Doug Evans wrote:

> 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)

Since stdout is line buffered by default (on Unix), if this is
working when the TUI is disabled, then it must be because there's
explicit gdb_flush(gdb_stdout) after "Reading symbols from foo..."
is printed, right?

Isn't the issue then that the TUI's implementation of
gdb_flush (tui/tui-file.c) should be doing whatever it
needs to flush the output?  Should it be calling wrefresh
if the file is gdb_stdout?  If we do that, and change tui_puts like:

 -  /* We could defer the following.  */
 -  wrefresh (w);
 -  fflush (stdout);
 +   if (c == '\n')
 +    gdb_flush (gdb_stdout);

would it work?

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 15:18   ` Pedro Alves
@ 2015-01-07 17:57     ` Eli Zaretskii
  2015-01-07 18:09       ` Doug Evans
  2015-01-07 18:00     ` Doug Evans
  1 sibling, 1 reply; 34+ messages in thread
From: Eli Zaretskii @ 2015-01-07 17:57 UTC (permalink / raw)
  To: Pedro Alves; +Cc: dje, gdb-patches

> Date: Wed, 07 Jan 2015 15:17:54 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: gdb-patches <gdb-patches@sourceware.org>
> 
> > Reading symbols from foo...done.
> > mumble ...
> > (gdb)
> 
> Since stdout is line buffered by default (on Unix), if this is
> working when the TUI is disabled, then it must be because there's
> explicit gdb_flush(gdb_stdout) after "Reading symbols from foo..."
> is printed, right?

Yes.

> Isn't the issue then that the TUI's implementation of
> gdb_flush (tui/tui-file.c) should be doing whatever it
> needs to flush the output?

Maybe.  I already made that change in my sandbox.  Alas, that's just
the tip of the iceberg.  We call single-character output functions,
like putchar and fputc all over the place, implicitly assuming that in
certain cases (e.g., when the stream is stderr), things are unbuffered
and the output appears immediately.  On top of that, functions that
output strings go through single-character output versions, so there's
no easy way of saying on, say, utils.c level whether the character
should or shouldn't appear immediately. It's a terrible mess.

> Should it be calling wrefresh if the file is gdb_stdout?

Only if that stream is a real file, not a string or whatever else we
support in ui-file.

> If we do that, and change tui_puts like:
> 
>  -  /* We could defer the following.  */
>  -  wrefresh (w);
>  -  fflush (stdout);
>  +   if (c == '\n')
>  +    gdb_flush (gdb_stdout);
> 
> would it work?

No, it's not enough.  There's also gdb_stderr and gdb_stdlog, at
least.  (tui_puts doesn't get the stream as its argument, so it
doesn't really know which stream it is serving.)  I'm trying to make
heads or tails out of this, but I'm not there yet.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 15:18   ` Pedro Alves
  2015-01-07 17:57     ` Eli Zaretskii
@ 2015-01-07 18:00     ` Doug Evans
  2015-01-07 18:12       ` Doug Evans
  2015-01-07 18:21       ` Eli Zaretskii
  1 sibling, 2 replies; 34+ messages in thread
From: Doug Evans @ 2015-01-07 18:00 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Eli Zaretskii, gdb-patches

On Wed, Jan 7, 2015 at 7:17 AM, Pedro Alves <palves@redhat.com> wrote:
> On 01/06/2015 06:37 PM, Doug Evans wrote:
>
>> 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)
>
> Since stdout is line buffered by default (on Unix), if this is
> working when the TUI is disabled, then it must be because there's
> explicit gdb_flush(gdb_stdout) after "Reading symbols from foo..."
> is printed, right?
>
> Isn't the issue then that the TUI's implementation of
> gdb_flush (tui/tui-file.c) should be doing whatever it
> needs to flush the output?  Should it be calling wrefresh
> if the file is gdb_stdout?  If we do that, and change tui_puts like:
>
>  -  /* We could defer the following.  */
>  -  wrefresh (w);
>  -  fflush (stdout);
>  +   if (c == '\n')
>  +    gdb_flush (gdb_stdout);
>
> would it work?

TBH I'm not entirely sure why the fflush (stdout) is there.
[There's another one in the file as well.]
There's a comment at the top of tui-io.c mentioning readline
needs some work w.r.t. stdout.

I don't know off hand if removing the fflush (stdout) will break anything,
but obviously in an ideal world it shouldn't be there.

So, yeah, it seems like gdb_flush for tui needs to do a wrefresh
(for gdb_stdout/stderr (/stdlog?))

I did some experimentation with pagination and "set height".
pagination does work, at least a little.
[The more I look into tui sources, and the more I play with tui,
the lower the bar I have for what I expect to work ...]

Do we need to do gdb_flush (gdb_stdout) if c == '\n'?

Normally in curses line buffering doesn't make any sense.
One paints the window and then does a refresh.
We want to add scrolling of the command line window on
top of that, but if the intent is for that to be handled by
gdb's standard set height mechanism (which could use
some TLC w.r.t. TUI), then the screen will be refreshed
at the "Type <return> to continue, ..." prompt.
I don't off hand know if TUI tries to give the user the
impression of scrolling if the user sets the height
to be larger than the physical command line window.
My impression is it doesn't.
And therefore, I think we don't need to do any call to
gdb_flush here.  Could be missing something though.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 17:57     ` Eli Zaretskii
@ 2015-01-07 18:09       ` Doug Evans
  2015-01-07 18:34         ` Eli Zaretskii
  0 siblings, 1 reply; 34+ messages in thread
From: Doug Evans @ 2015-01-07 18:09 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Pedro Alves, gdb-patches

On Wed, Jan 7, 2015 at 9:57 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Wed, 07 Jan 2015 15:17:54 +0000
>> From: Pedro Alves <palves@redhat.com>
>> CC: gdb-patches <gdb-patches@sourceware.org>
>>
>> > Reading symbols from foo...done.
>> > mumble ...
>> > (gdb)
>>
>> Since stdout is line buffered by default (on Unix), if this is
>> working when the TUI is disabled, then it must be because there's
>> explicit gdb_flush(gdb_stdout) after "Reading symbols from foo..."
>> is printed, right?
>
> Yes.
>
>> Isn't the issue then that the TUI's implementation of
>> gdb_flush (tui/tui-file.c) should be doing whatever it
>> needs to flush the output?
>
> Maybe.  I already made that change in my sandbox.  Alas, that's just
> the tip of the iceberg.  We call single-character output functions,
> like putchar and fputc all over the place, implicitly assuming that in
> certain cases (e.g., when the stream is stderr), things are unbuffered
> and the output appears immediately.  On top of that, functions that
> output strings go through single-character output versions, so there's
> no easy way of saying on, say, utils.c level whether the character
> should or shouldn't appear immediately. It's a terrible mess.
>
>> Should it be calling wrefresh if the file is gdb_stdout?
>
> Only if that stream is a real file, not a string or whatever else we
> support in ui-file.

tui_file_flush only gets called for tui files, so no worries there.

Though I see tui also has its own ui file strings (tui_sfileopen).
That can go I think.

>
>> If we do that, and change tui_puts like:
>>
>>  -  /* We could defer the following.  */
>>  -  wrefresh (w);
>>  -  fflush (stdout);
>>  +   if (c == '\n')
>>  +    gdb_flush (gdb_stdout);
>>
>> would it work?
>
> No, it's not enough.  There's also gdb_stderr and gdb_stdlog, at
> least. (tui_puts doesn't get the stream as its argument, so it
> doesn't really know which stream it is serving.)  I'm trying to make
> heads or tails out of this, but I'm not there yet.

Sure, but it does suggest tui_puts is the wrong place to do
any kind of flushing/refreshing.
Fortunately, tui_puts is internal to tui.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 18:00     ` Doug Evans
@ 2015-01-07 18:12       ` Doug Evans
  2015-01-07 18:34         ` Eli Zaretskii
  2015-01-07 18:21       ` Eli Zaretskii
  1 sibling, 1 reply; 34+ messages in thread
From: Doug Evans @ 2015-01-07 18:12 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Eli Zaretskii, gdb-patches

On Wed, Jan 7, 2015 at 10:00 AM, Doug Evans <dje@google.com> wrote:
> Do we need to do gdb_flush (gdb_stdout) if c == '\n'?
>
> Normally in curses line buffering doesn't make any sense.
> One paints the window and then does a refresh.
> We want to add scrolling of the command line window on
> top of that, but if the intent is for that to be handled by
> gdb's standard set height mechanism (which could use
> some TLC w.r.t. TUI), then the screen will be refreshed
> at the "Type <return> to continue, ..." prompt.
> I don't off hand know if TUI tries to give the user the
> impression of scrolling if the user sets the height
> to be larger than the physical command line window.
> My impression is it doesn't.
> And therefore, I think we don't need to do any call to
> gdb_flush here.  Could be missing something though.

Well, there is unfiltered output which doesn't
go through pagination.
Fun fun fun.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 18:00     ` Doug Evans
  2015-01-07 18:12       ` Doug Evans
@ 2015-01-07 18:21       ` Eli Zaretskii
  2015-01-07 18:56         ` Doug Evans
  1 sibling, 1 reply; 34+ messages in thread
From: Eli Zaretskii @ 2015-01-07 18:21 UTC (permalink / raw)
  To: Doug Evans; +Cc: palves, gdb-patches

> Date: Wed, 7 Jan 2015 10:00:32 -0800
> From: Doug Evans <dje@google.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches <gdb-patches@sourceware.org>
> 
> >  -  /* We could defer the following.  */
> >  -  wrefresh (w);
> >  -  fflush (stdout);
> >  +   if (c == '\n')
> >  +    gdb_flush (gdb_stdout);
> >
> > would it work?
> 
> TBH I'm not entirely sure why the fflush (stdout) is there.

Obviously, Someone(TM) relied on the fact that on Unix curses writes
to stdout.  There are many such places in the code, not only in TUI.

> [There's another one in the file as well.]

There are many.

> So, yeah, it seems like gdb_flush for tui needs to do a wrefresh
> (for gdb_stdout/stderr (/stdlog?))

At least, see my other message.  And it should treat them differently,
since their buffering is different, and the rest of GDB relies on that
a lot.  And tui_puts currently doesn't know which stream it serves.

> Do we need to do gdb_flush (gdb_stdout) if c == '\n'?

As I explained, this is not enough.

> Normally in curses line buffering doesn't make any sense.

??? The whole point of this discussion is to introduce some kind of
buffering to avoid terribly slow performance in TUI mode.

> One paints the window and then does a refresh.

Except that for the command window, there's no such thing as "paint
the window", AFAICS.  Various parts of GDB write out various text
strings whenever they feel like.  You don't have a single point where
you can tell the window is done.  Currently, we simply refresh every
character.  My original suggestion to use a newline as such a point
was an attempt to find an easy solution, and we already know why that
is not good enough.

> We want to add scrolling of the command line window on
> top of that, but if the intent is for that to be handled by
> gdb's standard set height mechanism (which could use
> some TLC w.r.t. TUI), then the screen will be refreshed
> at the "Type <return> to continue, ..." prompt.

Yes, of course.  And this is part of the problem that needs to be
solved.

> I don't off hand know if TUI tries to give the user the
> impression of scrolling if the user sets the height
> to be larger than the physical command line window.

It doesn't.  Stuff just scrolls off the screen and is lost forever.

> And therefore, I think we don't need to do any call to
> gdb_flush here.  Could be missing something though.

Where's "there"?

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-06 20:54     ` Doug Evans
@ 2015-01-07 18:30       ` Eli Zaretskii
  2015-01-07 19:08         ` Doug Evans
  0 siblings, 1 reply; 34+ messages in thread
From: Eli Zaretskii @ 2015-01-07 18:30 UTC (permalink / raw)
  To: Doug Evans; +Cc: gdb-patches

> Date: Tue, 6 Jan 2015 12:54:02 -0800
> From: Doug Evans <dje@google.com>
> Cc: gdb-patches <gdb-patches@sourceware.org>
> 
> >> 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.
> >
> > This will hit the same problem: how to know when to stop accumulating
> > and flush it out.
> 
> What I'm saying is tui_puts would still call wrefresh unconditionally.
> This is no different than your patch, conceptually, except that
> it'd be up to higher layers to not send tui_puts a character at a time for the
> common case of outputting strings.

In the current implementation, tui_puts almost _always_ gets a single
character.  This is because all the *_unfiltered output functions
eventually call null_write, which calls the to_fputs method one
character at a time (and has some apology to defend that).

> We send output a character at a time in part to support
> lines_per_page, chars_per_line, and wrap_column,
> relying on character-at-a-time buffering to save us.
> Alas for windows that doesn't apparently doesn't work (yay windows).

The problem is not Windows per se, it's the Windows console driver
implemented as part of ncurses.

> So one way to go, and again, this is just a possibility,
> is to not send tui_puts a character at a time.

This means a major redesign of how ui-file and friends work, much more
than the energy and time I have to spend on this issue.

> Is it possible to fix windows?
> If the bug really is there ...

It isn't.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 18:12       ` Doug Evans
@ 2015-01-07 18:34         ` Eli Zaretskii
  0 siblings, 0 replies; 34+ messages in thread
From: Eli Zaretskii @ 2015-01-07 18:34 UTC (permalink / raw)
  To: Doug Evans; +Cc: palves, gdb-patches

> Date: Wed, 7 Jan 2015 10:12:36 -0800
> From: Doug Evans <dje@google.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches <gdb-patches@sourceware.org>
> 
> On Wed, Jan 7, 2015 at 10:00 AM, Doug Evans <dje@google.com> wrote:
> > Do we need to do gdb_flush (gdb_stdout) if c == '\n'?
> >
> > Normally in curses line buffering doesn't make any sense.
> > One paints the window and then does a refresh.
> > We want to add scrolling of the command line window on
> > top of that, but if the intent is for that to be handled by
> > gdb's standard set height mechanism (which could use
> > some TLC w.r.t. TUI), then the screen will be refreshed
> > at the "Type <return> to continue, ..." prompt.
> > I don't off hand know if TUI tries to give the user the
> > impression of scrolling if the user sets the height
> > to be larger than the physical command line window.
> > My impression is it doesn't.
> > And therefore, I think we don't need to do any call to
> > gdb_flush here.  Could be missing something though.
> 
> Well, there is unfiltered output which doesn't
> go through pagination.
> Fun fun fun.

Yes, lots of it.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 18:09       ` Doug Evans
@ 2015-01-07 18:34         ` Eli Zaretskii
  2015-01-07 19:16           ` Doug Evans
  0 siblings, 1 reply; 34+ messages in thread
From: Eli Zaretskii @ 2015-01-07 18:34 UTC (permalink / raw)
  To: Doug Evans; +Cc: palves, gdb-patches

> Date: Wed, 7 Jan 2015 10:09:03 -0800
> From: Doug Evans <dje@google.com>
> Cc: Pedro Alves <palves@redhat.com>, gdb-patches <gdb-patches@sourceware.org>
> 
> >> Should it be calling wrefresh if the file is gdb_stdout?
> >
> > Only if that stream is a real file, not a string or whatever else we
> > support in ui-file.
> 
> tui_file_flush only gets called for tui files

That's not really true, as you yourself point out:

> Though I see tui also has its own ui file strings (tui_sfileopen).
> That can go I think.

I don't see how it could go: it's used.

> >> If we do that, and change tui_puts like:
> >>
> >>  -  /* We could defer the following.  */
> >>  -  wrefresh (w);
> >>  -  fflush (stdout);
> >>  +   if (c == '\n')
> >>  +    gdb_flush (gdb_stdout);
> >>
> >> would it work?
> >
> > No, it's not enough.  There's also gdb_stderr and gdb_stdlog, at
> > least. (tui_puts doesn't get the stream as its argument, so it
> > doesn't really know which stream it is serving.)  I'm trying to make
> > heads or tails out of this, but I'm not there yet.
> 
> Sure, but it does suggest tui_puts is the wrong place to do
> any kind of flushing/refreshing.

How else can you emulate the unbuffered nature of gdb_stderr and the
line-buffered nature of gdb_stdout, for example?  The GDB application
code implicitly expects that.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 18:21       ` Eli Zaretskii
@ 2015-01-07 18:56         ` Doug Evans
  2015-01-07 19:11           ` Eli Zaretskii
  0 siblings, 1 reply; 34+ messages in thread
From: Doug Evans @ 2015-01-07 18:56 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Pedro Alves, gdb-patches

On Wed, Jan 7, 2015 at 10:22 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Wed, 7 Jan 2015 10:00:32 -0800
>> From: Doug Evans <dje@google.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches <gdb-patches@sourceware.org>
>>
>> >  -  /* We could defer the following.  */
>> >  -  wrefresh (w);
>> >  -  fflush (stdout);
>> >  +   if (c == '\n')
>> >  +    gdb_flush (gdb_stdout);
>> >
>> > would it work?
>>
>> TBH I'm not entirely sure why the fflush (stdout) is there.
>
> Obviously, Someone(TM) relied on the fact that on Unix curses writes
> to stdout.  There are many such places in the code, not only in TUI.

Except that (and I'm relying on memory here so heads up),
curses programs don't need or do fflush (stdout).  Even Unix ones.

>> So, yeah, it seems like gdb_flush for tui needs to do a wrefresh
>> (for gdb_stdout/stderr (/stdlog?))
>
> At least, see my other message.  And it should treat them differently,
> since their buffering is different, and the rest of GDB relies on that
> a lot.  And tui_puts currently doesn't know which stream it serves.

It's easy enough to make tui_puts know what stream it's serving though,
so I'm not worried about that.

>> Normally in curses line buffering doesn't make any sense.
>
> ??? The whole point of this discussion is to introduce some kind of
> buffering to avoid terribly slow performance in TUI mode.

Yes, I know.

For reference sake, by "line buffering" I'm referring to stdio
line buffering.  Curses is different.  It handles buffering differently.
One writes to various windows/frames/whatever (I can't remember
this part of curses terminology), that can be anywhere on the screen,
or even write directly to specific rows/columns,
and then when one does a refresh curses optimizes the "transmission"
of characters to the #rows-by-#columns grid of characters that the
user sees.  There is no "line" as far as the physical connection
to the screen goes.  Way back when, ttys and 1200 baud connections
made this optimization important. :-)

>
>> One paints the window and then does a refresh.
>
> Except that for the command window, there's no such thing as "paint
> the window", AFAICS.

By "paint the window" I mean writing something into that part of the
screen.

> Various parts of GDB write out various text
> strings whenever they feel like.  You don't have a single point where
> you can tell the window is done.  Currently, we simply refresh every
> character.  My original suggestion to use a newline as such a point
> was an attempt to find an easy solution, and we already know why that
> is not good enough.

There's no "single" point, but OTOH there aren't many points either.

>> We want to add scrolling of the command line window on
>> top of that, but if the intent is for that to be handled by
>> gdb's standard set height mechanism (which could use
>> some TLC w.r.t. TUI), then the screen will be refreshed
>> at the "Type <return> to continue, ..." prompt.
>
> Yes, of course.  And this is part of the problem that needs to be
> solved.

Except that scrolling isn't straightforward in curses land.
Do we "scroll" unfiltered output (that hasn't gone through
the paginator) ?

>> I don't off hand know if TUI tries to give the user the
>> impression of scrolling if the user sets the height
>> to be larger than the physical command line window.
>
> It doesn't.  Stuff just scrolls off the screen and is lost forever.

Yeah, so we need a scroll buffer for TUI if we want
to solve that.

>> And therefore, I think we don't need to do any call to
>> gdb_flush here.  Could be missing something though.
>
> Where's "there"?

tui_puts, at least in its current implementation.
It's just an implementation detail though, so
depending on how we fix things that can change.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 18:30       ` Eli Zaretskii
@ 2015-01-07 19:08         ` Doug Evans
  2015-01-07 19:20           ` Eli Zaretskii
  0 siblings, 1 reply; 34+ messages in thread
From: Doug Evans @ 2015-01-07 19:08 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: gdb-patches

On Wed, Jan 7, 2015 at 10:30 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> The problem is not Windows per se, it's the Windows console driver
> implemented as part of ncurses.
>
>> So one way to go, and again, this is just a possibility,
>> is to not send tui_puts a character at a time.
>
> This means a major redesign of how ui-file and friends work, much more
> than the energy and time I have to spend on this issue.

Depends on how one approaches it.
I wasn't thinking of a major redesign when I said that.
If one wants to classify *any* change to the ui-file API as major, go for it,
but to me some changes are easier than others.
[It's not clear to me yet that any change will be necessary, btw.]

Note that while we do explicitly call *_unfiltered with single characters,
unfiltered output is not in itself broken up into single characters.

>> Is it possible to fix windows?
>> If the bug really is there ...
>
> It isn't.

This is good data to have.  Thanks.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 18:56         ` Doug Evans
@ 2015-01-07 19:11           ` Eli Zaretskii
  0 siblings, 0 replies; 34+ messages in thread
From: Eli Zaretskii @ 2015-01-07 19:11 UTC (permalink / raw)
  To: Doug Evans; +Cc: palves, gdb-patches

> Date: Wed, 7 Jan 2015 10:56:11 -0800
> From: Doug Evans <dje@google.com>
> Cc: Pedro Alves <palves@redhat.com>, gdb-patches <gdb-patches@sourceware.org>
> 
> >> So, yeah, it seems like gdb_flush for tui needs to do a wrefresh
> >> (for gdb_stdout/stderr (/stdlog?))
> >
> > At least, see my other message.  And it should treat them differently,
> > since their buffering is different, and the rest of GDB relies on that
> > a lot.  And tui_puts currently doesn't know which stream it serves.
> 
> It's easy enough to make tui_puts know what stream it's serving though,

Yes, and then we'd need to figure out which stream to pass to it in
its other callers (there are 6 of them).

> >> One paints the window and then does a refresh.
> >
> > Except that for the command window, there's no such thing as "paint
> > the window", AFAICS.
> 
> By "paint the window" I mean writing something into that part of the
> screen.

This is only known on the upper level of the GDB code, the one that
actually generates the stuff that is to be written on the screen.
Lower levels have no idea whether they were called to output a single
character that is just a single character, or a part of a string.

And since there are literally gazillions of calls to output functions
on the higher level, it is only natural to try to fix this in a
smaller number of places, which means on lower levels.  I still hope
it's possible.  Maybe I'm dreaming.

> > Various parts of GDB write out various text
> > strings whenever they feel like.  You don't have a single point where
> > you can tell the window is done.  Currently, we simply refresh every
> > character.  My original suggestion to use a newline as such a point
> > was an attempt to find an easy solution, and we already know why that
> > is not good enough.
> 
> There's no "single" point, but OTOH there aren't many points either.

Not sure what you mean here.  Can you identify those points, or
explain how to identify them?

> >> We want to add scrolling of the command line window on
> >> top of that, but if the intent is for that to be handled by
> >> gdb's standard set height mechanism (which could use
> >> some TLC w.r.t. TUI), then the screen will be refreshed
> >> at the "Type <return> to continue, ..." prompt.
> >
> > Yes, of course.  And this is part of the problem that needs to be
> > solved.
> 
> Except that scrolling isn't straightforward in curses land.
> Do we "scroll" unfiltered output (that hasn't gone through
> the paginator) ?

We don't, but every text terminal window on any modern system has a
scroll-back buffer and a scroll bar to make use of it.  When you are
in TUI mode, this is lost.

> >> And therefore, I think we don't need to do any call to
> >> gdb_flush here.  Could be missing something though.
> >
> > Where's "there"?
> 
> tui_puts, at least in its current implementation.
> It's just an implementation detail though, so
> depending on how we fix things that can change.

Again, I don't understand how else can you emulate the standard
buffering behavior of the standard streams, which we rely upon in too
many places.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 18:34         ` Eli Zaretskii
@ 2015-01-07 19:16           ` Doug Evans
  2015-01-07 19:32             ` Eli Zaretskii
  0 siblings, 1 reply; 34+ messages in thread
From: Doug Evans @ 2015-01-07 19:16 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Pedro Alves, gdb-patches

On Wed, Jan 7, 2015 at 10:34 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Wed, 7 Jan 2015 10:09:03 -0800
>> From: Doug Evans <dje@google.com>
>> Cc: Pedro Alves <palves@redhat.com>, gdb-patches <gdb-patches@sourceware.org>
>>
>> >> Should it be calling wrefresh if the file is gdb_stdout?
>> >
>> > Only if that stream is a real file, not a string or whatever else we
>> > support in ui-file.
>>
>> tui_file_flush only gets called for tui files
>
> That's not really true, as you yourself point out:
>
>> Though I see tui also has its own ui file strings (tui_sfileopen).
>> That can go I think.
>
> I don't see how it could go: it's used.

Can't it be replaced with mem_fileopen?
Maybe I'm missing something.

>> >> If we do that, and change tui_puts like:
>> >>
>> >>  -  /* We could defer the following.  */
>> >>  -  wrefresh (w);
>> >>  -  fflush (stdout);
>> >>  +   if (c == '\n')
>> >>  +    gdb_flush (gdb_stdout);
>> >>
>> >> would it work?
>> >
>> > No, it's not enough.  There's also gdb_stderr and gdb_stdlog, at
>> > least. (tui_puts doesn't get the stream as its argument, so it
>> > doesn't really know which stream it is serving.)  I'm trying to make
>> > heads or tails out of this, but I'm not there yet.
>>
>> Sure, but it does suggest tui_puts is the wrong place to do
>> any kind of flushing/refreshing.
>
> How else can you emulate the unbuffered nature of gdb_stderr and the
> line-buffered nature of gdb_stdout, for example?  The GDB application
> code implicitly expects that.

If one wants to restrict the problem to just gdb_stderr vs gdb_stdout
(which is insufficient, since gdb_stdout gets unfiltered output),
but for discussion's sake ...
The tui to_puts method, tui_file_fputs, knows what the stream is.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 19:08         ` Doug Evans
@ 2015-01-07 19:20           ` Eli Zaretskii
  2015-01-07 19:30             ` Doug Evans
  0 siblings, 1 reply; 34+ messages in thread
From: Eli Zaretskii @ 2015-01-07 19:20 UTC (permalink / raw)
  To: Doug Evans; +Cc: gdb-patches

> Date: Wed, 7 Jan 2015 11:08:41 -0800
> From: Doug Evans <dje@google.com>
> Cc: gdb-patches <gdb-patches@sourceware.org>
> 
> On Wed, Jan 7, 2015 at 10:30 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> > The problem is not Windows per se, it's the Windows console driver
> > implemented as part of ncurses.
> >
> >> So one way to go, and again, this is just a possibility,
> >> is to not send tui_puts a character at a time.
> >
> > This means a major redesign of how ui-file and friends work, much more
> > than the energy and time I have to spend on this issue.
> 
> Depends on how one approaches it.

If you have ideas how to do that without a major effort, please tell.
I was talking based on several hours of staring at that stuff, but
perhaps I missed something.

> Note that while we do explicitly call *_unfiltered with single characters,
> unfiltered output is not in itself broken up into single characters.

Not sure what you mean by that.  fputs_maybe_filtered, which is the
workhorse of most of the output functions, explicitly writes out the
stuff it gets one character at a time, by calling fputc_unfiltered.
How's that not breaking output?

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 19:20           ` Eli Zaretskii
@ 2015-01-07 19:30             ` Doug Evans
  2015-01-07 19:48               ` Eli Zaretskii
  0 siblings, 1 reply; 34+ messages in thread
From: Doug Evans @ 2015-01-07 19:30 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: gdb-patches

On Wed, Jan 7, 2015 at 11:20 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Wed, 7 Jan 2015 11:08:41 -0800
>> From: Doug Evans <dje@google.com>
>> Cc: gdb-patches <gdb-patches@sourceware.org>
>>
>> On Wed, Jan 7, 2015 at 10:30 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> > The problem is not Windows per se, it's the Windows console driver
>> > implemented as part of ncurses.
>> >
>> >> So one way to go, and again, this is just a possibility,
>> >> is to not send tui_puts a character at a time.
>> >
>> > This means a major redesign of how ui-file and friends work, much more
>> > than the energy and time I have to spend on this issue.
>>
>> Depends on how one approaches it.
>
> If you have ideas how to do that without a major effort, please tell.
> I was talking based on several hours of staring at that stuff, but
> perhaps I missed something.

I can't promise to have a patch ready before 7.9, but I'll put it
on my todo list.

>> Note that while we do explicitly call *_unfiltered with single characters,
>> unfiltered output is not in itself broken up into single characters.
>
> Not sure what you mean by that.  fputs_maybe_filtered, which is the
> workhorse of most of the output functions, explicitly writes out the
> stuff it gets one character at a time, by calling fputc_unfiltered.
> How's that not breaking output?

fputs_maybe_filtered is the workhorse for filtered output, and
it early exists for a number of things, like stream != gdb_stdout.
Most unfiltered output goes straight to fputs_unfiltered (which is the
wrapper around the ui-file to_fputs method).
I see there is fputstr{,n}_unfiltered which does things a character
at a time but it looks like it is only used by MI.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 19:16           ` Doug Evans
@ 2015-01-07 19:32             ` Eli Zaretskii
  2015-01-07 22:30               ` Pedro Alves
  0 siblings, 1 reply; 34+ messages in thread
From: Eli Zaretskii @ 2015-01-07 19:32 UTC (permalink / raw)
  To: Doug Evans; +Cc: palves, gdb-patches

> Date: Wed, 7 Jan 2015 11:15:59 -0800
> From: Doug Evans <dje@google.com>
> Cc: Pedro Alves <palves@redhat.com>, gdb-patches <gdb-patches@sourceware.org>
> 
> >> Sure, but it does suggest tui_puts is the wrong place to do
> >> any kind of flushing/refreshing.
> >
> > How else can you emulate the unbuffered nature of gdb_stderr and the
> > line-buffered nature of gdb_stdout, for example?  The GDB application
> > code implicitly expects that.
> 
> If one wants to restrict the problem to just gdb_stderr vs gdb_stdout
> (which is insufficient, since gdb_stdout gets unfiltered output),
> but for discussion's sake ...
> The tui to_puts method, tui_file_fputs, knows what the stream is.

So?  How does this help?  The stderr problem is simple, but what do
you do with stdout and stdlog?  Especially when tui_file_fputs is
called one character at a time in most cases.

We are going in circles.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 19:30             ` Doug Evans
@ 2015-01-07 19:48               ` Eli Zaretskii
  2015-01-07 20:45                 ` Doug Evans
  0 siblings, 1 reply; 34+ messages in thread
From: Eli Zaretskii @ 2015-01-07 19:48 UTC (permalink / raw)
  To: Doug Evans; +Cc: gdb-patches

> Date: Wed, 7 Jan 2015 11:30:37 -0800
> From: Doug Evans <dje@google.com>
> Cc: gdb-patches <gdb-patches@sourceware.org>
> 
> I can't promise to have a patch ready before 7.9, but I'll put it
> on my todo list.

Thanks.

> >> Note that while we do explicitly call *_unfiltered with single characters,
> >> unfiltered output is not in itself broken up into single characters.
> >
> > Not sure what you mean by that.  fputs_maybe_filtered, which is the
> > workhorse of most of the output functions, explicitly writes out the
> > stuff it gets one character at a time, by calling fputc_unfiltered.
> > How's that not breaking output?
> 
> fputs_maybe_filtered is the workhorse for filtered output, and
> it early exists for a number of things, like stream != gdb_stdout.

It is used both for gdb_stdout and for other streams.

> Most unfiltered output goes straight to fputs_unfiltered (which is the
> wrapper around the ui-file to_fputs method).

Once again, I fail to see how this fact, which I certainly noticed
already, helps to solve the problem.  When fputs_maybe_filtered is
called, we have no idea whether the text it gets is complete enough to
flush the stream after we are done with it.  That information is at
higher levels.  So we don't know whether to flush the stream after the
direct call to fputs_unfiltered.

> I see there is fputstr{,n}_unfiltered which does things a character
> at a time but it looks like it is only used by MI.

Grep for fputs_unfiltered callers, and you will see that there are
much more calls with only 1 or 2 characters.

Anyway, if we want to analyze the current callers and build our
solution on that, we will be limited to what we currently have.  Not
sure if this is good or bad, but that's exactly what I am trying to
do, and I still have some lose ends in the heuristics.

But a general solution is not possible that way, because a general
solution cannot depend on who does or doesn't call certain functions.
That general solution is what looked hard to me.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 19:48               ` Eli Zaretskii
@ 2015-01-07 20:45                 ` Doug Evans
  2015-01-07 21:59                   ` Doug Evans
  0 siblings, 1 reply; 34+ messages in thread
From: Doug Evans @ 2015-01-07 20:45 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: gdb-patches

On Wed, Jan 7, 2015 at 11:48 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Wed, 7 Jan 2015 11:30:37 -0800
>> From: Doug Evans <dje@google.com>
>> Cc: gdb-patches <gdb-patches@sourceware.org>
>>
>> I can't promise to have a patch ready before 7.9, but I'll put it
>> on my todo list.
>
> Thanks.
>
>> >> Note that while we do explicitly call *_unfiltered with single characters,
>> >> unfiltered output is not in itself broken up into single characters.
>> >
>> > Not sure what you mean by that.  fputs_maybe_filtered, which is the
>> > workhorse of most of the output functions, explicitly writes out the
>> > stuff it gets one character at a time, by calling fputc_unfiltered.
>> > How's that not breaking output?
>>
>> fputs_maybe_filtered is the workhorse for filtered output, and
>> it early exists for a number of things, like stream != gdb_stdout.
>
> It is used both for gdb_stdout and for other streams.

static void
fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
                      int filter)
{
  const char *lineptr;

  if (linebuffer == 0)
    return;

  /* Don't do any filtering if it is disabled.  */
  if (stream != gdb_stdout
      || !pagination_enabled
      || batch_flag
      || (lines_per_page == UINT_MAX && chars_per_line == UINT_MAX)
      || top_level_interpreter () == NULL
      || ui_out_is_mi_like_p (interp_ui_out (top_level_interpreter ())))
    {
      fputs_unfiltered (linebuffer, stream);
      return;
    }

  ...
}

It may be called for other streams, but it early exits
with a direct call to fputs_unfiltered if stream != gdb_stdout.

I don't know how to say it any plainer than that.

>
>> Most unfiltered output goes straight to fputs_unfiltered (which is the
>> wrapper around the ui-file to_fputs method).
>
> Once again, I fail to see how this fact, which I certainly noticed
> already, helps to solve the problem.  When fputs_maybe_filtered is
> called, we have no idea whether the text it gets is complete enough to
> flush the stream after we are done with it.  That information is at
> higher levels.  So we don't know whether to flush the stream after the
> direct call to fputs_unfiltered.

My theory is we can punt for streams != gdb_stdout,
and for them just always refresh inside calls to tui_file_fputs.
And for gdb_stdout rely on calls to gdb_flush to do
the refresh.

Seems like not that many lines of code to give it a try.

>
>> I see there is fputstr{,n}_unfiltered which does things a character
>> at a time but it looks like it is only used by MI.
>
> Grep for fputs_unfiltered callers, and you will see that there are
> much more calls with only 1 or 2 characters.

I already did, and pointed that out.
We don't know yet if they're a problem.
Seems like the main source of the problem would be gdb_stdout,
so let's try to fix that first and go from there.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 20:45                 ` Doug Evans
@ 2015-01-07 21:59                   ` Doug Evans
  2015-01-19 17:55                     ` Eli Zaretskii
  0 siblings, 1 reply; 34+ messages in thread
From: Doug Evans @ 2015-01-07 21:59 UTC (permalink / raw)
  To: Eli Zaretskii, gdb-patches

Doug Evans writes:
 > Seems like the main source of the problem would be gdb_stdout,
 > so let's try to fix that first and go from there.

Here's a prototype.
I left fflush (stdout) in to be conservative.

Does this resolve the performance issues you're seeing well enough?

2015-01-07  Doug Evans  <dje@google.com>

	PR tui/17810
	* tui/tui-command.c (tui_refresh_cmd_win): New function.
	* tui/tui-command.c (tui_refresh_cmd_win): Declare.
	* tui/tui-file.c: #include tui/tui-command.h.
	(tui_file_fputs): Refresh command window if stream is not gdb_stdout.
	(tui_file_flush): Refresh command window if stream is gdb_stdout or
	gdb_stderr.
	* tui/tui-io.c (tui_puts): Remove call to wrefresh, fflush.
	(tui_readline_output): Call tui_refresh_cmd_win.
	(print_filename): Ditto.
	(tui_rl_display_match_list): Ditto.

diff --git a/gdb/tui/tui-command.c b/gdb/tui/tui-command.c
index d1a5ddb..6d98db2 100644
--- a/gdb/tui/tui-command.c
+++ b/gdb/tui/tui-command.c
@@ -129,3 +129,18 @@ tui_dispatch_ctrl_char (unsigned int ch)
       return c;
     }
 }
+
+/* Refresh the command window.  */
+
+void
+tui_refresh_cmd_win (void)
+{
+  WINDOW *w = TUI_CMD_WIN->generic.handle;
+
+  wrefresh (w);
+
+  /* FIXME: It's not clear why this is here.
+     It was present in the original tui_puts code and is kept in order to
+     not introduce some subtle breakage.  */
+  fflush (stdout);
+}
diff --git a/gdb/tui/tui-command.h b/gdb/tui/tui-command.h
index 8cc5be4..174ba58 100644
--- a/gdb/tui/tui-command.h
+++ b/gdb/tui/tui-command.h
@@ -24,4 +24,6 @@
 
 extern unsigned int tui_dispatch_ctrl_char (unsigned int);
 
+extern void tui_refresh_cmd_win (void);
+
 #endif
diff --git a/gdb/tui/tui-file.c b/gdb/tui/tui-file.c
index b32cfa6..02d9082 100644
--- a/gdb/tui/tui-file.c
+++ b/gdb/tui/tui-file.c
@@ -20,7 +20,7 @@
 #include "ui-file.h"
 #include "tui/tui-file.h"
 #include "tui/tui-io.h"
-
+#include "tui/tui-command.h"
 #include "tui.h"
 
 /* A ``struct ui_file'' that is compatible with all the legacy
@@ -179,6 +179,10 @@ tui_file_fputs (const char *linebuffer, struct ui_file *file)
   else
     {
       tui_puts (linebuffer);
+      /* gdb_stdout is buffered, and the caller must gdb_flush it at
+	 appropriate times.  Other streams are not so buffered.  */
+      if (file != gdb_stdout)
+	tui_refresh_cmd_win ();
     }
 }
 
@@ -239,6 +243,12 @@ tui_file_flush (struct ui_file *file)
     case astring:
       break;
     case afile:
+      /* There is also gdb_stdlog, gdb_stdtarg, gdb_stdtargerr, but
+	 tui_setup_io maps those to gdb_stderr.  OTOH, do we need to make
+	 this conditional?  */
+      if (file == gdb_stdout
+	  || file == gdb_stderr)
+	tui_refresh_cmd_win ();
       fflush (stream->ts_filestream);
       break;
     }
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 233e7a6..c3ab612 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -158,7 +158,10 @@ tui_putc (char c)
   tui_puts (buf);
 }
 
-/* Print the string in the curses command window.  */
+/* Print the string in the curses command window.
+   The output is buffered.  It is up to the caller to refresh the screen
+   if necessary.  */
+
 void
 tui_puts (const char *string)
 {
@@ -187,10 +190,6 @@ tui_puts (const char *string)
          TUI_CMD_WIN->detail.command_info.curch);
   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.
@@ -316,6 +315,7 @@ tui_readline_output (int error, gdb_client_data data)
     {
       buf[size] = 0;
       tui_puts (buf);
+      tui_refresh_cmd_win ();
     }
 }
 #endif
@@ -367,6 +367,7 @@ print_filename (const char *to_print, const char *full_pathname)
     {
       PUTX (*s);
     }
+  tui_refresh_cmd_win ();
   return printed_len;
 }
 
@@ -425,6 +426,7 @@ tui_rl_display_match_list (char **matches, int len, int max)
       if (get_y_or_n () == 0)
 	{
 	  tui_puts ("\n");
+	  tui_refresh_cmd_win ();
 	  return;
 	}
     }

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 19:32             ` Eli Zaretskii
@ 2015-01-07 22:30               ` Pedro Alves
  2015-01-19 17:52                 ` Eli Zaretskii
  0 siblings, 1 reply; 34+ messages in thread
From: Pedro Alves @ 2015-01-07 22:30 UTC (permalink / raw)
  To: Eli Zaretskii, Doug Evans; +Cc: gdb-patches

On 01/07/2015 07:32 PM, Eli Zaretskii wrote:
> So?  How does this help?  The stderr problem is simple, but what do
> you do with stdout and stdlog?  Especially when tui_file_fputs is
> called one character at a time in most cases.

I'm confused on that the confusion is.  :-)

I think something like this should work.  I'm not really seeing a point
for those fflush(stdout)'s, btw.  Note that tui_stream->ts_filestream is
only ever used in tui_file_isatty; it's never written to.

From 2b4d8cfb56d67f9eded113a120dc5c18ea3781cc Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 7 Jan 2015 19:50:48 +0000
Subject: [PATCH] TUI emulate line buffering

when printing to stdout, only refresh the console window when either a
new line is seen or when an explicit flush is requested.
---
 gdb/tui/tui-file.c |  5 +++--
 gdb/tui/tui-io.c   | 53 +++++++++++++++++++++++++++++++++++++++++++++--------
 gdb/tui/tui-io.h   | 10 ++++++++--
 3 files changed, 56 insertions(+), 12 deletions(-)

diff --git a/gdb/tui/tui-file.c b/gdb/tui/tui-file.c
index b32cfa6..932df46 100644
--- a/gdb/tui/tui-file.c
+++ b/gdb/tui/tui-file.c
@@ -178,7 +178,7 @@ tui_file_fputs (const char *linebuffer, struct ui_file *file)
     }
   else
     {
-      tui_puts (linebuffer);
+      tui_puts_file (file, linebuffer);
     }
 }

@@ -239,7 +239,8 @@ tui_file_flush (struct ui_file *file)
     case astring:
       break;
     case afile:
-      fflush (stream->ts_filestream);
+      if (file == tui_stdout)
+	tui_flush_stdout ();
       break;
     }
 }
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 233e7a6..e7c5583 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -118,7 +118,7 @@ key_is_command_char (int ch)
 /* #undef TUI_USE_PIPE_FOR_READLINE */

 /* TUI output files.  */
-static struct ui_file *tui_stdout;
+struct ui_file *tui_stdout;
 static struct ui_file *tui_stderr;
 struct ui_out *tui_out;

@@ -148,6 +148,17 @@ static char *tui_rl_saved_prompt;

 static unsigned int tui_handle_resize_during_io (unsigned int);

+static void tui_puts (const char *string);
+
+void
+tui_flush_stdout (void)
+{
+  WINDOW *w;
+
+  w = TUI_CMD_WIN->generic.handle;
+  wrefresh (w);
+}
+
 static void
 tui_putc (char c)
 {
@@ -158,9 +169,13 @@ tui_putc (char c)
   tui_puts (buf);
 }

-/* Print the string in the curses command window.  */
-void
-tui_puts (const char *string)
+/* Print the string in the curses command window.  If LINEBUFFERED,
+   we're printing to stdout, and so like stdio, should only flush when
+   we see a new line.  If LINEBUFFERED is false, we'll flush after
+   printing the whole string.  */
+
+static void
+tui_puts_maybe_buffered (const char *string, int linebuffered)
 {
   static int tui_skip_line = -1;
   char c;
@@ -182,15 +197,36 @@ tui_puts (const char *string)
         }
       else if (c == '\n')
         tui_skip_line = -1;
+
+      if (linebuffered && c == '\n')
+	wrefresh (w);
     }
   getyx (w, TUI_CMD_WIN->detail.command_info.cur_line,
          TUI_CMD_WIN->detail.command_info.curch);
   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);
+  if (!linebuffered)
+    wrefresh (w);
+}
+
+/* Print the string in the curses command window, as if printing to
+   stdout.  */
+
+static void
+tui_puts (const char *string)
+{
+  tui_puts_maybe_buffered (string, 1);
+}
+
+/* Print the string in the curses command window.  FILE indicates
+   which file the caller is printing to.  If gdb_stdout, then emulate
+   line buffering.  */
+
+void
+tui_puts_file (struct ui_file *file, const char *string)
+{
+  tui_puts_maybe_buffered (string, file == tui_stdout);
 }

 /* Readline callback.
@@ -279,7 +315,6 @@ tui_redisplay_readline (void)
   TUI_CMD_WIN->detail.command_info.start_line -= height - 1;

   wrefresh (w);
-  fflush(stdout);
 }

 /* Readline callback to prepare the terminal.  It is called once each
@@ -412,6 +447,7 @@ tui_rl_display_match_list (char **matches, int len, int max)

   /* Screen dimension correspond to the TUI command window.  */
   int screenwidth = TUI_CMD_WIN->generic.width;
+  WINDOW *w = TUI_CMD_WIN->generic.handle;

   /* If there are many items, then ask the user if she really wants to
      see them all.  */
@@ -422,6 +458,7 @@ tui_rl_display_match_list (char **matches, int len, int max)
       xsnprintf (msg, sizeof (msg),
 		 "\nDisplay all %d possibilities? (y or n)", len);
       tui_puts (msg);
+      tui_flush_stdout ();
       if (get_y_or_n () == 0)
 	{
 	  tui_puts ("\n");
diff --git a/gdb/tui/tui-io.h b/gdb/tui/tui-io.h
index 8f96cfe..81d4337 100644
--- a/gdb/tui/tui-io.h
+++ b/gdb/tui/tui-io.h
@@ -24,8 +24,13 @@

 struct ui_out;

-/* Print the string in the curses command window.  */
-extern void tui_puts (const char *);
+/* Print the string in the curses command window.  FILE indicates
+   which file the caller is printing to.  If gdb_stdout, then emulate
+   line buffering.  */
+extern void tui_puts_file (struct ui_file *file, const char *string);
+
+/* Flush console output.  */
+extern void tui_flush_stdout (void);

 /* Setup the IO for curses or non-curses mode.  */
 extern void tui_setup_io (int mode);
@@ -41,6 +46,7 @@ extern int tui_getc (FILE *);
    changed the edited text.  */
 extern void tui_redisplay_readline (void);

+extern struct ui_file *tui_stdout;
 extern struct ui_out *tui_out;
 extern struct ui_out *tui_old_uiout;

-- 
1.9.3


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 22:30               ` Pedro Alves
@ 2015-01-19 17:52                 ` Eli Zaretskii
  0 siblings, 0 replies; 34+ messages in thread
From: Eli Zaretskii @ 2015-01-19 17:52 UTC (permalink / raw)
  To: Pedro Alves; +Cc: dje, gdb-patches

> Date: Wed, 07 Jan 2015 22:29:51 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: gdb-patches@sourceware.org
> 
> I think something like this should work.  I'm not really seeing a point
> for those fflush(stdout)'s, btw.  Note that tui_stream->ts_filestream is
> only ever used in tui_file_isatty; it's never written to.

This seems to fix the problem, thanks.  Note that I didn't try every
possible way of getting into fprintf_unfiltered and fputs_unfiltered,
there are too many of them.

A couple of minor comments below.

> @@ -239,7 +239,8 @@ tui_file_flush (struct ui_file *file)
>      case astring:
>        break;
>      case afile:
> -      fflush (stream->ts_filestream);
> +      if (file == tui_stdout)
> +	tui_flush_stdout ();
>        break;

I think this should be done for any file but stderr.

> @@ -412,6 +447,7 @@ tui_rl_display_match_list (char **matches, int len, int max)
> 
>    /* Screen dimension correspond to the TUI command window.  */
>    int screenwidth = TUI_CMD_WIN->generic.width;
> +  WINDOW *w = TUI_CMD_WIN->generic.handle;

Is this needed?

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-07 21:59                   ` Doug Evans
@ 2015-01-19 17:55                     ` Eli Zaretskii
  2015-01-19 18:32                       ` Doug Evans
  0 siblings, 1 reply; 34+ messages in thread
From: Eli Zaretskii @ 2015-01-19 17:55 UTC (permalink / raw)
  To: Doug Evans; +Cc: gdb-patches

> From: Doug Evans <dje@google.com>
> Date: Wed, 7 Jan 2015 13:59:06 -0800
> 
> Doug Evans writes:
>  > Seems like the main source of the problem would be gdb_stdout,
>  > so let's try to fix that first and go from there.
> 
> Here's a prototype.

Sorry for a long delay.  I tried this (and Pedro's) patch today.  They
both do the job, so one of them should IMO be committed, master and
branch.

> @@ -239,6 +243,12 @@ tui_file_flush (struct ui_file *file)
>      case astring:
>        break;
>      case afile:
> +      /* There is also gdb_stdlog, gdb_stdtarg, gdb_stdtargerr, but
> +	 tui_setup_io maps those to gdb_stderr.  OTOH, do we need to make
> +	 this conditional?  */
> +      if (file == gdb_stdout
> +	  || file == gdb_stderr)
> +	tui_refresh_cmd_win ();

I indeed think that the condition should be removed.  I see no need
for it: there's no reason to make any stream displayed on TUI more
than line-buffered.

Thanks.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-19 17:55                     ` Eli Zaretskii
@ 2015-01-19 18:32                       ` Doug Evans
  2015-01-31 21:37                         ` Eli Zaretskii
  0 siblings, 1 reply; 34+ messages in thread
From: Doug Evans @ 2015-01-19 18:32 UTC (permalink / raw)
  To: Eli Zaretskii, Pedro Alves; +Cc: gdb-patches

On Mon, Jan 19, 2015 at 9:55 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Doug Evans <dje@google.com>
>> Date: Wed, 7 Jan 2015 13:59:06 -0800
>>
>> Doug Evans writes:
>>  > Seems like the main source of the problem would be gdb_stdout,
>>  > so let's try to fix that first and go from there.
>>
>> Here's a prototype.
>
> Sorry for a long delay.  I tried this (and Pedro's) patch today.  They
> both do the job, so one of them should IMO be committed, master and
> branch.
>
>> @@ -239,6 +243,12 @@ tui_file_flush (struct ui_file *file)
>>      case astring:
>>        break;
>>      case afile:
>> +      /* There is also gdb_stdlog, gdb_stdtarg, gdb_stdtargerr, but
>> +      tui_setup_io maps those to gdb_stderr.  OTOH, do we need to make
>> +      this conditional?  */
>> +      if (file == gdb_stdout
>> +       || file == gdb_stderr)
>> +     tui_refresh_cmd_win ();
>
> I indeed think that the condition should be removed.  I see no need
> for it: there's no reason to make any stream displayed on TUI more
> than line-buffered.

I don't have a preference on which, with one condition.
If we're going to remove the fflush's let's do it as a separate patch.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-19 18:32                       ` Doug Evans
@ 2015-01-31 21:37                         ` Eli Zaretskii
  2015-02-03 17:52                           ` Pedro Alves
  0 siblings, 1 reply; 34+ messages in thread
From: Eli Zaretskii @ 2015-01-31 21:37 UTC (permalink / raw)
  To: Doug Evans; +Cc: palves, gdb-patches

> Date: Mon, 19 Jan 2015 10:32:48 -0800
> From: Doug Evans <xdje42@gmail.com>
> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> 
> On Mon, Jan 19, 2015 at 9:55 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> >> From: Doug Evans <dje@google.com>
> >> Date: Wed, 7 Jan 2015 13:59:06 -0800
> >>
> >> Doug Evans writes:
> >>  > Seems like the main source of the problem would be gdb_stdout,
> >>  > so let's try to fix that first and go from there.
> >>
> >> Here's a prototype.
> >
> > Sorry for a long delay.  I tried this (and Pedro's) patch today.  They
> > both do the job, so one of them should IMO be committed, master and
> > branch.
> >
> >> @@ -239,6 +243,12 @@ tui_file_flush (struct ui_file *file)
> >>      case astring:
> >>        break;
> >>      case afile:
> >> +      /* There is also gdb_stdlog, gdb_stdtarg, gdb_stdtargerr, but
> >> +      tui_setup_io maps those to gdb_stderr.  OTOH, do we need to make
> >> +      this conditional?  */
> >> +      if (file == gdb_stdout
> >> +       || file == gdb_stderr)
> >> +     tui_refresh_cmd_win ();
> >
> > I indeed think that the condition should be removed.  I see no need
> > for it: there's no reason to make any stream displayed on TUI more
> > than line-buffered.
> 
> I don't have a preference on which, with one condition.
> If we're going to remove the fflush's let's do it as a separate patch.

Ping!  Can one of these patches be committed, please, master and the
7.9 branch?

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-01-31 21:37                         ` Eli Zaretskii
@ 2015-02-03 17:52                           ` Pedro Alves
  2015-02-03 18:47                             ` Eli Zaretskii
  2015-02-04 11:55                             ` Pedro Alves
  0 siblings, 2 replies; 34+ messages in thread
From: Pedro Alves @ 2015-02-03 17:52 UTC (permalink / raw)
  To: Eli Zaretskii, Doug Evans; +Cc: gdb-patches

On 01/31/2015 09:59 AM, Eli Zaretskii wrote:
>> Date: Mon, 19 Jan 2015 10:32:48 -0800
>> From: Doug Evans <xdje42@gmail.com>

>> I don't have a preference on which, with one condition.
>> If we're going to remove the fflush's let's do it as a separate patch.

I'll do that.

> Ping!  Can one of these patches be committed, please, master and the
> 7.9 branch?

Sorry for the radio silence; I was traveling last week.

I'm working on this now.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-02-03 17:52                           ` Pedro Alves
@ 2015-02-03 18:47                             ` Eli Zaretskii
  2015-02-04 11:55                             ` Pedro Alves
  1 sibling, 0 replies; 34+ messages in thread
From: Eli Zaretskii @ 2015-02-03 18:47 UTC (permalink / raw)
  To: Pedro Alves; +Cc: xdje42, gdb-patches

> Date: Tue, 03 Feb 2015 18:52:38 +0100
> From: Pedro Alves <palves@redhat.com>
> CC: gdb-patches@sourceware.org
> 
> Sorry for the radio silence; I was traveling last week.
> 
> I'm working on this now.

Thank you.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-02-03 17:52                           ` Pedro Alves
  2015-02-03 18:47                             ` Eli Zaretskii
@ 2015-02-04 11:55                             ` Pedro Alves
  2015-02-04 12:27                               ` Pedro Alves
  2015-02-04 15:38                               ` Eli Zaretskii
  1 sibling, 2 replies; 34+ messages in thread
From: Pedro Alves @ 2015-02-04 11:55 UTC (permalink / raw)
  To: Eli Zaretskii, Doug Evans; +Cc: gdb-patches

On 02/03/2015 06:52 PM, Pedro Alves wrote:

> I'm working on this now.

Turns out that flushing on every line like in my patch was still
considerably (and noticeably) slower than delaying until an explicit
gdb_flush is done, like in Doug's.  So I went with Doug's version.

The recent changes to unify CLI/TUI interface to readline tab
completion made the patch even simpler.  Here's what I pushed
to master.

---
From 518be979d905d8e8708c70149fdb3207aba53aa1 Mon Sep 17 00:00:00 2001
From: Doug Evans <dje@google.com>
Date: Wed, 4 Feb 2015 12:27:28 +0100
Subject: [PATCH] Speed up GDB's TUI output

In the TUI mode, we call wrefresh after outputting every single
character.  This results in the I/O becoming very slow.  Fix this by
delaying refreshing the console window until an explicit flush of
gdb_stdout is requested, or a write to any other (unbuffered) file is
done.

2015-02-04  Doug Evans  <dje@google.com>
	    Pedro Alves  <palves@redhat.com>
	    Eli Zaretskii  <eliz@gnu.org>

	PR tui/17810
	* tui/tui-command.c (tui_refresh_cmd_win): New function.
	* tui/tui-command.c (tui_refresh_cmd_win): Declare.
	* tui/tui-file.c: #include tui/tui-command.h.
	(tui_file_fputs): Refresh command window if stream is not gdb_stdout.
	(tui_file_flush): Refresh command window if stream is gdb_stdout.
	* tui/tui-io.c (tui_puts): Remove calls to wrefresh, fflush.
---
 gdb/ChangeLog         | 12 ++++++++++++
 gdb/tui/tui-command.c | 15 +++++++++++++++
 gdb/tui/tui-command.h |  3 +++
 gdb/tui/tui-file.c    | 10 +++++++++-
 gdb/tui/tui-io.c      |  9 ++++-----
 5 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1116853..d5ec6d1 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,15 @@
+2015-02-04  Doug Evans  <dje@google.com>
+	    Pedro Alves  <palves@redhat.com>
+	    Eli Zaretskii  <eliz@gnu.org>
+
+	PR tui/17810
+	* tui/tui-command.c (tui_refresh_cmd_win): New function.
+	* tui/tui-command.c (tui_refresh_cmd_win): Declare.
+	* tui/tui-file.c: #include tui/tui-command.h.
+	(tui_file_fputs): Refresh command window if stream is not gdb_stdout.
+	(tui_file_flush): Refresh command window if stream is gdb_stdout.
+	* tui/tui-io.c (tui_puts): Remove calls to wrefresh, fflush.
+
 2015-02-04  Pedro Alves  <palves@redhat.com>
 
 	Fix build breakage.
diff --git a/gdb/tui/tui-command.c b/gdb/tui/tui-command.c
index d1a5ddb..3551063 100644
--- a/gdb/tui/tui-command.c
+++ b/gdb/tui/tui-command.c
@@ -129,3 +129,18 @@ tui_dispatch_ctrl_char (unsigned int ch)
       return c;
     }
 }
+
+/* See tui-command.h.  */
+
+void
+tui_refresh_cmd_win (void)
+{
+  WINDOW *w = TUI_CMD_WIN->generic.handle;
+
+  wrefresh (w);
+
+  /* FIXME: It's not clear why this is here.
+     It was present in the original tui_puts code and is kept in order to
+     not introduce some subtle breakage.  */
+  fflush (stdout);
+}
diff --git a/gdb/tui/tui-command.h b/gdb/tui/tui-command.h
index 8cc5be4..ede2ecc 100644
--- a/gdb/tui/tui-command.h
+++ b/gdb/tui/tui-command.h
@@ -24,4 +24,7 @@
 
 extern unsigned int tui_dispatch_ctrl_char (unsigned int);
 
+/* Refresh the command window.  */
+extern void tui_refresh_cmd_win (void);
+
 #endif
diff --git a/gdb/tui/tui-file.c b/gdb/tui/tui-file.c
index b32cfa6..4b4b92c 100644
--- a/gdb/tui/tui-file.c
+++ b/gdb/tui/tui-file.c
@@ -20,7 +20,7 @@
 #include "ui-file.h"
 #include "tui/tui-file.h"
 #include "tui/tui-io.h"
-
+#include "tui/tui-command.h"
 #include "tui.h"
 
 /* A ``struct ui_file'' that is compatible with all the legacy
@@ -179,6 +179,10 @@ tui_file_fputs (const char *linebuffer, struct ui_file *file)
   else
     {
       tui_puts (linebuffer);
+      /* gdb_stdout is buffered, and the caller must gdb_flush it at
+	 appropriate times.  Other streams are not so buffered.  */
+      if (file != gdb_stdout)
+	tui_refresh_cmd_win ();
     }
 }
 
@@ -239,6 +243,10 @@ tui_file_flush (struct ui_file *file)
     case astring:
       break;
     case afile:
+      /* gdb_stdout is buffered.  Other files are always flushed on
+	 every write.  */
+      if (file == gdb_stdout)
+	tui_refresh_cmd_win ();
       fflush (stream->ts_filestream);
       break;
     }
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 2b5a166..21b2a00 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -159,7 +159,10 @@ tui_putc (char c)
   tui_puts (buf);
 }
 
-/* Print the string in the curses command window.  */
+/* Print the string in the curses command window.
+   The output is buffered.  It is up to the caller to refresh the screen
+   if necessary.  */
+
 void
 tui_puts (const char *string)
 {
@@ -201,10 +204,6 @@ tui_puts (const char *string)
          TUI_CMD_WIN->detail.command_info.curch);
   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.
-- 
1.9.3


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-02-04 11:55                             ` Pedro Alves
@ 2015-02-04 12:27                               ` Pedro Alves
  2015-02-04 15:39                                 ` Eli Zaretskii
  2015-02-04 15:38                               ` Eli Zaretskii
  1 sibling, 1 reply; 34+ messages in thread
From: Pedro Alves @ 2015-02-04 12:27 UTC (permalink / raw)
  To: Eli Zaretskii, Doug Evans; +Cc: gdb-patches

On 02/04/2015 12:55 PM, Pedro Alves wrote:
> On 02/03/2015 06:52 PM, Pedro Alves wrote:
> 
>> I'm working on this now.
> 
> Turns out that flushing on every line like in my patch was still
> considerably (and noticeably) slower than delaying until an explicit
> gdb_flush is done, like in Doug's.  So I went with Doug's version.
> 
> The recent changes to unify CLI/TUI interface to readline tab
> completion made the patch even simpler.  Here's what I pushed
> to master.

And below's what I pushed to the 7.9 branch.  I noticed that here:

> @@ -425,6 +426,7 @@ tui_rl_display_match_list (char **matches, int len, int max)
>        if (get_y_or_n () == 0)
>  	{
>  	  tui_puts ("\n");
> +	  tui_refresh_cmd_win ();
>  	  return;
>  	}
>      }

get_y_or_n only flushes the output/question, if we're using the 
pipe trick:

/* Get a character from the command window.  This is called from the
   readline package.  */
int
tui_getc (FILE *fp)
{
  int ch;
  WINDOW *w;

  w = TUI_CMD_WIN->generic.handle;

#ifdef TUI_USE_PIPE_FOR_READLINE
  /* Flush readline output.  */
  tui_readline_output (0, 0);
#endif

so we must refresh before get_y_or_n too.

-------------
From: Doug Evans <dje@google.com>
Subject: [PATCH] Speed up GDB's TUI output

In the TUI mode, we call wrefresh after outputting every single
character.  This results in the I/O becoming very slow.  Fix this by
delaying refreshing the console window until an explicit flush of
gdb_stdout is requested, a write to any other (unbuffered) file is
done.

2015-02-04  Doug Evans  <dje@google.com>
	    Pedro Alves  <palves@redhat.com>
	    Eli Zaretskii  <eliz@gnu.org>

	PR tui/17810
	* tui/tui-command.c (tui_refresh_cmd_win): New function.
	* tui/tui-command.c (tui_refresh_cmd_win): Declare.
	* tui/tui-file.c: #include tui/tui-command.h.
	(tui_file_fputs): Refresh command window if stream is not gdb_stdout.
	(tui_file_flush): Refresh command window if stream is gdb_stdout.
	* tui/tui-io.c (tui_puts): Remove calls to wrefresh, fflush.
	(tui_readline_output): Call tui_refresh_cmd_win.
	(print_filename): Likewise.
	(tui_rl_display_match_list): Likewise.
---
 gdb/ChangeLog         | 15 +++++++++++++++
 gdb/tui/tui-command.c | 15 +++++++++++++++
 gdb/tui/tui-command.h |  3 +++
 gdb/tui/tui-file.c    | 10 +++++++++-
 gdb/tui/tui-io.c      | 14 +++++++++-----
 5 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5cca93e..1f23daee 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,18 @@
+2015-02-04  Doug Evans  <dje@google.com>
+	    Pedro Alves  <palves@redhat.com>
+	    Eli Zaretskii  <eliz@gnu.org>
+
+	PR tui/17810
+	* tui/tui-command.c (tui_refresh_cmd_win): New function.
+	* tui/tui-command.c (tui_refresh_cmd_win): Declare.
+	* tui/tui-file.c: #include tui/tui-command.h.
+	(tui_file_fputs): Refresh command window if stream is not gdb_stdout.
+	(tui_file_flush): Refresh command window if stream is gdb_stdout.
+	* tui/tui-io.c (tui_puts): Remove calls to wrefresh, fflush.
+	(tui_readline_output): Call tui_refresh_cmd_win.
+	(print_filename): Likewise.
+	(tui_rl_display_match_list): Likewise.
+
 2015-02-03  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	Filter out inferior gcc option -fpreprocessed.
diff --git a/gdb/tui/tui-command.c b/gdb/tui/tui-command.c
index d1a5ddb..3551063 100644
--- a/gdb/tui/tui-command.c
+++ b/gdb/tui/tui-command.c
@@ -129,3 +129,18 @@ tui_dispatch_ctrl_char (unsigned int ch)
       return c;
     }
 }
+
+/* See tui-command.h.  */
+
+void
+tui_refresh_cmd_win (void)
+{
+  WINDOW *w = TUI_CMD_WIN->generic.handle;
+
+  wrefresh (w);
+
+  /* FIXME: It's not clear why this is here.
+     It was present in the original tui_puts code and is kept in order to
+     not introduce some subtle breakage.  */
+  fflush (stdout);
+}
diff --git a/gdb/tui/tui-command.h b/gdb/tui/tui-command.h
index 8cc5be4..ede2ecc 100644
--- a/gdb/tui/tui-command.h
+++ b/gdb/tui/tui-command.h
@@ -24,4 +24,7 @@
 
 extern unsigned int tui_dispatch_ctrl_char (unsigned int);
 
+/* Refresh the command window.  */
+extern void tui_refresh_cmd_win (void);
+
 #endif
diff --git a/gdb/tui/tui-file.c b/gdb/tui/tui-file.c
index b32cfa6..4b4b92c 100644
--- a/gdb/tui/tui-file.c
+++ b/gdb/tui/tui-file.c
@@ -20,7 +20,7 @@
 #include "ui-file.h"
 #include "tui/tui-file.h"
 #include "tui/tui-io.h"
-
+#include "tui/tui-command.h"
 #include "tui.h"
 
 /* A ``struct ui_file'' that is compatible with all the legacy
@@ -179,6 +179,10 @@ tui_file_fputs (const char *linebuffer, struct ui_file *file)
   else
     {
       tui_puts (linebuffer);
+      /* gdb_stdout is buffered, and the caller must gdb_flush it at
+	 appropriate times.  Other streams are not so buffered.  */
+      if (file != gdb_stdout)
+	tui_refresh_cmd_win ();
     }
 }
 
@@ -239,6 +243,10 @@ tui_file_flush (struct ui_file *file)
     case astring:
       break;
     case afile:
+      /* gdb_stdout is buffered.  Other files are always flushed on
+	 every write.  */
+      if (file == gdb_stdout)
+	tui_refresh_cmd_win ();
       fflush (stream->ts_filestream);
       break;
     }
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 19e9485..13cc5fa 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -158,7 +158,10 @@ tui_putc (char c)
   tui_puts (buf);
 }
 
-/* Print the string in the curses command window.  */
+/* Print the string in the curses command window.
+   The output is buffered.  It is up to the caller to refresh the screen
+   if necessary.  */
+
 void
 tui_puts (const char *string)
 {
@@ -200,10 +203,6 @@ tui_puts (const char *string)
          TUI_CMD_WIN->detail.command_info.curch);
   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.
@@ -342,6 +341,7 @@ tui_readline_output (int error, gdb_client_data data)
     {
       buf[size] = 0;
       tui_puts (buf);
+      tui_refresh_cmd_win ();
     }
 }
 #endif
@@ -393,6 +393,7 @@ print_filename (const char *to_print, const char *full_pathname)
     {
       PUTX (*s);
     }
+  tui_refresh_cmd_win ();
   return printed_len;
 }
 
@@ -448,9 +449,11 @@ tui_rl_display_match_list (char **matches, int len, int max)
       xsnprintf (msg, sizeof (msg),
 		 "\nDisplay all %d possibilities? (y or n)", len);
       tui_puts (msg);
+      tui_refresh_cmd_win ();
       if (get_y_or_n () == 0)
 	{
 	  tui_puts ("\n");
+	  tui_refresh_cmd_win ();
 	  return;
 	}
     }
@@ -522,6 +525,7 @@ tui_rl_display_match_list (char **matches, int len, int max)
 	}
       tui_putc ('\n');
     }
+  tui_refresh_cmd_win ();
 }
 
 /* Setup the IO for curses or non-curses mode.
-- 
1.9.3


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-02-04 11:55                             ` Pedro Alves
  2015-02-04 12:27                               ` Pedro Alves
@ 2015-02-04 15:38                               ` Eli Zaretskii
  1 sibling, 0 replies; 34+ messages in thread
From: Eli Zaretskii @ 2015-02-04 15:38 UTC (permalink / raw)
  To: Pedro Alves; +Cc: xdje42, gdb-patches

> Date: Wed, 04 Feb 2015 12:55:37 +0100
> From: Pedro Alves <palves@redhat.com>
> CC: gdb-patches@sourceware.org
> 
> On 02/03/2015 06:52 PM, Pedro Alves wrote:
> 
> > I'm working on this now.
> 
> Turns out that flushing on every line like in my patch was still
> considerably (and noticeably) slower than delaying until an explicit
> gdb_flush is done, like in Doug's.  So I went with Doug's version.
> 
> The recent changes to unify CLI/TUI interface to readline tab
> completion made the patch even simpler.  Here's what I pushed
> to master.

Thanks!

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] Speed up "gdb -tui" output
  2015-02-04 12:27                               ` Pedro Alves
@ 2015-02-04 15:39                                 ` Eli Zaretskii
  0 siblings, 0 replies; 34+ messages in thread
From: Eli Zaretskii @ 2015-02-04 15:39 UTC (permalink / raw)
  To: Pedro Alves; +Cc: xdje42, gdb-patches

> Date: Wed, 04 Feb 2015 13:27:20 +0100
> From: Pedro Alves <palves@redhat.com>
> CC: gdb-patches@sourceware.org
> 
> And below's what I pushed to the 7.9 branch.

Thanks again!

^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, other threads:[~2015-02-04 15:39 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-06 16:12 [PATCH] Speed up "gdb -tui" output Eli Zaretskii
2015-01-06 18:37 ` Doug Evans
2015-01-06 19:06   ` Eli Zaretskii
2015-01-06 20:54     ` Doug Evans
2015-01-07 18:30       ` Eli Zaretskii
2015-01-07 19:08         ` Doug Evans
2015-01-07 19:20           ` Eli Zaretskii
2015-01-07 19:30             ` Doug Evans
2015-01-07 19:48               ` Eli Zaretskii
2015-01-07 20:45                 ` Doug Evans
2015-01-07 21:59                   ` Doug Evans
2015-01-19 17:55                     ` Eli Zaretskii
2015-01-19 18:32                       ` Doug Evans
2015-01-31 21:37                         ` Eli Zaretskii
2015-02-03 17:52                           ` Pedro Alves
2015-02-03 18:47                             ` Eli Zaretskii
2015-02-04 11:55                             ` Pedro Alves
2015-02-04 12:27                               ` Pedro Alves
2015-02-04 15:39                                 ` Eli Zaretskii
2015-02-04 15:38                               ` Eli Zaretskii
2015-01-07 15:18   ` Pedro Alves
2015-01-07 17:57     ` Eli Zaretskii
2015-01-07 18:09       ` Doug Evans
2015-01-07 18:34         ` Eli Zaretskii
2015-01-07 19:16           ` Doug Evans
2015-01-07 19:32             ` Eli Zaretskii
2015-01-07 22:30               ` Pedro Alves
2015-01-19 17:52                 ` Eli Zaretskii
2015-01-07 18:00     ` Doug Evans
2015-01-07 18:12       ` Doug Evans
2015-01-07 18:34         ` Eli Zaretskii
2015-01-07 18:21       ` Eli Zaretskii
2015-01-07 18:56         ` Doug Evans
2015-01-07 19:11           ` Eli Zaretskii

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).