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-3203: Part 1: Free list implementation
Date Fri, 17 Mar 2017 13:44:27 GMT
Jim Apple has posted comments on this change.

Change subject: IMPALA-3203: Part 1: Free list implementation

Patch Set 6:

File be/src/benchmarks/

PS6, Line 48: per thread
It might be easier to read if it showed total throughput

PS6, Line 59: i7-4790
Do you have Hyperthreading on?

PS6, Line 61: 0 iters
It might help to explain what this is before these benchmarks

Line 257: 
elide this empty line

How was this chosen?

PS6, Line 303: *

Line 308:     lock_guard<SpinLock> l(free_list->first);
In the case where there is one list per thread, why bother with the lock?

Line 350:     int64_t op = uniform_int_distribution<int64_t>(0, 1)(rng);

Line 405:         for (int num_threads : {1, 2, 4, CpuInfo::num_cores()}) {
Is it worth trying 2*num_cores?
File be/src/runtime/bufferpool/

PS6, Line 18: c
also use <cstdint> in free-list.h or use <stdlib.h> here

PS6, Line 50: vector<BufferHandle>* buffers
You can just return this out parameter and the [N]RVO will make it efficient:

PS6, Line 58: void*
const void*

PS6, Line 103: various numbers
Only LIST_SIZE; no need for a loop

Line 112:       vector<void*> addrs = GetSortedAddrs(buffers);
const vector...

PS6, Line 118: small_list.Size() - LIST_SIZE
always 0?

PS6, Line 120: two
File be/src/runtime/bufferpool/free-list.h:

PS6, Line 50: This helps to consolidate buffers within the address space
Sorry, I don't think I understand this - how does it help consolidate buffers?

PS6, Line 60: Get
Getters are often const methods. Maybe 'PopFreeBuffer'?

PS6, Line 60: bool
Can you just return nullptr on failure instead?

Line 69:   void AddFreeBuffer(BufferHandle buffer) {
If this will always be a moved argument, you can give it type BufferHandle&& buffer

PS6, Line 81: int
free_list_.size() is likely stored as a 64-bit integer type and is below returned as a 64-bit
integer type.

PS6, Line 84: heap

PS6, Line 115: Limited to 'max_capacity_' capacity

Line 117:   std::vector<BufferHandle> free_list_;
Since you need a double-ended priority queue, did you consider using a std::set?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia89acfa4efdecb96d3678443b4748932b4133b9b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message