Re: suspicious valgrind reports about radixtree/tidstore on arm64

On Thu, Jun 20, 2024 at 8:12 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

> On Thu, Jun 20, 2024 at 7:54 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >

> I agree with radixtree-fix-proposed.patch. Even if we zero more fields
> in the node it would not add noticeable overheads.

+1 in general, although I'm slightly concerned about this part:

> > (The RT_NODE_48 case could be optimized a bit if we cared
> > to swap the order of its slot_idxs[] and isset[] arrays;
> > then the initial zeroing could just go up to slot_idxs[].

- memset(n48->isset, 0, sizeof(n48->isset));
+ memset(n48, 0, offsetof(RT_NODE_48, children));
memset(n48->slot_idxs, RT_INVALID_SLOT_IDX, sizeof(n48->slot_idxs));

I was a bit surprised that neither gcc 14 nor clang 18 can figure out
that they can skip zeroing the slot index array since it's later
filled in with "invalid index", so they actually zero out 272 bytes
before re-initializing 256 of those bytes. It may not matter in
practice, but it's also not free, and trivial to avoid.

> > I don't know if there's any reason why the current order
> > is preferable.)
>
> IIUC there is no particular reason for the current order in RT_NODE_48.

Yeah. I found that simply swapping them enables clang to avoid
double-initialization, but gcc still can't figure it out and must be
told to stop at slot_idxs[]. I'd prefer to do it that way and document
that slot_idxs is purposefully the last member of the fixed part of
the struct. If that's agreeable I'll commit it that way tomorrow
unless someone beats me to it.