Re: PATCH: two slab-like memory allocators

On 10/02/2016 01:53 AM, Jim Nasby wrote:
> On 9/26/16 9:10 PM, Tomas Vondra wrote:
>> Attached is v2 of the patch, updated based on the review. That means:
>
> + /* make sure the block can store at least one chunk (with 1B for a
> bitmap)? */
> (and the comment below it)
>
> I find the question to be confusing... I think these would be better as
>
> + /* make sure the block can store at least one chunk (with 1B for a
> bitmap) */
> and
> + /* number of chunks can we fit into a block, including header and
> bitmap */
>

Thanks, will rephrase the comments a bit.

> I'm also wondering if a 1B freespace bitmap is actually useful. If
> nothing else, there should probably be a #define for the initial
> bitmap size.

That's not the point. The point is that we need to store at least one
entry per block, with one bit in a bitmap. But we can't store just a
single byte - we always allocate at least 1 byte. So it's more an
explanation for the "1" literal in the check, nothing more.

>
> + /* otherwise add it to the proper freelist bin */
> Looks like something went missing... :)
>

Ummm? The patch contains this:

+ /* otherwise add it to the proper freelist bin */
+ if (set->freelist[block->nfree])
+ set->freelist[block->nfree]->prev = block;
+
+ block->next = set->freelist[block->nfree];
+ set->freelist[block->nfree] = block;

Which does exactly the thing it should do. Or what is missing?

>
> Should zeroing the block in SlabAlloc be optional like it is with
> palloc/palloc0?
>

Good catch. The memset(,0) should not be in SlabAlloc() as all, as the
zeroing is handled in mctx.c, independently of the implementation.

> + /*
> + * If there are no free chunks in any existing block, create a new
> block
> + * and put it to the last freelist bucket.
> + */
> + if (set->minFreeCount == 0)
> Couldn't there be blocks that have a free count > minFreeCount? (In
> which case you don't want to just alloc a new block, no?)
>
> Nevermind, after reading further down I understand what's going on. I
> got confused by "we've decreased nfree for a block with the minimum
> count" until I got to the part that deals with minFreeCount = 0. I think
> it'd be helpful if the "we've decreased nfree" comment mentioned that if
> nfree is 0 we'll look for the correct value after updating the freelists.
>

Right. I think it'd be good to add an assert ensuring the minFreeCount
value matches the block freelist. Or at least SlabCheck() could check this.

> + block->bitmapptr[idx/8] |= (0x01 << (idx % 8));
> Did you consider using a pre-defined map of values to avoid the shift? I
> know I've seen that somewhere in the code...
>
> Patch 2...
>
> Doesn't GenSlabReset() need to actually free something? If the call
> magically percolates to the other contexts it'd be nice to note that in
> the comment.

No, the other contexts are created as children of GenSlab, so the memory
context infrastructure resets them automatically. I don't think this
needs to be mentioned in the comments - it's pretty basic part of the
parent-child context relationship.

Thanks!

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services