Subject: Re: PATCH: two slab-like memory allocators
To: Jim Nasby <Jim.Nasby@BlueTreble.com>, Petr Jelinek
 <petr@2ndquadrant.com>, pgsql-hackers@postgreSQL.org
References: <d15dff83-0b37-28ed-0809-95a5cc7292ad@2ndquadrant.com>
 <c5653004-fe91-5a13-409c-db9f34642b0c@2ndquadrant.com>
 <68b837a3-ce55-f9a2-f685-9eafac536f21@2ndquadrant.com>
 <885540a7-ac72-5a21-6dae-a4d5b50672da@2ndquadrant.com>
 <03110c11-ea72-7b4d-027d-6f19eb300b2a@2ndquadrant.com>
 <e2f2e9ac-e13b-5d1b-b3c1-1799f86024bc@BlueTreble.com>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <e400bfa9-bec6-0760-4e92-6416c605a001@2ndquadrant.com>
Date: Sun, 2 Oct 2016 02:34:12 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:46.0) Gecko/20100101
 Thunderbird/46.0a2
MIME-Version: 1.0
In-Reply-To: <e2f2e9ac-e13b-5d1b-b3c1-1799f86024bc@BlueTreble.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

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


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