Thread: 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: https://www.postgresql.org/message-id/20241219.151950.488757175470671324.ishii%40postgresql.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 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. 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
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 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?
> 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
>
>
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
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
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
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