Re: Add palloc_aligned() to allow arbitrary power of 2 memory alignment

Thanks for having a look.

On Tue, 8 Nov 2022 at 05:24, Andres Freund <andres(at)anarazel(dot)de> wrote:
> FWIW, to me this is really more part of mcxt.c than its own
> allocator... Particularly because MemoryContextAllocAligned() et al are
> implemented there.

I'm on the fence about this one. I thought it was nice that we have a
file per consumed MemoryContextMethodID. The thing that caused me to
add alignedalloc.c was the various other comments in the declaration
of mcxt_methods[] that mention where to find each of the methods being
assigned in that array (e.g /* aset.c */). I can change it back if
you feel strongly. I just don't.

> I doubtthere's ever a need to realloc such a pointer? Perhaps we could just
> elog(ERROR)?

Are you maybe locked into just thinking about your current planned use
case that we want to allocate BLCKSZ bytes in each case? It does not
seem impossible to me that someone will want something more than an
8-byte alignment and also might want to enlarge the allocation at some
point. I thought it might be more dangerous not to implement repalloc.
It might not be clear to someone using palloc_aligned() that there's
no code path that can call repalloc on the returned pointer.

> Hm, not that I can see a case for ever not using a power of two
> alignment... There's not really a "need" for the restriction, right? Perhaps
> we should note that?

TYPEALIGN() will not work correctly unless the alignment is a power of
2. We could modify it to, but that would require doing some modular
maths instead of bitmasking. That seems pretty horrible when the macro
is given a value that's not constant at compile time as we'd end up
with a (slow) divide in the code path. I think the restriction is a
good idea. I imagine there will never be any need to align to anything
that's not a power of 2.

> Should we handle the case where we get a suitably aligned pointer from
> MemoryContextAllocExtended() differently?

Maybe it would be worth the extra check. I'm trying to imagine future
use cases. Maybe if someone wanted to ensure that we're aligned to
CPU cache line boundaries then the chances of the pointer already
being aligned to 64 bytes is decent enough. The problem is it that
it's too late to save any memory, it just saves a bit of boxing and
unboxing of the redirect headers.

> > + /* XXX: should we adjust valgrind state here? */
>
> Probably still need to do this... Kinda hard to get right without the code
> getting exercised.

Yeah, that comment kept catching my eye. I agree. That should be
handled correctly. I'll work on that.

> Wonder if there's some minimal case we could actually use
> it for?

Is there anything we could align to CPU cacheline size that would
speed something up?

David