Re: [PATCH] C++-ify parse_format_string

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-12-03 09:12, Pedro Alves wrote:
> Use plain "time" first.  (You may have shifted work
> out of parse_format_string.)
> 
> With your test script as is, I get around:
> 
>  real    0m1.053s
>  user    0m0.985s
>  sys     0m0.068s
> 
> I like a testcase that runs a bit longer in order to have a
> better signal/noise ratio.  IMO 1 second is too short.  Bumping the
> number of iterations 10x (to 200000), I get (representative
> of a few runs),
> 
> before patch:
> 
>  real    0m9.432s
>  user    0m8.978s
>  sys     0m0.444s
> 
> after patch:
> 
>  real    0m10.322s
>  user    0m9.854s
>  sys     0m0.454s
> 
> there's some variation across runs, but after-patch compared
> to before-patch is consistently around 7%-11% slower, for me.

Indeed, I see similar results (except 200000 iterations take 30 seconds 
for me with my 10 years old CPU :)).

> Looking at "perf report -g", we see we're very inefficient
> while handling this specific use case, spending a lot of time
> in the C parser and in fprintf_filtered.  If those paths
> were tuned a bit the relative slowdown of parse_format_string
> would probably be more pronounced.
> 
> Note that parse_format_string is used to implement target-side
> dprintf, in gdbserver/ax.c:ax_printf.  This means that a slow
> down here affects inferior execution time too, which may be
> a less-contrived scenario.

I agree that we should not make parse_format_string slow just for fun, 
but in this particular case I think we should avoid parsing the format 
string on the fast path.  It should be possible to chew it to pieces 
earlier (when installing the breakpoint) and just access the pieces when 
we hit the dprintf.

> My point is still the same as ever -- it's not possible
> upfront to envision all the use cases users throw at us,
> given gdb scripting, etc.  So if easy, I think it's better
> to avoid premature pessimization:
> 
>  https://sourceware.org/ml/gdb-patches/2017-06/msg00506.html

The article you link defines premature pessimization as "when 
equivalently complex code would be faster".  In this case, the code is 
more complex (though maybe not by much) with a shared buffer, and I was 
wondering if that complexity was really worth the speedup, especially 
for something not expected to be on a fast path.  But since it's already 
implemented like this and we know it's faster, I agree it feels like a 
step backwards to remove it.  So I have no problem going with Tom's 
patch.

Simon