Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 1 Nov 2024 15:27:38 +0900
From: Michael Paquier <michael@paquier.xyz>
To: David Rowley <dgrowleyml@gmail.com>
Cc: Bertrand Drouvot <bertranddrouvot.pg@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: <ZyR02ofHiWG1HmLI@paquier.xyz>
References: <ZyCUsIu/xKFz0djS@ip-10-97-1-34.eu-west-3.compute.internal>
 <c9ade148-ff76-45c5-9760-1a45fbd42b00@eisentraut.org>
 <ZyDp946kZZ7OhaL5@ip-10-97-1-34.eu-west-3.compute.internal>
 <36371fab-f90c-427c-9c14-210e93e9cd12@eisentraut.org>
 <ZyH7HZfDh7DYNvKv@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAHut+Pu9Un427eoPuEetNhk2PZe1-sgSxEuG_2OuTwtxC3cBOw@mail.gmail.com>
 <ZyLWd2jR8FO7j5UL@paquier.xyz>
 <ZyMbMZMYPRIHGFzb@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZyMg8zXEfJZ74ZBX@paquier.xyz>
 <CAApHDvrXzPAr3FxoBuB7b3D-okNoNA2jxLun1rW8Yw5wkbqusw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="NJ1b5JVMdTICb1Gd"
Content-Disposition: inline
In-Reply-To: <CAApHDvrXzPAr3FxoBuB7b3D-okNoNA2jxLun1rW8Yw5wkbqusw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZyR02ofHiWG1HmLI%40paquier.xyz>
Precedence: bulk


--NJ1b5JVMdTICb1Gd
Content-Type: multipart/mixed; boundary="3xp3NznjLn2eWmmj"
Content-Disposition: inline


--3xp3NznjLn2eWmmj
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, Nov 01, 2024 at 05:45:44PM +1300, David Rowley wrote:
> I think this should be passing BLCKSZ rather than (BLCKSZ /
> sizeof(size_t)), otherwise, it'll just check the first 1 kilobyte is
> zero rather than the entire page.

Ugh, Friday brain fart.  The attached should be able to fix that, this
brings back the movl to its correct path:
-       movl    $1024, %esi
+       movl    $8192, %esi

Does that look fine to you?

> I didn't test how performance-critical this is, but the header comment
> for this function does use the words "cheaply detect".

Under gcc -O2 or -O3, the single-byte check or the 8-byte check don't
make a difference.  Please see the attached (allzeros.txt) for a quick
check if you want to check by yourself.  With 1M iterations, both
average around 3ms for 1M iterations on my laptop (not the fastest
thing around).

Under -O0, though, the difference is noticeable:
- 1-byte check: 3.52s for 1M iterations, averaging one check at
3.52ns.
- 8-byte check: 0.46s for 1M iterations, averaging one check at
0.46ns.

Even for that, I doubt that this is going to be noticeable in
practice, still the difference exists.
--
Michael

--3xp3NznjLn2eWmmj
Content-Type: text/x-csrc; charset=us-ascii
Content-Disposition: attachment; filename="allzeros.c"

#include <stdbool.h>
#include <stddef.h>
#include <string.h>

#define BLCKSZ 8192
#define LOOPS 1000000	/* 1M */

static inline bool
allzeros_char(const void *ptr, size_t len)
{
	const char *p = (const char *) ptr;

	for (size_t i = 0; i < len; i++)
	{
		if (p[i] != 0)
			return false;
	}
	return true;
}

static inline bool
allzeros_size_t(const void *ptr, size_t len)
{
	const size_t *p = (const size_t *) ptr;

	for (size_t i = 0; i < len / sizeof(size_t); i++)
	{
		if (p[i] != 0)
			return false;
	}
	return true;
}

int main()
{
	size_t pagebytes[BLCKSZ] = {0};

	for (int i = 0; i < LOOPS; i++)
	{
		allzeros_char(pagebytes, BLCKSZ);
		//allzeros_size_t(pagebytes, BLCKSZ);
	}

	return 0;
}

--3xp3NznjLn2eWmmj
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="page-allzeros.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/=
bufpage.c
index 5ee1e58cd4..db5954def2 100644
--- a/src/backend/storage/page/bufpage.c
+++ b/src/backend/storage/page/bufpage.c
@@ -88,7 +88,6 @@ bool
 PageIsVerifiedExtended(Page page, BlockNumber blkno, int flags)
 {
 	PageHeader	p =3D (PageHeader) page;
-	size_t	   *pagebytes;
 	bool		checksum_failure =3D false;
 	bool		header_sane =3D false;
 	uint16		checksum =3D 0;
@@ -124,9 +123,7 @@ PageIsVerifiedExtended(Page page, BlockNumber blkno, in=
t flags)
 	}
=20
 	/* Check all-zeroes case */
-	pagebytes =3D (size_t *) page;
-
-	if (pg_memory_is_all_zeros(pagebytes, (BLCKSZ / sizeof(size_t))))
+	if (pg_memory_is_all_zeros(page, BLCKSZ))
 		return true;
=20
 	/*

--3xp3NznjLn2eWmmj--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmckdNoACgkQnvQgOdby
QH3/QA//W8BfXdYhYJqvQhBMDUJDqjc6R6vOaRrxFHhLbytlR31N7Bhp1zXSlgXI
c2ujVbpUC18t4qAo94Ml/Q3uqdVdcO1wrc9+yzE0VAd6araIYWiQgwzIwGuTYAXV
otnO0g5bR3wfEY3CyPAiK/Wly2dSpVL4q4xIjw+qC4hKbH4q7WRRh4wQFsR6OCgG
nYsWehbreXt+fXTXj0D2+ITkYADAALLR2RVcLDKFnDqc+ZR5NYaWJl7vWG+Sq1Vt
BTuETckBawAV1UB+cvjeccBTALGEyeJqFVJ1DsjxZ8edcI3pVXjzmCD1WknJhKAZ
M942grrUAb2OHYDbpAxpqNjXpTOAw56aMXczeGTM/SpufJizNCYpKlhopUels9LM
bj2e9/n4DgJVcxIJ3xZOGudruOv5Z0HWHGbiSN9r8w0L0X2L1uTdV4z3gYN9d0H9
KZr6gCgrjBK7nngU1gg45yqH5UvVj8hTnMVfPbnUhKrcdpbPhWnbCIxuyEysVgHZ
F0kEDErs4oezKqbbIr7Zphzf5rEtCznBWQ+LtT94F9YzTEF9fs7DdnTbDmGV+m5D
67t2gjoTPlJCzqqGCTabXpLocqxXypQ2o0qgxYWLVdgINeObDSz5WH8zJZb0ErNB
EFt7ZRxWbyNOey8xXeKs8Gybz4fK5/4S8LOeKVl8WdYCyrMsvLs=
=SMZ/
-----END PGP SIGNATURE-----

--NJ1b5JVMdTICb1Gd--