Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 5 Nov 2024 13:31:58 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: David Rowley <dgrowleyml@gmail.com>,
	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: <ZymfviixIBCNvzTJ@paquier.xyz>
References: <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>
 <ZyTOgpIlQTgMBrah@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAApHDvoU1SfD6rWjGp2tpLd+VGg_TfXhPJgCSNY5Z5LULXiv1A@mail.gmail.com>
 <ZykBwloj77DVlhnN@ip-10-97-1-34.eu-west-3.compute.internal>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="mpLK473rJOm5hlxr"
Content-Disposition: inline
In-Reply-To: <ZykBwloj77DVlhnN@ip-10-97-1-34.eu-west-3.compute.internal>
Archived-At: <https://www.postgresql.org/message-id/ZymfviixIBCNvzTJ%40paquier.xyz>
Precedence: bulk


--mpLK473rJOm5hlxr
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Nov 04, 2024 at 05:17:54PM +0000, Bertrand Drouvot wrote:
> Hi,
>=20
> On Tue, Nov 05, 2024 at 12:24:48AM +1300, David Rowley wrote:
> > On Sat, 2 Nov 2024 at 01:50, Bertrand Drouvot
> > <bertranddrouvot.pg@gmail.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, somethin=
g like:
> >=20
> > I'm happy if you want to pick this up and continue working on it.
>=20
> Sure, please find attached v1, the changes are:
>=20
> - 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

+ * The test is divided into three phases for efficiency:
+ *  - Initial alignment (byte per byte comparison)
+ *  - Multiple bytes comparison at once
+ *  - Remaining bytes (byte per byte comparison)

It does not look like this insists enough on the alignment part of the
optization?  A MAXALIGN'd size would use only size_t comparisons, and
a pointer aligned would do no byte comparisons.

> - adding an Assert for ptr !=3D 0

I'm not sure that the Assert() addition is a good idea.  That could
get hot very easily depending on the caller, even if for assert
builds we don't care much about the performance, that could lead to
some paths being a lot slower.

On Fri, 01 Nov 2024 at 21:47, David Rowley <dgrowleyml(at)gmail(dot)com> wr=
ote:
> 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. 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.

These three don't matter in terms of performance.  The cycles spent
for all-zero checks of the checkpointer and bgwriter are nothing
compared to the concurrent I/O activity they handle, and the sizes are
small compared to the 8k pages.  The flush callback for relation
pgstats happens once at a given interval (see around
PGSTAT_[MIN|MAX]_INTERVAL).
--
Michael

--mpLK473rJOm5hlxr
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmcpn74ACgkQnvQgOdby
QH0Flw//ZWrFXPnvIEXWMNDwOs1gs9jCTyXnnpina3hwW2Ovm/I6FlvR8WXA1Z1s
MkPuVYzCKsATcRilEuR+Yii83eN0I8wF0c+DFt/BpXiIDEXrlVsil2WlM6uIijy3
zoSH9kSvfr/v45Z85JRy5BW8ttuEJzgVFstRQ3ueygWlUGeVPuPjO3ZSDsig6tBB
qj7096g/mzgp18qyCW8BrmkIFqG0BqIqA5pehWGy0V68j+1mzdB9I+qA+EUSxEwR
UVqEXazGXOGK5jigA3mIUQZGoOQc/dYzi/a9wrfk2Vx0k0bKoKpd6s2nL6+pwNUF
PD5fjwDZWG7AwW+RF6XxQdTgL0nEy7QKz/K4/ATDVxojyWITyf3z87yg3Ymgq9Pt
qbTIdWIPvSDOOdrXiIZMtQ6hWzB8Bhu364wmNFxx3v78vwZpbb/WRyb6SiuyAd4Q
qqKmmPmDmR4Q11XZHnk1y+yBlwSbzzTEDJ2STGKqGSfAzFzWdPz+2bV5SWKnhyDv
xNofNuxiWu4ivZ/KcIlnujcT0naekbxOxFil8WFJPOERB/STix3W2eDn2MDDEF7R
CUtgDG0R1u+OgdOYN6zHFEUHrx1UivZmvXri2mT7uSaaBJgaPwXMWnp/yGeZYgqB
UvMgvixelAmN2rPX85dwoCHxREuSwBId93uA6Cdt0LdvYkbqlcs=
=gwjt
-----END PGP SIGNATURE-----

--mpLK473rJOm5hlxr--