Re: Replace some cstring_to_text to cstring_to_text_with_len - Mailing list pgsql-hackers

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