Make use of pg_memory_is_all_zeros() in more places
Hi hackers,
While searching for memcmp() calls in "*stat*.c" files (due to [1]/messages/by-id/Z1hNLvcPgVLPxCoc@ip-10-97-1-34.eu-west-3.compute.internal), it appeared
that we could $SUBJECT. Please find attached a patch doing so.
While at it, I did a quick check on all the memcmp() calls (even those not in
"*stat*.c" files) and it looks to me that there is no others that could be replaced
with pg_memory_is_all_zeros().
[1]: /messages/by-id/Z1hNLvcPgVLPxCoc@ip-10-97-1-34.eu-west-3.compute.internal
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Make-use-of-pg_memory_is_all_zeros-in-more-places.patchtext/x-diff; charset=us-asciiDownload
From 9b58b74506e27f2cd3a0e7d00fa1c32c7aaf2212 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Tue, 10 Dec 2024 11:01:40 +0000
Subject: [PATCH v1] Make use of pg_memory_is_all_zeros() in more places
Some places are using a memset()/memcmp() combination to check that a structure
contains all zeros: make use of pg_memory_is_all_zeros() instead.
---
src/backend/utils/adt/pgstatfuncs.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
100.0% src/backend/utils/adt/
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 60a397dc56..089c8ad965 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -361,7 +361,6 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
/* Values only available to role member or pg_read_all_stats */
if (HAS_PGSTAT_PERMISSIONS(beentry->st_userid))
{
- SockAddr zero_clientaddr;
char *clipped_activity;
switch (beentry->st_state)
@@ -483,9 +482,8 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
nulls[11] = true;
/* A zeroed client addr means we don't know */
- memset(&zero_clientaddr, 0, sizeof(zero_clientaddr));
- if (memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
- sizeof(zero_clientaddr)) == 0)
+ if (pg_memory_is_all_zeros(&(beentry->st_clientaddr),
+ sizeof(beentry->st_clientaddr)))
{
nulls[12] = true;
nulls[13] = true;
@@ -880,7 +878,6 @@ pg_stat_get_backend_client_addr(PG_FUNCTION_ARGS)
{
int32 procNumber = PG_GETARG_INT32(0);
PgBackendStatus *beentry;
- SockAddr zero_clientaddr;
char remote_host[NI_MAXHOST];
int ret;
@@ -891,9 +888,8 @@ pg_stat_get_backend_client_addr(PG_FUNCTION_ARGS)
PG_RETURN_NULL();
/* A zeroed client addr means we don't know */
- memset(&zero_clientaddr, 0, sizeof(zero_clientaddr));
- if (memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
- sizeof(zero_clientaddr)) == 0)
+ if (pg_memory_is_all_zeros(&(beentry->st_clientaddr),
+ sizeof(beentry->st_clientaddr)))
PG_RETURN_NULL();
switch (beentry->st_clientaddr.addr.ss_family)
@@ -925,7 +921,6 @@ pg_stat_get_backend_client_port(PG_FUNCTION_ARGS)
{
int32 procNumber = PG_GETARG_INT32(0);
PgBackendStatus *beentry;
- SockAddr zero_clientaddr;
char remote_port[NI_MAXSERV];
int ret;
@@ -936,9 +931,8 @@ pg_stat_get_backend_client_port(PG_FUNCTION_ARGS)
PG_RETURN_NULL();
/* A zeroed client addr means we don't know */
- memset(&zero_clientaddr, 0, sizeof(zero_clientaddr));
- if (memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
- sizeof(zero_clientaddr)) == 0)
+ if (pg_memory_is_all_zeros(&(beentry->st_clientaddr),
+ sizeof(beentry->st_clientaddr)))
PG_RETURN_NULL();
switch (beentry->st_clientaddr.addr.ss_family)
--
2.34.1
On Tue, Dec 10, 2024 at 02:18:33PM +0000, Bertrand Drouvot wrote:
While searching for memcmp() calls in "*stat*.c" files (due to [1]), it appeared
that we could $SUBJECT. Please find attached a patch doing so.
- SockAddr zero_clientaddr;
- memset(&zero_clientaddr, 0, sizeof(zero_clientaddr));
- if (memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
- sizeof(zero_clientaddr)) == 0)
+ if (pg_memory_is_all_zeros(&(beentry->st_clientaddr),
+ sizeof(beentry->st_clientaddr)))It also means that you're removing these variables used only for the
all-zero comparisons. Makes sense to me.
--
Michael
Hi,
On Wed, Dec 11, 2024 at 03:03:34PM +0900, Michael Paquier wrote:
On Tue, Dec 10, 2024 at 02:18:33PM +0000, Bertrand Drouvot wrote:
While searching for memcmp() calls in "*stat*.c" files (due to [1]), it appeared
that we could $SUBJECT. Please find attached a patch doing so.- SockAddr zero_clientaddr; - memset(&zero_clientaddr, 0, sizeof(zero_clientaddr)); - if (memcmp(&(beentry->st_clientaddr), &zero_clientaddr, - sizeof(zero_clientaddr)) == 0) + if (pg_memory_is_all_zeros(&(beentry->st_clientaddr), + sizeof(beentry->st_clientaddr)))It also means that you're removing these variables used only for the
all-zero comparisons. Makes sense to me.
Yeap and also given its size (136 bytes), it looks like that it could provide
some performance benefits too (see check_SockAddr_is_zeroes.c attached):
$ gcc --version
gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ gcc -O2 check_SockAddr_is_zeroes.c -o check_SockAddr_is_zeroes; ./check_SockAddr_is_zeroes
memcmp: done in 21460 nanoseconds
pg_memory_is_all_zeros: done in 1307 nanoseconds (16.4193 times faster than memcmp)
$ /usr/local/gcc-14.1.0/bin/gcc-14.1.0 -O2 check_SockAddr_is_zeroes.c -o check_SockAddr_is_zeroes; ./check_SockAddr_is_zeroes
memcmp: done in 21012 nanoseconds
pg_memory_is_all_zeros: done in 4039 nanoseconds (5.20228 times faster than memcmp)
That's not a hot path but still interesting to see.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
