Thread: Improve a few appendStringInfo calls new to v18
Looks like v18 has grown a few appendStringInfo misusages, e.g. using appendStringInfo() when no formatting is needed or just using format "%s" instead of using appendStringInfoString(). I've attached a couple of patches. The 0001 is just my method for finding these, not for commit. 0002 contains the fixes to commit. Any objections to doing this soonish? Or in a few weeks? David
Attachment
On 10/04/2025 06:51, David Rowley wrote: > Looks like v18 has grown a few appendStringInfo misusages, e.g. using > appendStringInfo() when no formatting is needed or just using format > "%s" instead of using appendStringInfoString(). > > I've attached a couple of patches. The 0001 is just my method for > finding these, not for commit. Clever! > 0002 contains the fixes to commit. > > Any objections to doing this soonish? Or in a few weeks? Sure, let's do it. Why would we wait? -- Heikki Linnakangas Neon (https://neon.tech)
On Thu, Apr 10, 2025 at 11:24:36AM +0300, Heikki Linnakangas wrote: > On 10/04/2025 06:51, David Rowley wrote: >> Any objections to doing this soonish? Or in a few weeks? > > Sure, let's do it. Why would we wait? +1. You did something similar for v17 (commit 8461424), and it seems like an entirely reasonable post-feature-freeze routine to me. This probably isn't v18 material, but this reminds me of my idea to change appendStringInfoString() into a macro for appendBinaryStringInfo() so that the compiler can remove the runtime strlen() calls for string literals [0]. In most cases, the benefits are probably negligible, but StringInfo is sometimes used in hot paths. [0] https://postgr.es/m/20231218164135.GA530790%40nathanxps13 -- nathan
On Thu, 10 Apr 2025 at 20:24, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > > On 10/04/2025 06:51, David Rowley wrote: > > Any objections to doing this soonish? Or in a few weeks? > > Sure, let's do it. Why would we wait? Great. Pushed. Was considering waiting as I didn't know if there was a revert-fest looming or not, and this may have conflicted a bit. In hindsight, likely not a good thing to optimise for. David
On Fri, 11 Apr 2025 at 02:51, Nathan Bossart <nathandbossart@gmail.com> wrote: > This probably isn't v18 material, but this reminds me of my idea to change > appendStringInfoString() into a macro for appendBinaryStringInfo() so that > the compiler can remove the runtime strlen() calls for string literals [0]. > In most cases, the benefits are probably negligible, but StringInfo is > sometimes used in hot paths. > > [0] https://postgr.es/m/20231218164135.GA530790%40nathanxps13 That one has come up a few times. The most lengthy discussion I remember was in [1]. It didn't come to anything, but I don't think there were any objections to it, so maybe we should just do it. In the thread I did some measurements of binary size increases. For non-compile-time consts, it does mean putting the strlen() call in the calling function, which is a bit of overhead in terms of size. The macro trick I suggested should have fixed that, but I admit the macro is a bit ugly. The macro version also still has the overhead of having to pass the length of the string when it detects a compile-time const. David [1] https://postgr.es/m/flat/a0086cfc-ff0f-2827-20fe-52b591d2666c%40enterprisedb.com
On 10.04.25 05:51, David Rowley wrote:
> Looks like v18 has grown a few appendStringInfo misusages, e.g. using
> appendStringInfo() when no formatting is needed or just using format
> "%s" instead of using appendStringInfoString().
Would it be useful to augment appendStringInfo() something like this:
if (VA_ARGS_NARGS() == 0)
return appendStringInfoString(str, fmt);
?
Peter Eisentraut <peter@eisentraut.org> writes:
> Would it be useful to augment appendStringInfo() something like this:
> if (VA_ARGS_NARGS() == 0)
> return appendStringInfoString(str, fmt);
That would change the behavior in edge cases, for instance
appendStringInfo(str, "foo%%bar"). Maybe we'd never hit those,
but on the whole I'm not in love with the idea.
regards, tom lane
On Fri, Apr 11, 2025 at 10:40:57AM +1200, David Rowley wrote: > On Fri, 11 Apr 2025 at 02:51, Nathan Bossart <nathandbossart@gmail.com> wrote: >> This probably isn't v18 material, but this reminds me of my idea to change >> appendStringInfoString() into a macro for appendBinaryStringInfo() so that >> the compiler can remove the runtime strlen() calls for string literals [0]. >> In most cases, the benefits are probably negligible, but StringInfo is >> sometimes used in hot paths. > > That one has come up a few times. The most lengthy discussion I > remember was in [1]. It didn't come to anything, but I don't think > there were any objections to it, so maybe we should just do it. > > In the thread I did some measurements of binary size increases. For > non-compile-time consts, it does mean putting the strlen() call in the > calling function, which is a bit of overhead in terms of size. The > macro trick I suggested should have fixed that, but I admit the macro > is a bit ugly. The macro version also still has the overhead of having > to pass the length of the string when it detects a compile-time const. Thanks for the additional context. -- nathan