This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] C++-ify parse_format_string
- From: Simon Marchi <simon dot marchi at polymtl dot ca>
- To: Pedro Alves <palves at redhat dot com>
- Cc: gdb-patches at sourceware dot org, Tom Tromey <tom at tromey dot com>, Simon Marchi <simon dot marchi at ericsson dot com>
- Date: Sun, 03 Dec 2017 12:49:49 -0500
- Subject: Re: [PATCH] C++-ify parse_format_string
- Authentication-results: sourceware.org; auth=none
- References: <87h8tivyo4.fsf@tromey.com> <20171202203133.10770-1-simon.marchi@polymtl.ca> <315d390f-5b5d-f077-e730-ae4535729557@redhat.com>
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