Dan Hecht has posted comments on this change. Change subject: IMPALA-3203: Part 2: per-core free lists in buffer pool ......................................................................
Patch Set 13: (17 comments) mostly just comments about the headers so far. also, please rebase on the eviction policy fix. http://gerrit.cloudera.org:8080/#/c/6414/11/be/src/runtime/bufferpool/buffer-allocator.cc File be/src/runtime/bufferpool/buffer-allocator.cc: PS11, Line 109: capacity is this in number of buffers, or bytes? PS11, Line 127: clean_pages; indicate this on other members also protected by this lock. http://gerrit.cloudera.org:8080/#/c/6414/13/be/src/runtime/bufferpool/buffer-allocator.cc File be/src/runtime/bufferpool/buffer-allocator.cc: Line 35: static int64_t FREE_LIST_MAX_BYTES = 1024L * 1024L * 1024L; how are these chosen? PS13, Line 94: BufferSize PerSizeLists? http://gerrit.cloudera.org:8080/#/c/6414/13/be/src/runtime/bufferpool/buffer-allocator.h File be/src/runtime/bufferpool/buffer-allocator.h: Line 31: /// released to SystemAllocator. buffers allocation lengths requested from this class must be a power of 2, right? let's be explicit about the requirements. it's implied by bufferpool, I suppose, but let's be clear here since it's leveraged here. also let's be explicit that this allocator is used internally by the buffer pool. i.e. it is not a public interface outside of bufferpool. also would be good to explain this class' relationship with bufferpool given their tight coupling. i.e. this is not just "a buffer allcoator". this is *the* buffer allocator used by the buffer pool. Line 44: /// system error occurs (e.g. we can't allocate all of the required memory from the OS). what about allocations beyond the limit? are they guaranteed to fail, or are they best effort? PS13, Line 50: len required to be power of 2? PS13, Line 74: it the buffer? PS13, Line 81: free what does that mean? free back to the SystemAllocator? PS13, Line 108: 'min_buffer_len' hard to imagine what this input means and how it affects the result without looking at the code. PS13, Line 115: 'len' what is that? PS13, Line 122: The buffers are freed with 'system_allocator_' why would memory that comes from 'buffer_bytes_remaining_' need to be freed with the system allocator? I guess I'm not sure what scavenge mean in this context. PS13, Line 135: we support allocating that can be allocated. i.e. i assume it's not just unsupported, it wouldn't work. PS13, Line 138: we support allocating. same Line 159: static const int MAX_SCAVENGE_ATTEMPTS = 3; what happens after this number of retries? Line 164: }; how do we (or will we) expose whatever needs to be exposed from this (and buffer-pool itself) for debugging? metrics? http://gerrit.cloudera.org:8080/#/c/6414/13/be/src/runtime/bufferpool/buffer-pool-counters.h File be/src/runtime/bufferpool/buffer-pool-counters.h: Line 28: /// Amount of time spent trying to allocate a buffer. there are multiple levels of "allocate" in the bufferpool subsystem. are these talking about top level BufferPool::AllocateBuffer() or SystemAllocator, or somewhere between? -- 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: 13 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
