impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <ger...@cloudera.org>
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:

(24 comments)

http://gerrit.cloudera.org:8080/#/c/6410/6/be/src/benchmarks/free-lists-benchmark.cc
File be/src/benchmarks/free-lists-benchmark.cc:

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


PS6, Line 288: MAX_LIST_ENTRIES
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);
const


Line 405:         for (int num_threads : {1, 2, 4, CpuInfo::num_cores()}) {
Is it worth trying 2*num_cores?


http://gerrit.cloudera.org:8080/#/c/6410/6/be/src/runtime/bufferpool/free-list-test.cc
File be/src/runtime/bufferpool/free-list-test.cc:

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: http://en.cppreference.com/w/cpp/language/copy_elision


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
LIST_SIZE


http://gerrit.cloudera.org:8080/#/c/6410/6/be/src/runtime/bufferpool/free-list.h
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
minheap


PS6, Line 115: Limited to 'max_capacity_' capacity
Obsolete.


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 http://gerrit.cloudera.org:8080/6410
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia89acfa4efdecb96d3678443b4748932b4133b9b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message