[PATCH v1] elog.c: Remove special case which avoided %*s format strings..

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v1] elog.c: Remove special case which avoided %*s format strings..

Justin Pryzby
..which should no longer be needed since it was a performance hack for specific
platform snprintf, which are no longer used.

v1-0001-elog.c-Remove-special-case-which-avoided-s-format.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v1] elog.c: Remove special case which avoided %*s format strings..

Michael Paquier-2
On Sun, Aug 02, 2020 at 11:59:48PM -0500, Justin Pryzby wrote:
> ..which should no longer be needed since it was a performance hack for specific
> platform snprintf, which are no longer used.

Did you check if our implementation of src/port/snprintf.c makes %*s
much slower than %s or not?  FWIW, I have just run a small test on my
laptop, and running 100M calls of snprintf() with "%s" in a tight loop
takes 2.7s, with "%*s" and a padding of 0 it takes 4.2s.  So this test
tells that we are far from something that's substantially slower, and
to simplify the code your change makes sense.  Still, there could be a
point in keeping this optimization, but fix the comment to remove the
platform-dependent part of it.  Any thoughts?
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v1] elog.c: Remove special case which avoided %*s format strings..

David Rowley
On Tue, 4 Aug 2020 at 19:36, Michael Paquier <[hidden email]> wrote:
> Did you check if our implementation of src/port/snprintf.c makes %*s
> much slower than %s or not?  FWIW, I have just run a small test on my
> laptop, and running 100M calls of snprintf() with "%s" in a tight loop
> takes 2.7s, with "%*s" and a padding of 0 it takes 4.2s.  So this test
> tells that we are far from something that's substantially slower, and
> to simplify the code your change makes sense.  Still, there could be a
> point in keeping this optimization, but fix the comment to remove the
> platform-dependent part of it.  Any thoughts?

It's not just converting "%s" to "%*s", it's sometimes changing a
appendStringInfoString() call to appendStringInfo(). It's hard to
imagine the formatting version could ever be as fast as
appendStringInfo().

FWIW, the tests I did to check this when initially working on it are
in [1].  Justin, it would be good if you could verify you're making as
bad as what's mentioned on that thread again.

David

[1] https://www.postgresql.org/message-id/flat/20130924165104.GQ4832%40eldon.alvh.no-ip.org#4e8a716ff0bde1e950fe7ddca1d75454


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v1] elog.c: Remove special case which avoided %*s format strings..

Michael Paquier-2
On Tue, Aug 04, 2020 at 09:06:16PM +1200, David Rowley wrote:
> FWIW, the tests I did to check this when initially working on it are
> in [1].  Justin, it would be good if you could verify you're making as
> bad as what's mentioned on that thread again.

Ouch.  Thanks for the reference.  Indeed it looks that it would hurt
even with just a simple PL function.
--
Michael

signature.asc (849 bytes) Download Attachment