Re: Optimizing Node Files Support

Em qua., 4 de jan. de 2023 às 19:39, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> escreveu:

> vignesh C <vignesh21(at)gmail(dot)com> writes:
> > The patch does not apply on top of HEAD as in [1], please post a rebased
> patch:
>
> Yeah. The way that I'd been thinking of optimizing the copy functions
> was more or less as attached: continue to write all the COPY_SCALAR_FIELD
> macro calls, but just make them expand to no-ops after an initial memcpy
> of the whole node. This preserves flexibility to do something else while
> still getting the benefit of substituting memcpy for retail field copies.
> Having said that, I'm not very sure it's worth doing, because I do not
> see any major reduction in code size:
>
I think this option is worse.
By disabling these macros, you lose their use in other areas.
By putting more intelligence into gen_node_support.pl, to either use memcpy
or the macros is better, IMO.
In cases of one or two macros, would it be faster than memset?

> HEAD:
> text data bss dec hex filename
> 53601 0 0 53601 d161 copyfuncs.o
> w/patch:
> text data bss dec hex filename
> 49850 0 0 49850 c2ba copyfuncs.o
>
I haven't tested it on Linux, but on Windows, there is a significant
reduction.

head:
8,114,688 postgres.exe
121.281 copyfuncs.funcs.c

patched:
8,108,544 postgres.exe
95.321 copyfuncs.funcs.c

> I've not looked at the generated assembly code, but I suspect that
> my compiler is converting the memcpy's into inlined code that's
> hardly any smaller than field-by-field assignment. Also, it's
> rather debatable that it'd be faster, especially for node types
> that are mostly pointer fields, where the memcpy is going to be
> largely wasted effort.
>
IMO, with many field assignments, memcpy would be more faster.

>
> I tend to agree with John that the rest of the changes proposed
> in the v1 patch are not useful improvements, especially with
> no evidence offered that they'd make the code smaller or faster.
>
I tried using palloc_object, as you proposed, but several tests failed.
So I suspect that some fields are not filled in correctly.
It would be an advantage to avoid memset in the allocation (palloc0), but I
chose to keep it because of the errors.

This way, if we use palloc0, there is no need to set NULL on
COPY_STRING_FIELD.

Regarding COPY_POINTER_FIELD, it is wasteful to cast size_t to size_t.

v3 attached.

regards,
Ranier Vilela

> regards, tom lane
>
>