Tim Armstrong 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, Done Line 451: DCHECK_GT(target_bytes_to_free, 0); > DCHECK_GE(target_bytes_to_free, max_bytes_to_claim)? Done Line 512: int64_t bytes_claimed = bytes_freed; > why is that right? shouldn't it be: bytes_claimed is reassigned below on line 517 in the > case. What you suggested is a more direct way of expressing the same thing, so I'll go with that. Line 544: lists->num_free_buffers.Add(1); > it would be good to move AddFreeBuffer() and RemoveCleanPage() to be adjace Done PS16, Line 555: two Maintenance( > does this need updating? Done 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 Done PS16, Line 59: clean pages > clean unpinned pages Done PS16, Line 65: concurrently > delete Done PS16, Line 66: allocate > this is from the BufferAllocator, not the SystemAllocator, right? I'm not sure that I understood this question - it's releasing itto the system. PS16, Line 66: buffer > ... buffer from the BufferAllocator's free or clean page lists ... Done PS16, Line 164: . > I think we should also explicitly say: Done PS16, Line 164: is slower and > nit: redundant with "slower strategy" stated earlier. Done 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, Done 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( > What I'm suggesting is to just get rid of AddGcFunction() and just have the Yeah that's what I was thinking of too, I think the consequential changes add up. E.g. we'd probably also have to change how the TCMalloc consumption metric is hooked up - it doesn't make sense to externally hook in a TCMalloc consumption metric but have a built-in TCMalloc GC function. Then I think some backend tests that set up MemTrackers would be affected, depending on what the changes were. This code should go away or be simplified, yes. 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 mak Not necessarily - TCMalloc does decommit memory on it's own in some cases. I think that's best documented in the TCMalloc GC function comment, since that function is also where the TCMalloc metric is hooked up. -- 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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
