From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: David Fetter <david@fetter.org>, Jeff Janes <jeff.janes@gmail.com>,
        pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Why is pq_begintypsend so slow?
In-reply-to: <20200114224520.gh4qktr5kuhu5pyi@alap3.anarazel.de>
References: 
 <CAMkU=1whbRDUwa4eayD9+59K-coxO9senDkPRbTn3cg0pUz4AQ@mail.gmail.com>
 <31795.1578773977@sss.pgh.pa.us> <20200112014107.GC32763@fetter.org>
 <5450.1578797036@sss.pgh.pa.us> <28764.1578799965@sss.pgh.pa.us>
 <20200113231804.4zqjzggv3te2eo63@alap3.anarazel.de>
 <20200114224520.gh4qktr5kuhu5pyi@alap3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Tue, 14 Jan 2020 14:45:20 -0800"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <6540.1589819706.0@sss.pgh.pa.us>
Date: Mon, 18 May 2020 12:38:05 -0400
Message-ID: <6648.1589819885@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <6540.1589819706.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

Andres Freund <andres@anarazel.de> writes:
>> FWIW, I've also observed, in another thread (the node func generation
>> thing [1]), that inlining enlargeStringInfo() helps a lot, especially
>> when inlining some of its callers. Moving e.g. appendStringInfo() inlin=
e
>> allows the compiler to sometimes optimize away the strlen. But if
>> e.g. an inlined appendBinaryStringInfo() still calls enlargeStringInfo(=
)
>> unconditionally, successive appends cannot optimize away memory accesse=
s
>> for ->len/->data.

> With a set of patches doing so, int4send itself is not a significant
> factor for my test benchmark [1] anymore.

This thread seems to have died out, possibly because the last set of
patches that Andres posted was sufficiently complicated and invasive
that nobody wanted to review it.  I thought about this again after
seeing that [1] is mostly about pq_begintypsend overhead, and had
an epiphany: there isn't really a strong reason for pq_begintypsend
to be inserting bits into the buffer at all.  The bytes will be
filled by pq_endtypsend, and nothing in between should be touching
them.  So I propose 0001 attached.  It's poking into the stringinfo
abstraction a bit more than I would want to do if there weren't a
compelling performance reason to do so, but there evidently is.

With 0001, pq_begintypsend drops from being the top single routine
in a profile of a test case like [1] to being well down the list.
The next biggest cost compared to text-format output is that
printtup() itself is noticeably more expensive.  A lot of the extra
cost there seems to be from pq_sendint32(), which is getting inlined
into printtup(), and there probably isn't much we can do to make that
cheaper. But eliminating a common subexpression as in 0002 below does
help noticeably, at least with the rather old gcc I'm using.

For me, the combination of these two eliminates most but not quite
all of the cost penalty of binary over text output as seen in [1].

			regards, tom lane

[1] https://www.postgresql.org/message-id/CAMovtNoHFod2jMAKQjjxv209PCTJx5K=
c66anwWvX0mEiaXwgmA%40mail.gmail.com


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="0001-fix-pq_begintypsend.patch";
	charset="us-ascii"
Content-ID: <6540.1589819706.2@sss.pgh.pa.us>
Content-Description: 0001-fix-pq_begintypsend.patch

diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index a6f990c..03b7404 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -328,11 +328,16 @@ void
 pq_begintypsend(StringInfo buf)
 {
 	initStringInfo(buf);
-	/* Reserve four bytes for the bytea length word */
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
-	appendStringInfoCharMacro(buf, '\0');
+
+	/*
+	 * Reserve four bytes for the bytea length word.  We don't need to fill
+	 * them with anything (pq_endtypsend will do that), and this function is
+	 * enough of a hot spot that it's worth cheating to save some cycles. Note
+	 * in particular that we don't bother to guarantee that the buffer is
+	 * null-terminated.
+	 */
+	Assert(buf->maxlen > 4);
+	buf->len = 4;
 }
 
 /* --------------------------------

------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="0002-minor-printtup-fix.patch";
	charset="us-ascii"
Content-ID: <6540.1589819706.3@sss.pgh.pa.us>
Content-Description: 0002-minor-printtup-fix.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/access/common/printtup.c b/src/backend/access/com=
mon/printtup.c
index dd1bac0..a9315c6 100644
--- a/src/backend/access/common/printtup.c
+++ b/src/backend/access/common/printtup.c
@@ -438,11 +438,12 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
 		{
 			/* Binary output */
 			bytea	   *outputbytes;
+			int			outputlen;
 =

 			outputbytes =3D SendFunctionCall(&thisState->finfo, attr);
-			pq_sendint32(buf, VARSIZE(outputbytes) - VARHDRSZ);
-			pq_sendbytes(buf, VARDATA(outputbytes),
-						 VARSIZE(outputbytes) - VARHDRSZ);
+			outputlen =3D VARSIZE(outputbytes) - VARHDRSZ;
+			pq_sendint32(buf, outputlen);
+			pq_sendbytes(buf, VARDATA(outputbytes), outputlen);
 		}
 	}
 =


------- =_aaaaaaaaaa0--