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
> > > <bertranddrouvot...@gmail.com> 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
>From 7df597359510fd2d1aa12f74a4082643237c8452 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
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
