MIME-Version: 1.0
References: <ZupUDDyf1hHI4ibn@ip-10-97-1-34.eu-west-3.compute.internal>
 <3023d29e-0fe7-4bec-9028-7f3c0102e437@eisentraut.org> <ZusioyiXTyKpOP/2@ip-10-97-1-34.eu-west-3.compute.internal>
In-Reply-To: <ZusioyiXTyKpOP/2@ip-10-97-1-34.eu-west-3.compute.internal>
From: Peter Smith <smithpb2250@gmail.com>
Date: Tue, 29 Oct 2024 09:36:34 +1100
Message-ID: <CAHut+PvHmWiPyqiDRnD7FYsc4QskXpKEZAH3Z8Ahd_GSnRXWrw@mail.gmail.com>
Subject: Re: define pg_structiszero(addr, s, r)
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: Peter Eisentraut <peter@eisentraut.org>, pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAHut%2BPvHmWiPyqiDRnD7FYsc4QskXpKEZAH3Z8Ahd_GSnRXWrw%40mail.gmail.com>
Precedence: bulk

On Thu, Sep 19, 2024 at 4:57=E2=80=AFAM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Wed, Sep 18, 2024 at 10:03:21AM +0200, Peter Eisentraut wrote:
> > On 18.09.24 06:16, Bertrand Drouvot wrote:
> > > +#define pg_structiszero(addr, s, r)                                 =
                                       \
> > > +   do {                                                             =
                                                       \
> > > +           /* We assume this initializes to zeroes */               =
                       \
> > > +           static const s all_zeroes;                               =
                                       \
> > > +           r =3D (memcmp(addr, &all_zeroes, sizeof(all_zeroes)) =3D=
=3D 0);       \
> > > +   } while (0)
> >
>
> Thanks for the feedback.
>
> > This assumption is kind of the problem, isn't it?  Because, you can't a=
ssume
> > that.  And the existing code is arguably kind of wrong.  But moreover, =
this
> > macro also assumes that the "addr" argument has no random padding bits.
> >
> > In the existing code, you can maybe make a local analysis that the code=
 is
> > working correctly, although I'm not actually sure.
>
> I think it is but will give it a closer look.
>
> >  But if you are
> > repackaging this as a general macro under a general-sounding name, then=
 the
> > requirements should be more stringent.
>
> Agree. That said in v2 ([1]), it has been renamed to pgstat_entry_all_zer=
os().
>
> I think that I will:
>
> 1/ take a closer look regarding the existing assumption
> 2/ if 1/ outcome is fine, then add more detailed comments around
> pgstat_entry_all_zeros() to make sure it's not used outside of the existi=
ng
> context
>
> Sounds good to you?
>
> [1]: https://www.postgresql.org/message-id/ZuqHLCdZXtEsbyb/%40ip-10-97-1-=
34.eu-west-3.compute.internal
>
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com
>

Hi, if this is performance critical then FWIW my understanding is that
memcmp can outperform simple loop checking, and my hacky testing
seemed to confirm that.

See https://godbolt.org/z/GWY1ob9bE

static inline bool
is_all_zeros2(const char *p, size_t len)
{
    static size_t nz =3D 0;
    static const char *z =3D NULL;

    if (nz < len)
    {
        if (z) free((void *)z);
        nz =3D len;
        z =3D (const char *)calloc(1, nz);
    }

    return memcmp(p, z, len) =3D=3D 0;
}

~~

Executor x86-64 gcc 14.2 (C, Editor #1)
Program stdout

Iterate 1000000 times...
check zeros using loop -- elapsed=3D0.041196s
check zeros using memcmp -- elapsed=3D0.016407s

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