Subject: Re: define pg_structiszero(addr, s, r)

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 30 Oct 2024 09:23:41 +0000

Hi,

On Wed, Oct 30, 2024 at 09:09:54AM +0100, Peter Eisentraut wrote:
> On 29.10.24 14:58, Bertrand Drouvot wrote:
> > hi,
> > 
> > On Tue, Oct 29, 2024 at 10:23:37AM +0100, Peter Eisentraut wrote:
> > > On 29.10.24 08:54, Bertrand Drouvot wrote:
> > > > +static inline bool
> > > > +pg_mem_is_all_zeros(const char *p, size_t len)
> > > 
> > > This should be a void * pointer please.
> > 
> > Thanks for looking at it!
> > Yeah better, done in v4 attached.
> 
> Sorry for the confusion.  I didn't mean to discourage you from the const
> qualifier.  That should still be there.

doh! Of course we want it to be read only. No problem, I should have put more
thoughts on it. Done in v5 attached.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From e80e5cf946813185985bf5339ac996ccc0eab1f0 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Tue, 10 Sep 2024 01:22:02 +0000
Subject: [PATCH v5] New pg_mem_is_all_zeros(p, len) inline function

This new function allows to test if a memory region (starting at p) and of
size len is full of zeroes.
---
 src/backend/storage/page/bufpage.c               | 13 +------------
 src/backend/utils/activity/pgstat_bgwriter.c     |  5 +++--
 src/backend/utils/activity/pgstat_checkpointer.c |  7 +++----
 src/backend/utils/activity/pgstat_relation.c     |  7 ++-----
 src/include/utils/memutils.h                     | 16 ++++++++++++++++
 5 files changed, 25 insertions(+), 23 deletions(-)
  19.8% src/backend/storage/page/
  58.6% src/backend/utils/activity/
  21.5% src/include/utils/

diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index be6f1f62d2..ca4c68de1c 100644
--- a/src/backend/storage/page/bufpage.c
+++ b/src/backend/storage/page/bufpage.c
@@ -89,10 +89,8 @@ PageIsVerifiedExtended(Page page, BlockNumber blkno, int flags)
 {
 	PageHeader	p = (PageHeader) page;
 	size_t	   *pagebytes;
-	int			i;
 	bool		checksum_failure = false;
 	bool		header_sane = false;
-	bool		all_zeroes = false;
 	uint16		checksum = 0;
 
 	/*
@@ -126,18 +124,9 @@ PageIsVerifiedExtended(Page page, BlockNumber blkno, int flags)
 	}
 
 	/* Check all-zeroes case */
-	all_zeroes = true;
 	pagebytes = (size_t *) page;
-	for (i = 0; i < (BLCKSZ / sizeof(size_t)); i++)
-	{
-		if (pagebytes[i] != 0)
-		{
-			all_zeroes = false;
-			break;
-		}
-	}
 
-	if (all_zeroes)
+	if (pg_mem_is_all_zeros(pagebytes , (BLCKSZ / sizeof(size_t))))
 		return true;
 
 	/*
diff --git a/src/backend/utils/activity/pgstat_bgwriter.c b/src/backend/utils/activity/pgstat_bgwriter.c
index 364a7a2024..398e1d57ca 100644
--- a/src/backend/utils/activity/pgstat_bgwriter.c
+++ b/src/backend/utils/activity/pgstat_bgwriter.c
@@ -17,6 +17,7 @@
 
 #include "postgres.h"
 
+#include "utils/memutils.h"
 #include "utils/pgstat_internal.h"

@@ -30,7 +31,6 @@ void
 pgstat_report_bgwriter(void)
 {
 	PgStatShared_BgWriter *stats_shmem = &pgStatLocal.shmem->bgwriter;
-	static const PgStat_BgWriterStats all_zeroes;
 
 	Assert(!pgStatLocal.shmem->is_shutdown);
 	pgstat_assert_is_up();
@@ -39,7 +39,8 @@ pgstat_report_bgwriter(void)
 	 * This function can be called even if nothing at all has happened. In
 	 * this case, avoid unnecessarily modifying the stats entry.
 	 */
-	if (memcmp(&PendingBgWriterStats, &all_zeroes, sizeof(all_zeroes)) == 0)
+	if (pg_mem_is_all_zeros(&PendingBgWriterStats,
+							sizeof(struct PgStat_BgWriterStats)))
 		return;
 
 	pgstat_begin_changecount_write(&stats_shmem->changecount);
diff --git a/src/backend/utils/activity/pgstat_checkpointer.c b/src/backend/utils/activity/pgstat_checkpointer.c
index bbfc9c7e18..fb66a1a11d 100644
--- a/src/backend/utils/activity/pgstat_checkpointer.c
+++ b/src/backend/utils/activity/pgstat_checkpointer.c
@@ -17,6 +17,7 @@
 
 #include "postgres.h"
 
+#include "utils/memutils.h"
 #include "utils/pgstat_internal.h"

@@ -29,8 +30,6 @@ PgStat_CheckpointerStats PendingCheckpointerStats = {0};
 void
 pgstat_report_checkpointer(void)
 {
-	/* We assume this initializes to zeroes */
-	static const PgStat_CheckpointerStats all_zeroes;
 	PgStatShared_Checkpointer *stats_shmem = &pgStatLocal.shmem->checkpointer;
 
 	Assert(!pgStatLocal.shmem->is_shutdown);
@@ -40,8 +39,8 @@ pgstat_report_checkpointer(void)
 	 * This function can be called even if nothing at all has happened. In
 	 * this case, avoid unnecessarily modifying the stats entry.
 	 */
-	if (memcmp(&PendingCheckpointerStats, &all_zeroes,
-			   sizeof(all_zeroes)) == 0)
+	if (pg_mem_is_all_zeros(&PendingCheckpointerStats,
+							sizeof(struct PgStat_CheckpointerStats)))
 		return;
 
 	pgstat_begin_changecount_write(&stats_shmem->changecount);
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index 8a3f7d434c..bf1947f316 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -801,7 +801,6 @@ pgstat_twophase_postabort(TransactionId xid, uint16 info,
 bool
 pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
 {
-	static const PgStat_TableCounts all_zeroes;
 	Oid			dboid;
 	PgStat_TableStatus *lstats; /* pending stats entry  */
 	PgStatShared_Relation *shtabstats;
@@ -816,11 +815,9 @@ pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
 	 * Ignore entries that didn't accumulate any actual counts, such as
 	 * indexes that were opened by the planner but not used.
 	 */
-	if (memcmp(&lstats->counts, &all_zeroes,
-			   sizeof(PgStat_TableCounts)) == 0)
-	{
+	if (pg_mem_is_all_zeros(&lstats->counts,
+							sizeof(struct PgStat_TableCounts)))
 		return true;
-	}
 
 	if (!pgstat_lock_entry(entry_ref, nowait))
 		return false;
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index cd9596ff21..5470d3452b 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -189,4 +189,20 @@ extern MemoryContext BumpContextCreate(MemoryContext parent,
 #define SLAB_DEFAULT_BLOCK_SIZE		(8 * 1024)
 #define SLAB_LARGE_BLOCK_SIZE		(8 * 1024 * 1024)
 
+/*
+ * Test if a memory region starting at p and of size len is full of zeroes.
+ */
+static inline bool
+pg_mem_is_all_zeros(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;
+}
+
 #endif							/* MEMUTILS_H */
-- 
2.34.1

---

From: Peter Smith <[email-masked]>
Date: Thu, 31 Oct 2024 07:55:45 +1100

+/*

The function comment should say 'ptr' instead of 'p', right?

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Michael Paquier <[email-masked]>
Date: Thu, 31 Oct 2024 09:59:35 +0900

> The function comment should say 'ptr' instead of 'p', right?

Yes.

+static inline bool

While we're talking about wordsmithing things, I would not choose
"mem" for this routine, just stick to "memory".  There is not much in
the code that does memory-specific things like what you are proposing
here.  Still, this would be more consistent with the macros for memory 
barriers at least.  Hence, "pg_memory_is_all_zeros()" makes more
sense?

The location of memutils.h is sensible.

Extra space not required after pagebytes.
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 31 Oct 2024 05:52:49 +0000

Hi,

> > The function comment should say 'ptr' instead of 'p', right?

Thank you both for looking at it. Agree, done in the new version attached. 

> barriers at least.  Hence, "pg_memory_is_all_zeros()" makes more

That makes sense to me, done that way in the attached.

> The location of memutils.h is sensible.

Thanks for sharing your thoughts on it.

> Extra space not required after pagebytes.

Fat finger here, thanks!

Regards,

>From 88f5ebbbee3cdc7b773a1d0f5a7cf6f9d909b784 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Tue, 10 Sep 2024 01:22:02 +0000
Subject: [PATCH v6] New pg_memory_is_all_zeros(ptr, len) inline function

This new function allows to test if a memory region starting at ptr and of
size len is full of zeroes.
---
 src/backend/storage/page/bufpage.c               | 13 +------------
 src/backend/utils/activity/pgstat_bgwriter.c     |  5 +++--
 src/backend/utils/activity/pgstat_checkpointer.c |  7 +++----
 src/backend/utils/activity/pgstat_relation.c     |  7 ++-----
 src/include/utils/memutils.h                     | 16 ++++++++++++++++
 5 files changed, 25 insertions(+), 23 deletions(-)
  19.6% src/backend/storage/page/
  58.8% src/backend/utils/activity/
  21.4% src/include/utils/

index be6f1f62d2..5ee1e58cd4 100644
 
 	/*

-	if (all_zeroes)
+	if (pg_memory_is_all_zeros(pagebytes, (BLCKSZ / sizeof(size_t))))
 		return true;
 
 	/*
diff --git a/src/backend/utils/activity/pgstat_bgwriter.c b/src/backend/utils/activity/pgstat_bgwriter.c
index 364a7a2024..85d53d82f2 100644
--- a/src/backend/utils/activity/pgstat_bgwriter.c
+++ b/src/backend/utils/activity/pgstat_bgwriter.c
@@ -17,6 +17,7 @@
 
 #include "postgres.h"

@@ -30,7 +31,6 @@ void
 
+	if (pg_memory_is_all_zeros(&PendingBgWriterStats,
 
index bbfc9c7e18..258337cfb8 100644
 
 #include "postgres.h"

 
+	if (pg_memory_is_all_zeros(&PendingCheckpointerStats,
 
index 8a3f7d434c..337954d15e 100644
+	if (pg_memory_is_all_zeros(&lstats->counts,
 
index cd9596ff21..0094492d9c 100644
 
+/*
+ * Test if a memory region starting at ptr and of size len is full of zeroes.
+ */
+static inline bool
+pg_memory_is_all_zeros(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;
+}
+
 #endif							/* MEMUTILS_H */
-- 
2.34.1

---

From: Michael Paquier <[email-masked]>
Date: Thu, 31 Oct 2024 15:17:23 +0900

> That makes sense to me, done that way in the attached.

Seems kind of OK seen from here.  I am wondering if others more
comments about the name of the macro, its location, the fact that we
still have pagebytes in bufpage.c, etc.
--
Michael

---

From: David Rowley <[email-masked]>
Date: Fri, 1 Nov 2024 17:45:44 +1300

> still have pagebytes in bufpage.c, etc.

This change looks incorrect:

@@ -126,18 +124,9 @@ PageIsVerifiedExtended(Page page, BlockNumber
blkno, int flags)
    }

-   if (all_zeroes)

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.

However, I've also performance concerns as if I look at the assembly
of PageIsVerifiedExtended, I see the zero checking is now done 1 byte
at a time:

(gcc 11.4)

leaq 1024(%rbx), %rdx <-- 1KB bug
.p2align 4,,10
.p2align 3
.L60:
cmpb $0, (%rax) <-- check 1 byte is zero.
jne .L59
addq $1, %rax <-- increment by 1 byte.
cmpq %rax, %rdx <-- check if we've done 1024 bytes yet.
jne .L60

Whereas previously it was doing:

movq %rbx, %rax
leaq 8192(%rbx), %rdx <-- 8KB
jmp .L60
.p2align 4,,10
.p2align 3
.L90:
addq $8, %rax <-- increment by 8 bytes (or sizeof(size_t))
cmpq %rax, %rdx
je .L61
.L60:
cmpq $0, (%rax)  <-- checks an entire 8 bytes are zero.

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

 * This is called when a page has just been read in from disk.  The idea is
 * to cheaply detect trashed pages before we go nuts following bogus line
 * pointers, testing invalid transaction identifiers, etc.

so it seems a bit dangerous to switch this loop to byte-at-a-time
rather than doing 8 bytes at once without testing the performance
isn't affected.

David

---

From: Michael Paquier <[email-masked]>
Date: Fri, 1 Nov 2024 15:27:38 +0900

> 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?

> 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
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 = (PageHeader) page;
-	size_t	   *pagebytes;
 	bool		checksum_failure = false;
 	bool		header_sane = false;
 	uint16		checksum = 0;
@@ -124,9 +123,7 @@ PageIsVerifiedExtended(Page page, BlockNumber blkno, int flags)
 	}
 
 	/* Check all-zeroes case */
-	pagebytes = (size_t *) page;
-
-	if (pg_memory_is_all_zeros(pagebytes, (BLCKSZ / sizeof(size_t))))
+	if (pg_memory_is_all_zeros(page, BLCKSZ))
 		return true;
 
 	/*

---

From: Bertrand Drouvot <[email-masked]>
Date: Fri, 1 Nov 2024 06:33:51 +0000

Hi,

Nice catch, indeed by using the new function we are changing the pointer arithmetic
here and then we should pass BLCKSZ instead.

> rather than doing 8 bytes at once without testing the performance

Agree, I did a quick test (see [0]) and it looks like it's significantly slower
using the new inline function.

We could try to write a more elaborate version of pg_memory_is_all_zeros(), but
as it looks like there is only one use case, then it's probably better to not
implement (revert) this change here and "just" add a comment as to why pg_memory_is_all_zeros()
should not be used here, thoughts?

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

Regards,

---

From: Michael Paquier <[email-masked]>
Date: Fri, 1 Nov 2024 16:14:09 +0900

> [0]: https://godbolt.org/z/xqnW4MPY5

Note that the two printf() calls make the code less optimized.

Anyway, I see the following from bufpage.s for these lines under -O2:

1) On HEAD at 07e9e28b56db:
.LVL306:
   .loc 3 201 23 is_stmt 1 discriminator 1 view .LVU547
   cmpq    $1024, %rbx <- Yep, that's wrong.
   jne     .L417

2) On HEAD at 49d6c7d8daba:
.LVL299:
   .loc 1 131 16 is_stmt 0 discriminator 1 view .LVU524
   cmpq    $8192, %rbx
   je      .L419

3) With the patch sent at [1]:
.LVL306:
   .loc 3 201 23 is_stmt 1 discriminator 1 view .LVU545
   cmpq    $8192, %rbx
   jne     .L417

So it does not matter one way or another for 2) or 3), does it?

[1]: https://www.postgresql.org/message-id/[email-masked]
--
Michael

---

From: David Rowley <[email-masked]>
Date: Fri, 1 Nov 2024 20:19:39 +1300

> practice, still the difference exists.

The reason you're not seeing the slowdown with -O2 and -O3 is because
your compiler didn't think there was anything to do so didn't emit the
code you were trying to benchmark.  Try looking at allzeros.s after
doing "gcc allzeros.c -S -O2".

I've attached an updated version for you to try. I used a volatile
bool and assigned the function result to it to prevent the compiler
from optimising out the test.

$ gcc allzeros.c -O2 -o allzeros
$ ./allzeros
char: done in 1607800 nanoseconds
size_t: done in 208800 nanoseconds (7.70019 times faster)

$ gcc allzeros.c -O3 -o allzeros
$ ./allzeros
char: done in 1584500 nanoseconds
size_t: done in 225700 nanoseconds (7.02038 times faster)

David

---

From: David Rowley <[email-masked]>
Date: Fri, 1 Nov 2024 20:21:50 +1300

> should not be used here, thoughts?

My vote is to just revert this usage of the function. Anything more
elaborate would need to check pointer alignment before using any types
larger than char. The previous code does not need to do that because
the page is going to be at least MAXALIGNed.

David

---

From: David Rowley <[email-masked]>
Date: Fri, 1 Nov 2024 20:27:45 +1300

> So it does not matter one way or another for 2) or 3), does it?

The patch in [1] will fix the bug. But I'm still concerned about the
performance implications of moving to byte-at-a-time processing. There
are about 8 times more instructions being expected to do the same
work.

David

---

From: Michael Paquier <[email-masked]>
Date: Fri, 1 Nov 2024 16:36:45 +0900

> the page is going to be at least MAXALIGNed.

Fine, here you go.  The attached reverts back this part in bufpage.c
to what it was in 49d6c7d8daba.
--
Michael
diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index 5ee1e58cd4..be6f1f62d2 100644
--- a/src/backend/storage/page/bufpage.c
+++ b/src/backend/storage/page/bufpage.c
@@ -89,8 +89,10 @@ PageIsVerifiedExtended(Page page, BlockNumber blkno, int flags)
 {
 	PageHeader	p = (PageHeader) page;
 	size_t	   *pagebytes;
+	int			i;
 	bool		checksum_failure = false;
 	bool		header_sane = false;
+	bool		all_zeroes = false;
 	uint16		checksum = 0;
 
 	/*
@@ -124,9 +126,18 @@ PageIsVerifiedExtended(Page page, BlockNumber blkno, int flags)
 	}
 
 	/* Check all-zeroes case */
+	all_zeroes = true;
 	pagebytes = (size_t *) page;
+	for (i = 0; i < (BLCKSZ / sizeof(size_t)); i++)
+	{
+		if (pagebytes[i] != 0)
+		{
+			all_zeroes = false;
+			break;
+		}
+	}
 
+	if (all_zeroes)
 		return true;
 
 	/*

---

From: Bertrand Drouvot <[email-masked]>
Date: Fri, 1 Nov 2024 07:44:22 +0000

Hi,

> to what it was in 49d6c7d8daba.

Thanks! Worth to add a comment as to why pg_memory_is_all_zeros() should not
be used here?

Regards,

---

From: Michael Paquier <[email-masked]>
Date: Fri, 1 Nov 2024 16:49:21 +0900

On Fri, Nov 01, 2024 at 07:44:22AM +0000, Bertrand Drouvot wrote:
> Thanks!

Cool, will fix that in a bit.

> 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.
--
Michael

---

From: David Rowley <[email-masked]>
Date: Fri, 1 Nov 2024 21:47:05 +1300

> 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. 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've attached what I thought a more optimal version might look like in
case anyone thinks making it better is a good idea.

David

---

From: Bertrand Drouvot <[email-masked]>
Date: Fri, 1 Nov 2024 12:50:10 +0000

Hi,

On Fri, Nov 01, 2024 at 09:47:05PM +1300, David Rowley wrote:
> 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

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).

> 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,

---

From: Japin Li <[email-masked]>
Date: Fri, 01 Nov 2024 23:01:13 +0800

> with inlining the optimised version is that it's more code to inline.
+1

Is there a possible overflow?

+	const char *end = &p[len];

How about use MAXALIGN64 macro here?

When handling the aligned, is it possible to handle multiple values
(such as 4 or 8) in one iteration?

It might be faster. However, I'm not tested.

-- 
Regrads,
Japin Li

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 4 Nov 2024 09:50:05 +0000

Hi,

> the PageIsVerifiedExtended() case.

Also, 

5 ===

Shouldn't we handle the cases where ptr is NULL and/or len == 0? (should probably
have already been done in the current version of pg_memory_is_all_zeros() though).

Regards,

---

From: David Rowley <[email-masked]>
Date: Tue, 5 Nov 2024 00:24:48 +1300

> 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. I'm
mostly just keen to not leave the suboptimal version of the function
in core as it is.

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.

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.

David

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 4 Nov 2024 17:17:54 +0000

Hi,

On Tue, Nov 05, 2024 at 12:24:48AM +1300, David Rowley wrote:
> 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

> 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?

> then there's a chance a committer might re-commit what's just been

Agree.

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

Regards,

>From f23a5d8aca945bd5762f34095eb0de5ecbe23dfa Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Fri, 1 Nov 2024 11:46:29 +0000
Subject: [PATCH v1] Optimize pg_memory_is_all_zeros()

pg_memory_is_all_zeros() is currently doing byte per byte comparison and so
could lead to performance regression or penalties when multi bytes comparison
could be done instead.

Let's provide an optimized version that divides the checks into three phases for
efficiency:

- Initial alignment (byte per byte comparison)
- Multiple bytes comparison at once
- Remaining bytes (byte per byte comparison)

In passing, let's make use of this new version in PageIsVerifiedExtended() now
that multi bytes comparison is handled.
---
 src/backend/storage/page/bufpage.c | 13 +----------
 src/include/utils/memutils.h       | 36 +++++++++++++++++++++++++++---
 2 files changed, 34 insertions(+), 15 deletions(-)
  19.9% src/backend/storage/page/
  80.0% src/include/utils/

index be6f1f62d2..aa264f61b9 100644
 
 	/*

-	if (all_zeroes)
+	if (pg_memory_is_all_zeros(pagebytes, BLCKSZ))
 		return true;
 
 	/*
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index 3590c8bad9..5b992a8bbe 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -192,17 +192,47 @@ extern MemoryContext BumpContextCreate(MemoryContext parent,
 /*
  * Test if a memory region starting at "ptr" and of size "len" is full of
  * zeroes.
+ *
+ * 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)
+ *
+ * Caller must ensure that "ptr" is non-null.
  */
 static inline bool
 pg_memory_is_all_zeros(const void *ptr, size_t len)
 {
-	const char *p = (const char *) ptr;
+	const unsigned char *p = (const unsigned char *) ptr;
+	const unsigned char *end = &p[len];
+	const unsigned char *aligned_end = (const unsigned char *) ((uintptr_t) end & (~(sizeof(size_t) - 1)));
 
-	for (size_t i = 0; i < len; i++)
+	Assert(ptr != NULL);
+
+	/* Compare bytes, byte by byte, until the pointer "p" is aligned */
+	while (((uintptr_t) p & (sizeof(size_t) - 1)) != 0)
 	{
-		if (p[i] != 0)
+		if (p == end)
+			return true;
+
+		if (*p++ != 0)
 			return false;
 	}
+
+	/* Multiple bytes comparison(s) at once */
+	for (; p < aligned_end; p += sizeof(size_t))
+	{
+		if (*(size_t *) p != 0)
+			return false;
+	}
+
+	/* Compare remaining bytes, byte by byte */
+	while (p < end)
+	{
+		if (*p++ != 0)
+			return false;
+	}
+
 	return true;
 }
 
-- 
2.34.1

---

From: Ranier Vilela <[email-masked]>
Date: Mon, 4 Nov 2024 14:38:54 -0300

Hi.

> - propose a commit message

I think we can add a small optimization to this last patch [1].
The variable *aligned_end* is only needed in the second loop (for).
So, only before the for loop do we actually declare it.

Result before this change:
check zeros using BERTRAND 1 0.000031s

Result after this change:
check zeros using BERTRAND 1 0.000018s

+ const unsigned char *aligned_end;

+ /* Multiple bytes comparison(s) at once */
+ aligned_end = (const unsigned char *) ((uintptr_t) end &
(~(sizeof(size_t) - 1)));
+ for (; p < aligned_end; p += sizeof(size_t))

best regards,
Ranier Vilela

[1] https://godbolt.org/z/fvdzjbnc7

---

From: David Rowley <[email-masked]>
Date: Tue, 5 Nov 2024 16:23:34 +1300

On Tue, 5 Nov 2024 at 06:39, Ranier Vilela <[email-masked]> wrote:
> + aligned_end = (const unsigned char *) ((uintptr_t) end & (~(sizeof(size_t) - 1)));

I think we all need to stop using Godbolt's servers to run benchmarks
on. These servers are likely to be running various other workloads in
highly virtualised environments and are not going to be stable servers
that would give consistent benchmark results.

I tried your optimisation in the attached allzeros.c and here are my results:

# My version
$ gcc allzeros.c -O2 -o allzeros && for i in {1..3}; do ./allzeros; done
char: done in 1566400 nanoseconds
size_t: done in 195400 nanoseconds (8.01638 times faster than char)
char: done in 1537500 nanoseconds
size_t: done in 196300 nanoseconds (7.8324 times faster than char)
char: done in 1543600 nanoseconds
size_t: done in 196300 nanoseconds (7.86347 times faster than char)

# Ranier's optimization
$ gcc allzeros.c -O2 -D RANIERS_OPTIMIZATION -o allzeros && for i in
{1..3}; do ./allzeros; done
char: done in 1943100 nanoseconds
size_t: done in 531700 nanoseconds (3.6545 times faster than char)
char: done in 1957200 nanoseconds
size_t: done in 458400 nanoseconds (4.26963 times faster than char)
char: done in 1949500 nanoseconds
size_t: done in 469000 nanoseconds (4.15672 times faster than char)

Seems to be about half as fast with gcc on -O2

David

---

From: David Rowley <[email-masked]>
Date: Tue, 5 Nov 2024 17:08:41 +1300

> I think we can add a small optimization to this last patch [1].

I think if you want to make it faster, you could partially unroll the
inner-most loop, like:

// size_t * 4
for (; p < aligned_end - (sizeof(size_t) * 3); p += sizeof(size_t) * 4)
{
    if (((size_t *) p)[0] != 0 | ((size_t *) p)[1] != 0 | ((size_t *)
p)[2] != 0 | ((size_t *) p)[3] != 0)
        return false;
}

$ gcc allzeros.c -O2 -o allzeros && ./allzeros
char: done in 1595000 nanoseconds
size_t: done in 198300 nanoseconds (8.04337 times faster than char)
size_t * 4: done in 81500 nanoseconds (19.5706 times faster than char)
size_t * 8: done in 71000 nanoseconds (22.4648 times faster than char)

The final one above is 110GB/sec, so probably only going that fast
because the memory being checked is in L1. DDR5 is only 64GB/sec. So
it's probably overkill to unroll the loop that much.

Also, doing something like that means the final byte-at-a-time loop
might have more to do, which might cases with a long remainder slower.
To make up for that there's some incentive to introduce yet another
loop to process single size_t's up to aligned_end. Then you end up
with even more code.

I was happy enough with my patch with Bertrand's comments.  I'm not
sure why unsigned chars are better than chars. It doesn't seem to have
any effect on the compiled code.

David

---

From: Michael Paquier <[email-masked]>
Date: Tue, 5 Nov 2024 13:12:03 +0900

> char: done in 1957200 nanoseconds

I am not seeing numbers as good as yours, but the winner is clear as
well here:

$ gcc allzeros.c -O2 -o allzeros && for i in {1..3}; do
./allzeros; done
char: done in 6578995 nanoseconds
size_t: done in 829916 nanoseconds (7.9273 times faster than char)
char: done in 6581465 nanoseconds
size_t: done in 829948 nanoseconds (7.92997 times faster than char)
char: done in 6585748 nanoseconds
size_t: done in 834929 nanoseconds (7.88779 times faster than char)

$ gcc allzeros.c -O2 -D RANIERS_OPTIMIZATION -o allzeros && for i in
{1..3}; do ./allzeros;
done char: done in 6591803 nanoseconds
size_t: done in 1236102 nanoseconds (5.33273 times faster than char)
char: done in 6606219 nanoseconds
size_t: done in 1235979 nanoseconds (5.34493 times faster than char)
char: done in 6594413 nanoseconds
size_t: done in 1238770 nanoseconds (5.32336 times faster than char)

I'm surprised to see that assigning aligned_end at these two different
locations has this much effect once the compiler optimizes the
surroundings, but well.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Tue, 5 Nov 2024 13:31:58 +0900

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 != 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.

> 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

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 5 Nov 2024 05:49:22 +0000

Hi,

> any effect on the compiled code.

I think that unsigned chars is better than char for byte level memory
operations (no sign extension issues).

Though I agree that using char in this function is not an issue as p is only
compared with 0. This is more a matter of taste here.

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 5 Nov 2024 05:59:46 +0000

Hi,

> a pointer aligned would do no byte comparisons.

I'm not sure to get this one. Is it more clear in the code comments that
we can start multiple bytes comparison once p is aligned?

> some paths being a lot slower.

Yeah, agree, removed in v2 attached.

Regards,

>From 7df597359510fd2d1aa12f74a4082643237c8452 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Fri, 1 Nov 2024 11:46:29 +0000
Subject: [PATCH v2] Optimize pg_memory_is_all_zeros()

In passing, let's make use of this new version in PageIsVerifiedExtended() now
that multi bytes comparison is handled.
---
 src/backend/storage/page/bufpage.c | 13 +-----------
 src/include/utils/memutils.h       | 34 +++++++++++++++++++++++++++---
 2 files changed, 32 insertions(+), 15 deletions(-)
  20.3% src/backend/storage/page/
  79.6% src/include/utils/

 	/*

-	if (all_zeroes)
 
 	/*
index 3590c8bad9..15bf32336e 100644
@@ -192,17 +192,45 @@ extern MemoryContext BumpContextCreate(MemoryContext parent,

-- 
2.34.1

---

From: Ranier Vilela <[email-masked]>
Date: Tue, 5 Nov 2024 09:49:53 -0300

> Seems to be about half as fast with gcc on -O2
Thanks for test coding.

I've tried with msvc 2022 32bits
Here the results:

C:\usr\src\tests\allzeros>allzeros
char: done in 71431900 nanoseconds
size_t: done in 18010900 nanoseconds (3.96604 times faster than char)

C:\usr\src\tests\allzeros>allzeros
char: done in 71070100 nanoseconds
size_t: done in 19654300 nanoseconds (3.61601 times faster than char)

C:\usr\src\tests\allzeros>allzeros
char: done in 68682400 nanoseconds
size_t: done in 19841100 nanoseconds (3.46162 times faster than char)

C:\usr\src\tests\allzeros>allzeros
char: done in 63215100 nanoseconds
size_t: done in 17920200 nanoseconds (3.52759 times faster than char)

C:\usr\src\tests\allzeros>c /DRANIERS_OPTIMIZATION

Microsoft (R) Program Maintenance Utility Versão 14.40.33813.0
Direitos autorais da Microsoft Corporation. Todos os direitos reservados.

C:\usr\src\tests\allzeros>allzeros
char: done in 67213800 nanoseconds
size_t: done in 15049200 nanoseconds (4.46627 times faster than char)

C:\usr\src\tests\allzeros>allzeros
char: done in 51505900 nanoseconds
size_t: done in 13645700 nanoseconds (3.77452 times faster than char)

C:\usr\src\tests\allzeros>allzeros
char: done in 62852600 nanoseconds
size_t: done in 17863800 nanoseconds (3.51843 times faster than char)

C:\usr\src\tests\allzeros>allzeros
char: done in 51877200 nanoseconds
size_t: done in 13759900 nanoseconds (3.77017 times faster than char)

The function used to replace clock_getime is:
timespec_get(ts, TIME_UTC)

best regards,
Ranier Vilela

---

From: Ranier Vilela <[email-masked]>
Date: Tue, 5 Nov 2024 09:53:19 -0300

> I am not seeing numbers as good as yours, but the winner is clear as
Thanks for testing.

> locations has this much effect once the compiler optimizes the
I think that's a plus point for the benefit of not touching the memory if
it's not explicitly necessary.

best regards,
Ranier Vilela

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 5 Nov 2024 15:03:23 +0000

Hi,

> p)[2] != 0 | ((size_t *) p)[3] != 0)

Another option could be to use SIMD instructions to check multiple bytes
is zero in a single operation. Maybe just an idea to keep in mind and experiment
if we feel the need later on.

Regards,

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 6 Nov 2024 19:34:19 +0100

> if we feel the need later on.

Speaking of which, couldn't you just use

     pg_popcount(ptr, len) == 0

?

---

From: David Rowley <[email-masked]>
Date: Thu, 7 Nov 2024 08:05:10 +1300

>      pg_popcount(ptr, len) == 0

That might be quite good for small lengths or for use cases where the
memory is always or almost always zero. The problem is there's no
early exit when you find the first non-zero which means, for larger
lengths, reading much more memory. That'll both take longer and
possibly evict cache lines which might be useful to have in the near
future.

David

---

From: Michael Paquier <[email-masked]>
Date: Thu, 7 Nov 2024 09:45:44 +0900

> possibly evict cache lines which might be useful to have in the near

Didn't know this one either, thanks for the explanation.
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 7 Nov 2024 08:41:52 +0000

Hi,

> Didn't know this one either, thanks for the explanation.

+1, thanks!

Regards,