Dan Hecht has posted comments on this change. Change subject: IMPALA-3203: Part 2: per-core free lists in buffer pool ......................................................................
Patch Set 16: (15 comments) http://gerrit.cloudera.org:8080/#/c/6414/16/be/src/runtime/bufferpool/buffer-allocator.cc File be/src/runtime/bufferpool/buffer-allocator.cc: PS16, Line 58: The caller can claim up to 'max_bytes_to_claim' to allocate a buffer from the : /// system hmm is that true? what if the number of bytes freed is less than that? oh, reading on, I see that the actual number of bytes the caller can claim is returned as well. So, how about specifying as the input is target_bytes_to_free/target_bytes_to_claim and the output is the actual bytes freed and bytes claimed? And rewording the comment in those terms? Line 451: DCHECK_GT(target_bytes_to_free, 0); DCHECK_GE(target_bytes_to_free, max_bytes_to_claim)? Line 512: int64_t bytes_claimed = bytes_freed; why is that right? shouldn't it be: bytes_claimed = min(bytes_freed, max_bytes_to_claim); if (bytes_freed > bytes_claimed) ... add to system_bytes_remaining_... Line 544: lists->num_free_buffers.Add(1); it would be good to move AddFreeBuffer() and RemoveCleanPage() to be adjacent, so it's more obvious that their code has similarities (in case they need updating, etc). PS16, Line 555: two Maintenance( does this need updating? http://gerrit.cloudera.org:8080/#/c/6414/16/be/src/runtime/bufferpool/buffer-allocator.h File be/src/runtime/bufferpool/buffer-allocator.h: PS16, Line 59: stored inside the buffer allocator. "stored" is kinda confusing and ambiguous. maybe say similar to #2: in the BufferAllocator's clean page lists. PS16, Line 59: clean pages clean unpinned pages just to clarify PS16, Line 65: concurrently delete PS16, Line 66: allocate this is from the BufferAllocator, not the SystemAllocator, right? PS16, Line 66: buffer ... buffer from the BufferAllocator's free or clean page lists ... (i.e. we're not talking about releasing a 2MB buffer from the client to fit in the reservation, right?) PS16, Line 164: . I think we should also explicitly say: If 'slow_but_sure' is false, then this functional optimistically tries to reclaim the memory but may not reclaim all 'target_bytes' of memory. PS16, Line 164: is slower and nit: redundant with "slower strategy" stated earlier. http://gerrit.cloudera.org:8080/#/c/6414/16/be/src/runtime/bufferpool/buffer-pool.cc File be/src/runtime/bufferpool/buffer-pool.cc: PS16, Line 374: write how about: "write handle", since the write I/O is done already. or really, I think you can delete this first clause. http://gerrit.cloudera.org:8080/#/c/6414/15/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: PS15, Line 315: ion( > I agree it would probably be better to design the AddGcFunction() interface What I'm suggesting is to just get rid of AddGcFunction() and just have the mem-tracker.cc code call the functions in order. Or maybe you're saying it's not so simple since not all the objects of the called methods are singletons? In any case, if it is a bunch of work, I'm fine with just clarifying the order. This code should hopefully go away / simplify as we move more things under buffer-pool, right? http://gerrit.cloudera.org:8080/#/c/6414/16/be/src/runtime/mem-tracker.h File be/src/runtime/mem-tracker.h: PS16, Line 300: if none of the other : /// previously-added GC don't we need to still call the tc-malloc GC code? Otherwise, we won't make progress against lowering the process consumption and so still can't make new allocations, no? -- 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: 16 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
