Date: Fri, 1 Nov 2024 12:50:10 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: David Rowley <dgrowleyml@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>,
	Peter Smith <smithpb2250@gmail.com>,
	Peter Eisentraut <peter@eisentraut.org>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: define pg_structiszero(addr, s, r)
Message-ID: <ZyTOgpIlQTgMBrah@ip-10-97-1-34.eu-west-3.compute.internal>
References: <ZyLWd2jR8FO7j5UL@paquier.xyz>
 <ZyMbMZMYPRIHGFzb@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZyMg8zXEfJZ74ZBX@paquier.xyz>
 <CAApHDvrXzPAr3FxoBuB7b3D-okNoNA2jxLun1rW8Yw5wkbqusw@mail.gmail.com>
 <ZyR2Tzdmj9c6E/QB@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAApHDvrEDWPEyWQF7UhAt+8JR6w2eVpv1xP21FqFH+c=8Lb3JA@mail.gmail.com>
 <ZySFDUu5-1jConZE@paquier.xyz>
 <ZySG1uHmmgQT+dpE@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZySIAShKvyGxdc8n@paquier.xyz>
 <CAApHDvqULM7KhreSa8PU2bshrguOaRdJsfjKCTvDkByYRFLaMg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAApHDvqULM7KhreSa8PU2bshrguOaRdJsfjKCTvDkByYRFLaMg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZyTOgpIlQTgMBrah%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

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