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

From: Peter Eisentraut <[email-masked]>
Date: Wed, 18 Sep 2024 10:03:21 +0200

On 18.09.24 06:16, Bertrand Drouvot wrote:
> +#define pg_structiszero(addr, s, r)									\
> +	do {															\
> +		/* We assume this initializes to zeroes */					\
> +		static const s all_zeroes;									\
> +		r = (memcmp(addr, &all_zeroes, sizeof(all_zeroes)) == 0);	\
> +	} while (0)

This assumption is kind of the problem, isn't it?  Because, you can't 
assume that.  And the existing code is arguably kind of wrong.  But 
moreover, this macro also assumes that the "addr" argument has no random 
padding bits.

In the existing code, you can maybe make a local analysis that the code 
is working correctly, although I'm not actually sure.  But if you are 
repackaging this as a general macro under a general-sounding name, then 
the requirements should be more stringent.

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 18 Sep 2024 18:57:39 +0000

Hi,

> > +		r = (memcmp(addr, &all_zeroes, sizeof(all_zeroes)) == 0);	\

Thanks for the feedback.
 
> This assumption is kind of the problem, isn't it?  Because, you can't assume
> that.  And the existing code is arguably kind of wrong.  But moreover, this
> macro also assumes that the "addr" argument has no random padding bits.
> 
> In the existing code, you can maybe make a local analysis that the code is
> working correctly, although I'm not actually sure.

I think it is but will give it a closer look.

>  But if you are
> repackaging this as a general macro under a general-sounding name, then the
> requirements should be more stringent.

Agree. That said in v2 ([1]), it has been renamed to pgstat_entry_all_zeros().

I think that I will:

1/ take a closer look regarding the existing assumption
2/ if 1/ outcome is fine, then add more detailed comments around
pgstat_entry_all_zeros() to make sure it's not used outside of the existing
context 

Sounds good to you?

[1]: https://www.postgresql.org/message-id/ZuqHLCdZXtEsbyb/%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Heikki Linnakangas <[email-masked]>
Date: Mon, 28 Oct 2024 16:32:51 +0200

>>> +		r = (memcmp(addr, &all_zeroes, sizeof(all_zeroes)) == 0);	\

Not new with this patch, but do we guarantee padding bytes to be zeros?

How about this instead:

static inline bool
pg_is_all_zeros(const char *p, size_t len)
{
   for (size_t i = 0; i < len; i++)
   {
     if (p[i] != 0)
         return false;
   }
   return true;
}

Is there's a de facto standard name for that function? I was surprised 
that I couldn't find one with a quick google search. That seems like the 
kind of small utility function that every C program needs.

How performance sensitive is this? If it's not, then the above seems 
like the most straightforward way to do this, which is good. If it is 
performance sensitive, it's still good, because the compiler can 
optimize that well: https://godbolt.org/z/x9hPWjheq.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

---

From: Ranier Vilela <[email-masked]>
Date: Mon, 28 Oct 2024 12:02:48 -0300

Em seg., 28 de out. de 2024 às 11:33, Heikki Linnakangas <[email-masked]>
escreveu:

> >>> +   do {
>                                                       \
> It seems to me that this way is more optimized.

static inline bool
is_all_zeros(const char *p, size_t len)
{
  for (size_t i = len; i >= 0; i--)
  {
    if (p[i] != 0)
        return false;
  }
  return true;
}

main:
        sub     rsp, 24
        lea     rdx, [rsp + 12]
        lea     rcx, [rsp + 16]
        lea     rdi, [rip + .L.str]
        lea     rsi, [rsp + 8]
        xor     eax, eax
        call    __isoc99_scanf@PLT
        lea     rdi, [rip + .L.str.1]
        xor     esi, esi
        xor     eax, eax
        call    printf@PLT
        xor     eax, eax
        add     rsp, 24
        ret

best regards,
Ranier Vilela

---

From: Tom Lane <[email-masked]>
Date: Mon, 28 Oct 2024 11:08:31 -0400

Ranier Vilela <[email-masked]> writes:
> It seems to me that [reversing the loop direction] is more optimized.

That's far from clear: you're ignoring the possibility that memory
access logic is better optimized for forward scanning than reverse
scanning.  I'd stick with the forward scan without some extremely
detailed testing.

			regards, tom lane

---

From: Ranier Vilela <[email-masked]>
Date: Mon, 28 Oct 2024 13:29:08 -0300

> scanning.  I'd stick with the forward scan without some extremely
I don't disagree.
After posting, I was wondering if the first possible is not zero, it should
probably be at the beginning and not at the end.

best regards,
Ranier Vilela

---

From: Peter Smith <[email-masked]>
Date: Tue, 29 Oct 2024 09:36:34 +1100

> [1]: https://www.postgresql.org/message-id/ZuqHLCdZXtEsbyb/%40ip-10-97-1-34.eu-west-3.compute.internal

Hi, if this is performance critical then FWIW my understanding is that
memcmp can outperform simple loop checking, and my hacky testing
seemed to confirm that.

See https://godbolt.org/z/GWY1ob9bE

static inline bool
is_all_zeros2(const char *p, size_t len)
{
    static size_t nz = 0;
    static const char *z = NULL;

    if (nz < len)
    {
        if (z) free((void *)z);
        nz = len;
        z = (const char *)calloc(1, nz);
    }

    return memcmp(p, z, len) == 0;
}

~~

Executor x86-64 gcc 14.2 (C, Editor #1)
Program stdout

Iterate 1000000 times...
check zeros using loop -- elapsed=0.041196s
check zeros using memcmp -- elapsed=0.016407s

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

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 29 Oct 2024 07:54:24 +0000

Hi,

On Mon, Oct 28, 2024 at 04:32:51PM +0200, Heikki Linnakangas wrote:
> Not new with this patch

Thanks for looking at it! 

> but do we guarantee padding bytes to be zeros?

I can see padding in one of the 3 structs of interest: PgStat_BgWriterStats and
PgStat_CheckpointerStats have no padding but PgStat_TableCounts has:

(gdb) ptype /o struct PgStat_TableCounts
/* offset      |    size */  type = struct PgStat_TableCounts {
/*      0      |       8 */    PgStat_Counter numscans;
/*      8      |       8 */    PgStat_Counter tuples_returned;
/*     16      |       8 */    PgStat_Counter tuples_fetched;
/*     24      |       8 */    PgStat_Counter tuples_inserted;
/*     32      |       8 */    PgStat_Counter tuples_updated;
/*     40      |       8 */    PgStat_Counter tuples_deleted;
/*     48      |       8 */    PgStat_Counter tuples_hot_updated;
/*     56      |       8 */    PgStat_Counter tuples_newpage_updated;
/*     64      |       1 */    _Bool truncdropped;
/* XXX  7-byte hole      */
/*     72      |       8 */    PgStat_Counter delta_live_tuples;
/*     80      |       8 */    PgStat_Counter delta_dead_tuples;
/*     88      |       8 */    PgStat_Counter changed_tuples;
/*     96      |       8 */    PgStat_Counter blocks_fetched;
/*    104      |       8 */    PgStat_Counter blocks_hit;

                               /* total size (bytes):  112 */
                             }

According to my testing, I can see that "static const PgStat_TableCounts all_zeroes"
all_zeroes is fully made of zeros (padding included). OTOH I would not bet on the
fact that's guaranteed to be the case 100% of the time.

But even, if that is not the case I don't think that it is a big issue: the
check is in pgstat_relation_flush_cb() to decide if we want to avoid unnecessarily
modifying the stats entry. If padding would contain non zeros then we would
"just" unnecessarily modify the stats entry (adding "zeros" to the shared stats).

>   for (size_t i = 0; i < len; i++)

Yeah, that sounds good to me. It's more generic than the initial proposal that
was taking care of a struct memory area. Also, the same "logic" is already
in PageIsVerifiedExtended().

V3 attached is using the above proposal and also makes the change in
PageIsVerifiedExtended(). Then, the new inline function has been placed in 
utils/memutils.h (not sure that's the best place though).

> Is there's a de facto standard name for that function? I was surprised that
> I couldn't find one with a quick google search.

Same here. I was just able to find "memiszero" in [0]. So the naming proposal
in v3 is pg_mem_is_all_zeros().

> That seems like the kind of
> small utility function that every C program needs.

Yeah.

> How performance sensitive is this? 

I don't think that's very sensitive. I think it's "cheap" as compared to what lead
to those code paths (stats increments).

> If it's not, then the above seems like
> the most straightforward way to do this, which is good. If it is performance
> sensitive, it's still good, because the compiler can optimize that well:
> https://godbolt.org/z/x9hPWjheq.

Yeah, I also think that's fine. Peter Smith did some testing in [1] comparing
memcmp and simple loop checking (thanks Peter for the testing!):

"
Iterate 1000000 times...
"

So, in this test, the loop is 0.024789s longer means 0.024789s/1000000=24 Nanosecond
slower per comparison (If my math is correct). I don't see this as a red flag and
still go with the loop proposal as this is the one already used in
PageIsVerifiedExtended(). 

[0]: https://in3.readthedocs.io/en/develop/api-c.html
[1]: https://www.postgresql.org/message-id/CAHut%2BPvHmWiPyqiDRnD7FYsc4QskXpKEZAH3Z8Ahd_GSnRXWrw%40mail.gmail.com

Regards,

>From aa1b45af21fd363bb4c7b202f7c3aa654048b208 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Tue, 10 Sep 2024 01:22:02 +0000
Subject: [PATCH v3] 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                     | 14 ++++++++++++++
 5 files changed, 23 insertions(+), 23 deletions(-)
  20.6% src/backend/storage/page/
  61.1% src/backend/utils/activity/
  18.1% src/include/utils/

diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index be6f1f62d2..306090c778 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((const char *) 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..d9c21cf665 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((const char *) &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..6f3e2a8a45 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((const char *) &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..3bbec950b1 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((const char *) &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..2f331514f3 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -189,4 +189,18 @@ 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 char *p, size_t len)
+{
+	for (size_t i = 0; i < len; i++)
+	{
+		if (p[i] != 0)
+			return false;
+	}
+	return true;
+}
+
 #endif							/* MEMUTILS_H */
-- 
2.34.1