From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Jim Nasby <Jim.Nasby@BlueTreble.com>
To: Petr Jelinek <petr@2ndquadrant.com>
To: pgsql-hackers@postgreSQL.org
Subject: Re: PATCH: two slab-like memory allocators
Date: Sun, 2 Oct 2016 02:34:12 +0200
Message-ID: <e400bfa9-bec6-0760-4e92-6416c605a001@2ndquadrant.com> (raw)
In-Reply-To: <e2f2e9ac-e13b-5d1b-b3c1-1799f86024bc@BlueTreble.com>
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>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, Jim.Nasby@BlueTreble.com, petr@2ndquadrant.com
Subject: Re: PATCH: two slab-like memory allocators
In-Reply-To: <e400bfa9-bec6-0760-4e92-6416c605a001@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox