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) */
+    /* 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

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
+     * 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.


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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to