Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Add new StringInfo APIs to allow callers to specify the buffer size.
- a9dcbb4d5c00 18.0 landed
-
Proposal: add new API to stringinfo
Tatsuo Ishii <ishii@postgresql.org> — 2024-12-25T03:37:04Z
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
-
Re: Proposal: add new API to stringinfo
Michael Paquier <michael@paquier.xyz> — 2024-12-25T05:28:55Z
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
-
Re: Proposal: add new API to stringinfo
Tatsuo Ishii <ishii@postgresql.org> — 2024-12-26T00:53:09Z
> 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
-
Re: Proposal: add new API to stringinfo
David Rowley <dgrowleyml@gmail.com> — 2024-12-26T03:15:42Z
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
-
Re: Proposal: add new API to stringinfo
Michael Paquier <michael@paquier.xyz> — 2024-12-26T03:37:12Z
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
-
Re: Proposal: add new API to stringinfo
Tatsuo Ishii <ishii@postgresql.org> — 2024-12-28T05:45:46Z
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
-
Re: Proposal: add new API to stringinfo
Andrew Dunstan <andrew@dunslane.net> — 2025-01-03T22:16:58Z
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
-
Re: Proposal: add new API to stringinfo
Gurjeet Singh <gurjeet@singh.im> — 2025-01-03T22:55:04Z
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
-
Re: Proposal: add new API to stringinfo
Tatsuo Ishii <ishii@postgresql.org> — 2025-01-04T04:54:39Z
>> 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
-
Re: Proposal: add new API to stringinfo
Tatsuo Ishii <ishii@postgresql.org> — 2025-01-05T01:51:29Z
> 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 > >
-
Re: Proposal: add new API to stringinfo
Gurjeet Singh <gurjeet@singh.im> — 2025-01-05T04:37:25Z
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 -
Re: Proposal: add new API to stringinfo
Tatsuo Ishii <ishii@postgresql.org> — 2025-01-05T06:33:11Z
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 -
Re: Proposal: add new API to stringinfo
Tatsuo Ishii <ishii@postgresql.org> — 2025-01-05T08:03:26Z
> 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. 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
-
Re: Proposal: add new API to stringinfo
David Rowley <dgrowleyml@gmail.com> — 2025-01-05T08:55:58Z
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 -
Re: Proposal: add new API to stringinfo
Tatsuo Ishii <ishii@postgresql.org> — 2025-01-06T01:57:23Z
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 -
Re: Proposal: add new API to stringinfo
Nathan Bossart <nathandbossart@gmail.com> — 2025-01-06T19:20:13Z
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 -
Re: Proposal: add new API to stringinfo
Tatsuo Ishii <ishii@postgresql.org> — 2025-01-07T06:57:57Z
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 -
Re: Proposal: add new API to stringinfo
Nathan Bossart <nathandbossart@gmail.com> — 2025-01-08T17:49:45Z
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
-
Re: Proposal: add new API to stringinfo
Tatsuo Ishii <ishii@postgresql.org> — 2025-01-09T06:21:41Z
> 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
-
Re: Proposal: add new API to stringinfo
Nathan Bossart <nathandbossart@gmail.com> — 2025-01-09T23:27:03Z
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 -
Re: Proposal: add new API to stringinfo
Tatsuo Ishii <ishii@postgresql.org> — 2025-01-10T02:31:34Z
> 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 -
Re: Proposal: add new API to stringinfo
Nathan Bossart <nathandbossart@gmail.com> — 2025-01-10T19:26:06Z
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
-
Re: Proposal: add new API to stringinfo
Tatsuo Ishii <ishii@postgresql.org> — 2025-01-10T23:25:48Z
> 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