On Sun, Feb 12, 2023 at 11:01 PM Andres Freund <and...@anarazel.de> wrote:
>
> On 2023-02-12 19:59:00 +0530, Bharath Rupireddy wrote:
> > Thanks for looking at it. We know that we don't change the zbuffer in
> > the function, so can we avoid static const and have just a static
> > variable, like the attached
> > v1-0001-Use-static-variable-to-avoid-memset-calls-in-pg_p.patch? Do
> > you see any problem with it?
>
> Making it static const is better, because it allows the memory for the
> variable to be put in a readonly section.
Okay.
> > /* Prepare to write out a lot of copies of our zero buffer at once. */
> > for (i = 0; i < lengthof(iov); ++i)
> > {
> > - iov[i].iov_base = zbuffer.data;
> > + iov[i].iov_base = (void *) (unconstify(PGAlignedBlock *,
> > &zbuffer)->data);
> > iov[i].iov_len = zbuffer_sz;
> > }
>
> Another thing: I think we should either avoid iterating over all the IOVs if
> we don't need them, or, even better, initialize the array as a constant, once.
How about like the attached patch? It makes the iovec static variable
and points the zero buffer only once/for the first time to iovec. This
avoids for-loop on every call.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From 5533330b606affb614980a4b814d9e44e73d5489 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Mon, 13 Feb 2023 04:25:34 +0000
Subject: [PATCH v1] Optimize zero buffer handling in pg_pwrite_zeros()
1. Use static const zero-filled buffer to avoid memset-ing on
every call.
2. Use static iovec and point the zero-filled buffer for the
first time through to avoid for loop initializing iovec on every
call.
Reported-by: Andres Freund
Author: Bharath Rupireddy
Discussion: https://www.postgresql.org/message-id/20230211224424.r25uw4rsv6taukxk%40awork3.anarazel.de
---
src/common/file_utils.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
index 4dae534152..08688eb1b1 100644
--- a/src/common/file_utils.c
+++ b/src/common/file_utils.c
@@ -539,25 +539,35 @@ pg_pwritev_with_retry(int fd, const struct iovec *iov, int iovcnt, off_t offset)
ssize_t
pg_pwrite_zeros(int fd, size_t size)
{
- PGAlignedBlock zbuffer; /* worth BLCKSZ */
- size_t zbuffer_sz;
- struct iovec iov[PG_IOV_MAX];
+ /*
+ * Zero-filled buffer worth BLCKSZ. We use static variable to avoid zeroing
+ * the buffer on every call. Also, with the const qualifier, it is ensured
+ * that the buffer is placed in read-only section of the process's memory.
+ */
+ static const PGAlignedBlock zbuffer = {0};
+ static size_t zbuffer_sz = BLCKSZ;
+ static bool first_time = true;
+ static struct iovec iov[PG_IOV_MAX] = {0};
int blocks;
size_t remaining_size = 0;
int i;
ssize_t written;
ssize_t total_written = 0;
- zbuffer_sz = sizeof(zbuffer.data);
-
- /* Zero-fill the buffer. */
- memset(zbuffer.data, 0, zbuffer_sz);
-
- /* Prepare to write out a lot of copies of our zero buffer at once. */
- for (i = 0; i < lengthof(iov); ++i)
+ /*
+ * If first time through, point the zero buffer to iovec structure. This
+ * avoids for loop on every call.
+ */
+ if (first_time)
{
- iov[i].iov_base = zbuffer.data;
- iov[i].iov_len = zbuffer_sz;
+ /* Prepare to write out a lot of copies of our zero buffer at once. */
+ for (i = 0; i < lengthof(iov); ++i)
+ {
+ iov[i].iov_base = (void *) (unconstify(PGAlignedBlock *, &zbuffer)->data);
+ iov[i].iov_len = zbuffer_sz;
+ }
+
+ first_time = false;
}
/* Loop, writing as many blocks as we can for each system call. */
--
2.34.1
