Обсуждение: Replace some cstring_to_text to cstring_to_text_with_len

On Wed, Aug 30, 2023 at 03:00:13PM -0300, Ranier Vilela wrote:
> cstring_to_text has a small overhead, because call strlen for
> pointer to char parameter.
>
> Is it worth the effort to avoid this, where do we know the size of the
> parameter?

Are there workloads where this matters?
--
Michael

On Thu, Aug 31, 2023 at 6:07 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Em qui., 31 de ago. de 2023 às 00:22, Michael Paquier <michael@paquier.xyz> escreveu:
>>
>> On Wed, Aug 30, 2023 at 03:00:13PM -0300, Ranier Vilela wrote:
>> > cstring_to_text has a small overhead, because call strlen for
>> > pointer to char parameter.
>> >
>> > Is it worth the effort to avoid this, where do we know the size of the
>> > parameter?
>>
>> Are there workloads where this matters?
>
> None, but note this change has the same spirit of 8b26769bc.

- return cstring_to_text("");
+ return cstring_to_text_with_len("", 0);

This looks worse, so we'd better be getting something in return.

I agree this is a bit ugly. I wonder if we'd be better off creating a function that returned an empty text value, so we'd just avoid converting the empty cstring altogether and say:

  return empty_text();

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

> On 2023-08-31 Th 07:41, John Naylor wrote:
>>
>> On Thu, Aug 31, 2023 at 6:07 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
>> >
>> > Em qui., 31 de ago. de 2023 às 00:22, Michael Paquier
>> <michael@paquier.xyz> escreveu:
>> >>
>> >> On Wed, Aug 30, 2023 at 03:00:13PM -0300, Ranier Vilela wrote:
>> >> > cstring_to_text has a small overhead, because call strlen for
>> >> > pointer to char parameter.
>> >> >
>> >> > Is it worth the effort to avoid this, where do we know the size
>> of the
>> >> > parameter?
>> >>
>> >> Are there workloads where this matters?
>> >
>> > None, but note this change has the same spirit of 8b26769bc.
>>
>> - return cstring_to_text("");
>> + return cstring_to_text_with_len("", 0);
>>
>> This looks worse, so we'd better be getting something in return.
>
>
> I agree this is a bit ugly. I wonder if we'd be better off creating a
> function that returned an empty text value, so we'd just avoid 
> converting the empty cstring altogether and say:
>
>   return empty_text();

Or we could generalise it for any string literal (of which there are
slightly more¹ non-empty than empty in calls to
cstring_to_text(_with_len)):

#define literal_to_text(str) cstring_to_text_with_len("" str "", sizeof(str)-1)

[1]: 

~/src/postgresql $ rg 'cstring_to_text(_with_len)?\("[^"]+"' | wc -l
17
~/src/postgresql $ rg 'cstring_to_text(_with_len)?\(""' | wc -l
15

- ilmari

Ranier Vilela <ranier.vf@gmail.com> writes:

> Em qui., 31 de ago. de 2023 às 10:12, Dagfinn Ilmari Mannsåker <
> ilmari@ilmari.org> escreveu:
>
>> Andrew Dunstan <andrew@dunslane.net> writes:
>>
>> > On 2023-08-31 Th 07:41, John Naylor wrote:
>> >>
>> >> On Thu, Aug 31, 2023 at 6:07 PM Ranier Vilela <ranier.vf@gmail.com>
>> wrote:
>> >> >
>> >> > Em qui., 31 de ago. de 2023 às 00:22, Michael Paquier
>> >> <michael@paquier.xyz> escreveu:
>> >> >>
>> >> >> On Wed, Aug 30, 2023 at 03:00:13PM -0300, Ranier Vilela wrote:
>> >> >> > cstring_to_text has a small overhead, because call strlen for
>> >> >> > pointer to char parameter.
>> >> >> >
>> >> >> > Is it worth the effort to avoid this, where do we know the size
>> >> of the
>> >> >> > parameter?
>> >> >>
>> >> >> Are there workloads where this matters?
>> >> >
>> >> > None, but note this change has the same spirit of 8b26769bc.
>> >>
>> >> - return cstring_to_text("");
>> >> + return cstring_to_text_with_len("", 0);
>> >>
>> >> This looks worse, so we'd better be getting something in return.
>> >
>> >
>> > I agree this is a bit ugly. I wonder if we'd be better off creating a
>> > function that returned an empty text value, so we'd just avoid
>> > converting the empty cstring altogether and say:
>> >
>> >   return empty_text();
>>
>> Or we could generalise it for any string literal (of which there are
>> slightly more¹ non-empty than empty in calls to
>> cstring_to_text(_with_len)):
>>
>> #define literal_to_text(str) cstring_to_text_with_len("" str "",
>> sizeof(str)-1)
>>
> I do not agree, I think this will get worse.

How exactly will it get worse?  It's exactly equivalent to
cstring_to_text_with_len("", 0), since sizeof() is a compile-time
construct, and the string concatenation makes it fail if the argument is
not a literal string.

Whether we want an even-more-optimised version for an empty text value
is another matter, but I doubt it'd be worth it.  Another option would
be to make cstring_to_text(_with_len) static inline functions, which
lets the compiler eliminate the memcpy() call when len == 0.

In fact, after playing around a bit (https://godbolt.org/z/x51aYGadh),
it seems like GCC, Clang and MSVC all eliminate the strlen() and
memcpy() calls for cstring_to_text("") under -O2 if it's static inline.

- ilmari

On 31.08.23 16:10, Ranier Vilela wrote:
> Em qui., 31 de ago. de 2023 às 09:51, Andrew Dunstan 
> <andrew@dunslane.net <mailto:andrew@dunslane.net>> escreveu:
> 
> 
>     On 2023-08-31 Th 07:41, John Naylor wrote:
>>
>>     On Thu, Aug 31, 2023 at 6:07 PM Ranier Vilela <ranier.vf@gmail.com
>>     <mailto:ranier.vf@gmail.com>> wrote:
>>     >
>>     > Em qui., 31 de ago. de 2023 às 00:22, Michael Paquier
>>     <michael@paquier.xyz <mailto:michael@paquier.xyz>> escreveu:
>>     >>
>>     >> On Wed, Aug 30, 2023 at 03:00:13PM -0300, Ranier Vilela wrote:
>>     >> > cstring_to_text has a small overhead, because call strlen for
>>     >> > pointer to char parameter.
>>     >> >
>>     >> > Is it worth the effort to avoid this, where do we know the
>>     size of the
>>     >> > parameter?
>>     >>
>>     >> Are there workloads where this matters?
>>     >
>>     > None, but note this change has the same spirit of 8b26769bc.
>>
>>     - return cstring_to_text("");
>>     + return cstring_to_text_with_len("", 0);
>>
>>     This looks worse, so we'd better be getting something in return.
> 
> 
>     I agree this is a bit ugly. I wonder if we'd be better off creating
>     a function that returned an empty text value, so we'd just avoid
>     converting the empty cstring altogether and say:
> 
>        return empty_text();
> 
> Hi,
> Thanks for the suggestion,  I agreed.
> 
> New patch is attached.

I think these patches make the code uniformly uglier and harder to 
understand.

If a performance benefit could be demonstrated, then making 
cstring_to_text() an inline function could be sensible.  But I wouldn't 
go beyond that.

Ranier Vilela <ranier.vf@gmail.com> writes:

> Em qui., 31 de ago. de 2023 às 12:12, Dagfinn Ilmari Mannsåker <
> ilmari@ilmari.org> escreveu:
>
>> Ranier Vilela <ranier.vf@gmail.com> writes:
>>
>> > Em qui., 31 de ago. de 2023 às 10:12, Dagfinn Ilmari Mannsåker <
>> > ilmari@ilmari.org> escreveu:
>> >
>> >> Andrew Dunstan <andrew@dunslane.net> writes:
>> >>
>> >> > On 2023-08-31 Th 07:41, John Naylor wrote:
>> >> >>
>> >> >> On Thu, Aug 31, 2023 at 6:07 PM Ranier Vilela <ranier.vf@gmail.com>
>> >> wrote:
>> >> >> >
>> >> >> > Em qui., 31 de ago. de 2023 às 00:22, Michael Paquier
>> >> >> <michael@paquier.xyz> escreveu:
>> >> >> >>
>> >> >> >> On Wed, Aug 30, 2023 at 03:00:13PM -0300, Ranier Vilela wrote:
>> >> >> >> > cstring_to_text has a small overhead, because call strlen for
>> >> >> >> > pointer to char parameter.
>> >> >> >> >
>> >> >> >> > Is it worth the effort to avoid this, where do we know the size
>> >> >> of the
>> >> >> >> > parameter?
>> >> >> >>
>> >> >> >> Are there workloads where this matters?
>> >> >> >
>> >> >> > None, but note this change has the same spirit of 8b26769bc.
>> >> >>
>> >> >> - return cstring_to_text("");
>> >> >> + return cstring_to_text_with_len("", 0);
>> >> >>
>> >> >> This looks worse, so we'd better be getting something in return.
>> >> >
>> >> >
>> >> > I agree this is a bit ugly. I wonder if we'd be better off creating a
>> >> > function that returned an empty text value, so we'd just avoid
>> >> > converting the empty cstring altogether and say:
>> >> >
>> >> >   return empty_text();
>> >>
>> >> Or we could generalise it for any string literal (of which there are
>> >> slightly more¹ non-empty than empty in calls to
>> >> cstring_to_text(_with_len)):
>> >>
>> >> #define literal_to_text(str) cstring_to_text_with_len("" str "",
>> >> sizeof(str)-1)
>> >>
>> > I do not agree, I think this will get worse.
>>
>> How exactly will it get worse?  It's exactly equivalent to
>> cstring_to_text_with_len("", 0), since sizeof() is a compile-time
>> construct, and the string concatenation makes it fail if the argument is
>> not a literal string.
>>
> I think that concatenation makes the strings twice bigger, doesn't it?

No, it's just taking advantage of the fact that C string literals can be
split into multiple pieces separated by whitespace (like in SQL, but
without requiring a newline between them).  E.g. "foo" "bar" is exactly
equivalent to "foobar" after parsing.  The reason to use it in the macro
is to make it a syntax error if the argument is not a literal string but
instead a string variable, becuause in that case the sizeof() would
return the size of the pointer, not the string.

- ilmari

Peter Eisentraut <peter@eisentraut.org> writes:

> On 31.08.23 16:10, Ranier Vilela wrote:
>> Em qui., 31 de ago. de 2023 às 09:51, Andrew Dunstan
>> <andrew@dunslane.net <mailto:andrew@dunslane.net>> escreveu:
>> 
>>     On 2023-08-31 Th 07:41, John Naylor wrote:
>>>
>>>     On Thu, Aug 31, 2023 at 6:07 PM Ranier Vilela <ranier.vf@gmail.com
>>>     <mailto:ranier.vf@gmail.com>> wrote:
>>>     >
>>>     > Em qui., 31 de ago. de 2023 às 00:22, Michael Paquier
>>>     <michael@paquier.xyz <mailto:michael@paquier.xyz>> escreveu:
>>>     >>
>>>     >> On Wed, Aug 30, 2023 at 03:00:13PM -0300, Ranier Vilela wrote:
>>>     >> > cstring_to_text has a small overhead, because call strlen for
>>>     >> > pointer to char parameter.
>>>     >> >
>>>     >> > Is it worth the effort to avoid this, where do we know the
>>>     size of the
>>>     >> > parameter?
>>>     >>
>>>     >> Are there workloads where this matters?
>>>     >
>>>     > None, but note this change has the same spirit of 8b26769bc.
>>>
>>>     - return cstring_to_text("");
>>>     + return cstring_to_text_with_len("", 0);
>>>
>>>     This looks worse, so we'd better be getting something in return.
>> 
>>     I agree this is a bit ugly. I wonder if we'd be better off creating
>>     a function that returned an empty text value, so we'd just avoid
>>     converting the empty cstring altogether and say:
>>        return empty_text();
>> Hi,
>> Thanks for the suggestion,  I agreed.
>> New patch is attached.
>
> I think these patches make the code uniformly uglier and harder to
> understand.
>
> If a performance benefit could be demonstrated, then making
> cstring_to_text() an inline function could be sensible.  But I wouldn't 
> go beyond that.

I haven't benchmarked it yet, but here's a patch that inlines them and
changes callers of cstring_to_text_with_len() with a aliteral string and
constant length to cstring_to_text().

On an x86-64 Linux build (meson with -Dbuildtype=debugoptimized
-Dcassert=true), the inlining increases the size of the text section of
the postgres binary from 9719722 bytes to 9750557, i.e. an increase of
30KiB or 0.3%, while the change to cstring_to_text() makes zero
difference (as expected from my investigation).

- ilmari

From e332e5229dafd94e44ad8608c5fa2d3c58d80e0e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Thu, 31 Aug 2023 19:11:55 +0100
Subject: [PATCH] Inline cstring_to_text(_with_len) functions

This lets the compiler optimise out the strlen() and memcpy() calls
when they are called with a literal string or constant length.

Thus, convert cstring_to_text_with_length("literal", 7) calls to plain
cstring_to_text("literal") to avoid having to count string lenghts
manually.
---
 contrib/btree_gin/btree_gin.c     |  2 +-
 contrib/hstore/hstore_io.c        |  4 ++--
 src/backend/utils/adt/json.c      |  4 ++--
 src/backend/utils/adt/jsonfuncs.c |  4 ++--
 src/backend/utils/adt/varlena.c   | 32 +----------------------------
 src/include/utils/builtins.h      | 34 +++++++++++++++++++++++++++++--
 6 files changed, 40 insertions(+), 40 deletions(-)

diff --git a/contrib/btree_gin/btree_gin.c b/contrib/btree_gin/btree_gin.c
index c50d68ce18..59db475791 100644
--- a/contrib/btree_gin/btree_gin.c
+++ b/contrib/btree_gin/btree_gin.c
@@ -347,7 +347,7 @@ GIN_SUPPORT(cidr, true, leftmostvalue_inet, network_cmp)
 static Datum
 leftmostvalue_text(void)
 {
-    return PointerGetDatum(cstring_to_text_with_len("", 0));
+    return PointerGetDatum(cstring_to_text(""));
 }
 
 GIN_SUPPORT(text, true, leftmostvalue_text, bttextcmp)
diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
index 999ddad76d..7e3b52fbef 100644
--- a/contrib/hstore/hstore_io.c
+++ b/contrib/hstore/hstore_io.c
@@ -1347,7 +1347,7 @@ hstore_to_json_loose(PG_FUNCTION_ARGS)
                 dst;
 
     if (count == 0)
-        PG_RETURN_TEXT_P(cstring_to_text_with_len("{}", 2));
+        PG_RETURN_TEXT_P(cstring_to_text("{}"));
 
     initStringInfo(&tmp);
     initStringInfo(&dst);
@@ -1402,7 +1402,7 @@ hstore_to_json(PG_FUNCTION_ARGS)
                 dst;
 
     if (count == 0)
-        PG_RETURN_TEXT_P(cstring_to_text_with_len("{}", 2));
+        PG_RETURN_TEXT_P(cstring_to_text("{}"));
 
     initStringInfo(&tmp);
     initStringInfo(&dst);
diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index 2c620809b2..cd2b494259 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -1290,7 +1290,7 @@ json_build_object(PG_FUNCTION_ARGS)
 Datum
 json_build_object_noargs(PG_FUNCTION_ARGS)
 {
-    PG_RETURN_TEXT_P(cstring_to_text_with_len("{}", 2));
+    PG_RETURN_TEXT_P(cstring_to_text("{}"));
 }
 
 Datum
@@ -1346,7 +1346,7 @@ json_build_array(PG_FUNCTION_ARGS)
 Datum
 json_build_array_noargs(PG_FUNCTION_ARGS)
 {
-    PG_RETURN_TEXT_P(cstring_to_text_with_len("[]", 2));
+    PG_RETURN_TEXT_P(cstring_to_text("[]"));
 }
 
 /*
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index a4bfa5e404..b8845635ac 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -1800,8 +1800,8 @@ JsonbValueAsText(JsonbValue *v)
 
         case jbvBool:
             return v->val.boolean ?
-                cstring_to_text_with_len("true", 4) :
-                cstring_to_text_with_len("false", 5);
+                cstring_to_text("true") :
+                cstring_to_text("false");
 
         case jbvString:
             return cstring_to_text_with_len(v->val.string.val,
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 72e1e24fe0..1529498e8b 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -171,36 +171,6 @@ static void text_format_append_string(StringInfo buf, const char *str,
  *     CONVERSION ROUTINES EXPORTED FOR USE BY C CODE                             *
  *****************************************************************************/
 
-/*
- * cstring_to_text
- *
- * Create a text value from a null-terminated C string.
- *
- * The new text value is freshly palloc'd with a full-size VARHDR.
- */
-text *
-cstring_to_text(const char *s)
-{
-    return cstring_to_text_with_len(s, strlen(s));
-}
-
-/*
- * cstring_to_text_with_len
- *
- * Same as cstring_to_text except the caller specifies the string length;
- * the string need not be null_terminated.
- */
-text *
-cstring_to_text_with_len(const char *s, int len)
-{
-    text       *result = (text *) palloc(len + VARHDRSZ);
-
-    SET_VARSIZE(result, len + VARHDRSZ);
-    memcpy(VARDATA(result), s, len);
-
-    return result;
-}
-
 /*
  * text_to_cstring
  *
@@ -4827,7 +4797,7 @@ array_to_text_internal(FunctionCallInfo fcinfo, ArrayType *v,
 
     /* if there are no elements, return an empty string */
     if (nitems == 0)
-        return cstring_to_text_with_len("", 0);
+        return cstring_to_text("");
 
     element_type = ARR_ELEMTYPE(v);
     initStringInfo(&buf);
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index 2f8b46d6da..198a88cb37 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -17,6 +17,7 @@
 #include "fmgr.h"
 #include "nodes/nodes.h"
 #include "utils/fmgrprotos.h"
+#include "varatt.h"
 
 /* Sign + the most decimal digits an 8-byte number could have */
 #define MAXINT8LEN 20
@@ -86,8 +87,37 @@ extern void generate_operator_clause(fmStringInfo buf,
 extern int    bpchartruelen(char *s, int len);
 
 /* popular functions from varlena.c */
-extern text *cstring_to_text(const char *s);
-extern text *cstring_to_text_with_len(const char *s, int len);
+
+/*
+ * cstring_to_text_with_len
+ *
+ * Same as cstring_to_text except the caller specifies the string length;
+ * the string need not be null-terminated.
+ */
+static inline text *
+cstring_to_text_with_len(const char *s, int len)
+{
+    text       *result = (text *) palloc(len + VARHDRSZ);
+
+    SET_VARSIZE(result, len + VARHDRSZ);
+    memcpy(VARDATA(result), s, len);
+
+    return result;
+}
+
+/*
+ * cstring_to_text
+ *
+ * Create a text value from a null-terminated C string.
+ *
+ * The new text value is freshly palloc'd with a full-size VARHDR.
+ */
+static inline text *
+cstring_to_text(const char *s)
+{
+    return cstring_to_text_with_len(s, strlen(s));
+}
+
 extern char *text_to_cstring(const text *t);
 extern void text_to_cstring_buffer(const text *src, char *dst, size_t dst_len);
 
-- 
2.39.2