define pg_structiszero(addr, s, r)

On Wed, Sep 18, 2024 at 04:16:12AM +0000, Bertrand Drouvot wrote:

The macro is created in pgstat_internal.h as it looks like that "only" the
statistics related code would benefit of it currently (could be moved to other
header file later on if needed).

I'm OK to add a helper macro in pgstat_internal.h as this is a pattern
used only for some stats kinds (the other one I'm aware of is the
allzero check for pages around bufmgr.c), cleaning up all these static
declarations to make the memcpy() calls cheaper. That can also be
useful for anybody doing a custom pgstats kind, fixed or
variable-numbered.

#define pg_structiszero(addr, s, r) \

Locating that at the top of pgstat_internal.h seems a bit out of order
to me. Perhaps it would be better to move it closer to the inline
functions?

Also, is this the best name to use here? Right, this is something
that may be quite generic. However, if we limit its scope in the
stats, perhaps this should be named pgstat_entry_all_zeros() or
something like that?
--
Michael

Hi,

On Wed, Sep 18, 2024 at 03:07:15PM +0900, Michael Paquier wrote:

On Wed, Sep 18, 2024 at 04:16:12AM +0000, Bertrand Drouvot wrote:

The macro is created in pgstat_internal.h as it looks like that "only" the
statistics related code would benefit of it currently (could be moved to other
header file later on if needed).

I'm OK to add a helper macro in pgstat_internal.h as this is a pattern
used only for some stats kinds (the other one I'm aware of is the
allzero check for pages around bufmgr.c), cleaning up all these static
declarations to make the memcpy() calls cheaper. That can also be
useful for anybody doing a custom pgstats kind, fixed or
variable-numbered.

Thanks for looking at it!

#define pg_structiszero(addr, s, r) \

Locating that at the top of pgstat_internal.h seems a bit out of order
to me. Perhaps it would be better to move it closer to the inline
functions?

Makes sense, done that way in v2 attached.

Also, is this the best name to use here? Right, this is something
that may be quite generic. However, if we limit its scope in the
stats, perhaps this should be named pgstat_entry_all_zeros() or
something like that?

Agree, we could still rename it later on if there is a need outside of
the statistics code area. Done in v2.

Regards,

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

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.

Hi,

On Wed, Sep 18, 2024 at 10:03:21AM +0200, Peter Eisentraut wrote:

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)

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]/messages/by-id/ZuqHLCdZXtEsbyb/@ip-10-97-1-34.eu-west-3.compute.internal), 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]: /messages/by-id/ZuqHLCdZXtEsbyb/@ip-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

On 18/09/2024 21:57, Bertrand Drouvot wrote:

On Wed, Sep 18, 2024 at 10:03:21AM +0200, Peter Eisentraut wrote:

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)

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)

Em seg., 28 de out. de 2024 às 11:33, Heikki Linnakangas <hlinnaka@iki.fi>
escreveu:

On 18/09/2024 21:57, Bertrand Drouvot wrote:

On Wed, Sep 18, 2024 at 10:03:21AM +0200, Peter Eisentraut wrote:

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)

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;
}

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

Ranier Vilela <ranier.vf@gmail.com> 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

Em seg., 28 de out. de 2024 às 12:08, Tom Lane <tgl@sss.pgh.pa.us> escreveu:

Ranier Vilela <ranier.vf@gmail.com> 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.

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

On Thu, Sep 19, 2024 at 4:57 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

Hi,

On Wed, Sep 18, 2024 at 10:03:21AM +0200, Peter Eisentraut wrote:

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)

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]: /messages/by-id/ZuqHLCdZXtEsbyb/@ip-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

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

Hi,

On Mon, Oct 28, 2024 at 04:32:51PM +0200, Heikki Linnakangas wrote:

On 18/09/2024 21:57, Bertrand Drouvot wrote:

On Wed, Sep 18, 2024 at 10:03:21AM +0200, Peter Eisentraut wrote:

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)

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

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;
}

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]https://in3.readthedocs.io/en/develop/api-c.html. 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]/messages/by-id/CAHut+PvHmWiPyqiDRnD7FYsc4QskXpKEZAH3Z8Ahd_GSnRXWrw@mail.gmail.com comparing
memcmp and simple loop checking (thanks Peter for the testing!):

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

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]: /messages/by-id/CAHut+PvHmWiPyqiDRnD7FYsc4QskXpKEZAH3Z8Ahd_GSnRXWrw@mail.gmail.com

Regards,

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

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.

On 29/10/2024 09:54, Bertrand Drouvot wrote:

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...
check zeros using loop -- elapsed=0.041196s
check zeros using memcmp -- elapsed=0.016407s
"

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 believe that test program is bogus. Look at the assembly code; the
compiler optimized away the loops.

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

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.

Regards,

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

Hi,

On Tue, Oct 29, 2024 at 11:39:03AM +0200, Heikki Linnakangas wrote:

On 29/10/2024 09:54, Bertrand Drouvot wrote:

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...
check zeros using loop -- elapsed=0.041196s
check zeros using memcmp -- elapsed=0.016407s
"

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 believe that test program is bogus. Look at the assembly code; the
compiler optimized away the loops.

Oh right. It looks like that moving the "scanf" within each loop "helps" and
that both give pretty comparable results.

Regards,

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

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.

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

+/*
+ * 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)

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

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

On Thu, Oct 31, 2024 at 07:55:45AM +1100, Peter Smith wrote:

+/*
+ * 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)

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

Yes.

+static inline bool
+pg_mem_is_all_zeros(const void *ptr, size_t len)

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.

+ if (pg_mem_is_all_zeros(pagebytes , (BLCKSZ / sizeof(size_t))))

Extra space not required after pagebytes.
--
Michael

Hi,

On Thu, Oct 31, 2024 at 09:59:35AM +0900, Michael Paquier wrote:

On Thu, Oct 31, 2024 at 07:55:45AM +1100, Peter Smith wrote:

+/*
+ * 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)

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

Yes.

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

+static inline bool
+pg_mem_is_all_zeros(const void *ptr, size_t len)

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?

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.

+ if (pg_mem_is_all_zeros(pagebytes , (BLCKSZ / sizeof(size_t))))

Extra space not required after pagebytes.

Fat finger here, thanks!

Regards,

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