This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] C++-ify parse_format_string


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]