Re: Why is pq_begintypsend so slow? - Mailing list pgsql-hackers

David Fetter <david@fetter.org> writes:
> On Sat, Jan 11, 2020 at 03:19:37PM -0500, Tom Lane wrote:
>> Jeff Janes <jeff.janes@gmail.com> writes:
>>> I saw that the hotspot was pq_begintypsend at 20%, which was twice the
>>> percentage as the next place winner (AllocSetAlloc).

>> I'd be inclined to replace the appendStringInfoCharMacro calls with
>> appendStringInfoSpaces(buf, 4) --- I don't think we care exactly what
>> is inserted into those bytes at this point.  And maybe
>> appendStringInfoSpaces could stand some micro-optimization, too.
>> Use a memset and a single len adjustment, perhaps?

> Please find attached a patch that does it both of the things you
> suggested.

I've been fooling around with this here.  On the test case Jeff
describes --- COPY BINARY tab TO '/dev/null' where tab contains
100 float8 columns filled from random() --- I can reproduce his
results.  pq_begintypsend is the top hotspot and if perf's
localization is accurate, it's the instructions that fetch
str->len that hurt the most.  Still not very clear why that is...

Converting pq_begintypsend to use appendStringInfoSpaces helps
a bit; it takes my test case from 91725 ms to 88847 ms, or about
3% savings.  Noodling around with appendStringInfoSpaces doesn't
help noticeably; I tried memset, as well as open-coding (cf
patch below) but the results were all well within the noise
threshold.

I saw at this point that the remaining top spots were
enlargeStringInfo and appendBinaryStringInfo, so I experimented
with inlining them (again, see patch below).  That *did* move
the needle: down to 72691 ms, or 20% better than HEAD.  Of
course, that comes at a code-size cost, but it's only about
13kB growth:

before:
$ size src/backend/postgres 
   text    data     bss     dec     hex filename
7485285   58088  203328 7746701  76348d src/backend/postgres
after:
$ size src/backend/postgres 
   text    data     bss     dec     hex filename
7498652   58088  203328 7760068  7668c4 src/backend/postgres

That's under two-tenths of a percent.  (This'd affect frontend
executables too, and I didn't check them.)

Seems like this is worth pursuing, especially if it can be
shown to improve any other cases noticeably.  It might be
worth inlining some of the other trivial stringinfo functions,
though I'd tread carefully on that.

            regards, tom lane

diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index a6f990c..0fc8c3f 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -328,11 +328,8 @@ 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; contents not important */
+    appendStringInfoSpaces(buf, 4);
 }

 /* --------------------------------
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index 0badc46..8f8bb0d 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -207,43 +207,21 @@ appendStringInfoSpaces(StringInfo str, int count)
 {
     if (count > 0)
     {
+        char       *ptr;
+
         /* Make more room if needed */
         enlargeStringInfo(str, count);

         /* OK, append the spaces */
+        ptr = str->data + str->len;
+        str->len += count;
         while (--count >= 0)
-            str->data[str->len++] = ' ';
-        str->data[str->len] = '\0';
+            *ptr++ = ' ';
+        *ptr = '\0';
     }
 }

 /*
- * appendBinaryStringInfo
- *
- * Append arbitrary binary data to a StringInfo, allocating more space
- * if necessary. Ensures that a trailing null byte is present.
- */
-void
-appendBinaryStringInfo(StringInfo str, const char *data, int datalen)
-{
-    Assert(str != NULL);
-
-    /* Make more room if needed */
-    enlargeStringInfo(str, datalen);
-
-    /* OK, append the data */
-    memcpy(str->data + str->len, data, datalen);
-    str->len += datalen;
-
-    /*
-     * Keep a trailing null in place, even though it's probably useless for
-     * binary data.  (Some callers are dealing with text but call this because
-     * their input isn't null-terminated.)
-     */
-    str->data[str->len] = '\0';
-}
-
-/*
  * appendBinaryStringInfoNT
  *
  * Append arbitrary binary data to a StringInfo, allocating more space
@@ -263,7 +241,7 @@ appendBinaryStringInfoNT(StringInfo str, const char *data, int datalen)
 }

 /*
- * enlargeStringInfo
+ * enlargeStringInfo_OOL
  *
  * Make sure there is enough space for 'needed' more bytes
  * ('needed' does not include the terminating null).
@@ -274,13 +252,16 @@ appendBinaryStringInfoNT(StringInfo str, const char *data, int datalen)
  * can save some palloc overhead by enlarging the buffer before starting
  * to store data in it.
  *
+ * We normally reach here only if enlargement is needed, since callers
+ * go through the inline function which doesn't call this otherwise.
+ *
  * NB: In the backend, because we use repalloc() to enlarge the buffer, the
  * string buffer will remain allocated in the same memory context that was
  * current when initStringInfo was called, even if another context is now
  * current.  This is the desired and indeed critical behavior!
  */
 void
-enlargeStringInfo(StringInfo str, int needed)
+enlargeStringInfo_OOL(StringInfo str, int needed)
 {
     int            newlen;

diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 5a2a3db..30ba826 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -87,6 +87,25 @@ extern void initStringInfo(StringInfo str);
 extern void resetStringInfo(StringInfo str);

 /*------------------------
+ * enlargeStringInfo
+ * Make sure a StringInfo's buffer can hold at least 'needed' more bytes
+ * ('needed' does not include the terminating null).
+ * The inlined code eliminates the common case where no work is needed.
+ */
+extern void enlargeStringInfo_OOL(StringInfo str, int needed);
+
+static inline void
+enlargeStringInfo(StringInfo str, int needed)
+{
+    /*
+     * Do the test in unsigned arithmetic so that if "needed" is negative,
+     * we'll go to the out-of-line code which will error out.
+     */
+    if ((unsigned) needed >= (unsigned) (str->maxlen - str->len))
+        enlargeStringInfo_OOL(str, needed);
+}
+
+/*------------------------
  * appendStringInfo
  * Format text data under the control of fmt (an sprintf-style format string)
  * and append it to whatever is already in str.  More space is allocated
@@ -139,10 +158,27 @@ extern void appendStringInfoSpaces(StringInfo str, int count);
 /*------------------------
  * appendBinaryStringInfo
  * Append arbitrary binary data to a StringInfo, allocating more space
- * if necessary.
+ * if necessary.  Ensures that a trailing null byte is present.
  */
-extern void appendBinaryStringInfo(StringInfo str,
-                                   const char *data, int datalen);
+static inline void
+appendBinaryStringInfo(StringInfo str, const char *data, int datalen)
+{
+    Assert(str != NULL);
+
+    /* Make more room if needed */
+    enlargeStringInfo(str, datalen);
+
+    /* OK, append the data */
+    memcpy(str->data + str->len, data, datalen);
+    str->len += datalen;
+
+    /*
+     * Keep a trailing null in place, even though it's probably useless for
+     * binary data.  (Some callers are dealing with text but call this because
+     * their input isn't null-terminated.)
+     */
+    str->data[str->len] = '\0';
+}

 /*------------------------
  * appendBinaryStringInfoNT
@@ -152,10 +188,4 @@ extern void appendBinaryStringInfo(StringInfo str,
 extern void appendBinaryStringInfoNT(StringInfo str,
                                      const char *data, int datalen);

-/*------------------------
- * enlargeStringInfo
- * Make sure a StringInfo's buffer can hold at least 'needed' more bytes.
- */
-extern void enlargeStringInfo(StringInfo str, int needed);
-
 #endif                            /* STRINGINFO_H */