Hi,
On Fri, Nov 01, 2024 at 09:47:05PM +1300, David Rowley wrote:
> On Fri, 1 Nov 2024 at 20:49, Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Fri, Nov 01, 2024 at 07:44:22AM +0000, Bertrand Drouvot wrote:
> > > Worth to add a comment as to why pg_memory_is_all_zeros() should not
> > > be used here?
> >
> > I would not add one in bufpage.c, documenting that where
> > pg_memory_is_all_zeros() is defined may be more adapted.
>
> The thought of having to write a comment to warn people not to use it
> for performance-critical things makes me think it might be better just
> to write a more optimal version of the function so we don't need to
> warn people.
Yeah, that's probably a good idea to write a more elaborate function.
> I looked around at the callers of the function I saw the
> following numbers of bytes being used for the length: 8192 (the one in
> question), 88, 32 and 112.
>
> I don't know how performance-critical the final three of those are,
> but I imagine all apart from the 32-byte one might be better with a
> non-inlined and more optimised version of the function. The problem
> with inlining the optimised version is that it's more code to inline.
I agree that's more code to inline and contains multiple loops and branches.
For the last 3 callers, I think that non inline would still be "cheap" as compared
to what lead to those code paths (stats increments).
> 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:
1 ===
+ while (((uintptr_t) p & (sizeof(size_t) - 1)) != 0)
Add a comment like "Checks bytes, byte by byte, until the pointer is aligned"?
2 ===
+ for (; p < aligned_end; p += sizeof(size_t))
Add a comment like "Multiple bytes comparison(s) at once"?
3 ===
+ while (p < end)
+ {
Add a comment like "Compare remaining bytes, byte by byte"?
4 ===
Out of curiosity I did test your proposal and it performs well (see [0]) for
the PageIsVerifiedExtended() case.
[0]: https://godbolt.org/z/Mdaxz5W7c
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
