Re: appendBinaryStringInfo stuff

Hi,

On 2022-12-19 21:29:10 +1300, David Rowley wrote:
> On Mon, 19 Dec 2022 at 21:12, Andres Freund <andres(at)anarazel(dot)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