Hi, On 2022-12-19 21:29:10 +1300, David Rowley wrote: > On Mon, 19 Dec 2022 at 21:12, Andres Freund <and...@anarazel.de> wrote: > > Perhaps we should make appendStringInfoString() a static inline function > > - most compilers can compute strlen() of a constant string at compile > > time. > > I had wondered about that, but last time I looked into it there was a > small increase in the size of the binary from doing it. Perhaps it > does not matter, but it's something to consider.
I'd not be too worried about that in this case. > Re-thinking, I wonder if we could use the same macro trick used in > ereport_domain(). Something like: > > #ifdef HAVE__BUILTIN_CONSTANT_P > #define appendStringInfoString(str, s) \ > __builtin_constant_p(s) ? \ > appendBinaryStringInfo(str, s, sizeof(s) - 1) : \ > appendStringInfoStringInternal(str, s) > #else > #define appendStringInfoString(str, s) \ > appendStringInfoStringInternal(str, s) > #endif > > and rename the existing function to appendStringInfoStringInternal. > > Because __builtin_constant_p is a known compile-time constant, it > should be folded to just call the corresponding function during > compilation. Several compilers can optimize away repeated strlen() calls, even if the string isn't a compile-time constant. So I'm not really convinced that tying inlining-strlen to __builtin_constant_p() is a good ida. > Just looking at the binary sizes for postgres. I see: > > unpatched = 9972128 bytes > inline function = 9990064 bytes That seems acceptable to me. > macro trick = 9984968 bytes > > I'm currently not sure why the macro trick increases the binary at > all. I understand why the inline function does. I think Tom's explanation is on point. I've in the past looked at stringinfo.c being the bottleneck in a bunch of places and concluded that we really need to remove the function call in the happy path entirely - we should have an enlargeStringInfo() that we can call externally iff needed and then implement the rest of appendBinaryStringInfo() etc in an inline function. That allows the compiler to e.g. optimize out the repeated maintenance of the \0 write etc. Greetings, Andres Freund