Tim Armstrong has posted comments on this change. Change subject: IMPALA-4674: Part 1: port BufferedTupleStream to BufferPool ......................................................................
Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/5811/6/be/src/runtime/buffered-tuple-stream-v2.cc File be/src/runtime/buffered-tuple-stream-v2.cc: PS6, Line 112: DCHECK_LE(read_ptr_, read_end_ptr); > would be nice to write this and line 106 the same to make it a little faste Done Line 252: if (!read_page_->is_pinned()) { > why does this not have to check if read and write page are the same? I ended up reworking the buffer management as discussed to take advantage of the new pin-counting support. There are still some tricky corner cases but I think the invariants are clearer now, and we can provide stronger guarantees about being able to iterate through unpinned streams. Line 276: if (!pinned_ && write_page_ != &pages_.front()) { > this could use a comment: Done Line 332: && (&page == write_page_ || (read_write_ && &page == &*read_page_))) { > why is this right to unpin the read_page_ if !read_write_? This got reworked with the other changes http://gerrit.cloudera.org:8080/#/c/5811/6/be/src/runtime/buffered-tuple-stream-v2.h File be/src/runtime/buffered-tuple-stream-v2.h: PS6, Line 517: CalcNumPinned > this looks dead Done -- To view, visit http://gerrit.cloudera.org:8080/5811 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7bb47a818564ac19a6be4ca88db0578f4ea0b709 Gerrit-PatchSet: 6 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
