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 Sat, 25 Mar 2017 05:13:55 GMT
Dan Hecht has posted comments on this change.

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


Patch Set 3:

(5 comments)

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

PS3, Line 424: Released reservation and dirty unpinned should cancel out.
I'm not sure what this comment is saying.


PS3, Line 517: reservation
why do we pass this rather than just get it from the client?


PS3, Line 520: the invariant
the eviction policy.


Line 541:   // We cleared up enough space to bump 'buffers_in_use_bytes_'.
this comment is kind of cryptic. what is it trying to tell me?

I wonder if the buffer_in_use_bytes_ accounting should always just happen next to the code
that adds it to the pinned_pages_ list, since that's the thing we're aggregating for.


Line 542:   buffers_in_use_bytes_ += len;
It's starting to become difficult to keep track of what these aggregators are, and non-trivial
to verify their correctness. I think it might help to always move the count adjustment to
be next to the list adjustment. (Though it'd have to also be incremented in the case of buffer
allocation).

Maybe it's even worth going one step further and having a simple wrapper around InternalList<Page>
that also increment/decrements the aggregator when enqueuing/dequeuing (and also making the
aggragate counts be 1:1 with the lists). that would also redefine dirty_unpinned_bytes_ to
not include in-flight bytes, but that might be clearer since the name parallels the list,
yet the byte count is slightly different. And it would also mean keeping a separate count
for "anonymous buffer" bytes (ones not associated with pages).

Alternatively, I wonder if dirty_unpinned_bytes_ is still needed/helpful with this new count?

What do you think?


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