Date: Thu, 7 Dec 2023 17:12:51 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Joe Conway <mail@joeconway.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Andrew Dunstan <andrew@dunslane.net>,
	Davin Shearer <davin@apache.org>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: micro-optimizing json.c
Message-ID: <20231207231251.GB3359478@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="J2SCkAp4GZ/dPZZf"
Content-Disposition: inline
Archived-At: <https://www.postgresql.org/message-id/20231207231251.GB3359478%40nathanxps13>
Precedence: bulk


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

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

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

From d69046e5f6d9c570aafa54bf3a99eb9d63fd439e Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 7 Dec 2023 16:45:45 -0600
Subject: [PATCH v1 1/1] avoid some strlen() calls in json.c

---
 src/backend/utils/adt/json.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index cb4311e75f..b22d9d5b4c 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -178,6 +178,7 @@ datum_to_json_internal(Datum val, bool is_null, StringInfo result,
 {
 	char	   *outputstr;
 	text	   *jsontext;
+	int			len;
 
 	check_stack_depth();
 
@@ -220,9 +221,12 @@ datum_to_json_internal(Datum val, bool is_null, StringInfo result,
 			/*
 			 * Don't call escape_json for a non-key if it's a valid JSON
 			 * number.
+			 *
+			 * Saving the strlen() result allows us to avoid another expensive
+			 * strlen() call in appendStringInfoString().
 			 */
-			if (!key_scalar && IsValidJsonNumber(outputstr, strlen(outputstr)))
-				appendStringInfoString(result, outputstr);
+			if (!key_scalar && IsValidJsonNumber(outputstr, (len = strlen(outputstr))))
+				appendBinaryStringInfo(result, outputstr, len);
 			else
 				escape_json(result, outputstr);
 			pfree(outputstr);
@@ -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


--J2SCkAp4GZ/dPZZf--