impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (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 03:47:19 GMT
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 <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