From: Jim Nasby <Jim.Nasby@BlueTreble.com>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Petr Jelinek <petr@2ndquadrant.com>
To: pgsql-hackers@postgreSQL.org
Subject: Re: PATCH: two slab-like memory allocators
Date: Sat, 1 Oct 2016 18:53:16 -0500
Message-ID: <e2f2e9ac-e13b-5d1b-b3c1-1799f86024bc@BlueTreble.com> (raw)
In-Reply-To: <03110c11-ea72-7b4d-027d-6f19eb300b2a@2ndquadrant.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>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
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 */
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.
+ /* otherwise add it to the proper freelist bin */
Looks like something went missing... :)
Should zeroing the block in SlabAlloc be optional like it is with
palloc/palloc0?
+ /*
+ * 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.
+ 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.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461
--
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: Jim.Nasby@BlueTreble.com, tomas.vondra@2ndquadrant.com, petr@2ndquadrant.com
Subject: Re: PATCH: two slab-like memory allocators
In-Reply-To: <e2f2e9ac-e13b-5d1b-b3c1-1799f86024bc@BlueTreble.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