impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
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:

(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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message