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-5113: fix dirty unpinned invariant
Date Mon, 27 Mar 2017 18:04:13 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-5113: fix dirty unpinned invariant
......................................................................


Patch Set 4:

(8 comments)

Nice cleanup.

http://gerrit.cloudera.org:8080/#/c/6469/4/be/src/runtime/bufferpool/buffer-pool-internal.h
File be/src/runtime/bufferpool/buffer-pool-internal.h:

PS4, Line 219: or
?


Line 222:   /// locks should be held by the caller.
this should talk specifically about reservations.


PS4, Line 233: CleanPagesBeforeAllocationLocked
the name seems a bit misleading since we have to use it in places other than buffer allocation.
 If we remove CleanPagesBeoreAllocation() (see comments in cc file), maybe just rename this
to CleanPages() or similar.


Line 237:   /// accounting. No page or client locks should be held by the caller.
let's mention specifically that it releases the buffer to the client's reservation.


PS4, Line 336: Only used for debugging.
remove this now that the byte count is presumably used for non-debugging.


http://gerrit.cloudera.org:8080/#/c/6469/4/be/src/runtime/bufferpool/buffer-pool.cc
File be/src/runtime/bufferpool/buffer-pool.cc:

Line 131:   client->impl_->reservation()->AllocateFrom(len);
why does this happen after AllocateBufferInternal()? In the case of AllocateBuffer(), the
reservation consumption happens first, right? Also, the documentation for AllocateBufferInternal()
says it assumes the reservation has already been updated.

Also, it's a little unfortunate that we have to expose both CleanPagesBeforeAllocation() and
PrepareToAllocateBuffer() from the client (I guess since this path we don't want to account
it as an allocated buffer). I wonder if it'd be better to revert this change, and make AddNewPinnedPage()
decrement buffers_allocated_bytes_ since its job is to effectively finalizing the page mapping
in the client accounting.


Line 505:   // Clean pages before updating the accounting.
that's obvious from the code. explain the "why" instead.


http://gerrit.cloudera.org:8080/#/c/6469/4/be/src/runtime/bufferpool/buffer-pool.h
File be/src/runtime/bufferpool/buffer-pool.h:

Line 251:   class PageList;
is this only for tests? if so, let's comment that.


-- 
To view, visit http://gerrit.cloudera.org:8080/6469
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I07e08acb6cf6839bfccbd09258c093b1c8252b25
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message