Re: PATCH: two slab-like memory allocators - Mailing list pgsql-hackers

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