Re: Reducing the chunk header sizes on all memory context types

On Wed, 10 Aug 2022 at 06:44, Andres Freund <andres(at)anarazel(dot)de> wrote:
> I think it's fine, given that we can change this at any time, but it's
> probably worth to explicitly agree that this will for now restrict us to 8
> context methods?

I know there was some discussion about this elsewhere in this thread
about 8 possibly not being enough, but that seems to have concluded
with we'll just make more space if we ever need to.

To make that easier, I've adapted the code in memutils_memorychunk.h
to separate out the max values for the block offset from the max value
for the chunk size. The chunk size is the one we'd likely want to
lower if we ever needed more bits. I think this also helps document
the maxBlockSize limitations in aset.c and generation.c.

> > +/*
> > + * MemoryContextMethodID
> > + * A unique identifier for each MemoryContext implementation which
> > + * indicates the index into the mcxt_methods[] array. See mcxt.c.
> > + */
> > +typedef enum MemoryContextMethodID
> > +{
> > + MCTX_ASET_ID = 0,
>
> Is there a reason to reserve 0 here? Practically speaking the 8-byte header
> will always contain not just zeroes, but I don't think the patch currently
> enforces that. It's probably not worth wasting a "valuable" entry here...

The code was just being explicit about that being set to 0. I'm not
really sure I see this as reserving 0. I've removed the = 0 anyway
since it wasn't really doing anything useful.

> > + * Although MemoryChunks are used by each of our MemoryContexts, other
> > + * implementations may choose to implement their own method for storing chunk

> Well, there can't be other implementations other than ours. So maybe phrase it
> as "future implementations"?

Yeah, that seems better.

> > + * 1. 3-bits to indicate the MemoryContextMethodID
> > + * 2. 1-bit to indicate if the chunk is externally managed (see below)
> > + * 3. 30-bits for the amount of memory which was reserved for the chunk
> > + * 4. 30-bits for the number of bytes that must be subtracted from the chunk
> > + * to obtain the address of the block that the chunk is stored on.

> Hm. So really only the first four bits have eactly that layout, correct?
> Perhaps that could be clarified somehow?

I've clarified that #3 and #4 are unused in external chunks.

> > +#define MEMORYCHUNK_EXTERNAL_BIT (1 << 3)
>
> We should probably have a define for the three bits reserved for the context
> id, likely in _internal.h

I've added both MEMORY_CONTEXT_METHODID_BITS and
MEMORY_CONTEXT_METHODID_MASK and tidied up the defines in
memutils_memorychunk.h so that they'll follow on from whatever
MEMORY_CONTEXT_METHODID_BITS is set to.

> > +typedef struct AllocFreelistLink
> > +{
> > + MemoryChunk *next;
> > +} AllocFreelistLink;
>
> I know we have no agreement on that, but I personally would add
> AllocFreelistLink to typedefs.list and then re-pgindent ;)

I tend to leave that up to the build farm to generate. I really wasn't
sure which should sort first out of the following:

MemoryContextMethods
MemoryContextMethodID

The correct answer depends on if the sort is case-sensitive or not. I
imagine it is since it is in C, but don't really know if the buildfarm
will generate the same.

I've added them in the above order now.

> Hm. We don't mark the chunk header as noaccess anymore? If so, why? I realize
> it'd be a bit annoying because there's plenty places that look at it, but I
> think it's also a good way to catch errors.

I don't think I've really changed anything here. If I understand
correctly the pointer to the MemoryContext was not marked as NOACCESS
before. I guessed that's because it's accessed outside of aset.c. I've
kept that due to how the 3 lower bits are still accessed outside of
aset.c. It's just that we're stuffing more information into that
8-byte variable now.

> > +static const MemoryContextMethods mcxt_methods[] = {
...
> Mildly wondering whether we ought to use designated initializers instead,
> given we're whacking it around already. Too easy to get the order wrong when
> adding new members, and we might want to have optional callbacks too.

I've adjusted how this array is initialized now.

I've attached version 3 of the patch.

Thanks for having a look at this.

David