Tim Armstrong has posted comments on this change. Change subject: IMPALA-3203: Part 2: per-core free lists in buffer pool ......................................................................
Patch Set 13: (15 comments) 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? Pretty arbitrarily, I was just guestimating at what a reasonable upper bound to the amount of memory to hold onto was. Potentially we don't need these and could just rely on scavenging/maintenance to shrink the lists if they get too large. PS13, Line 94: BufferSize > PerSizeLists? Works for me. I struggled to come up with a good name since it's more of a grouping of data structures than an abstraction. 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, Done 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 ar Done PS13, Line 50: len > required to be power of 2? Done PS13, Line 74: it > the buffer? Done PS13, Line 81: free > what does that mean? free back to the SystemAllocator? Tried to clarify. Not sure if the level of detail is appropriate for this comment. PS13, Line 108: 'min_buffer_len' > hard to imagine what this input means and how it affects the result without Done PS13, Line 115: 'len' > what is that? Done PS13, Line 122: The buffers are freed with 'system_allocator_' > why would memory that comes from 'buffer_bytes_remaining_' need to be freed That part was confusing. Reworded to make it clearer that there are two different strategies to come up with the memory. The code works out simpler to implement all the strategies in this method, but I guess it makes the name ScavengeBuffers less accurate. I'm not sure what a better alternative mightbe. PS13, Line 135: we support allocating > that can be allocated. i.e. i assume it's not just unsupported, it wouldn't Done PS13, Line 138: we support allocating. > same Done Line 159: static const int MAX_SCAVENGE_ATTEMPTS = 3; > what happens after this number of retries? elaborated in the comment. Line 164: }; > how do we (or will we) expose whatever needs to be exposed from this (and b Most of the perf counters are client-centric, so we will get a lot of info in the profiles for debugging query performance. We'll also get information through the MemTracker hierarchy (you can see used/reserved). We have DebugString() here too that dumps out the current state of the allocator when an allocation unexpectedly fails. I have some basic memory metrics (bytes in use, etc) in a follow-on patch that also switches to using mmap(). It would be easy enough to add more global metrics but it's unclear to me what would be actually useful for debugging. E.g. total #/bytes of allocations and total #/bytes of buffers released to the system might be marginally useful. 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 th Done -- 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
