impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3203: Part 2: per-core free lists in buffer pool
Date Thu, 06 Apr 2017 06:09:01 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3203: Part 2: per-core free lists in buffer pool

Patch Set 16:

File be/src/runtime/bufferpool/

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, 

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 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

PS16, Line 555: two Maintenance(
> does this need updating?
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 

PS16, Line 59: clean pages
> clean unpinned pages

PS16, Line 65: concurrently
> delete

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 ...

PS16, Line 164: .
> I think we should also explicitly say:

PS16, Line 164: is slower and
> nit: redundant with "slower strategy" stated earlier.
File be/src/runtime/bufferpool/

PS16, Line 374: write
> how about: "write handle", since the write I/O is done already. or really, 
File be/src/runtime/

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.
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I612bd1cd0f0e87f7d8186e5bedd53a22f2d80832
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message