Re: define pg_structiszero(addr, s, r)

Hi,

On Tue, Nov 05, 2024 at 12:24:48AM +1300, David Rowley wrote:
> On Sat, 2 Nov 2024 at 01:50, Bertrand Drouvot
> <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> >
> > On Fri, Nov 01, 2024 at 09:47:05PM +1300, David Rowley wrote:
> > > I've attached what I thought a more optimal version might look like in
> > > case anyone thinks making it better is a good idea.
> > >
> >
> > Thanks for the proposal!
> >
> > I like the idea, I think that's worth to add a few comments, something like:
>
> I'm happy if you want to pick this up and continue working on it.

Sure, please find attached v1, the changes are:

- switch from "const char" to "const unsigned char" (could have been done in the
current version of pg_memory_is_all_zeros() though)
- added some comments
- adding an Assert for ptr != 0
- re-introduce the function call in PageIsVerifiedExtended()
- propose a commit message

> One thing you might want to consider is if it's worth having a macro
> to help decide if you want to inline the function for smaller sizes
> and not inline for larger sizes. A macro that checks something like:
> if (__builtin_constant_p(len) && len <= 32) could call an inline
> version of the function for smaller sizes and do a function call for
> lager sizes. Compilers seem to have heuristics that result in
> behaviour like this for library functions such as memset and memcpy.
> Maybe some experimentation with godbolt.org would yield the crossover
> point in bytes where compilers switch tactics.

Yeah, I did some experiments in [0], what we can observe:

- with gcc 14.2 (X86-64), the assembly code contains calls to memset and memcpy
as soon as the BLCKSZ > 8192 (means, IIUC, those are inlined if <= 8192)
- with clang 19.1.0 the switch is done once BLCKSZ > 256

Given that our sizes of interest here are 8192, 88, 32 and 112 (as mentioned
up-thead) and if we follow the same logic as the compilers above do for memset
and memcpy, then we may want to keep the function inlined (gcc case) or do
a switch at 256 (clang). The switch at 256 would concern only the page case.

More use cases could come in the future with different sizes but it looks
hazardous to commit on a len that would trigger the inline/non-inline switch
on our side.

So, I'm tempted to just keep the function inlined, thoughts?

> I just feel that at the rate we receive small code change suggestions
> on the mailing lists, it's just a matter of time before someone will
> come along and suggest we use pg_memory_is_all_zeros() in
> PageIsVerifiedExtended() again. If we don't optimize that function,
> then there's a chance a committer might re-commit what's just been
> reverted.

Agree.

[0]: https://godbolt.org/z/hrYda53Tj

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com