public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v3] Add "Backpatch" regions in wait_event_names.txt
36+ messages / 7 participants
[nested] [flat]
* [PATCH v3] Add "Backpatch" regions in wait_event_names.txt
@ 2024-03-18 08:34 Bertrand Drouvot <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Bertrand Drouvot @ 2024-03-18 08:34 UTC (permalink / raw)
When backpatching, adding an event will renumber others, which can make an
extension report the wrong event until recompiled. This is due to the fact that
generate-wait_event_types.pl sorts events to position them. The "Backpatch"
region added here ensures no ordering is done for the wait events found after
this delimiter.
---
.../activity/generate-wait_event_types.pl | 26 ++++++++++++++++++-
.../utils/activity/wait_event_names.txt | 18 ++++++++++++-
2 files changed, 42 insertions(+), 2 deletions(-)
100.0% src/backend/utils/activity/
diff --git a/src/backend/utils/activity/generate-wait_event_types.pl b/src/backend/utils/activity/generate-wait_event_types.pl
index f1adf0e8e7..d129d94889 100644
--- a/src/backend/utils/activity/generate-wait_event_types.pl
+++ b/src/backend/utils/activity/generate-wait_event_types.pl
@@ -38,7 +38,9 @@ die "Not possible to specify --docs and --code simultaneously"
open my $wait_event_names, '<', $ARGV[0] or die;
+my @backpatch_lines;
my @lines;
+my $backpatch = 0;
my $section_name;
my $note;
my $note_name;
@@ -59,10 +61,26 @@ while (<$wait_event_names>)
{
$section_name = $_;
$section_name =~ s/^.*- //;
+ $backpatch = 0;
next;
}
- push(@lines, $section_name . "\t" . $_);
+ # Are we dealing with backpatch wait events?
+ if (/^Backpatch:$/)
+ {
+ $backpatch = 1;
+ next;
+ }
+
+ # Backpatch wait events won't be sorted during code generation
+ if ($gen_code && $backpatch)
+ {
+ push(@backpatch_lines, $section_name . "\t" . $_);
+ }
+ else
+ {
+ push(@lines, $section_name . "\t" . $_);
+ }
}
# Sort the lines based on the second column.
@@ -70,6 +88,12 @@ while (<$wait_event_names>)
my @lines_sorted =
sort { uc((split(/\t/, $a))[1]) cmp uc((split(/\t/, $b))[1]) } @lines;
+# If we are generating code then concat @lines_sorted and @backpatch_lines.
+if ($gen_code)
+{
+ push(@lines_sorted, @backpatch_lines);
+}
+
# Read the sorted lines and populate the hash table
foreach my $line (@lines_sorted)
{
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index c08e00d1d6..0c4788fe77 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -24,7 +24,10 @@
# SGML tables of wait events for inclusion in the documentation.
#
# When adding a new wait event, make sure it is placed in the appropriate
-# ClassName section.
+# ClassName section. Put it in its natural position in the master branch, and
+# then put it in the "Backpatch:" region for any other branch (should the wait
+# event be backpatched).
+# Ensure that the backpatch regions are always empty on the master branch.
#
# WaitEventLWLock and WaitEventLock have their own C code for their wait event
# enums and function names. Hence, for these, only the SGML documentation is
@@ -61,6 +64,7 @@ WAL_SENDER_MAIN "Waiting in main loop of WAL sender process."
WAL_SUMMARIZER_WAL "Waiting in WAL summarizer for more WAL to be generated."
WAL_WRITER_MAIN "Waiting in main loop of WAL writer process."
+Backpatch:
#
# Wait Events - Client
@@ -82,6 +86,7 @@ WAIT_FOR_STANDBY_CONFIRMATION "Waiting for WAL to be received and flushed by the
WAL_SENDER_WAIT_FOR_WAL "Waiting for WAL to be flushed in WAL sender process."
WAL_SENDER_WRITE_DATA "Waiting for any activity when processing replies from WAL receiver in WAL sender process."
+Backpatch:
#
# Wait Events - IPC
@@ -149,6 +154,7 @@ WAL_RECEIVER_WAIT_START "Waiting for startup process to send initial data for st
WAL_SUMMARY_READY "Waiting for a new WAL summary to be generated."
XACT_GROUP_UPDATE "Waiting for the group leader to update transaction status at end of a parallel operation."
+Backpatch:
#
# Wait Events - Timeout
@@ -169,6 +175,7 @@ VACUUM_DELAY "Waiting in a cost-based vacuum delay point."
VACUUM_TRUNCATE "Waiting to acquire an exclusive lock to truncate off any empty pages at the end of a table vacuumed."
WAL_SUMMARIZER_ERROR "Waiting after a WAL summarizer error."
+Backpatch:
#
# Wait Events - IO
@@ -256,6 +263,7 @@ WAL_SYNC "Waiting for a WAL file to reach durable storage."
WAL_SYNC_METHOD_ASSIGN "Waiting for data to reach durable storage while assigning a new WAL sync method."
WAL_WRITE "Waiting for a write to a WAL file."
+Backpatch:
#
# Wait Events - Buffer Pin
@@ -265,6 +273,7 @@ Section: ClassName - WaitEventBufferPin
BUFFER_PIN "Waiting to acquire an exclusive pin on a buffer."
+Backpatch:
#
# Wait Events - Extension
@@ -274,6 +283,8 @@ Section: ClassName - WaitEventExtension
Extension "Waiting in an extension."
+Backpatch:
+
#
# Wait Events - LWLock
#
@@ -330,6 +341,8 @@ DSMRegistry "Waiting to read or update the dynamic shared memory registry."
InjectionPoint "Waiting to read or update information related to injection points."
SerialControl "Waiting to read or update shared <filename>pg_serial</filename> state."
+# Don't create a Backpatch region here.
+
#
# END OF PREDEFINED LWLOCKS (DO NOT CHANGE THIS LINE)
#
@@ -377,6 +390,7 @@ SerialSLRU "Waiting to access the serializable transaction conflict SLRU cache."
SubtransSLRU "Waiting to access the sub-transaction SLRU cache."
XactSLRU "Waiting to access the transaction status SLRU cache."
+# Don't create a Backpatch region here.
#
# Wait Events - Lock
@@ -399,3 +413,5 @@ object "Waiting to acquire a lock on a non-relation database object."
userlock "Waiting to acquire a user lock."
advisory "Waiting to acquire an advisory user lock."
applytransaction "Waiting to acquire a lock on a remote transaction being applied by a logical replication subscriber."
+
+Backpatch:
--
2.34.1
--m1fyrkxSXqjY+FO1--
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-10-30 09:23 Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Bertrand Drouvot @ 2024-10-30 09:23 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; [email protected]
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
Attachments:
[text/x-diff] v5-0001-New-pg_mem_is_all_zeros-p-len-inline-function.patch (5.5K, ../../[email protected]/2-v5-0001-New-pg_mem_is_all_zeros-p-len-inline-function.patch)
download | inline diff:
From e80e5cf946813185985bf5339ac996ccc0eab1f0 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email protected]>
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
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-10-30 20:55 Peter Smith <[email protected]>
parent: Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Peter Smith @ 2024-10-30 20:55 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
+/*
+ * 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
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-10-31 00:59 Michael Paquier <[email protected]>
parent: Peter Smith <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Michael Paquier @ 2024-10-31 00:59 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-10-31 05:52 Bertrand Drouvot <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Bertrand Drouvot @ 2024-10-31 05:52 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
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
Attachments:
[text/x-diff] v6-0001-New-pg_memory_is_all_zeros-ptr-len-inline-functio.patch (5.5K, ../../[email protected]/2-v6-0001-New-pg_memory_is_all_zeros-ptr-len-inline-functio.patch)
download | inline diff:
From 88f5ebbbee3cdc7b773a1d0f5a7cf6f9d909b784 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email protected]>
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/
diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index be6f1f62d2..5ee1e58cd4 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_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"
+#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_memory_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..258337cfb8 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_memory_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..337954d15e 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_memory_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..0094492d9c 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 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
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-10-31 06:17 Michael Paquier <[email protected]>
parent: Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Michael Paquier @ 2024-10-31 06:17 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On Thu, Oct 31, 2024 at 05:52:49AM +0000, Bertrand Drouvot wrote:
> 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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-01 04:45 David Rowley <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 2 replies; 36+ messages in thread
From: David Rowley @ 2024-11-01 04:45 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On Thu, 31 Oct 2024 at 19:17, Michael Paquier <[email protected]> wrote:
> 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.
This change looks incorrect:
@@ -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_memory_is_all_zeros(pagebytes, (BLCKSZ / sizeof(size_t))))
return true;
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
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-01 06:27 Michael Paquier <[email protected]>
parent: David Rowley <[email protected]>
1 sibling, 1 reply; 36+ messages in thread
From: Michael Paquier @ 2024-11-01 06:27 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
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
Attachments:
[text/x-csrc] allzeros.c (699B, ../../[email protected]/2-allzeros.c)
download | inline:
#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;
}
[text/x-diff] page-allzeros.patch (729B, ../../[email protected]/3-page-allzeros.patch)
download | inline diff:
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;
/*
[application/pgp-signature] signature.asc (833B, ../../[email protected]/4-signature.asc)
download
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-01 06:33 Bertrand Drouvot <[email protected]>
parent: David Rowley <[email protected]>
1 sibling, 2 replies; 36+ messages in thread
From: Bertrand Drouvot @ 2024-11-01 06:33 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Michael Paquier <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
Hi,
On Fri, Nov 01, 2024 at 05:45:44PM +1300, David Rowley wrote:
> On Thu, 31 Oct 2024 at 19:17, Michael Paquier <[email protected]> wrote:
> > 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.
>
> This change looks incorrect:
>
> @@ -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_memory_is_all_zeros(pagebytes, (BLCKSZ / sizeof(size_t))))
> return true;
>
> 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.
Thanks for looking at it!
Nice catch, indeed by using the new function we are changing the pointer arithmetic
here and then we should pass BLCKSZ instead.
> 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.
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,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-01 07:14 Michael Paquier <[email protected]>
parent: Bertrand Drouvot <[email protected]>
1 sibling, 1 reply; 36+ messages in thread
From: Michael Paquier @ 2024-11-01 07:14 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: David Rowley <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On Fri, Nov 01, 2024 at 06:33:51AM +0000, Bertrand Drouvot wrote:
> 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
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 protected]
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-01 07:19 David Rowley <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: David Rowley @ 2024-11-01 07:19 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On Fri, 1 Nov 2024 at 19:27, Michael Paquier <[email protected]> wrote:
> 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.
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
#include <stdbool.h>
#include <stddef.h>
#include <string.h>
#include <stdio.h>
#include <stdint.h>
#include <time.h>
#define BLCKSZ 8192
#define LOOPS 1000
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;
}
#define NANOSEC_PER_SEC 1000000000
// Returns difference in nanoseconds
int64_t
get_clock_diff(struct timespec *t1, struct timespec *t2)
{
int64_t nanosec = (t1->tv_sec - t2->tv_sec) * NANOSEC_PER_SEC;
nanosec += (t1->tv_nsec - t2->tv_nsec);
return nanosec;
}
int main()
{
size_t pagebytes[BLCKSZ] = {0};
volatile bool result;
struct timespec start,end;
int64_t char_time, size_t_time;
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &start);
for (int i = 0; i < LOOPS; i++)
{
result = allzeros_char(pagebytes, BLCKSZ);
}
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &end);
char_time = get_clock_diff(&end, &start);
printf("char: done in %ld nanoseconds\n", char_time);
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &start);
for (int i = 0; i < LOOPS; i++)
{
result = allzeros_size_t(pagebytes, BLCKSZ);
}
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &end);
size_t_time = get_clock_diff(&end, &start);
printf("size_t: done in %ld nanoseconds (%g times faster)\n", size_t_time, (double) char_time / size_t_time);
return 0;
}
Attachments:
[text/plain] allzeros_v2.c (1.6K, ../../CAApHDvoL5B87E8YamUu+-5RhuGzV1C8tvHQehZ78nV1nCXq7Jw@mail.gmail.com/2-allzeros_v2.c)
download | inline:
#include <stdbool.h>
#include <stddef.h>
#include <string.h>
#include <stdio.h>
#include <stdint.h>
#include <time.h>
#define BLCKSZ 8192
#define LOOPS 1000
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;
}
#define NANOSEC_PER_SEC 1000000000
// Returns difference in nanoseconds
int64_t
get_clock_diff(struct timespec *t1, struct timespec *t2)
{
int64_t nanosec = (t1->tv_sec - t2->tv_sec) * NANOSEC_PER_SEC;
nanosec += (t1->tv_nsec - t2->tv_nsec);
return nanosec;
}
int main()
{
size_t pagebytes[BLCKSZ] = {0};
volatile bool result;
struct timespec start,end;
int64_t char_time, size_t_time;
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &start);
for (int i = 0; i < LOOPS; i++)
{
result = allzeros_char(pagebytes, BLCKSZ);
}
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &end);
char_time = get_clock_diff(&end, &start);
printf("char: done in %ld nanoseconds\n", char_time);
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &start);
for (int i = 0; i < LOOPS; i++)
{
result = allzeros_size_t(pagebytes, BLCKSZ);
}
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &end);
size_t_time = get_clock_diff(&end, &start);
printf("size_t: done in %ld nanoseconds (%g times faster)\n", size_t_time, (double) char_time / size_t_time);
return 0;
}
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-01 07:21 David Rowley <[email protected]>
parent: Bertrand Drouvot <[email protected]>
1 sibling, 1 reply; 36+ messages in thread
From: David Rowley @ 2024-11-01 07:21 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Michael Paquier <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On Fri, 1 Nov 2024 at 19:33, Bertrand Drouvot
<[email protected]> wrote:
> 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?
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
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-01 07:27 David Rowley <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: David Rowley @ 2024-11-01 07:27 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On Fri, 1 Nov 2024 at 20:14, Michael Paquier <[email protected]> wrote:
> 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?
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
> [1]: https://www.postgresql.org/message-id/[email protected]
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-01 07:36 Michael Paquier <[email protected]>
parent: David Rowley <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Michael Paquier @ 2024-11-01 07:36 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On Fri, Nov 01, 2024 at 08:21:50PM +1300, David Rowley wrote:
> 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.
Fine, here you go. The attached reverts back this part in bufpage.c
to what it was in 49d6c7d8daba.
--
Michael
Attachments:
[text/x-diff] bufpage-revert.patch (895B, ../../[email protected]/2-bufpage-revert.patch)
download | inline diff:
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 (pg_memory_is_all_zeros(pagebytes, (BLCKSZ / sizeof(size_t))))
+ if (all_zeroes)
return true;
/*
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-01 07:44 Bertrand Drouvot <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Bertrand Drouvot @ 2024-11-01 07:44 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: David Rowley <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
Hi,
On Fri, Nov 01, 2024 at 04:36:45PM +0900, Michael Paquier wrote:
> On Fri, Nov 01, 2024 at 08:21:50PM +1300, David Rowley wrote:
> > 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.
>
> Fine, here you go. The attached reverts back this part in bufpage.c
> 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,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-01 07:49 Michael Paquier <[email protected]>
parent: Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Michael Paquier @ 2024-11-01 07:49 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: David Rowley <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-01 08:47 David Rowley <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 2 replies; 36+ messages in thread
From: David Rowley @ 2024-11-01 08:47 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On Fri, 1 Nov 2024 at 20:49, Michael Paquier <[email protected]> wrote:
>
> On Fri, Nov 01, 2024 at 07:44:22AM +0000, Bertrand Drouvot wrote:
> > 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.
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
Attachments:
[application/octet-stream] pg_memory_is_all_zeros_speedup.patch (808B, ../../CAApHDvqULM7KhreSa8PU2bshrguOaRdJsfjKCTvDkByYRFLaMg@mail.gmail.com/2-pg_memory_is_all_zeros_speedup.patch)
download | inline diff:
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index 3590c8bad9..0f50fc9f3d 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -197,12 +197,30 @@ static inline bool
pg_memory_is_all_zeros(const void *ptr, size_t len)
{
const char *p = (const char *) ptr;
+ const char *end = &p[len];
+ const char *aligned_end = (const char *) ((uintptr_t) end & (~(sizeof(size_t) - 1)));
- for (size_t i = 0; i < len; i++)
+ while (((uintptr_t) p & (sizeof(size_t) - 1)) != 0)
{
- if (p[i] != 0)
+ if (p == end)
+ return true;
+
+ if (*p++ != 0)
+ return false;
+ }
+
+ for (; p < aligned_end; p += sizeof(size_t))
+ {
+ if (*(size_t *) p != 0)
return false;
}
+
+ while (p < end)
+ {
+ if (*p++ != 0)
+ return false;
+ }
+
return true;
}
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-01 12:50 Bertrand Drouvot <[email protected]>
parent: David Rowley <[email protected]>
1 sibling, 2 replies; 36+ messages in thread
From: Bertrand Drouvot @ 2024-11-01 12:50 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Michael Paquier <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
Hi,
On Fri, Nov 01, 2024 at 09:47:05PM +1300, David Rowley wrote:
> On Fri, 1 Nov 2024 at 20:49, Michael Paquier <[email protected]> wrote:
> >
> > On Fri, Nov 01, 2024 at 07:44:22AM +0000, Bertrand Drouvot wrote:
> > > 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.
>
> 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.
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
> 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 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).
> 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, 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,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-01 15:01 Japin Li <[email protected]>
parent: David Rowley <[email protected]>
1 sibling, 0 replies; 36+ messages in thread
From: Japin Li @ 2024-11-01 15:01 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Michael Paquier <[email protected]>; Bertrand Drouvot <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On Fri, 01 Nov 2024 at 21:47, David Rowley <[email protected]> wrote:
> On Fri, 1 Nov 2024 at 20:49, Michael Paquier <[email protected]> wrote:
>>
>> On Fri, Nov 01, 2024 at 07:44:22AM +0000, Bertrand Drouvot wrote:
>> > 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.
>
> 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.
>
+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
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-04 09:50 Bertrand Drouvot <[email protected]>
parent: Bertrand Drouvot <[email protected]>
1 sibling, 0 replies; 36+ messages in thread
From: Bertrand Drouvot @ 2024-11-04 09:50 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Michael Paquier <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
Hi,
On Fri, Nov 01, 2024 at 12:50:10PM +0000, Bertrand Drouvot wrote:
> Hi,
>
> On Fri, Nov 01, 2024 at 09:47:05PM +1300, David Rowley wrote:
> > On Fri, 1 Nov 2024 at 20:49, Michael Paquier <[email protected]> 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, 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.
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,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-04 11:24 David Rowley <[email protected]>
parent: Bertrand Drouvot <[email protected]>
1 sibling, 1 reply; 36+ messages in thread
From: David Rowley @ 2024-11-04 11:24 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Michael Paquier <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On Sat, 2 Nov 2024 at 01:50, Bertrand Drouvot
<[email protected]> 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, 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
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-04 17:17 Bertrand Drouvot <[email protected]>
parent: David Rowley <[email protected]>
0 siblings, 2 replies; 36+ messages in thread
From: Bertrand Drouvot @ 2024-11-04 17:17 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Michael Paquier <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
Hi,
On Tue, Nov 05, 2024 at 12:24:48AM +1300, David Rowley wrote:
> On Sat, 2 Nov 2024 at 01:50, Bertrand Drouvot
> <[email protected]> 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, something like:
>
> 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
> 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.
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?
> 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.
Agree.
[0]: https://godbolt.org/z/hrYda53Tj
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v1-0001-Optimize-pg_memory_is_all_zeros.patch (3.3K, ../../[email protected]/2-v1-0001-Optimize-pg_memory_is_all_zeros.patch)
download | inline diff:
From f23a5d8aca945bd5762f34095eb0de5ecbe23dfa Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email protected]>
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/
diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index be6f1f62d2..aa264f61b9 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_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
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-04 17:38 Ranier Vilela <[email protected]>
parent: Bertrand Drouvot <[email protected]>
1 sibling, 2 replies; 36+ messages in thread
From: Ranier Vilela @ 2024-11-04 17:38 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: David Rowley <[email protected]>; Michael Paquier <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
Hi.
Em seg., 4 de nov. de 2024 às 14:18, Bertrand Drouvot <
[email protected]> escreveu:
> Hi,
>
> On Tue, Nov 05, 2024 at 12:24:48AM +1300, David Rowley wrote:
> > On Sat, 2 Nov 2024 at 01:50, Bertrand Drouvot
> > <[email protected]> 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, something
> like:
> >
> > 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
>
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
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-05 03:23 David Rowley <[email protected]>
parent: Ranier Vilela <[email protected]>
1 sibling, 2 replies; 36+ messages in thread
From: David Rowley @ 2024-11-05 03:23 UTC (permalink / raw)
To: Ranier Vilela <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Michael Paquier <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On Tue, 5 Nov 2024 at 06:39, Ranier Vilela <[email protected]> wrote:
> 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))
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
#include <stdbool.h>
#include <stddef.h>
#include <string.h>
#include <stdio.h>
#include <stdint.h>
#include <time.h>
#define BLCKSZ 8192
#define LOOPS 1000
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;
}
bool
pg_memory_is_all_zeros(const void *ptr, size_t len)
{
const char *p = (const char *) ptr;
const char *end = &p[len];
#ifdef RANIERS_OPTIMIZATION
const char *aligned_end;
#else
const char *aligned_end = (const char *) ((uintptr_t) end & (~(sizeof(size_t) - 1)));
#endif
while (((uintptr_t) p & (sizeof(size_t) - 1)) != 0)
{
if (p == end)
return true;
if (*p++ != 0)
return false;
}
#ifdef RANIERS_OPTIMIZATION
aligned_end = (const char *) ((uintptr_t) end & (~(sizeof(size_t) - 1)));
#endif
for (; p < aligned_end; p += sizeof(size_t))
{
if (*(size_t *) p != 0)
return false;
}
while (p < end)
{
if (*p++ != 0)
return false;
}
return true;
}
#define NANOSEC_PER_SEC 1000000000
// Returns difference in nanoseconds
int64_t
get_clock_diff(struct timespec *t1, struct timespec *t2)
{
int64_t nanosec = (t1->tv_sec - t2->tv_sec) * NANOSEC_PER_SEC;
nanosec += (t1->tv_nsec - t2->tv_nsec);
return nanosec;
}
int main()
{
size_t pagebytes[BLCKSZ] = {0};
volatile bool result;
struct timespec start,end;
int64_t char_time, size_t_time;
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &start);
for (int i = 0; i < LOOPS; i++)
{
result = allzeros_char(pagebytes, BLCKSZ);
}
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &end);
char_time = get_clock_diff(&end, &start);
printf("char: done in %ld nanoseconds\n", char_time);
#if TEST_PRE_ALIGNED_VERSION
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &start);
for (int i = 0; i < LOOPS; i++)
{
result = allzeros_size_t(pagebytes, BLCKSZ);
}
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &end);
size_t_time = get_clock_diff(&end, &start);
printf("size_t: done in %ld nanoseconds (%g times faster than char)\n", size_t_time, (double) char_time / size_t_time);
#endif
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &start);
for (int i = 0; i < LOOPS; i++)
{
result = pg_memory_is_all_zeros(pagebytes, BLCKSZ);
}
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &end);
size_t_time = get_clock_diff(&end, &start);
printf("size_t: done in %ld nanoseconds (%g times faster than char)\n", size_t_time, (double) char_time / size_t_time);
return 0;
}
Attachments:
[text/plain] allzeros.c (2.6K, ../../CAApHDvrYW5m8iwwd3NxZxtcNJ_+M9BdMiDidvyUcPKJHxb4Zdg@mail.gmail.com/2-allzeros.c)
download | inline:
#include <stdbool.h>
#include <stddef.h>
#include <string.h>
#include <stdio.h>
#include <stdint.h>
#include <time.h>
#define BLCKSZ 8192
#define LOOPS 1000
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;
}
bool
pg_memory_is_all_zeros(const void *ptr, size_t len)
{
const char *p = (const char *) ptr;
const char *end = &p[len];
#ifdef RANIERS_OPTIMIZATION
const char *aligned_end;
#else
const char *aligned_end = (const char *) ((uintptr_t) end & (~(sizeof(size_t) - 1)));
#endif
while (((uintptr_t) p & (sizeof(size_t) - 1)) != 0)
{
if (p == end)
return true;
if (*p++ != 0)
return false;
}
#ifdef RANIERS_OPTIMIZATION
aligned_end = (const char *) ((uintptr_t) end & (~(sizeof(size_t) - 1)));
#endif
for (; p < aligned_end; p += sizeof(size_t))
{
if (*(size_t *) p != 0)
return false;
}
while (p < end)
{
if (*p++ != 0)
return false;
}
return true;
}
#define NANOSEC_PER_SEC 1000000000
// Returns difference in nanoseconds
int64_t
get_clock_diff(struct timespec *t1, struct timespec *t2)
{
int64_t nanosec = (t1->tv_sec - t2->tv_sec) * NANOSEC_PER_SEC;
nanosec += (t1->tv_nsec - t2->tv_nsec);
return nanosec;
}
int main()
{
size_t pagebytes[BLCKSZ] = {0};
volatile bool result;
struct timespec start,end;
int64_t char_time, size_t_time;
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &start);
for (int i = 0; i < LOOPS; i++)
{
result = allzeros_char(pagebytes, BLCKSZ);
}
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &end);
char_time = get_clock_diff(&end, &start);
printf("char: done in %ld nanoseconds\n", char_time);
#if TEST_PRE_ALIGNED_VERSION
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &start);
for (int i = 0; i < LOOPS; i++)
{
result = allzeros_size_t(pagebytes, BLCKSZ);
}
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &end);
size_t_time = get_clock_diff(&end, &start);
printf("size_t: done in %ld nanoseconds (%g times faster than char)\n", size_t_time, (double) char_time / size_t_time);
#endif
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &start);
for (int i = 0; i < LOOPS; i++)
{
result = pg_memory_is_all_zeros(pagebytes, BLCKSZ);
}
clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &end);
size_t_time = get_clock_diff(&end, &start);
printf("size_t: done in %ld nanoseconds (%g times faster than char)\n", size_t_time, (double) char_time / size_t_time);
return 0;
}
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-05 04:08 David Rowley <[email protected]>
parent: Ranier Vilela <[email protected]>
1 sibling, 2 replies; 36+ messages in thread
From: David Rowley @ 2024-11-05 04:08 UTC (permalink / raw)
To: Ranier Vilela <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Michael Paquier <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On Tue, 5 Nov 2024 at 06:39, Ranier Vilela <[email protected]> wrote:
> 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
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-05 04:12 Michael Paquier <[email protected]>
parent: David Rowley <[email protected]>
1 sibling, 1 reply; 36+ messages in thread
From: Michael Paquier @ 2024-11-05 04:12 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Ranier Vilela <[email protected]>; Bertrand Drouvot <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On Tue, Nov 05, 2024 at 04:23:34PM +1300, David Rowley wrote:
> 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 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
> size_t: done in 531700 nanoseconds (3.6545 times faster than char)
> 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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-05 04:31 Michael Paquier <[email protected]>
parent: Bertrand Drouvot <[email protected]>
1 sibling, 1 reply; 36+ messages in thread
From: Michael Paquier @ 2024-11-05 04:31 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: David Rowley <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On Mon, Nov 04, 2024 at 05:17:54PM +0000, Bertrand Drouvot wrote:
> Hi,
>
> On Tue, Nov 05, 2024 at 12:24:48AM +1300, David Rowley wrote:
> > On Sat, 2 Nov 2024 at 01:50, Bertrand Drouvot
> > <[email protected]> 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, something like:
> >
> > 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
+ * 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 != 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> wrote:
> 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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-05 05:49 Bertrand Drouvot <[email protected]>
parent: David Rowley <[email protected]>
1 sibling, 0 replies; 36+ messages in thread
From: Bertrand Drouvot @ 2024-11-05 05:49 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Ranier Vilela <[email protected]>; Michael Paquier <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
Hi,
On Tue, Nov 05, 2024 at 05:08:41PM +1300, David Rowley wrote:
> 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.
>
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,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-05 05:59 Bertrand Drouvot <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Bertrand Drouvot @ 2024-11-05 05:59 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: David Rowley <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
Hi,
On Tue, Nov 05, 2024 at 01:31:58PM +0900, Michael Paquier wrote:
> On Mon, Nov 04, 2024 at 05:17:54PM +0000, Bertrand Drouvot wrote:
> > Hi,
> >
> > On Tue, Nov 05, 2024 at 12:24:48AM +1300, David Rowley wrote:
> > > On Sat, 2 Nov 2024 at 01:50, Bertrand Drouvot
> > > <[email protected]> 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, something like:
> > >
> > > 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
>
> + * 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.
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?
> > - 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.
Yeah, agree, removed in v2 attached.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v2-0001-Optimize-pg_memory_is_all_zeros.patch (3.3K, ../../[email protected]/2-v2-0001-Optimize-pg_memory_is_all_zeros.patch)
download | inline diff:
From 7df597359510fd2d1aa12f74a4082643237c8452 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email protected]>
Date: Fri, 1 Nov 2024 11:46:29 +0000
Subject: [PATCH v2] 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 | 34 +++++++++++++++++++++++++++---
2 files changed, 32 insertions(+), 15 deletions(-)
20.3% src/backend/storage/page/
79.6% src/include/utils/
diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index be6f1f62d2..aa264f61b9 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_memory_is_all_zeros(pagebytes, BLCKSZ))
return true;
/*
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index 3590c8bad9..15bf32336e 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -192,17 +192,45 @@ 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)));
+
+ /* Compare bytes, byte by byte, until the pointer "p" is aligned */
+ while (((uintptr_t) p & (sizeof(size_t) - 1)) != 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;
+ }
- for (size_t i = 0; i < len; i++)
+ /* Compare remaining bytes, byte by byte */
+ while (p < end)
{
- if (p[i] != 0)
+ if (*p++ != 0)
return false;
}
+
return true;
}
--
2.34.1
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-05 12:49 Ranier Vilela <[email protected]>
parent: David Rowley <[email protected]>
1 sibling, 0 replies; 36+ messages in thread
From: Ranier Vilela @ 2024-11-05 12:49 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Michael Paquier <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
Em ter., 5 de nov. de 2024 às 00:23, David Rowley <[email protected]>
escreveu:
> On Tue, 5 Nov 2024 at 06:39, Ranier Vilela <[email protected]> wrote:
> > 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))
>
> 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
>
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
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-05 12:53 Ranier Vilela <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Ranier Vilela @ 2024-11-05 12:53 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: David Rowley <[email protected]>; Bertrand Drouvot <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
Em ter., 5 de nov. de 2024 às 01:12, Michael Paquier <[email protected]>
escreveu:
> On Tue, Nov 05, 2024 at 04:23:34PM +1300, David Rowley wrote:
> > 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 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
> > size_t: done in 531700 nanoseconds (3.6545 times faster than char)
> > char: done in 1957200 nanoseconds
>
> I am not seeing numbers as good as yours, but the winner is clear as
> well here:
>
Thanks for testing.
>
> $ 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.
>
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
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-05 15:03 Bertrand Drouvot <[email protected]>
parent: David Rowley <[email protected]>
1 sibling, 1 reply; 36+ messages in thread
From: Bertrand Drouvot @ 2024-11-05 15:03 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Ranier Vilela <[email protected]>; Michael Paquier <[email protected]>; Peter Smith <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
Hi,
On Tue, Nov 05, 2024 at 05:08:41PM +1300, David Rowley wrote:
> On Tue, 5 Nov 2024 at 06:39, Ranier Vilela <[email protected]> wrote:
> > 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;
> }
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,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-06 18:34 Peter Eisentraut <[email protected]>
parent: Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Peter Eisentraut @ 2024-11-06 18:34 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; David Rowley <[email protected]>; +Cc: Ranier Vilela <[email protected]>; Michael Paquier <[email protected]>; Peter Smith <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On 05.11.24 16:03, Bertrand Drouvot wrote:
> On Tue, Nov 05, 2024 at 05:08:41PM +1300, David Rowley wrote:
>> On Tue, 5 Nov 2024 at 06:39, Ranier Vilela <[email protected]> wrote:
>>> 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;
>> }
>
> 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.
Speaking of which, couldn't you just use
pg_popcount(ptr, len) == 0
?
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-06 19:05 David Rowley <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: David Rowley @ 2024-11-06 19:05 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Ranier Vilela <[email protected]>; Michael Paquier <[email protected]>; Peter Smith <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On Thu, 7 Nov 2024 at 07:34, Peter Eisentraut <[email protected]> wrote:
> Speaking of which, couldn't you just use
>
> 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
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-07 00:45 Michael Paquier <[email protected]>
parent: David Rowley <[email protected]>
0 siblings, 1 reply; 36+ messages in thread
From: Michael Paquier @ 2024-11-07 00:45 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Bertrand Drouvot <[email protected]>; Ranier Vilela <[email protected]>; Peter Smith <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
On Thu, Nov 07, 2024 at 08:05:10AM +1300, David Rowley wrote:
> 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.
Didn't know this one either, thanks for the explanation.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 36+ messages in thread
* Re: define pg_structiszero(addr, s, r)
@ 2024-11-07 08:41 Bertrand Drouvot <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 36+ messages in thread
From: Bertrand Drouvot @ 2024-11-07 08:41 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: David Rowley <[email protected]>; Peter Eisentraut <[email protected]>; Ranier Vilela <[email protected]>; Peter Smith <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]
Hi,
On Thu, Nov 07, 2024 at 09:45:44AM +0900, Michael Paquier wrote:
> On Thu, Nov 07, 2024 at 08:05:10AM +1300, David Rowley wrote:
> > 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.
>
> Didn't know this one either, thanks for the explanation.
+1, thanks!
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 36+ messages in thread
end of thread, other threads:[~2024-11-07 08:41 UTC | newest]
Thread overview: 36+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-03-18 08:34 [PATCH v3] Add "Backpatch" regions in wait_event_names.txt Bertrand Drouvot <[email protected]>
2024-10-30 09:23 Re: define pg_structiszero(addr, s, r) Bertrand Drouvot <[email protected]>
2024-10-30 20:55 ` Re: define pg_structiszero(addr, s, r) Peter Smith <[email protected]>
2024-10-31 00:59 ` Re: define pg_structiszero(addr, s, r) Michael Paquier <[email protected]>
2024-10-31 05:52 ` Re: define pg_structiszero(addr, s, r) Bertrand Drouvot <[email protected]>
2024-10-31 06:17 ` Re: define pg_structiszero(addr, s, r) Michael Paquier <[email protected]>
2024-11-01 04:45 ` Re: define pg_structiszero(addr, s, r) David Rowley <[email protected]>
2024-11-01 06:27 ` Re: define pg_structiszero(addr, s, r) Michael Paquier <[email protected]>
2024-11-01 07:19 ` Re: define pg_structiszero(addr, s, r) David Rowley <[email protected]>
2024-11-01 06:33 ` Re: define pg_structiszero(addr, s, r) Bertrand Drouvot <[email protected]>
2024-11-01 07:14 ` Re: define pg_structiszero(addr, s, r) Michael Paquier <[email protected]>
2024-11-01 07:27 ` Re: define pg_structiszero(addr, s, r) David Rowley <[email protected]>
2024-11-01 07:21 ` Re: define pg_structiszero(addr, s, r) David Rowley <[email protected]>
2024-11-01 07:36 ` Re: define pg_structiszero(addr, s, r) Michael Paquier <[email protected]>
2024-11-01 07:44 ` Re: define pg_structiszero(addr, s, r) Bertrand Drouvot <[email protected]>
2024-11-01 07:49 ` Re: define pg_structiszero(addr, s, r) Michael Paquier <[email protected]>
2024-11-01 08:47 ` Re: define pg_structiszero(addr, s, r) David Rowley <[email protected]>
2024-11-01 12:50 ` Re: define pg_structiszero(addr, s, r) Bertrand Drouvot <[email protected]>
2024-11-04 09:50 ` Re: define pg_structiszero(addr, s, r) Bertrand Drouvot <[email protected]>
2024-11-04 11:24 ` Re: define pg_structiszero(addr, s, r) David Rowley <[email protected]>
2024-11-04 17:17 ` Re: define pg_structiszero(addr, s, r) Bertrand Drouvot <[email protected]>
2024-11-04 17:38 ` Re: define pg_structiszero(addr, s, r) Ranier Vilela <[email protected]>
2024-11-05 03:23 ` Re: define pg_structiszero(addr, s, r) David Rowley <[email protected]>
2024-11-05 04:12 ` Re: define pg_structiszero(addr, s, r) Michael Paquier <[email protected]>
2024-11-05 12:53 ` Re: define pg_structiszero(addr, s, r) Ranier Vilela <[email protected]>
2024-11-05 12:49 ` Re: define pg_structiszero(addr, s, r) Ranier Vilela <[email protected]>
2024-11-05 04:08 ` Re: define pg_structiszero(addr, s, r) David Rowley <[email protected]>
2024-11-05 05:49 ` Re: define pg_structiszero(addr, s, r) Bertrand Drouvot <[email protected]>
2024-11-05 15:03 ` Re: define pg_structiszero(addr, s, r) Bertrand Drouvot <[email protected]>
2024-11-06 18:34 ` Re: define pg_structiszero(addr, s, r) Peter Eisentraut <[email protected]>
2024-11-06 19:05 ` Re: define pg_structiszero(addr, s, r) David Rowley <[email protected]>
2024-11-07 00:45 ` Re: define pg_structiszero(addr, s, r) Michael Paquier <[email protected]>
2024-11-07 08:41 ` Re: define pg_structiszero(addr, s, r) Bertrand Drouvot <[email protected]>
2024-11-05 04:31 ` Re: define pg_structiszero(addr, s, r) Michael Paquier <[email protected]>
2024-11-05 05:59 ` Re: define pg_structiszero(addr, s, r) Bertrand Drouvot <[email protected]>
2024-11-01 15:01 ` Re: define pg_structiszero(addr, s, r) Japin Li <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox