Dan Hecht has posted comments on this change. Change subject: IMPALA-3203: Part 2: per-core free lists in buffer pool ......................................................................
Patch Set 15: (14 comments) Next batch. I've gone through buffer-allocator.h/.cc. http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/runtime/bufferpool/buffer-allocator.cc File be/src/runtime/bufferpool/buffer-allocator.cc: PS15, Line 137: Return the buffer size for a buffer of the given length. doesn't match PS15, Line 138: GetBuffersOfSize GetListsForSize? PS15, Line 419: FreeBuffers this name is a bit confusing now that i see it in use since "free buffers" is also used as a noun. Anyway, this code might be clearer if instead of this method, the free-list exposes a way to get N buffers from the tail, and then this class calls the system_allocator_->Free(). That way, all system_allocator_ Allocate()/Free() happens in one layer and FreeList doesn't have to know about SystemAllocator. (You'd have to remove FreeAll() also, but that seems like you could just use the same routine by passing the number of buffers in the list as the count). PS15, Line 421: so that other threads : // can claim the memory. not sure what this comment is saying. PS15, Line 423: bytes_freed > 0 when would this be false? PS15, Line 478: !al.owns_lock() && why have this condition here? it's still correct to skip the whole block if the lock is already held, right? Line 488: // Figure out how many buffers we should free of this size. i was misled by this comment since at this point, buffers_to_free is only how many buffers from the free list (not how many buffers we should free of this size). PS15, Line 489: bytes_to_free this is so similarly named to buffers_to_free and buffer_bytes_to_free but has a different meaning in that it's the target, not the accumulated. That makes the loop a bit harder to understand. So, how about renaming this to 'target_bytes' or something? Line 496: // that they are freed based on memory address in the expected order. this is basically the same as evicting the pages, right? it would help to use that terminology here to make the connection explicit. PS15, Line 525: int64_t max_to_claim = claim_memory ? bytes_to_free : 0; : if (bytes_freed > max_to_claim) { : // Add back the extra for other threads before releasing the lock to avoid race : // where the other thread may not be able to find enough buffers. : parent_->system_bytes_remaining_.Add(bytes_freed - max_to_claim); : bytes_freed = max_to_claim; : } as mentioned earlier in the function comment, this is pretty confusing. PS15, Line 551: BufferHandle buffer; : { : lock_guard<SpinLock> pl(page->buffer_lock); : buffer = move(page->buffer); : } : lists->free_buffers.AddFreeBuffer(move(buffer)); : lists->num_free_buffers.Add(1); why doesn't this need to do the other stuff that FreeBufferArena::AddFreeBuffer() does? Why not just remove 'claim_buffer' and this block, and then have the buffer-pool just always call back down into BufferAllocator::Free(). Oh we just talked about that and you pointed out that then a client would temporarily be over its reservation while the lock is dropped. But, shouldn't this block do everything FreeBufferArena::AddFreeBuffer() does? PS15, Line 569: touched needed? Line 571: // least one, we guarantee that an idle list will shrink to zero entries. can you add some motivation for why we need to care about the previous two intervals, rather than, say, just the last interval? PS15, Line 576: bytes_freed > 0 when would this be false? -- 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
