Обсуждение: micro-optimizing json.c

Moving this to a new thread...

On Thu, Dec 07, 2023 at 07:15:28AM -0500, Joe Conway wrote:
> On 12/6/23 21:56, Nathan Bossart wrote:
>> On Wed, Dec 06, 2023 at 03:20:46PM -0500, Tom Lane wrote:
>> > If Nathan's perf results hold up elsewhere, it seems like some
>> > micro-optimization around the text-pushing (appendStringInfoString)
>> > might be more useful than caching.  The 7% spent in cache lookups
>> > could be worth going after later, but it's not the top of the list.
>> 
>> Hah, it turns out my benchmark of 110M integers really stresses the
>> JSONTYPE_NUMERIC path in datum_to_json_internal().  That particular path
>> calls strlen() twice: once for IsValidJsonNumber(), and once in
>> appendStringInfoString().  If I save the result from IsValidJsonNumber()
>> and give it to appendBinaryStringInfo() instead, the COPY goes ~8% faster.
>> It's probably worth giving datum_to_json_internal() a closer look in a new
>> thread.
> 
> Yep, after looking through that code I was going to make the point that your
> 11 integer test was over indexing on that one type. I am sure there are
> other micro-optimizations to be made here, but I also think that it is
> outside the scope of the COPY TO JSON patch.

Here's a patch that removes a couple of strlen() calls that showed up
prominently in perf for a COPY TO (FORMAT json) on 110M integers.  On my
laptop, I see a 20% speedup from ~23.6s to ~18.9s for this test.

I plan to test the other types as well, and I'd also like to look into the
caching mentioned above if/when COPY TO (FORMAT json) is committed.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, 2023-12-07 at 17:12 -0600, Nathan Bossart wrote:
> Here's a patch that removes a couple of strlen() calls that showed up
> prominently in perf for a COPY TO (FORMAT json) on 110M integers.  On
> my
> laptop, I see a 20% speedup from ~23.6s to ~18.9s for this test.

Nice improvement. The use of (len = ...) in a conditional is slightly
out of the ordinary, but it makes the conditionals a bit simpler and
you have a comment, so it's fine with me.

I wonder, if there were an efficient cast from numeric to text, then
perhaps you could avoid the strlen() entirely? Maybe there's a way to
use a static buffer to even avoid the palloc() in get_str_from_var()?
Not sure these are worth the effort; just brainstorming.

In any case, +1 to your simple change.

Regards,
    Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:
> Nice improvement. The use of (len = ...) in a conditional is slightly
> out of the ordinary, but it makes the conditionals a bit simpler and
> you have a comment, so it's fine with me.

Yeah.  It's a little not per project style, but I don't see a nice
way to do it differently, granting that we don't want to put the
strlen() ahead of the !key_scalar test.  More straightforward
coding would end up with two else-path calls of escape_json,
which doesn't seem all that much more straightforward.

> I wonder, if there were an efficient cast from numeric to text, then
> perhaps you could avoid the strlen() entirely?

Hmm ... I think that might not be the way to think about it.  What
I'm wondering is why we need a test as expensive as IsValidJsonNumber
in the first place, given that we know this is a numeric data type's
output.  ISTM we only need to reject "Inf"/"-Inf" and "NaN", which
surely does not require a full parse.  Skip over a sign, check for
"I"/"N", and you're done.

... and for that matter, why does quoting of Inf/NaN require
that we apply something as expensive as escape_json?  Several other
paths in this switch have no hesitation about assuming that they
can just plaster double quotes around what was emitted.  How is
that safe for timestamps but not Inf/NaN?

> In any case, +1 to your simple change.

If we end up not using IsValidJsonNumber then this strlen hackery
would become irrelevant, so maybe that idea should be looked at first.

            regards, tom lane

On Fri, 8 Dec 2023 at 12:13, Nathan Bossart <nathandbossart@gmail.com> wrote:
> Here's a patch that removes a couple of strlen() calls that showed up
> prominently in perf for a COPY TO (FORMAT json) on 110M integers.  On my
> laptop, I see a 20% speedup from ~23.6s to ~18.9s for this test.

+ seplen = use_line_feeds ? sizeof(",\n ") - 1 : sizeof(",") - 1;

Most modern compilers will be fine with just:

seplen = strlen(sep);

I had to go back to clang 3.4.1 and GCC 4.1.2 to see the strlen() call
with that code [1].

With:

  if (needsep)
- appendStringInfoString(result, sep);
+ appendBinaryStringInfo(result, sep, seplen);

I might be neater to get rid of the if condition and have:

sep = use_line_feeds ? ",\n " : ",";
seplen = strlen(sep);
slen = 0;

...
for (int i = 0; i < tupdesc->natts; i++)
{
    ...
    appendBinaryStringInfo(result, sep, slen);
    slen = seplen;
    ...
}

David

[1] https://godbolt.org/z/8dq8a88bP

David Rowley <dgrowleyml@gmail.com> writes:
> I might be neater to get rid of the if condition and have:
> [ calls of appendBinaryStringInfo with len 0 ]

Hmm, if we are trying to micro-optimize, I seriously doubt that
that's an improvement.

            regards, tom lane

On Thu, Dec 07, 2023 at 07:40:30PM -0500, Tom Lane wrote:
> Hmm ... I think that might not be the way to think about it.  What
> I'm wondering is why we need a test as expensive as IsValidJsonNumber
> in the first place, given that we know this is a numeric data type's
> output.  ISTM we only need to reject "Inf"/"-Inf" and "NaN", which
> surely does not require a full parse.  Skip over a sign, check for
> "I"/"N", and you're done.
> 
> ... and for that matter, why does quoting of Inf/NaN require
> that we apply something as expensive as escape_json?  Several other
> paths in this switch have no hesitation about assuming that they
> can just plaster double quotes around what was emitted.  How is
> that safe for timestamps but not Inf/NaN?

I did both of these in v2, although I opted to test that the first
character after the optional '-' was a digit instead of testing that it was
_not_ an 'I' or 'N'.  I think that should be similar performance-wise, and
maybe it's a bit more future-proof in case someone invents some new
notation for a numeric data type (/shrug).  In any case, this seems to
speed up my test by another half a second or so.

I think there are some similar improvements that we can make for
JSONTYPE_BOOL and JSONTYPE_CAST, but I haven't tested them yet.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart@gmail.com> writes:
> I did both of these in v2, although I opted to test that the first
> character after the optional '-' was a digit instead of testing that it was
> _not_ an 'I' or 'N'.

Yeah, I thought about that too after sending my message.  This version
LGTM, although maybe the comment could be slightly more verbose with
explicit reference to Inf/NaN as being the cases we need to quote.

> I think there are some similar improvements that we can make for
> JSONTYPE_BOOL and JSONTYPE_CAST, but I haven't tested them yet.

I am suspicious of using

    appendStringInfo(result, "\"%s\"", ...);

in each of these paths; snprintf is not a terribly cheap thing.
It might be worth expanding that to appendStringInfoChar/
appendStringInfoString/appendStringInfoChar.

            regards, tom lane

On Fri, Dec 08, 2023 at 04:11:52PM +1300, David Rowley wrote:
> + seplen = use_line_feeds ? sizeof(",\n ") - 1 : sizeof(",") - 1;
> 
> Most modern compilers will be fine with just:
> 
> seplen = strlen(sep);
> 
> I had to go back to clang 3.4.1 and GCC 4.1.2 to see the strlen() call
> with that code [1].

Hm.  I tried this first, but my compiler (gcc 9.4.0 on this machine) was
still doing the strlen()...

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Dec 07, 2023 at 10:28:50PM -0500, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> I did both of these in v2, although I opted to test that the first
>> character after the optional '-' was a digit instead of testing that it was
>> _not_ an 'I' or 'N'.
> 
> Yeah, I thought about that too after sending my message.  This version
> LGTM, although maybe the comment could be slightly more verbose with
> explicit reference to Inf/NaN as being the cases we need to quote.

Done.

>> I think there are some similar improvements that we can make for
>> JSONTYPE_BOOL and JSONTYPE_CAST, but I haven't tested them yet.
> 
> I am suspicious of using
> 
>     appendStringInfo(result, "\"%s\"", ...);
> 
> in each of these paths; snprintf is not a terribly cheap thing.
> It might be worth expanding that to appendStringInfoChar/
> appendStringInfoString/appendStringInfoChar.

WFM.  I'll tackle JSONTYPE_BOOL and JSONTYPE_CAST next...

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Thu, Dec 07, 2023 at 10:28:50PM -0500, Tom Lane wrote:
>> Yeah, I thought about that too after sending my message.  This version
>> LGTM, although maybe the comment could be slightly more verbose with
>> explicit reference to Inf/NaN as being the cases we need to quote.

> Done.

This version works for me.

            regards, tom lane

On Fri, Dec 8, 2023 at 10:32 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Fri, Dec 08, 2023 at 04:11:52PM +1300, David Rowley wrote:
> > + seplen = use_line_feeds ? sizeof(",\n ") - 1 : sizeof(",") - 1;
> >
> > Most modern compilers will be fine with just:
> >
> > seplen = strlen(sep);
> >
> > I had to go back to clang 3.4.1 and GCC 4.1.2 to see the strlen() call
> > with that code [1].
>
> Hm.  I tried this first, but my compiler (gcc 9.4.0 on this machine) was
> still doing the strlen()...

This is less verbose and still compiles with constants:

use_line_feeds ? strlen(",\n ") : strlen(",");

On Fri, Dec 08, 2023 at 11:51:15AM +0700, John Naylor wrote:
> This is less verbose and still compiles with constants:
> 
> use_line_feeds ? strlen(",\n ") : strlen(",");

This one worked on my machine.  I've committed the patch with that change.
Thanks everyone for the reviews!

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Here are a couple more easy micro-optimizations in nearby code.  I've split
them into individual patches for review, but I'll probably just combine
them into one patch before committing.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart@gmail.com> writes:
> Here are a couple more easy micro-optimizations in nearby code.  I've split
> them into individual patches for review, but I'll probably just combine
> them into one patch before committing.

LGTM

            regards, tom lane

On Fri, Dec 08, 2023 at 05:56:20PM -0500, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> Here are a couple more easy micro-optimizations in nearby code.  I've split
>> them into individual patches for review, but I'll probably just combine
>> them into one patch before committing.
> 
> LGTM

Committed.  Thanks for reviewing!

For the record, I did think about changing appendStringInfoString() into a
macro or an inline function so that any calls with a string literal would
benefit from this sort of optimization, but I was on-the-fence about it
because it requires some special knowledge, i.e., you have to know to
provide string literals to remove the runtime calls to strlen().  Perhaps
this is worth further exploration...

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com