Re: Replace some cstring_to_text to cstring_to_text_with_len

Peter Eisentraut <peter(at)eisentraut(dot)org> writes:

> On 31.08.23 16:10, Ranier Vilela wrote:
>> Em qui., 31 de ago. de 2023 às 09:51, Andrew Dunstan
>> <andrew(at)dunslane(dot)net <mailto:andrew(at)dunslane(dot)net>> escreveu:
>>
>> On 2023-08-31 Th 07:41, John Naylor wrote:
>>>
>>> On Thu, Aug 31, 2023 at 6:07 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com
>>> <mailto:ranier(dot)vf(at)gmail(dot)com>> wrote:
>>> >
>>> > Em qui., 31 de ago. de 2023 às 00:22, Michael Paquier
>>> <michael(at)paquier(dot)xyz <mailto:michael(at)paquier(dot)xyz>> escreveu:
>>> >>
>>> >> On Wed, Aug 30, 2023 at 03:00:13PM -0300, Ranier Vilela wrote:
>>> >> > cstring_to_text has a small overhead, because call strlen for
>>> >> > pointer to char parameter.
>>> >> >
>>> >> > Is it worth the effort to avoid this, where do we know the
>>> size of the
>>> >> > parameter?
>>> >>
>>> >> Are there workloads where this matters?
>>> >
>>> > None, but note this change has the same spirit of 8b26769bc.
>>>
>>> - return cstring_to_text("");
>>> + return cstring_to_text_with_len("", 0);
>>>
>>> This looks worse, so we'd better be getting something in return.
>>
>> I agree this is a bit ugly. I wonder if we'd be better off creating
>> a function that returned an empty text value, so we'd just avoid
>> converting the empty cstring altogether and say:
>>   return empty_text();
>> Hi,
>> Thanks for the suggestion,  I agreed.
>> New patch is attached.
>
> I think these patches make the code uniformly uglier and harder to
> understand.
>
> If a performance benefit could be demonstrated, then making
> cstring_to_text() an inline function could be sensible. But I wouldn't
> go beyond that.

I haven't benchmarked it yet, but here's a patch that inlines them and
changes callers of cstring_to_text_with_len() with a aliteral string and
constant length to cstring_to_text().

On an x86-64 Linux build (meson with -Dbuildtype=debugoptimized
-Dcassert=true), the inlining increases the size of the text section of
the postgres binary from 9719722 bytes to 9750557, i.e. an increase of
30KiB or 0.3%, while the change to cstring_to_text() makes zero
difference (as expected from my investigation).

- ilmari