Message-ID: <7c3224111d9edcc16e8e3c8242de5a82c4ee2a73.camel@j-davis.com>
Subject: Re: Reduce TupleHashEntryData struct size by half
From: Jeff Davis <pgsql@j-davis.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>, pgsql-hackers@postgresql.org
Date: Mon, 18 Nov 2024 12:22:28 -0800
In-Reply-To: <0f3e7407-fb19-42c0-ad10-46eace6c1399@iki.fi>
References: <817d244237878cebdff0bc363718feaf49a1ea7d.camel@j-davis.com>
	 <0f3e7407-fb19-42c0-ad10-46eace6c1399@iki.fi>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/7c3224111d9edcc16e8e3c8242de5a82c4ee2a73.camel%40j-davis.com>
Precedence: bulk

On Mon, 2024-11-18 at 12:13 +0200, Heikki Linnakangas wrote:
> Seems pretty uncontroversial. You removed the typedef for struct=20
> TupleHashEntryData, which is a bit unusual for our usual source
> style.=20
> Was there a reason for that?

If it's private to a file and I don't intend to use it a lot, I do it
to cut down typedefs.list bloat. I'll go ahead and add the typedef back
to match the style, though.

>=20
> Hmm, it would seem more straightforward to store it in the beginning,
> i.e. have something like this:
>=20
> struct {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 void *additional;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 MinimalTupleData mtup;
> } ;

That was my first approach, but it requires an additional memcpy,
because ExecCopySlotMinimalTuple() does it's own palloc. I used
repalloc() because it will often have space at the end of the chunk
anyway, and not need to memcpy(). Maybe that's not significant but it
did seem detectable in some perf tests.

But perhaps we can go further and get rid of the "additional" pointer
and inline the pergroup data and the grouping key tuple into the same
palloc chunk? That would cut out a palloc and the 8 wasted bytes on the
pointer.

> Come to think of it, how important is it that we use MinimalTuple
> here=20
> at all? Some other representation could be faster to deal with in=20
> TupleHashTableMatch() anyway.

What did you have in mind? That sounds like a good idea orthogonal to
reducing the bucket size.

Alternatively, MinimalTuple is not very "minimal", and perhaps we can
just make it better.

>=20
> > 0004: Removes the "status" field from TupleHashEntryData, using
> > firstTuple=3D=3DNULL to mean "empty", otherwise "in use". Hack: need an
> > additional "special" pointer value to mean "input slot" now that
> > NULL
> > means "empty".
>=20
> +1

For the FIRSTTUPLE_INPUTSLOT marker, do you think it's cleaner to use
what I did:

  const static MinimalTuple FIRSTTUPLE_INPUTSLOT =3D (MinimalTuple) 0x1;

or something like:

  static MinimalTupleData dummy =3D {0};
  const static MinimalTuple FIRSTTUPLE_INPUTSLOT =3D &dummy;

?

>=20
> On architectures with non-strict alignment, it doesn't matter as a=20
> simple load/store instruction is the fastest option anyway.

My intuition is that the cost of dereferencing that pointer (to memory
which is not expected to be in cache) is going to be way higher than
the cost of a couple extra instructions to do the unaligned access.

Regards,
	Jeff Davis