On 02/01/2020 01:15, John Naylor wrote:
> I wrote:
>
>> Currently, we include the function name string in each FmgrBuiltin
>> struct, whose size is 24 bytes on 64 bit platforms. As far as I can
>> tell, the name is usually unused, so the attached (WIP, untested)
>> patch stores it separately, reducing this struct to 16 bytes.
>>
>> We can go one step further and allocate the names as a single
>> character string, reducing the binary size. It doesn't help much to
>> store offsets, since there are ~40k characters, requiring 32-bit
>> offsets. If we instead compute the offset on the fly from stored name
>> lengths, we can use 8-bit values, saving 19kB of space in the binary
>> over using string pointers.
>
> I tested with the attached C function to make sure
> fmgr_internal_function() still returned the correct answer. I assume
> this is not a performance critical function, but I still wanted to see
> if there was a visible performance regression. I get this when calling
> fmgr_internal_function() 100k times:
>
> master: 833ms
> patch: 886ms
Hmm. I was actually expecting this to slightly speed up
fmgr_internal_function(), now that all the names fit in a smaller amount
of cache. I guess there are more branches or a data dependency or
something now. I'm not too worried about that though. If it mattered, we
should switch to binary searching the array.
> The point of the patch is to increase the likelihood of
> fmgr_isbuiltin() finding the fmgr_builtins[] element in L1 cache. It
> seems harder to put a number on that for a realistic workload, but
> reducing the array size by 1/3 couldn't hurt.
Yeah. Nevertheless, it would be nice to be able to demonstrate the
benefit in some test, at least. It feels hard to justify committing a
performance patch if we can't show the benefit. Otherwise, we should
just try to keep it as simple as possible, to optimize for readability.
A similar approach was actually discussed a couple of years back:
https://www.postgresql.org/message-id/bd13812c-c4ae-3788-5b28-5633beed2929%40iki.fi.
The conclusion then was that it's not worth the trouble or the code
complication. So I think this patch is Rejected, unless you can come up
with a test case that concretely shows the benefit.
- Heikki
