Proposal: add new API to stringinfo
Currently the StringInfo package provides StringInfo object creation
API with fixed length initial allocation size (1024 bytes). However,
if we want to allocate much smaller size of initial allocation, this
is waste of space.
Background: While working on this:
/messages/by-id/20241219.151950.488757175470671324.ishii@postgresql.org
I need to create lots of StringInfo many (like 100k) objects if a
Window frame has that number of rows. In this case postgres allocates
1024 * 100k = 97.7MB of memory, which is too much because I usually
only need 10 or so bytes for each StringInfo data buffer.
To solve the problem I need to hack the internal of StringInfo object
like this.
len = 10;
str = makeStringInfo();
pfree(encoded_str->data);
str->data = (char *)palloc0(len);
str->maxlen = len;
This is not only ugly but breaks interface boundary. If we have
another API like makeStringInfoWithSize(), I could do that in much
better and simple way:
len = 10;
str = makeStringInfoWithSize(len);
Attached is a patch to implement it. In the patch I add two new APIs.
extern StringInfo makeStringInfoWithSize(int size);
extern void initStringInfoWithSize(StringInfo str, int size);
Maybe I could re-invent the wheel by copying stringinfo.c, but I think
there are some uses cases like me, and it could justify in adding more
code to stringinfo.c.
Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Attachments:
stringinfo.patchtext/x-patch; charset=us-asciiDownload
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index 6192e65477..e740b499fa 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -46,6 +46,24 @@ makeStringInfo(void)
return res;
}
+/*
+ * makeStringInfoWithSize
+ *
+ * Create an empty 'StringInfoData' & return a pointer to it.
+ * The initial memory allocation size is specified by 'size'.
+ */
+StringInfo
+makeStringInfoWithSize(int size)
+{
+ StringInfo res;
+
+ res = (StringInfo) palloc(sizeof(StringInfoData));
+
+ initStringInfoWithSize(res, size);
+
+ return res;
+}
+
/*
* initStringInfo
*
@@ -57,6 +75,20 @@ initStringInfo(StringInfo str)
{
int size = 1024; /* initial default buffer size */
+ initStringInfoWithSize(str, size);
+}
+
+/*
+ * initStringInfoWithSize
+ *
+ * Same as initStringInfo except accepting additional 'size' argument for the
+ * initial memory allocation.
+ */
+void
+initStringInfoWithSize(StringInfo str, int size)
+{
+ Assert(size > 0);
+
str->data = (char *) palloc(size);
str->maxlen = size;
resetStringInfo(str);
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index cd9632e3fc..5d603aa0cc 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -60,6 +60,10 @@ typedef StringInfoData *StringInfo;
* StringInfo stringptr = makeStringInfo();
* Both the StringInfoData and the data buffer are palloc'd.
*
+ * StringInfo stringptr = makeStringInfoWithSize(size);
+ * Both the StringInfoData and the data buffer are palloc'd.
+ * The data buffer is allocated with size 'size'.
+ *
* StringInfoData string;
* initStringInfo(&string);
* The data buffer is palloc'd but the StringInfoData is just local.
@@ -106,6 +110,13 @@ typedef StringInfoData *StringInfo;
*/
extern StringInfo makeStringInfo(void);
+/*------------------------
+ * makeStringInfo
+ * Create an empty 'StringInfoData' & return a pointer to it.
+ * The data buffer is allocated with size 'size'.
+ */
+extern StringInfo makeStringInfoWithSize(int size);
+
/*------------------------
* initStringInfo
* Initialize a StringInfoData struct (with previously undefined contents)
@@ -113,6 +124,14 @@ extern StringInfo makeStringInfo(void);
*/
extern void initStringInfo(StringInfo str);
+/*------------------------
+ * initStringInfoWithSize
+ * Initialize a StringInfoData struct (with previously undefined contents)
+ * to describe an empty string.
+ * The data buffer is allocated with size 'size'.
+ */
+extern void initStringInfoWithSize(StringInfo str, int size);
+
/*------------------------
* initReadOnlyStringInfo
* Initialize a StringInfoData struct from an existing string without copying
On Wed, Dec 25, 2024 at 12:37:04PM +0900, Tatsuo Ishii wrote:
Attached is a patch to implement it. In the patch I add two new APIs.
extern StringInfo makeStringInfoWithSize(int size);
extern void initStringInfoWithSize(StringInfo str, int size);Maybe I could re-invent the wheel by copying stringinfo.c, but I think
there are some uses cases like me, and it could justify in adding more
code to stringinfo.c.
Not sure how other feel about that, but I am not really convinced that
we need two APIs. Saying that, having more control over the initial
size used for a StringInfo could provide better practices in some
cases. This reminds me of the ALLOCSET_SMALL_* fields in memutils.h,
hence an idea would be an initStringInfoExtended() that you could
combine with two #define two: one for the "default" of 1024 and a
second one for "small", like 32 or 64 (?), that can be used at will
with the new API call. Then you could switch initStringInfo() to
become a macro of the new "extended" call. Just an idea.
--
Michael
On Wed, Dec 25, 2024 at 12:37:04PM +0900, Tatsuo Ishii wrote:
Attached is a patch to implement it. In the patch I add two new APIs.
extern StringInfo makeStringInfoWithSize(int size);
extern void initStringInfoWithSize(StringInfo str, int size);Maybe I could re-invent the wheel by copying stringinfo.c, but I think
there are some uses cases like me, and it could justify in adding more
code to stringinfo.c.Not sure how other feel about that, but I am not really convinced that
we need two APIs.
We could make initStringInfoWithSize() as a static function if we
don't want to add two new APIs. The reason it is exported in the patch
is that I thought it maybe useful for callers who use stringinfo in
this pattern.
StringInfoData string;
initStringInfoWithSize(&string);
Saying that, having more control over the initial
size used for a StringInfo could provide better practices in some
cases. This reminds me of the ALLOCSET_SMALL_* fields in memutils.h,
hence an idea would be an initStringInfoExtended() that you could
combine with two #define two: one for the "default" of 1024 and a
second one for "small", like 32 or 64 (?), that can be used at will
with the new API call. Then you could switch initStringInfo() to
become a macro of the new "extended" call. Just an idea.
But then the extensions that use stringinfo.c need to be recompiled?
Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
On Wed, 25 Dec 2024 at 16:37, Tatsuo Ishii <ishii@postgresql.org> wrote:
Currently the StringInfo package provides StringInfo object creation
API with fixed length initial allocation size (1024 bytes). However,
if we want to allocate much smaller size of initial allocation, this
is waste of space.
I think it would be good to have some method to allow callers to
specify the initial size. I'm personally surprise that the 1024
setting has been ok with all callers so far. StringInfo has always
reminded me of C#'s StringBuilder class, which has an overloaded
constructor to allow callers to specify the initial size. It seems
they realised that the default size isn't always ideal, so I don't
find it surprising that we've realised that too.
If you have trouble convincing people we need this for some new
reason, then maybe you could review the existing callers to see if
some of the existing call sites make it any more convincing.
A very quick review, I found:
send_message_to_frontend() initStringInfo(&buf) 1024 is probably too big
HandleParallelMessages() seems to know exactly how many bytes are
needed. Can this use a read-only StringInfo?
send_feedback() seems to know what size of the buffer it needs
buf_init() knows the exact size.
I didn't review the patch in detail, but I think "initsize" would be a
better parameter name than "size".
David
On Thu, Dec 26, 2024 at 09:53:09AM +0900, Tatsuo Ishii wrote:
Saying that, having more control over the initial
size used for a StringInfo could provide better practices in some
cases. This reminds me of the ALLOCSET_SMALL_* fields in memutils.h,
hence an idea would be an initStringInfoExtended() that you could
combine with two #define two: one for the "default" of 1024 and a
second one for "small", like 32 or 64 (?), that can be used at will
with the new API call. Then you could switch initStringInfo() to
become a macro of the new "extended" call. Just an idea.But then the extensions that use stringinfo.c need to be recompiled?
New APIs are materials for HEAD, so recompilation needs to happen
anyway. Using a macro makes things slightly simpler and it would not
break unnecessarily the compilation of existing extensions.
--
Michael
Michael said:
New APIs are materials for HEAD, so recompilation needs to happen
anyway. Using a macro makes things slightly simpler and it would not
break unnecessarily the compilation of existing extensions.
Ok.
David said:
I didn't review the patch in detail, but I think "initsize" would be a
better parameter name than "size".
Ok, will change to "initsize".
With opinions from Michael and David , what about following additional APIs?
#define STRINGINFO_DEFAULT_SIZE 1024 /* default initial allocation size */
#define STRINGINFO_SMALL_SIZE 64 /* small initial allocation size */
#define makeStringInfo makeStringInfoExtended(STRINGINFO_DEFAULT_SIZE)
#define initStringInfo(str) initStringInfoExtended(str, STRINGINFO_DEFAULT_SIZE)
extern StringInfo makeStringInfoExtended(int initsize);
extern void initStringInfoExtended(StringInfo str, int initsize);
Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
On 2024-12-28 Sa 12:45 AM, Tatsuo Ishii wrote:
Michael said:
New APIs are materials for HEAD, so recompilation needs to happen
anyway. Using a macro makes things slightly simpler and it would not
break unnecessarily the compilation of existing extensions.Ok.
David said:
I didn't review the patch in detail, but I think "initsize" would be a
better parameter name than "size".Ok, will change to "initsize".
With opinions from Michael and David , what about following additional APIs?
#define STRINGINFO_DEFAULT_SIZE 1024 /* default initial allocation size */
#define STRINGINFO_SMALL_SIZE 64 /* small initial allocation size */#define makeStringInfo makeStringInfoExtended(STRINGINFO_DEFAULT_SIZE)
#define initStringInfo(str) initStringInfoExtended(str, STRINGINFO_DEFAULT_SIZE)extern StringInfo makeStringInfoExtended(int initsize);
extern void initStringInfoExtended(StringInfo str, int initsize);
Seems like a good idea.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Fri, Dec 27, 2024 at 9:46 PM Tatsuo Ishii <ishii@postgresql.org> wrote:
With opinions from Michael and David , what about following additional APIs?
...
#define makeStringInfo makeStringInfoExtended(STRINGINFO_DEFAULT_SIZE)
Minor nit: the definition of the macro should contain parentheses, like so:
#define makeStringInfo() ....
Best regards,
Gurjeet
http://Gurje.et
With opinions from Michael and David , what about following additional
APIs?#define STRINGINFO_DEFAULT_SIZE 1024 /* default initial allocation size
#*/
#define STRINGINFO_SMALL_SIZE 64 /* small initial allocation size */#define makeStringInfo makeStringInfoExtended(STRINGINFO_DEFAULT_SIZE)
#define initStringInfo(str) initStringInfoExtended(str,
#STRINGINFO_DEFAULT_SIZE)extern StringInfo makeStringInfoExtended(int initsize);
extern void initStringInfoExtended(StringInfo str, int initsize);Seems like a good idea.
Thanks.
Minor nit: the definition of the macro should contain parentheses, like so:
#define makeStringInfo() ....
Indeed. Thanks for pointing it out.
Attached is the patch for this.
Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Attachments:
v1-0001-Add-new-StringInfo-APIs.patchtext/x-patch; charset=us-asciiDownload
From fe0f556264d0d25469da93896c312d7957936531 Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii <ishii@postgresql.org>
Date: Sat, 4 Jan 2025 13:27:50 +0900
Subject: [PATCH v1] Add new StringInfo APIs.
Previously StringInfo only provided APIs with fixed initial memory
allocation size for the data buffer: 1024 bytes. This is inappropreate
for some callers that need less memory buffer. To fix this, new APIs
that accept the initial memory allocation size parameter are added:
extern StringInfo makeStringInfoExtended(int initsize);
extern void initStringInfoExtended(StringInfo str, int initsize);
Existing APIs (makeStringInfo() and initStringInfo()) now become
macros to call makeStringInfoExtended() and initStringInfoExtended()
respectively. This means that callers of makeStringInfo() and
initStringInfo() need to be recompiled.
---
src/common/stringinfo.c | 23 +++++++++++++----------
src/include/lib/stringinfo.h | 34 +++++++++++++++++++++++++++++++---
2 files changed, 44 insertions(+), 13 deletions(-)
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index 55d2fbb864d..6838f9659aa 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -30,35 +30,38 @@
/*
- * makeStringInfo
+ * makeStringInfoExtended(int initsize)
*
* Create an empty 'StringInfoData' & return a pointer to it.
+ * The initial memory allocation size is specified by 'initsize'.
*/
StringInfo
-makeStringInfo(void)
+makeStringInfoExtended(int initsize)
{
StringInfo res;
+ Assert(initsize > 0);
+
res = (StringInfo) palloc(sizeof(StringInfoData));
- initStringInfo(res);
+ initStringInfoExtended(res, initsize);
return res;
}
/*
- * initStringInfo
+ * initStringInfoExtended
*
- * Initialize a StringInfoData struct (with previously undefined contents)
- * to describe an empty string.
+ * Initialize a StringInfoData struct (with previously undefined contents).
+ * The initial memory allocation size is specified by 'initsize'.
*/
void
-initStringInfo(StringInfo str)
+initStringInfoExtended(StringInfo str, int initsize)
{
- int size = 1024; /* initial default buffer size */
+ Assert(initsize > 0);
- str->data = (char *) palloc(size);
- str->maxlen = size;
+ str->data = (char *) palloc(initsize);
+ str->maxlen = initsize;
resetStringInfo(str);
}
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 335208a9fda..37e91b73714 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -55,11 +55,15 @@ typedef StringInfoData *StringInfo;
/*------------------------
- * There are four ways to create a StringInfo object initially:
+ * There are six ways to create a StringInfo object initially:
*
* StringInfo stringptr = makeStringInfo();
* Both the StringInfoData and the data buffer are palloc'd.
*
+ * StringInfo stringptr = makeStringInfoExtended(initsize);
+ * Same as makeStringInfo except the data buffer is allocated
+ * with size 'initsize'.
+ *
* StringInfoData string;
* initStringInfo(&string);
* The data buffer is palloc'd but the StringInfoData is just local.
@@ -67,6 +71,11 @@ typedef StringInfoData *StringInfo;
* only live as long as the current routine.
*
* StringInfoData string;
+ * initStringInfoExtended(&string, initsize);
+ * Same as initStringInfo except the data buffer is allocated
+ * with size 'initsize'.
+ *
+ * StringInfoData string;
* initReadOnlyStringInfo(&string, existingbuf, len);
* The StringInfoData's data field is set to point directly to the
* existing buffer and the StringInfoData's len is set to the given len.
@@ -100,18 +109,37 @@ typedef StringInfoData *StringInfo;
*-------------------------
*/
+#define STRINGINFO_DEFAULT_SIZE 1024 /* default initial allocation size */
+#define STRINGINFO_SMALL_SIZE 64 /* small initial allocation size */
+
/*------------------------
* makeStringInfo
* Create an empty 'StringInfoData' & return a pointer to it.
*/
-extern StringInfo makeStringInfo(void);
+#define makeStringInfo() makeStringInfoExtended(STRINGINFO_DEFAULT_SIZE)
+
+/*------------------------
+ * makeStringInfoExtended
+ * Create an empty 'StringInfoData' & return a pointer to it.
+ * The data buffer is allocated with size 'initsize'.
+ */
+extern StringInfo makeStringInfoExtended(int initsize);
/*------------------------
* initStringInfo
* Initialize a StringInfoData struct (with previously undefined contents)
* to describe an empty string.
*/
-extern void initStringInfo(StringInfo str);
+#define initStringInfo(str) \
+ initStringInfoExtended(str, STRINGINFO_DEFAULT_SIZE)
+
+/*------------------------
+ * initStringInfoExtended
+ * Initialize a StringInfoData struct (with previously undefined contents)
+ * to describe an empty string.
+ * The data buffer is allocated with size 'initsize'.
+ */
+extern void initStringInfoExtended(StringInfo str, int initsize);
/*------------------------
* initReadOnlyStringInfo
--
2.25.1
If you have trouble convincing people we need this for some new
reason, then maybe you could review the existing callers to see if
some of the existing call sites make it any more convincing.A very quick review, I found:
send_message_to_frontend() initStringInfo(&buf) 1024 is probably too big
HandleParallelMessages() seems to know exactly how many bytes are
needed. Can this use a read-only StringInfo?
send_feedback() seems to know what size of the buffer it needs
I looked into send_feedback(). Maybe with the new API we could do
something like attached?
Show quoted text
buf_init() knows the exact size.
I didn't review the patch in detail, but I think "initsize" would be a
better parameter name than "size".David
Attachments:
worker.patchtext/x-patch; charset=us-asciiDownload
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 334bf3e7aff..66cc4ec49a1 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -3885,8 +3885,10 @@ send_feedback(XLogRecPtr recvpos, bool force, bool requestReply)
if (!reply_message)
{
MemoryContext oldctx = MemoryContextSwitchTo(ApplyContext);
-
- reply_message = makeStringInfo();
+ int initsize =
+ sizeof(char) + sizeof(int64) * 3 +
+ sizeof(TimestampTz) + sizeof(bool) + 1;
+ reply_message = makeStringInfoExtended(initsize);
MemoryContextSwitchTo(oldctx);
}
else
On Fri, Jan 3, 2025 at 8:54 PM Tatsuo Ishii <ishii@postgresql.org> wrote:
Attached is the patch for this.
Hi Tatsuo,
I believe the newline endings in your patches are causing the patch application
to fail. I experienced this with your initial patch, as well as with the latest
patch. Converting it to unix-style line endings seems to solve the problem. I'm
using the `patch` utility supplied with macos 13, in case that matters.
```
$ patch -p 1 < v1-0001-Add-new-StringInfo-APIs.patch
patching file 'src/common/stringinfo.c'
1 out of 1 hunks failed--saving rejects to 'src/common/stringinfo.c.rej'
patching file 'src/include/lib/stringinfo.h'
3 out of 3 hunks failed--saving rejects to 'src/include/lib/stringinfo.h.rej'
$ dos2unix v1-0001-Add-new-StringInfo-APIs.patch
dos2unix: converting file v1-0001-Add-new-StringInfo-APIs.patch to
Unix format...
$ patch -p1 < v1-0001-Add-new-StringInfo-APIs.patch
patching file 'src/common/stringinfo.c'
patching file 'src/include/lib/stringinfo.h'
```
The GNU patch cli (from Homebrew) seems to be able to deal with CRLF line
terminators somewhat gracefully.
```
$ patch -p1 < v1-0001-Add-new-StringInfo-APIs.patch
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/common/stringinfo.c
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/include/lib/stringinfo.h
```
After dealing with all this, I noticed that your patch has the commit message,
as well. So tried to apply it with `git am` command, and that was clean, as
expected.
- * Initialize a StringInfoData struct (with previously undefined contents)
- * to describe an empty string.
+ * Initialize a StringInfoData struct (with previously undefined contents).
+ * The initial memory allocation size is specified by 'initsize'.In the above hunk, your patch removes the text 'to describe an empty string'. I
don't think it needs to do that.
+#define initStringInfo(str) \
+ initStringInfoExtended(str, STRINGINFO_DEFAULT_SIZE)In the above hunk, the macro definition is split over 2 lines which seems
unnecessary. It seems to fit on one line just fine, with 80-car limit, as below.
#define initStringInfo(str) initStringInfoExtended(str, STRINGINFO_DEFAULT_SIZE)
+/*------------------------
+ * initStringInfoExtended
+ * Initialize a StringInfoData struct (with previously undefined contents)
+ * to describe an empty string.
+ * The data buffer is allocated with size 'initsize'.
+ */In the above hunk, it would look better if the last sentence was either made
part of the previous paragraph, or if there was a blank line between the two
paragraphs. I'd prefer the former, like below.
/*------------------------
* initStringInfoExtended
* Initialize a StringInfoData struct (with previously undefined contents) to
* describe an empty string. The data buffer is allocated with size 'initsize'.
*/
Personally, I'd prefer the parameter to be called simply 'size', instead of
'initsize'. The fact that the the function names begin with 'make' and 'init', I
feel the 'init' in parameter name is extraneous. But since this is a matter of
taste, no strong objections from me; the committer may choose to name it however
they prefer.
Just out of curiousity, would it be okay to shorten the 'Extended' in the name
to just 'Ext'. makeStringInfoExt() and initStringInfoExt() seem to be sufficent
to convey the meaning, while helping to keep line lenghts short at call sites.
A slightly edited commit message, if the committer wishes to use it:
Added new StringInfo APIs to allow callers to specify buffer size
Previously StringInfo APIs allocated buffers with fixed allocation size of
1024 bytes. This may be too large and inappropriate for some callers that
can do with smaller memory buffers. To fix this, introduce new APIs that
allow callers to specify initial buffer size.
extern StringInfo makeStringInfoExtended(int initsize);
extern void initStringInfoExtended(StringInfo str, int initsize);
Existing APIs (makeStringInfo() and initStringInfo()) are now macros to call
makeStringInfoExtended() and initStringInfoExtended(), respectively, with
the default buffer size of 1024.
Best regards,
Gurjeet
http://Gurje.et
Hi Gurjeet,
Thanks for looking into my patch.
On Fri, Jan 3, 2025 at 8:54 PM Tatsuo Ishii <ishii@postgresql.org> wrote:
Attached is the patch for this.
Hi Tatsuo,
I believe the newline endings in your patches are causing the patch application
to fail. I experienced this with your initial patch, as well as with the latest
patch. Converting it to unix-style line endings seems to solve the problem. I'm
using the `patch` utility supplied with macos 13, in case that matters.
I am using emacs + mew (MUA) and it gives the attachment MIME Type as
"Text/X-Patch(us-ascii)". I suspect the MIME type makes a trouble on
some MUAs. Sorry for it. Next time I will change the MIME type to
"Application/Octet-Stream" which is used in most patch posts.
- * Initialize a StringInfoData struct (with previously undefined contents) - * to describe an empty string. + * Initialize a StringInfoData struct (with previously undefined contents). + * The initial memory allocation size is specified by 'initsize'.In the above hunk, your patch removes the text 'to describe an empty string'. I
don't think it needs to do that.
You are right. Will fix.
+#define initStringInfo(str) \ + initStringInfoExtended(str, STRINGINFO_DEFAULT_SIZE)In the above hunk, the macro definition is split over 2 lines which seems
unnecessary. It seems to fit on one line just fine, with 80-car limit, as below.#define initStringInfo(str) initStringInfoExtended(str, STRINGINFO_DEFAULT_SIZE)
Initially I did so but according to the "Committing checklist"
https://wiki.postgresql.org/wiki/Committing_checklist
It recommends to not write lines longer than 78-char, rather than
80-char.
Verify that long lines are not better broken into several shorter lines:
git diff origin/master | grep -E '^(\+|diff)' | sed 's/^+//' | expand -t4 | awk "length > 78 || /^diff/"
+/*------------------------ + * initStringInfoExtended + * Initialize a StringInfoData struct (with previously undefined contents) + * to describe an empty string. + * The data buffer is allocated with size 'initsize'. + */In the above hunk, it would look better if the last sentence was either made
part of the previous paragraph, or if there was a blank line between the two
paragraphs. I'd prefer the former, like below./*------------------------
* initStringInfoExtended
* Initialize a StringInfoData struct (with previously undefined contents) to
* describe an empty string. The data buffer is allocated with size 'initsize'.
*/
Ok, will fix.
Personally, I'd prefer the parameter to be called simply 'size', instead of
'initsize'. The fact that the the function names begin with 'make' and 'init', I
feel the 'init' in parameter name is extraneous. But since this is a matter of
taste, no strong objections from me; the committer may choose to name it however
they prefer.
I don't have strong preference neither but thinking about the fact
that those functions initially allocate the specified size of memory,
then they stretch memory twice if the previous size is not sufficient,
probably 'initsize' would give less confusion. If we use the parameter
name 'size', some users may think that the functions keep on
allocating memory by the size specified by 'size' parameter.
Just out of curiousity, would it be okay to shorten the 'Extended' in the name
to just 'Ext'. makeStringInfoExt() and initStringInfoExt() seem to be sufficent
to convey the meaning, while helping to keep line lenghts short at call sites.
I think it's a good idea. I will change 'Extended' to 'Ext' in the
next patch.
A slightly edited commit message, if the committer wishes to use it:
Thanks.
Previously StringInfo APIs allocated buffers with fixed allocation size of
maybe we want to have 'initial' in front of "allocation size"?
Previously StringInfo APIs allocated buffers with fixed initial allocation size of
Added new StringInfo APIs to allow callers to specify buffer size
Previously StringInfo APIs allocated buffers with fixed allocation size of
1024 bytes. This may be too large and inappropriate for some callers that
can do with smaller memory buffers. To fix this, introduce new APIs that
allow callers to specify initial buffer size.extern StringInfo makeStringInfoExtended(int initsize);
extern void initStringInfoExtended(StringInfo str, int initsize);Existing APIs (makeStringInfo() and initStringInfo()) are now macros to call
makeStringInfoExtended() and initStringInfoExtended(), respectively, with
the default buffer size of 1024.
Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Hi Gurjeet,
Thanks for looking into my patch.
On Fri, Jan 3, 2025 at 8:54 PM Tatsuo Ishii <ishii@postgresql.org> wrote:
Attached is the patch for this.
Hi Tatsuo,
I believe the newline endings in your patches are causing the patch application
to fail. I experienced this with your initial patch, as well as with the latest
patch. Converting it to unix-style line endings seems to solve the problem. I'm
using the `patch` utility supplied with macos 13, in case that matters.I am using emacs + mew (MUA) and it gives the attachment MIME Type as
"Text/X-Patch(us-ascii)". I suspect the MIME type makes a trouble on
some MUAs. Sorry for it. Next time I will change the MIME type to
"Application/Octet-Stream" which is used in most patch posts.- * Initialize a StringInfoData struct (with previously undefined contents) - * to describe an empty string. + * Initialize a StringInfoData struct (with previously undefined contents). + * The initial memory allocation size is specified by 'initsize'.In the above hunk, your patch removes the text 'to describe an empty string'. I
don't think it needs to do that.You are right. Will fix.
+#define initStringInfo(str) \ + initStringInfoExtended(str, STRINGINFO_DEFAULT_SIZE)In the above hunk, the macro definition is split over 2 lines which seems
unnecessary. It seems to fit on one line just fine, with 80-car limit, as below.#define initStringInfo(str) initStringInfoExtended(str, STRINGINFO_DEFAULT_SIZE)
Initially I did so but according to the "Committing checklist"
https://wiki.postgresql.org/wiki/Committing_checklistIt recommends to not write lines longer than 78-char, rather than
80-char.Verify that long lines are not better broken into several shorter lines:
git diff origin/master | grep -E '^(\+|diff)' | sed 's/^+//' | expand -t4 | awk "length > 78 || /^diff/"+/*------------------------ + * initStringInfoExtended + * Initialize a StringInfoData struct (with previously undefined contents) + * to describe an empty string. + * The data buffer is allocated with size 'initsize'. + */In the above hunk, it would look better if the last sentence was either made
part of the previous paragraph, or if there was a blank line between the two
paragraphs. I'd prefer the former, like below./*------------------------
* initStringInfoExtended
* Initialize a StringInfoData struct (with previously undefined contents) to
* describe an empty string. The data buffer is allocated with size 'initsize'.
*/Ok, will fix.
Personally, I'd prefer the parameter to be called simply 'size', instead of
'initsize'. The fact that the the function names begin with 'make' and 'init', I
feel the 'init' in parameter name is extraneous. But since this is a matter of
taste, no strong objections from me; the committer may choose to name it however
they prefer.I don't have strong preference neither but thinking about the fact
that those functions initially allocate the specified size of memory,
then they stretch memory twice if the previous size is not sufficient,
probably 'initsize' would give less confusion. If we use the parameter
name 'size', some users may think that the functions keep on
allocating memory by the size specified by 'size' parameter.Just out of curiousity, would it be okay to shorten the 'Extended' in the name
to just 'Ext'. makeStringInfoExt() and initStringInfoExt() seem to be sufficent
to convey the meaning, while helping to keep line lenghts short at call sites.I think it's a good idea. I will change 'Extended' to 'Ext' in the
next patch.A slightly edited commit message, if the committer wishes to use it:
Thanks.
Previously StringInfo APIs allocated buffers with fixed allocation size of
maybe we want to have 'initial' in front of "allocation size"?
Previously StringInfo APIs allocated buffers with fixed initial allocation size of
Added new StringInfo APIs to allow callers to specify buffer size
Previously StringInfo APIs allocated buffers with fixed allocation size of
1024 bytes. This may be too large and inappropriate for some callers that
can do with smaller memory buffers. To fix this, introduce new APIs that
allow callers to specify initial buffer size.extern StringInfo makeStringInfoExtended(int initsize);
extern void initStringInfoExtended(StringInfo str, int initsize);Existing APIs (makeStringInfo() and initStringInfo()) are now macros to call
makeStringInfoExtended() and initStringInfoExtended(), respectively, with
the default buffer size of 1024.
Attached is the v2 patch to reflect above.
Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Attachments:
v2-0001-Add-new-StringInfo-APIs-to-allow-callers-to-speci.patchapplication/octet-streamDownload
From 1c9e8ce0d0fff5a110df8053142742fcd240c1c5 Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii <ishii@postgresql.org>
Date: Sun, 5 Jan 2025 17:01:09 +0900
Subject: [PATCH v2] Add new StringInfo APIs to allow callers to specify buffer
size.
Previously StringInfo APIs allocated buffers with fixed initial
allocation size of 1024 bytes. This may be too large and inappropriate
for some callers that can do with smaller memory buffers. To fix this,
introduce new APIs that allow callers to specify initial buffer size.
extern StringInfo makeStringInfoExt(int initsize);
extern void initStringInfoExt(StringInfo str, int initsize);
Existing APIs (makeStringInfo() and initStringInfo()) are now macros
to call makeStringInfoExtended() and initStringInfoExtended(),
respectively, with the default buffer size of 1024.
---
src/common/stringinfo.c | 20 ++++++++++++--------
src/include/lib/stringinfo.h | 33 ++++++++++++++++++++++++++++++---
2 files changed, 42 insertions(+), 11 deletions(-)
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index 55d2fbb864d..3c369eecd02 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -30,35 +30,39 @@
/*
- * makeStringInfo
+ * makeStringInfoExt(int initsize)
*
* Create an empty 'StringInfoData' & return a pointer to it.
+ * The initial memory allocation size is specified by 'initsize'.
*/
StringInfo
-makeStringInfo(void)
+makeStringInfoExt(int initsize)
{
StringInfo res;
+ Assert(initsize > 0);
+
res = (StringInfo) palloc(sizeof(StringInfoData));
- initStringInfo(res);
+ initStringInfoExt(res, initsize);
return res;
}
/*
- * initStringInfo
+ * initStringInfoExt
*
* Initialize a StringInfoData struct (with previously undefined contents)
* to describe an empty string.
+ * The initial memory allocation size is specified by 'initsize'.
*/
void
-initStringInfo(StringInfo str)
+initStringInfoExt(StringInfo str, int initsize)
{
- int size = 1024; /* initial default buffer size */
+ Assert(initsize > 0);
- str->data = (char *) palloc(size);
- str->maxlen = size;
+ str->data = (char *) palloc(initsize);
+ str->maxlen = initsize;
resetStringInfo(str);
}
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 335208a9fda..4c8bab88a48 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -55,11 +55,15 @@ typedef StringInfoData *StringInfo;
/*------------------------
- * There are four ways to create a StringInfo object initially:
+ * There are six ways to create a StringInfo object initially:
*
* StringInfo stringptr = makeStringInfo();
* Both the StringInfoData and the data buffer are palloc'd.
*
+ * StringInfo stringptr = makeStringInfoExt(initsize);
+ * Same as makeStringInfo except the data buffer is allocated
+ * with size 'initsize'.
+ *
* StringInfoData string;
* initStringInfo(&string);
* The data buffer is palloc'd but the StringInfoData is just local.
@@ -67,6 +71,11 @@ typedef StringInfoData *StringInfo;
* only live as long as the current routine.
*
* StringInfoData string;
+ * initStringInfoExt(&string, initsize);
+ * Same as initStringInfo except the data buffer is allocated
+ * with size 'initsize'.
+ *
+ * StringInfoData string;
* initReadOnlyStringInfo(&string, existingbuf, len);
* The StringInfoData's data field is set to point directly to the
* existing buffer and the StringInfoData's len is set to the given len.
@@ -100,18 +109,36 @@ typedef StringInfoData *StringInfo;
*-------------------------
*/
+#define STRINGINFO_DEFAULT_SIZE 1024 /* default initial allocation size */
+#define STRINGINFO_SMALL_SIZE 64 /* small initial allocation size */
+
/*------------------------
* makeStringInfo
* Create an empty 'StringInfoData' & return a pointer to it.
*/
-extern StringInfo makeStringInfo(void);
+#define makeStringInfo() makeStringInfoExt(STRINGINFO_DEFAULT_SIZE)
+
+/*------------------------
+ * makeStringInfoExt
+ * Create an empty 'StringInfoData' & return a pointer to it.
+ * The data buffer is allocated with size 'initsize'.
+ */
+extern StringInfo makeStringInfoExt(int initsize);
/*------------------------
* initStringInfo
* Initialize a StringInfoData struct (with previously undefined contents)
* to describe an empty string.
*/
-extern void initStringInfo(StringInfo str);
+#define initStringInfo(str) initStringInfoExt(str, STRINGINFO_DEFAULT_SIZE)
+
+/*------------------------
+ * initStringInfoExt
+ * Initialize a StringInfoData struct (with previously undefined contents) to
+ * describe an empty string. The data buffer is allocated with size
+ * 'initsize'.
+ */
+extern void initStringInfoExt(StringInfo str, int initsize);
/*------------------------
* initReadOnlyStringInfo
--
2.25.1
On Sun, 5 Jan 2025 at 21:03, Tatsuo Ishii <ishii@postgresql.org> wrote:
Attached is the v2 patch to reflect above.
A quick review.
I don't see any users of STRINGINFO_SMALL_SIZE, so I question the
value in defining it.
I think it might be worth adding a comment to initStringInfoExt and
makeStringInfoExt which mentions the valid ranges of initsize. 1 to
MaxAllocSize by the looks of it. The reason that it's good to document
this is that if we ever get any bug reports where some code is passing
something like 0 as the size, we'll know right away that it's the
caller's fault rather than the callee.
I'm also curious if this change has much of a meaningful impact in the
size of the compiled binary. The macro idea isn't quite how I'd have
done it as it does mean passing an extra parameter for every current
callsite. If I were doing this, I'd just have created the new
functions in stringinfo.c and have the current functions call the new
external function and rely on the compiler inlining, basically, that's
the same as what makeStringInfo() does today with initStringInfo().
I'd have done it like:
StringInfo
makeStringInfo(void)
{
return makeStringInfoExt(STRINGINFO_DEFAULT_SIZE);
}
Using current master, you can see from the following that
initStringInfo is inlined, despite being an external function. I've no
reason to doubt the compiler won't do the same for makeStringInfo()
and makeStringInfoExt().
$ gcc src/common/stringinfo.c -I src/include -O2 -S | cat stringinfo.s
| grep makeStringInfo -A 10
.globl makeStringInfo
.type makeStringInfo, @function
makeStringInfo:
.LFB94:
.cfi_startproc
endbr64
pushq %r12
.cfi_def_cfa_offset 16
.cfi_offset 12, -16
movl $24, %edi
call palloc@PLT
movl $1024, %edi
movq %rax, %r12
--
.size makeStringInfo, .-makeStringInfo
.p2align 4
.globl initStringInfo
.type initStringInfo, @function
initStringInfo:
.LFB95:
.cfi_startproc
endbr64
pushq %rbx
.cfi_def_cfa_offset 16
.cfi_offset 3, -16
Note, there is no "call initStringInfo"
David
Hi David,
Thanks for the review.
On Sun, 5 Jan 2025 at 21:03, Tatsuo Ishii <ishii@postgresql.org> wrote:
Attached is the v2 patch to reflect above.
A quick review.
I don't see any users of STRINGINFO_SMALL_SIZE, so I question the
value in defining it.I think it might be worth adding a comment to initStringInfoExt and
makeStringInfoExt which mentions the valid ranges of initsize. 1 to
MaxAllocSize by the looks of it. The reason that it's good to document
this is that if we ever get any bug reports where some code is passing
something like 0 as the size, we'll know right away that it's the
caller's fault rather than the callee.
Agreed.
I'm also curious if this change has much of a meaningful impact in the
size of the compiled binary. The macro idea isn't quite how I'd have
done it as it does mean passing an extra parameter for every current
callsite. If I were doing this, I'd just have created the new
functions in stringinfo.c and have the current functions call the new
external function and rely on the compiler inlining, basically, that's
the same as what makeStringInfo() does today with initStringInfo().I'd have done it like:
StringInfo
makeStringInfo(void)
{
return makeStringInfoExt(STRINGINFO_DEFAULT_SIZE);
}Using current master, you can see from the following that
initStringInfo is inlined, despite being an external function. I've no
reason to doubt the compiler won't do the same for makeStringInfo()
and makeStringInfoExt().$ gcc src/common/stringinfo.c -I src/include -O2 -S | cat stringinfo.s
| grep makeStringInfo -A 10
[snip]
Note, there is no "call initStringInfo"
I have tried with this direction. See attached v2 patch. In the asm
code I don't see "call makeStringInfoExt" as expected. But I see
"call initStringInfoExt". I assume this is an expected behavior.
$ gcc src/common/stringinfo.c -I src/include -O2 -S; cat stringinfo.s|grep makeStringInfo -A 20
.globl makeStringInfoExt
.type makeStringInfoExt, @function
makeStringInfoExt:
.LFB99:
.cfi_startproc
endbr64
pushq %r12
.cfi_def_cfa_offset 16
.cfi_offset 12, -16
pushq %rbp
.cfi_def_cfa_offset 24
.cfi_offset 6, -24
subq $8, %rsp
.cfi_def_cfa_offset 32
testl %edi, %edi
jle .L11
movl %edi, %ebp
movl $24, %edi
call palloc@PLT
movl %ebp, %esi
movq %rax, %rdi
movq %rax, %r12
call initStringInfoExt
--
.size makeStringInfoExt, .-makeStringInfoExt
.p2align 4
.globl makeStringInfo
.type makeStringInfo, @function
makeStringInfo:
.LFB98:
.cfi_startproc
endbr64
pushq %r12
.cfi_def_cfa_offset 16
.cfi_offset 12, -16
movl $24, %edi
call palloc@PLT
movl $1024, %esi
movq %rax, %r12
movq %rax, %rdi
call initStringInfoExt
movq %r12, %rax
popq %r12
.cfi_def_cfa_offset 8
ret
.cfi_endproc
.LFE98:
.size makeStringInfo, .-makeStringInfo
.section .rodata.str1.1
.LC2:
.string "str->maxlen != 0"
.text
.p2align 4
.globl resetStringInfo
.type resetStringInfo, @function
resetStringInfo:
.LFB102:
.cfi_startproc
endbr64
movl 12(%rdi), %edx
testl %edx, %edx
je .L19
movq (%rdi), %rax
movb $0, (%rax)
movl $0, 8(%rdi)
movl $0, 16(%rdi)
ret
.L19:
Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Attachments:
v2-0001-Add-new-StringInfo-APIs-to-allow-callers-to-speci.patchapplication/octet-streamDownload
From 7fd33c77a138c32694721b35dc8bd66b5d6f8522 Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii <ishii@postgresql.org>
Date: Mon, 6 Jan 2025 08:59:42 +0900
Subject: [PATCH v2] Add new StringInfo APIs to allow callers to specify buffer
size.
Previously StringInfo APIs allocated buffers with fixed initial
allocation size of 1024 bytes. This may be too large and inappropriate
for some callers that can do with smaller memory buffers. To fix this,
introduce new APIs that allow callers to specify initial buffer size.
extern StringInfo makeStringInfoExt(int initsize);
extern void initStringInfoExt(StringInfo str, int initsize);
Existing APIs (makeStringInfo() and initStringInfo()) are changed to
call makeStringInfoExt and initStringInfoExt respectively with the
default buffer size of 1024.
---
src/common/stringinfo.c | 37 ++++++++++++++++++++++++++++++++----
src/include/lib/stringinfo.h | 29 +++++++++++++++++++++++++++-
2 files changed, 61 insertions(+), 5 deletions(-)
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index 55d2fbb864d..aa7e73604c8 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -36,12 +36,27 @@
*/
StringInfo
makeStringInfo(void)
+{
+ return makeStringInfoExt(STRINGINFO_DEFAULT_SIZE);
+}
+
+/*
+ * makeStringInfoExt(int initsize)
+ *
+ * Create an empty 'StringInfoData' & return a pointer to it.
+ * The initial memory allocation size is specified by 'initsize'.
+ * The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+StringInfo
+makeStringInfoExt(int initsize)
{
StringInfo res;
+ Assert(initsize > 0);
+
res = (StringInfo) palloc(sizeof(StringInfoData));
- initStringInfo(res);
+ initStringInfoExt(res, initsize);
return res;
}
@@ -55,10 +70,24 @@ makeStringInfo(void)
void
initStringInfo(StringInfo str)
{
- int size = 1024; /* initial default buffer size */
+ return initStringInfoExt(str, STRINGINFO_DEFAULT_SIZE);
+}
+
+/*
+ * initStringInfoExt
+ *
+ * Initialize a StringInfoData struct (with previously undefined contents)
+ * to describe an empty string.
+ * The initial memory allocation size is specified by 'initsize'.
+ * The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+void
+initStringInfoExt(StringInfo str, int initsize)
+{
+ Assert(initsize > 0);
- str->data = (char *) palloc(size);
- str->maxlen = size;
+ str->data = (char *) palloc(initsize);
+ str->maxlen = initsize;
resetStringInfo(str);
}
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 335208a9fda..c96df989bb0 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -55,11 +55,15 @@ typedef StringInfoData *StringInfo;
/*------------------------
- * There are four ways to create a StringInfo object initially:
+ * There are six ways to create a StringInfo object initially:
*
* StringInfo stringptr = makeStringInfo();
* Both the StringInfoData and the data buffer are palloc'd.
*
+ * StringInfo stringptr = makeStringInfoExt(initsize);
+ * Same as makeStringInfo except the data buffer is allocated
+ * with size 'initsize'.
+ *
* StringInfoData string;
* initStringInfo(&string);
* The data buffer is palloc'd but the StringInfoData is just local.
@@ -67,6 +71,11 @@ typedef StringInfoData *StringInfo;
* only live as long as the current routine.
*
* StringInfoData string;
+ * initStringInfoExt(&string, initsize);
+ * Same as initStringInfo except the data buffer is allocated
+ * with size 'initsize'.
+ *
+ * StringInfoData string;
* initReadOnlyStringInfo(&string, existingbuf, len);
* The StringInfoData's data field is set to point directly to the
* existing buffer and the StringInfoData's len is set to the given len.
@@ -100,12 +109,22 @@ typedef StringInfoData *StringInfo;
*-------------------------
*/
+#define STRINGINFO_DEFAULT_SIZE 1024 /* default initial allocation size */
+
/*------------------------
* makeStringInfo
* Create an empty 'StringInfoData' & return a pointer to it.
*/
extern StringInfo makeStringInfo(void);
+/*------------------------
+ * makeStringInfoExt
+ * Create an empty 'StringInfoData' & return a pointer to it.
+ * The data buffer is allocated with size 'initsize'.
+ * The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+extern StringInfo makeStringInfoExt(int initsize);
+
/*------------------------
* initStringInfo
* Initialize a StringInfoData struct (with previously undefined contents)
@@ -113,6 +132,14 @@ extern StringInfo makeStringInfo(void);
*/
extern void initStringInfo(StringInfo str);
+/*------------------------
+ * initStringInfoExt
+ * Initialize a StringInfoData struct (with previously undefined contents) to
+ * describe an empty string. The data buffer is allocated with size
+ * 'initsize'. The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+extern void initStringInfoExt(StringInfo str, int initsize);
+
/*------------------------
* initReadOnlyStringInfo
* Initialize a StringInfoData struct from an existing string without copying
--
2.25.1
On Mon, Jan 06, 2025 at 10:57:23AM +0900, Tatsuo Ishii wrote:
I'm also curious if this change has much of a meaningful impact in the
size of the compiled binary. The macro idea isn't quite how I'd have
done it as it does mean passing an extra parameter for every current
callsite. If I were doing this, I'd just have created the new
functions in stringinfo.c and have the current functions call the new
external function and rely on the compiler inlining, basically, that's
the same as what makeStringInfo() does today with initStringInfo().I'd have done it like:
StringInfo
makeStringInfo(void)
{
return makeStringInfoExt(STRINGINFO_DEFAULT_SIZE);
}Using current master, you can see from the following that
initStringInfo is inlined, despite being an external function. I've no
reason to doubt the compiler won't do the same for makeStringInfo()
and makeStringInfoExt().$ gcc src/common/stringinfo.c -I src/include -O2 -S | cat stringinfo.s
| grep makeStringInfo -A 10[snip]
Note, there is no "call initStringInfo"
I have tried with this direction. See attached v2 patch. In the asm
code I don't see "call makeStringInfoExt" as expected. But I see
"call initStringInfoExt". I assume this is an expected behavior.
Wouldn't that mean that initStringInfoExt() is not getting inlined? I
wonder if you need to create an inlined helper function that both the
original and the new "ext" versions use to stop gcc from emitting these
CALL instructions. For example:
static inline void
initStringInfoInternal(StringInfo str, int initsize)
{
...
}
static inline StringInfo
makeStringInfoInternal(int initsize)
{
StringInfo res = (StringInfo) palloc(sizeof(StringInfoData));
initStringInfoInternal(res, initsize);
return res;
}
StringInfo
makeStringInfo(void)
{
return makeStringInfoInternal(STRINGINFO_DEFAULT_SIZE);
}
StringInfo
makeStringInfoExt(int initsize)
{
return makeStringInfoInternal(initsize);
}
void
initStringInfo(StringInfo str)
{
initStringInfoInternal(str, STRINGINFO_DEFAULT_SIZE);
}
void
initStringInfoExt(StringInfo str, int initsize)
{
initStringInfoInternal(str, initsize);
}
That's admittedly quite verbose, but it ensures that all of these functions
only use the inlined versions of each other. If that works, it might be
worth doing something similar to resetStringInfo() (assuming it is not
already getting inlined).
--
nathan
Hi Nathan,
Thanks for looking into the patch.
I have tried with this direction. See attached v2 patch. In the asm
code I don't see "call makeStringInfoExt" as expected. But I see
"call initStringInfoExt". I assume this is an expected behavior.Wouldn't that mean that initStringInfoExt() is not getting inlined?
Yes, but I thought additional inlining is not necessary if we only
care performance. But I haven't run benchmark yet. So this time I
tried it.
I created a small function:
Datum
stringinfo_test(PG_FUNCTION_ARGS)
{
StringInfo s;
int n;
int i;
n = PG_GETARG_INT32(0);
for (i = 0; i < n; i++)
{
s = makeStringInfo();
destroyStringInfo(s);
}
PG_RETURN_VOID();
}
and called the function with the iteration number 1000000 (1M) from
pgbench, repeating 30 seconds (if you are interested in the function,
please look into attached file). Then script given to pgbench is:
select stringinfo_test(1000000);
The result (average of 3 runs) are:
master: 76.993034 tps
with v2 patch: 75.945942 tps
So the v2 patch version is 1.3787% slower than master. Do you think we
need further inlining?
I
wonder if you need to create an inlined helper function that both the
original and the new "ext" versions use to stop gcc from emitting these
CALL instructions. For example:static inline void
initStringInfoInternal(StringInfo str, int initsize)
{
...
}static inline StringInfo
makeStringInfoInternal(int initsize)
{
StringInfo res = (StringInfo) palloc(sizeof(StringInfoData));initStringInfoInternal(res, initsize);
return res;
}StringInfo
makeStringInfo(void)
{
return makeStringInfoInternal(STRINGINFO_DEFAULT_SIZE);
}StringInfo
makeStringInfoExt(int initsize)
{
return makeStringInfoInternal(initsize);
}void
initStringInfo(StringInfo str)
{
initStringInfoInternal(str, STRINGINFO_DEFAULT_SIZE);
}void
initStringInfoExt(StringInfo str, int initsize)
{
initStringInfoInternal(str, initsize);
}That's admittedly quite verbose, but it ensures that all of these functions
only use the inlined versions of each other. If that works, it might be
worth doing something similar to resetStringInfo() (assuming it is not
already getting inlined).
Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Attachments:
On Tue, Jan 07, 2025 at 03:57:57PM +0900, Tatsuo Ishii wrote:
So the v2 patch version is 1.3787% slower than master. Do you think we
need further inlining?
If it's not too much work to coax the compiler to do so, then I don't see a
strong reason to avoid it. Granted, there are probably much more expensive
parts of the StringInfo support functions (e.g., palloc()), but these are
hot enough code paths that IMHO it's worth saving whatever cycles we can.
--
nathan
If it's not too much work to coax the compiler to do so, then I don't see a
strong reason to avoid it. Granted, there are probably much more expensive
parts of the StringInfo support functions (e.g., palloc()), but these are
hot enough code paths that IMHO it's worth saving whatever cycles we can.
Ok, I have created v3 patch to do more inlining as you suggested. With
the patch I confirmed that there's no call to functions except palloc
in makeStringInfo, makeStringInfoExt, initStringInfo and
initStringInfoExt in the asm codes (see attached stringinfo.s).
Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Attachments:
v3-0001-Add-new-StringInfo-APIs-to-allow-callers-to-speci.patchapplication/octet-streamDownload
From 121ec71c7b9d7f01a5a960c9a1ab09c0607a9e23 Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii <ishii@postgresql.org>
Date: Thu, 9 Jan 2025 14:59:50 +0900
Subject: [PATCH v3] Add new StringInfo APIs to allow callers to specify buffer
size.
Previously StringInfo APIs allocated buffers with fixed initial
allocation size of 1024 bytes. This may be too large and inappropriate
for some callers that can do with smaller memory buffers. To fix this,
introduce new APIs that allow callers to specify initial buffer size.
extern StringInfo makeStringInfoExt(int initsize);
extern void initStringInfoExt(StringInfo str, int initsize);
Existing APIs (makeStringInfo() and initStringInfo()) are changed to
call makeStringInfoExt and initStringInfoExt respectively (via inline
helper functions makeStringInfoInternal and initStringInfoInternal),
with the default buffer size of 1024.
---
src/common/stringinfo.c | 71 +++++++++++++++++++++++++++++++-----
src/include/lib/stringinfo.h | 29 ++++++++++++++-
2 files changed, 89 insertions(+), 11 deletions(-)
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index 55d2fbb864d..da0ffdbb0e3 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -29,6 +29,40 @@
#include "lib/stringinfo.h"
+/*
+ * initStringInfoInternal
+ *
+ * Initialize a StringInfoData struct (with previously undefined contents)
+ * to describe an empty string.
+ * The initial memory allocation size is specified by 'initsize'.
+ * The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+static inline void
+initStringInfoInternal(StringInfo str, int initsize)
+{
+ Assert(initsize > 0);
+
+ str->data = (char *) palloc(initsize);
+ str->maxlen = initsize;
+ resetStringInfo(str);
+}
+
+/*
+ * makeStringInfoInternal(int initsize)
+ *
+ * Create an empty 'StringInfoData' & return a pointer to it.
+ * The initial memory allocation size is specified by 'initsize'.
+ * The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+static inline StringInfo
+makeStringInfoInternal(int initsize)
+{
+ StringInfo res = (StringInfo) palloc(sizeof(StringInfoData));
+
+ initStringInfoInternal(res, initsize);
+ return res;
+}
+
/*
* makeStringInfo
*
@@ -37,13 +71,20 @@
StringInfo
makeStringInfo(void)
{
- StringInfo res;
-
- res = (StringInfo) palloc(sizeof(StringInfoData));
-
- initStringInfo(res);
+ return makeStringInfoInternal(STRINGINFO_DEFAULT_SIZE);
+}
- return res;
+/*
+ * makeStringInfoExt(int initsize)
+ *
+ * Create an empty 'StringInfoData' & return a pointer to it.
+ * The initial memory allocation size is specified by 'initsize'.
+ * The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+StringInfo
+makeStringInfoExt(int initsize)
+{
+ return makeStringInfoInternal(initsize);
}
/*
@@ -55,11 +96,21 @@ makeStringInfo(void)
void
initStringInfo(StringInfo str)
{
- int size = 1024; /* initial default buffer size */
+ return initStringInfoInternal(str, STRINGINFO_DEFAULT_SIZE);
+}
- str->data = (char *) palloc(size);
- str->maxlen = size;
- resetStringInfo(str);
+/*
+ * initStringInfoExt
+ *
+ * Initialize a StringInfoData struct (with previously undefined contents)
+ * to describe an empty string.
+ * The initial memory allocation size is specified by 'initsize'.
+ * The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+void
+initStringInfoExt(StringInfo str, int initsize)
+{
+ initStringInfoInternal(str, initsize);
}
/*
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 335208a9fda..c96df989bb0 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -55,11 +55,15 @@ typedef StringInfoData *StringInfo;
/*------------------------
- * There are four ways to create a StringInfo object initially:
+ * There are six ways to create a StringInfo object initially:
*
* StringInfo stringptr = makeStringInfo();
* Both the StringInfoData and the data buffer are palloc'd.
*
+ * StringInfo stringptr = makeStringInfoExt(initsize);
+ * Same as makeStringInfo except the data buffer is allocated
+ * with size 'initsize'.
+ *
* StringInfoData string;
* initStringInfo(&string);
* The data buffer is palloc'd but the StringInfoData is just local.
@@ -67,6 +71,11 @@ typedef StringInfoData *StringInfo;
* only live as long as the current routine.
*
* StringInfoData string;
+ * initStringInfoExt(&string, initsize);
+ * Same as initStringInfo except the data buffer is allocated
+ * with size 'initsize'.
+ *
+ * StringInfoData string;
* initReadOnlyStringInfo(&string, existingbuf, len);
* The StringInfoData's data field is set to point directly to the
* existing buffer and the StringInfoData's len is set to the given len.
@@ -100,12 +109,22 @@ typedef StringInfoData *StringInfo;
*-------------------------
*/
+#define STRINGINFO_DEFAULT_SIZE 1024 /* default initial allocation size */
+
/*------------------------
* makeStringInfo
* Create an empty 'StringInfoData' & return a pointer to it.
*/
extern StringInfo makeStringInfo(void);
+/*------------------------
+ * makeStringInfoExt
+ * Create an empty 'StringInfoData' & return a pointer to it.
+ * The data buffer is allocated with size 'initsize'.
+ * The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+extern StringInfo makeStringInfoExt(int initsize);
+
/*------------------------
* initStringInfo
* Initialize a StringInfoData struct (with previously undefined contents)
@@ -113,6 +132,14 @@ extern StringInfo makeStringInfo(void);
*/
extern void initStringInfo(StringInfo str);
+/*------------------------
+ * initStringInfoExt
+ * Initialize a StringInfoData struct (with previously undefined contents) to
+ * describe an empty string. The data buffer is allocated with size
+ * 'initsize'. The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+extern void initStringInfoExt(StringInfo str, int initsize);
+
/*------------------------
* initReadOnlyStringInfo
* Initialize a StringInfoData struct from an existing string without copying
--
2.25.1
On Thu, Jan 09, 2025 at 03:21:41PM +0900, Tatsuo Ishii wrote:
Ok, I have created v3 patch to do more inlining as you suggested. With
the patch I confirmed that there's no call to functions except palloc
in makeStringInfo, makeStringInfoExt, initStringInfo and
initStringInfoExt in the asm codes (see attached stringinfo.s).
Looks generally reasonable to me.
+/*
+ * initStringInfoInternal
+ *
+ * Initialize a StringInfoData struct (with previously undefined contents)
+ * to describe an empty string.
+ * The initial memory allocation size is specified by 'initsize'.
+ * The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+static inline void
+initStringInfoInternal(StringInfo str, int initsize)
+{
+ Assert(initsize > 0);
+
+ str->data = (char *) palloc(initsize);
+ str->maxlen = initsize;
+ resetStringInfo(str);
+}nitpick: Should we Assert(initsize <= MaxAllocSize) here, too?
--
nathan
Looks generally reasonable to me.
Thanks for looking into the patch.
+/* + * initStringInfoInternal + * + * Initialize a StringInfoData struct (with previously undefined contents) + * to describe an empty string. + * The initial memory allocation size is specified by 'initsize'. + * The valid range for 'initsize' is 1 to MaxAllocSize. + */ +static inline void +initStringInfoInternal(StringInfo str, int initsize) +{ + Assert(initsize > 0); + + str->data = (char *) palloc(initsize); + str->maxlen = initsize; + resetStringInfo(str); +}nitpick: Should we Assert(initsize <= MaxAllocSize) here, too?
Agreed. I have replaced the Assert with this in the attached v4 patch.
Assert(initsize >= 1 && initsize <= MaxAllocSize);
Note, I changed "initsize > 0" to "initsize >= 1" to better match with
the comment:
* The valid range for 'initsize' is 1 to MaxAllocSize.
If there's no objection, I am going to commit the patch.
Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Attachments:
v4-0001-Add-new-StringInfo-APIs-to-allow-callers-to-speci.patchapplication/octet-streamDownload
From bb86e85e442935290e617022519f1f2411bdd3e4 Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii <ishii@postgresql.org>
Date: Fri, 10 Jan 2025 11:13:11 +0900
Subject: [PATCH v4] Add new StringInfo APIs to allow callers to specify the
buffer size.
Previously StringInfo APIs allocated buffers with fixed initial
allocation size of 1024 bytes. This may be too large and inappropriate
for some callers that can do with smaller memory buffers. To fix this,
introduce new APIs that allow callers to specify initial buffer size.
extern StringInfo makeStringInfoExt(int initsize);
extern void initStringInfoExt(StringInfo str, int initsize);
Existing APIs (makeStringInfo() and initStringInfo()) are changed to
call makeStringInfoExt and initStringInfoExt respectively (via inline
helper functions makeStringInfoInternal and initStringInfoInternal),
with the default buffer size of 1024.
---
src/common/stringinfo.c | 71 +++++++++++++++++++++++++++++++-----
src/include/lib/stringinfo.h | 29 ++++++++++++++-
2 files changed, 89 insertions(+), 11 deletions(-)
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index 55d2fbb864d..f4317003411 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -29,6 +29,40 @@
#include "lib/stringinfo.h"
+/*
+ * initStringInfoInternal
+ *
+ * Initialize a StringInfoData struct (with previously undefined contents)
+ * to describe an empty string.
+ * The initial memory allocation size is specified by 'initsize'.
+ * The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+static inline void
+initStringInfoInternal(StringInfo str, int initsize)
+{
+ Assert(initsize >= 1 && initsize <= MaxAllocSize);
+
+ str->data = (char *) palloc(initsize);
+ str->maxlen = initsize;
+ resetStringInfo(str);
+}
+
+/*
+ * makeStringInfoInternal(int initsize)
+ *
+ * Create an empty 'StringInfoData' & return a pointer to it.
+ * The initial memory allocation size is specified by 'initsize'.
+ * The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+static inline StringInfo
+makeStringInfoInternal(int initsize)
+{
+ StringInfo res = (StringInfo) palloc(sizeof(StringInfoData));
+
+ initStringInfoInternal(res, initsize);
+ return res;
+}
+
/*
* makeStringInfo
*
@@ -37,13 +71,20 @@
StringInfo
makeStringInfo(void)
{
- StringInfo res;
-
- res = (StringInfo) palloc(sizeof(StringInfoData));
-
- initStringInfo(res);
+ return makeStringInfoInternal(STRINGINFO_DEFAULT_SIZE);
+}
- return res;
+/*
+ * makeStringInfoExt(int initsize)
+ *
+ * Create an empty 'StringInfoData' & return a pointer to it.
+ * The initial memory allocation size is specified by 'initsize'.
+ * The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+StringInfo
+makeStringInfoExt(int initsize)
+{
+ return makeStringInfoInternal(initsize);
}
/*
@@ -55,11 +96,21 @@ makeStringInfo(void)
void
initStringInfo(StringInfo str)
{
- int size = 1024; /* initial default buffer size */
+ return initStringInfoInternal(str, STRINGINFO_DEFAULT_SIZE);
+}
- str->data = (char *) palloc(size);
- str->maxlen = size;
- resetStringInfo(str);
+/*
+ * initStringInfoExt
+ *
+ * Initialize a StringInfoData struct (with previously undefined contents)
+ * to describe an empty string.
+ * The initial memory allocation size is specified by 'initsize'.
+ * The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+void
+initStringInfoExt(StringInfo str, int initsize)
+{
+ initStringInfoInternal(str, initsize);
}
/*
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 335208a9fda..c96df989bb0 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -55,11 +55,15 @@ typedef StringInfoData *StringInfo;
/*------------------------
- * There are four ways to create a StringInfo object initially:
+ * There are six ways to create a StringInfo object initially:
*
* StringInfo stringptr = makeStringInfo();
* Both the StringInfoData and the data buffer are palloc'd.
*
+ * StringInfo stringptr = makeStringInfoExt(initsize);
+ * Same as makeStringInfo except the data buffer is allocated
+ * with size 'initsize'.
+ *
* StringInfoData string;
* initStringInfo(&string);
* The data buffer is palloc'd but the StringInfoData is just local.
@@ -67,6 +71,11 @@ typedef StringInfoData *StringInfo;
* only live as long as the current routine.
*
* StringInfoData string;
+ * initStringInfoExt(&string, initsize);
+ * Same as initStringInfo except the data buffer is allocated
+ * with size 'initsize'.
+ *
+ * StringInfoData string;
* initReadOnlyStringInfo(&string, existingbuf, len);
* The StringInfoData's data field is set to point directly to the
* existing buffer and the StringInfoData's len is set to the given len.
@@ -100,12 +109,22 @@ typedef StringInfoData *StringInfo;
*-------------------------
*/
+#define STRINGINFO_DEFAULT_SIZE 1024 /* default initial allocation size */
+
/*------------------------
* makeStringInfo
* Create an empty 'StringInfoData' & return a pointer to it.
*/
extern StringInfo makeStringInfo(void);
+/*------------------------
+ * makeStringInfoExt
+ * Create an empty 'StringInfoData' & return a pointer to it.
+ * The data buffer is allocated with size 'initsize'.
+ * The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+extern StringInfo makeStringInfoExt(int initsize);
+
/*------------------------
* initStringInfo
* Initialize a StringInfoData struct (with previously undefined contents)
@@ -113,6 +132,14 @@ extern StringInfo makeStringInfo(void);
*/
extern void initStringInfo(StringInfo str);
+/*------------------------
+ * initStringInfoExt
+ * Initialize a StringInfoData struct (with previously undefined contents) to
+ * describe an empty string. The data buffer is allocated with size
+ * 'initsize'. The valid range for 'initsize' is 1 to MaxAllocSize.
+ */
+extern void initStringInfoExt(StringInfo str, int initsize);
+
/*------------------------
* initReadOnlyStringInfo
* Initialize a StringInfoData struct from an existing string without copying
--
2.25.1
On Fri, Jan 10, 2025 at 11:31:34AM +0900, Tatsuo Ishii wrote:
If there's no objection, I am going to commit the patch.
No objections here.
--
nathan
No objections here.
V4 patch pushed. Thanks.
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp