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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
