Date: Thu, 7 Dec 2023 21:20:28 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Jeff Davis <pgsql@j-davis.com>, Joe Conway <mail@joeconway.com>,
	Andrew Dunstan <andrew@dunslane.net>,
	Davin Shearer <davin@apache.org>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: micro-optimizing json.c
Message-ID: <20231208032028.GA3468226@nathanxps13>
References: <20231207231251.GB3359478@nathanxps13>
 <8d5cce64d7413eff04c5595a0a80d2b5a809e2fe.camel@j-davis.com>
 <1340019.1701996030@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="azLHFNyN32YCQGCU"
Content-Disposition: inline
In-Reply-To: <1340019.1701996030@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/20231208032028.GA3468226%40nathanxps13>
Precedence: bulk


--azLHFNyN32YCQGCU
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--azLHFNyN32YCQGCU
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-avoid-some-strlen-calls-in-json.c.patch"

From dc5ec1a2204398fa25390f31771eaa57816a96a1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
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


--azLHFNyN32YCQGCU--