Dan Hecht has posted comments on this change. Change subject: IMPALA-3203: Part 2: per-core free lists in buffer pool ......................................................................
Patch Set 15: (13 comments) Here's the remaining comments for this iteration. http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/common/init.cc File be/src/common/init.cc: PS15, Line 101: weird indentation and close parenthesis is missing. http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/runtime/bufferpool/buffer-pool-counters.h File be/src/runtime/bufferpool/buffer-pool-counters.h: PS15, Line 28: allocate buffers this makes it confusing because i'm not sure what "level" of allocation it's talking about. Is it just: Total amount of time spent inside BufferAllocator::AllocateBuffer(), or some subset of that? Line 36: RuntimeProfile::Counter* bytes_alloced; similarly, for these it's not obvious which level of "allocation" we're talking about. i.e. system or buffer-allocator or buffer-pool. let's be specific. http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/runtime/bufferpool/buffer-pool-internal.h File be/src/runtime/bufferpool/buffer-pool-internal.h: PS15, Line 48: BufferPool::clean_pages. shouldn't this be: in a FreeBufferArena clean_pages list or something? Line 77: #include "common/atomic.h" why here? doesn't look used by this header. http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/runtime/bufferpool/buffer-pool.cc File be/src/runtime/bufferpool/buffer-pool.cc: PS15, Line 375: clean up the write this and the fact that the method is called "CancelWriteAndRestoreData" makes it sound like the write might be in-flight. But from this path, it can't be, right? So, how about exposing it as just RestoreData(), and putting the handle->Cancel/WaitForWrite() in the buffered-block-mgr code, so that things are better lined up with the new code? PS15, Line 385: the : // earlier 'evicted' check. this no longer exists. http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/runtime/bufferpool/buffer-pool.h File be/src/runtime/bufferpool/buffer-pool.h: PS15, Line 247: free unneeded memory. Does this mean "release unneeded memory back to the system allocator" or something different? PS15, Line 273: ObjectPool obj_pool_; okay, but why did we move to obj_pool instead of the scoped_ptr? http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/runtime/disk-io-mgr.cc File be/src/runtime/disk-io-mgr.cc: PS15, Line 763: int idx = 0; idx < free_buffers_.size when we free everything, it probably doesn't matter which order we traverse the lists. Now that we might free only 'bytes_to_free' which order is the best? Is smallest to largest intentional? http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: PS15, Line 315: from cheapest to most expensive is that really the reason? we have multiple levels of GC, i.e. allocated back to tc_malloc, and then tc_malloc back to system. So doesn't that really dictate the order that we should run them? And since order does matter, this whole AddGcFunction() interface seems problematic. Why not just have mem-tracker statically call them in the right order so we can see exactly the order they are called? http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/util/cpu-info.h File be/src/util/cpu-info.h: PS15, Line 133: index what is this used for? does the index value have any significance or just to give a dense numbering within a numa node? PS15, Line 133: in delete -- To view, visit http://gerrit.cloudera.org:8080/6414 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I612bd1cd0f0e87f7d8186e5bedd53a22f2d80832 Gerrit-PatchSet: 15 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
