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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
