impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3200: Implement suballocator for splitting buffers
Date Mon, 05 Dec 2016 18:00:42 GMT
Jim Apple has posted comments on this change.

Change subject: IMPALA-3200: Implement suballocator for splitting buffers

Patch Set 11:

Commit Message:

Line 14: larger chunks. This helps avoid external fragmentation and is quite
> I really really really really don't want "implement a general-purpose memor
I think there are a few ways to get both what I am concerned about (buddy allocator internal
fragmentation reduction) and what you want (a simple allocator).

One way to is static_assert that sizeof(HashTable::Bucket) is a power of 2. I think this will
be true in the status quo.

Another is to add a parameter to the constructor of the Suballocator that sets the size of
an Atom. All blocks would be of size 2^i * sizeof(Atom), and the logic would remain the same.
Setting Atom = HashTable::Bucket would reduce internal fragmentation.
File be/src/bufferpool/

Line 107:   static void ExpectReservationUnused(ReservationTracker& reservation) {
What happened to the const?
File be/src/bufferpool/suballocator.h:

Line 171: /// An allocation made by a Suballocator. Each allocation returned by Suballocator
> But why shouldn't we use it for a different case where it's the right tool?
Just so I'm clear, in the current system, a Suballocation may have several pointers to it,
but the only unique_ptr is:

1. in the free_lists_ if it is free and the head of its free list

2. In the Suballocation before it if it is free but not the head of its free list

3. In its left child if it is not free and already split.

4. In some client code if it is not free and not split.

Additionally, at the function boundaries of the methods of Suballocator and Suballocation,
there is always exactly one unique_ptr to each Suballocation.

Did I get those right?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8bfe0e429f67ad273f7c7d0816703a9e6c3da788
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message