Dan Hecht has posted comments on this change. Change subject: IMPALA-3203: Part 2: per-core free lists in buffer pool ......................................................................
Patch Set 14: (24 comments) Haven't made it all the way, but probably worth getting this first set of comments out. http://gerrit.cloudera.org:8080/#/c/6414/14/be/src/runtime/bufferpool/buffer-allocator.cc File be/src/runtime/bufferpool/buffer-allocator.cc: PS14, Line 55: buffer page? or maybe "if a buffer for a clean page"? PS14, Line 57: Try why are these "Try", whereas PopFreeBuffer is not? Line 59: /// Free memory from this arena up to 'target_bytes'. Free memory back to the system (there are multiple levels of freeing/allocating, so helps to be explicit). PS14, Line 62: is returned delete PS14, Line 62: Returns If 'claim_memory' is true, returns ... PS14, Line 77: . what happens if claim_buffer is false? PS14, Line 79: bool another case where we don't have "Try". let's be consistent (either delete Try and let it be implied by bool return, or add "Try" to any that can fail to do it). PS14, Line 109: capacity is that in terms of buffer counts or bytes? PS14, Line 124: allocate reclaim? Line 156: int64_t upper_bound = buffer_bytes_limit == 0 ? 1L : 1L comment this: // Find largest power of 2 smaller than 'buffer_bytes_limit'. (assuming i got that right) PS14, Line 174: )); << min_buffer_len_ ? Line 216: } can either of these legitimately happen, or does it indicate a bug? PS14, Line 251: lock all the arenas on the : // last attempt to guarantee success. instead of expressing as locking all arenas, it might be clearer to express as "initial, but faster attempts" and "final but slower and guaranteed to succeed". i.e. lock_arenas could be renamed final_attempt or something. because it's kind of hard to see from here how the locking is going to work. Line 265: "attempts:\n$3", len, delta, max_scavenge_attempts_, pool_->DebugString())); this indicates an accounting bug, right? add a comment to that effect. Line 296: if (bytes_found == target_bytes) return bytes_found; i wonder if this code should be in the caller so that this routine is purely about scavenging buffers. how tightly coupled is this code to scavenging? Line 302: if (lock_arenas) arena_locks.resize(per_core_arenas_.size()); i don't understand what guarantee this locking provides. the reservations guarantee that this memory must be somewhere, right? PS14, Line 313: last does this mean "final" or "previous"? and why is this possible. i don't understand this comment or what the code that follows is doing, especially given that previous attempts may have partially fulfilled the target, yet this calculation wouldn't know about that. Line 347: return arena->RemoveCleanPage(claim_buffer, page); is there an ABA problem here where the page was clean in arena 0, then evicted, then repinned and recleaned against arena 1, and now we're looking at the old arena? or do we not care? Line 353: note to self: restart here Line 544: lists->clean_pages.Remove(page); combine previous two lines? http://gerrit.cloudera.org:8080/#/c/6414/14/be/src/runtime/bufferpool/buffer-allocator.h File be/src/runtime/bufferpool/buffer-allocator.h: PS14, Line 114: sizes size PS14, Line 124: no_partial_decrease negatives in names are harder to reason about (passing false leads to a double negative). maybe "require_full_decrease"? PS14, Line 126: memory to allocate 'target_bytes' through various : /// strategies Tries to reclaim enough memory so that 'target_bytes' can be allocated from the system allocator by the caller. (or similar) ("allocate" is such an overloaded term so lets be explciit). i think it would also help to clarify that what we're doing here is claiming memory from various sources so that it can be combined into one larger buffer. i.e. movitation for how this is different than just goign to the free and clean lists. PS14, Line 161: buffer_bytes_remaining_ I think the code would be more intuitive if this were called "system_bytes_remaining_", since this really tracks how much more can be allocated from the system allocator. Not how much more bytes of buffer is left, or something. And then maybe buffer_bytes_limit_ should be system_bytes_limit_ for similar reason. The fact that the bytes will be used by buffers is kind of obvious given the purpose of this class. -- 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: 14 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
