Subject: Re: PATCH: two slab-like memory allocators
To: Tomas Vondra <tomas.vondra@2ndquadrant.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>
From: Jim Nasby <Jim.Nasby@BlueTreble.com>
Message-ID: <e2f2e9ac-e13b-5d1b-b3c1-1799f86024bc@BlueTreble.com>
Date: Sat, 1 Oct 2016 18:53:16 -0500
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0)
 Gecko/20100101 Thunderbird/45.3.0
MIME-Version: 1.0
In-Reply-To: <03110c11-ea72-7b4d-027d-6f19eb300b2a@2ndquadrant.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Received-SPF: None (protection.outlook.com: bluetreble.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Oct 2016 23:53:21.9892 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR11MB1643
X-Pg-Spam-Score: -1.9 (-)
List-Archive: <http://archives.postgresql.org/pgsql-hackers>
List-Help: <mailto:majordomo@postgresql.org?body=help>
List-ID: <pgsql-hackers.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@postgresql.org>
List-Post: <mailto:pgsql-hackers@postgresql.org>
List-Subscribe: <mailto:majordomo@postgresql.org?body=sub%20pgsql-hackers>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
X-Mailing-List: pgsql-hackers
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

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