Re: micro-optimizing json.c
On Fri, Dec 08, 2023 at 05:56:20PM -0500, Tom Lane wrote: > Nathan Bossart 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
Re: micro-optimizing json.c
Nathan Bossart 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
Re: micro-optimizing json.c
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
>From 79727aab47c7d7cd733ce21c8241051de6c9ae1e Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 8 Dec 2023 14:03:00 -0600
Subject: [PATCH v1 1/3] micro-optimize bool code path in
datum_to_json_internal()
shows about an 18% speed up on my machine
---
src/backend/utils/adt/json.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index e585981d08..4ac5837a23 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -208,11 +208,14 @@ datum_to_json_internal(Datum val, bool is_null, StringInfo result,
composite_to_json(val, result, false);
break;
case JSONTYPE_BOOL:
- outputstr = DatumGetBool(val) ? "true" : "false";
if (key_scalar)
-escape_json(result, outputstr);
+appendStringInfoChar(result, '"');
+ if (DatumGetBool(val))
+appendBinaryStringInfo(result, "true", strlen("true"));
else
-appendStringInfoString(result, outputstr);
+appendBinaryStringInfo(result, "false", strlen("false"));
+ if (key_scalar)
+appendStringInfoChar(result, '"');
break;
case JSONTYPE_NUMERIC:
outputstr = OidOutputFunctionCall(outfuncoid, val);
--
2.25.1
>From bcba82c9ba451c011ac66a70d6cad5be3ccf2d67 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 8 Dec 2023 14:10:00 -0600
Subject: [PATCH v1 2/3] micro-optimize null code path in
datum_to_json_internal()
shows about a 29% speed up on my machine
---
src/backend/utils/adt/json.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index 4ac5837a23..24e12244cc 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -186,7 +186,7 @@ datum_to_json_internal(Datum val, bool is_null, StringInfo result,
if (is_null)
{
- appendStringInfoString(result, "null");
+ appendBinaryStringInfo(result, "null", strlen("null"));
return;
}
--
2.25.1
>From 35324988e9c42c275806763bd0bf6b570616a8e7 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Fri, 8 Dec 2023 14:15:56 -0600
Subject: [PATCH v1 3/3] micro-optimize cast code path in
datum_to_json_internal()
---
src/backend/utils/adt/json.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index 24e12244cc..aa20cf60ea 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -278,9 +278,8 @@ datum_to_json_internal(Datum val, bool is_null, StringInfo result,
case JSONTYPE_CAST:
/* outfuncoid refers to a cast function, not an output function */
jsontext = DatumGetTextPP(OidFunctionCall1(outfuncoid, val));
- outputstr = text_to_cstring(jsontext);
- appendStringInfoString(result, outputstr);
- pfree(outputstr);
+ appendBinaryStringInfo(result, VARDATA_ANY(jsontext),
+ VARSIZE_ANY_EXHDR(jsontext));
pfree(jsontext);
break;
default:
--
2.25.1
Re: micro-optimizing json.c
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
Re: micro-optimizing json.c
On Fri, Dec 8, 2023 at 10:32 AM Nathan Bossart 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(",");
Re: micro-optimizing json.c
Nathan Bossart 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
Re: micro-optimizing json.c
On Thu, Dec 07, 2023 at 10:28:50PM -0500, Tom Lane wrote:
> Nathan Bossart 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
>From 48ef8db324c007eef8d5e5fe015e1294e6e410b4 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Thu, 7 Dec 2023 16:45:45 -0600
Subject: [PATCH v3 1/1] avoid some strlen() calls in json.c
---
src/backend/utils/adt/json.c | 37
1 file changed, 29 insertions(+), 8 deletions(-)
diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index cb4311e75f..f975526f33 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -218,13 +218,22 @@ datum_to_json_internal(Datum val, bool is_null, StringInfo result,
outputstr = OidOutputFunctionCall(outfuncoid, val);
/*
- * Don't call escape_json for a non-key if it's a valid JSON
- * number.
+ * Don't quote a non-key if it's a valid JSON number (i.e., not
+ * "Infinity", "-Infinity", or "NaN"). Since we know this is a
+ * numeric data type's output, we simplify and open-code the
+ * validation for better performance.
*/
- if (!key_scalar && IsValidJsonNumber(outputstr, strlen(outputstr)))
+ if (!key_scalar &&
+((*outputstr >= '0' && *outputstr <= '9') ||
+ (*outputstr == '-' &&
+ (outputstr[1] >= '0' && outputstr[1] <= '9'
appendStringInfoString(result, outputstr);
else
-escape_json(result, outputstr);
+ {
+appendStringInfoChar(result, '"');
+appendStringInfoString(result, outputstr);
+appendStringInfoChar(result, '"');
+ }
pfree(outputstr);
break;
case JSONTYPE_DATE:
@@ -232,7 +241,9 @@ datum_to_json_internal(Datum val, bool is_null, StringInfo result,
char buf[MAXDATELEN + 1];
JsonEncodeDateTime(buf, val, DATEOID, NULL);
-appendStringInfo(result, "\"%s\"", buf);
+appendStringInfoChar(result, '"');
+appendStringInfoString(result, buf);
+appendStringInfoChar(result, '"');
}
break;
case JSONTYPE_TIMESTAMP:
@@ -240,7 +251,9 @@ datum_to_json_internal(Datum val, bool is_null, StringInfo result,
char buf[MAXDATELEN + 1];
JsonEncodeDateTime(buf, val, TIMESTAMPOID, NULL);
-appendStringInfo(result, "\"%s\"", buf);
+appendStringInfoChar(result, '"');
+appendStringInfoString(result, buf);
+appendStringInfoChar(result, '"');
}
break;
case JSONTYPE_TIMESTAMPTZ:
@@ -248,7 +261,9 @@ datum_to_json_internal(Datum val, bool is_null, StringInfo result,
char buf[MAXDATELEN + 1];
JsonEncodeDateTime(buf, val, TIMESTAMPTZOID, NULL);
-appendStringInfo(result, "\"%s\"", buf);
+appendStringInfoChar(result, '"');
+appendStringInfoString(result, buf);
+appendStringInfoChar(result, '"');
}
break;
case JSONTYPE_JSON:
@@ -502,8 +517,14 @@ composite_to_json(Datum composite, StringInfo result, bool use_line_feeds)
int i;
bool needsep = false;
const char *sep;
+ int seplen;
+ /*
+ * We can avoid expensive strlen() calls by precalculating the separator
+ * length.
+ */
sep = use_line_feeds ? ",\n " : ",";
+ seplen = use_line_feeds ? sizeof(",\n ") - 1 : sizeof(",") - 1;
td = DatumGetHeapTupleHeader(composite);
@@ -532,7 +553,7 @@ composite_to_json(Datum composite, StringInfo result, bool use_line_feeds)
continue;
if (needsep)
- appendStringInfoString(result, sep);
+ appendBinaryStringInfo(result, sep, seplen);
needsep = true;
attname = NameStr(att->attname);
--
2.25.1
Re: micro-optimizing json.c
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
Re: micro-optimizing json.c
Nathan Bossart 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
Re: micro-optimizing json.c
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
>From dc5ec1a2204398fa25390f31771eaa57816a96a1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Thu, 7 Dec 2023 16:45:45 -0600
Subject: [PATCH v2 1/1] avoid some strlen() calls in json.c
---
src/backend/utils/adt/json.c | 20 +++-
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index cb4311e75f..640d9123ed 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -218,13 +218,17 @@ datum_to_json_internal(Datum val, bool is_null, StringInfo result,
outputstr = OidOutputFunctionCall(outfuncoid, val);
/*
- * Don't call escape_json for a non-key if it's a valid JSON
- * number.
+ * Don't quote a non-key if it's a valid JSON number. Since we
+ * know this is a numeric data type's output, we simplify and
+ * open-code the validation for better performance.
*/
- if (!key_scalar && IsValidJsonNumber(outputstr, strlen(outputstr)))
+ if (!key_scalar &&
+((*outputstr >= '0' && *outputstr <= '9') ||
+ (*outputstr == '-' &&
+ (outputstr[1] >= '0' && outputstr[1] <= '9'
appendStringInfoString(result, outputstr);
else
-escape_json(result, outputstr);
+appendStringInfo(result, "\"%s\"", outputstr);
pfree(outputstr);
break;
case JSONTYPE_DATE:
@@ -502,8 +506,14 @@ composite_to_json(Datum composite, StringInfo result, bool use_line_feeds)
int i;
bool needsep = false;
const char *sep;
+ int seplen;
+ /*
+ * We can avoid expensive strlen() calls by precalculating the separator
+ * length.
+ */
sep = use_line_feeds ? ",\n " : ",";
+ seplen = use_line_feeds ? sizeof(",\n ") - 1 : sizeof(",") - 1;
td = DatumGetHeapTupleHeader(composite);
@@ -532,7 +542,7 @@ composite_to_json(Datum composite, StringInfo result, bool use_line_feeds)
continue;
if (needsep)
- appendStringInfoString(result, sep);
+ appendBinaryStringInfo(result, sep, seplen);
needsep = true;
attname = NameStr(att->attname);
--
2.25.1
Re: micro-optimizing json.c
David Rowley 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
Re: micro-optimizing json.c
On Fri, 8 Dec 2023 at 12:13, 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.
+ 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
Re: micro-optimizing json.c
Jeff Davis 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
Re: micro-optimizing json.c
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
