IMPALA-5629: avoid expensive list::size() call As a workaround until we move to GCC5+, explicitly track the pages_ list size. This is not too bad in practice since it is only mutated in 3 places.
Testing: Ran buffered-tuple-stream-v2-test (the only coverage of BufferedTupleStreamV2 currently). Reran the query with the perf issue, confirmed that it was no longer spending lots of time in BufferedTupleStreamV2::AdvanceWritePage(). Change-Id: Id83fcf68dcc3ea729df167885f999ff32b861e66 Reviewed-on: http://gerrit.cloudera.org:8080/7382 Reviewed-by: Dan Hecht <[email protected]> Tested-by: Impala Public Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/7761692f Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/7761692f Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/7761692f Branch: refs/heads/master Commit: 7761692fb1cfb92778ece3a1e5305494a158dde9 Parents: 3939591 Author: Tim Armstrong <[email protected]> Authored: Fri Jul 7 17:10:23 2017 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Tue Jul 11 03:10:16 2017 +0000 ---------------------------------------------------------------------- be/src/runtime/buffered-tuple-stream-v2.cc | 31 ++++++++++++++----------- be/src/runtime/buffered-tuple-stream-v2.h | 3 +++ 2 files changed, 21 insertions(+), 13 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/7761692f/be/src/runtime/buffered-tuple-stream-v2.cc ---------------------------------------------------------------------- diff --git a/be/src/runtime/buffered-tuple-stream-v2.cc b/be/src/runtime/buffered-tuple-stream-v2.cc index 90d9c12..faa9008 100644 --- a/be/src/runtime/buffered-tuple-stream-v2.cc +++ b/be/src/runtime/buffered-tuple-stream-v2.cc @@ -54,6 +54,7 @@ BufferedTupleStreamV2::BufferedTupleStreamV2(RuntimeState* state, node_id_(-1), buffer_pool_(state->exec_env()->buffer_pool()), buffer_pool_client_(buffer_pool_client), + num_pages_(0), total_byte_size_(0), has_read_iterator_(false), read_page_reservation_(buffer_pool_client_), @@ -113,6 +114,7 @@ BufferedTupleStreamV2::~BufferedTupleStreamV2() { void BufferedTupleStreamV2::CheckConsistency() const { DCHECK_EQ(bytes_pinned_, CalcBytesPinned()) << DebugString(); + DCHECK_EQ(pages_.size(), num_pages_) << DebugString(); for (const Page& page : pages_) { DCHECK_EQ(ExpectedPinCount(pinned_, &page), page.pin_count()) << DebugString(); // Only one large row per page. @@ -178,7 +180,7 @@ string BufferedTupleStreamV2::DebugString() const { } else { ss << write_page_reservation_.GetReservation(); } - ss << "\n # pages=" << pages_.size() << " pages=[\n"; + ss << "\n # pages=" << num_pages_ << " pages=[\n"; for (const Page& page : pages_) { ss << "{" << page.DebugString() << "}"; if (&page != &pages_.back()) ss << ",\n"; @@ -250,6 +252,7 @@ void BufferedTupleStreamV2::Close(RowBatch* batch, RowBatch::FlushMode flush) { read_page_reservation_.Close(); write_page_reservation_.Close(); pages_.clear(); + num_pages_ = 0; bytes_pinned_ = 0; closed_ = true; } @@ -294,7 +297,7 @@ bool BufferedTupleStreamV2::NeedWriteReservation() const { } bool BufferedTupleStreamV2::NeedWriteReservation(bool stream_pinned) const { - return NeedWriteReservation(stream_pinned, pages_.size(), has_write_iterator(), + return NeedWriteReservation(stream_pinned, num_pages_, has_write_iterator(), write_page_ != nullptr, has_read_write_page()); } @@ -320,7 +323,7 @@ bool BufferedTupleStreamV2::NeedReadReservation() const { bool BufferedTupleStreamV2::NeedReadReservation(bool stream_pinned) const { return NeedReadReservation( - stream_pinned, pages_.size(), has_read_iterator(), read_page_ != pages_.end()); + stream_pinned, num_pages_, has_read_iterator(), read_page_ != pages_.end()); } bool BufferedTupleStreamV2::NeedReadReservation(bool stream_pinned, int64_t num_pages, @@ -356,6 +359,7 @@ Status BufferedTupleStreamV2::NewWritePage(int64_t page_len) noexcept { total_byte_size_ += page_len; pages_.push_back(std::move(new_page)); + ++num_pages_; write_page_ = &pages_.back(); DCHECK_EQ(write_page_->num_rows, 0); write_ptr_ = write_buffer->data(); @@ -385,15 +389,15 @@ Status BufferedTupleStreamV2::AdvanceWritePage( // if the stream is empty. int64_t write_reservation_to_restore = 0, read_reservation_to_restore = 0; if (NeedWriteReservation( - pinned_, pages_.size(), true, write_page_ != nullptr, has_read_write_page()) - && !NeedWriteReservation(pinned_, pages_.size() + 1, true, true, false)) { + pinned_, num_pages_, true, write_page_ != nullptr, has_read_write_page()) + && !NeedWriteReservation(pinned_, num_pages_ + 1, true, true, false)) { write_reservation_to_restore = default_page_len_; } // If the stream is pinned, we need to keep the previous write page pinned for reading. // Check if we saved reservation for this case. - if (NeedReadReservation(pinned_, pages_.size(), has_read_iterator(), + if (NeedReadReservation(pinned_, num_pages_, has_read_iterator(), read_page_ != pages_.end(), true, write_page_ != nullptr) - && !NeedReadReservation(pinned_, pages_.size() + 1, has_read_iterator(), + && !NeedReadReservation(pinned_, num_pages_ + 1, has_read_iterator(), read_page_ != pages_.end(), true, true)) { read_reservation_to_restore = default_page_len_; } @@ -446,9 +450,9 @@ void BufferedTupleStreamV2::InvalidateWriteIterator() { // No more pages will be appended to stream - do not need any write reservation. write_page_reservation_.Close(); // May not need a read reservation once the write iterator is invalidated. - if (NeedReadReservation(pinned_, pages_.size(), has_read_iterator(), + if (NeedReadReservation(pinned_, num_pages_, has_read_iterator(), read_page_ != pages_.end(), true, write_page_ != nullptr) - && !NeedReadReservation(pinned_, pages_.size(), has_read_iterator(), + && !NeedReadReservation(pinned_, num_pages_, has_read_iterator(), read_page_ != pages_.end(), false, false)) { buffer_pool_client_->RestoreReservation(&read_page_reservation_, default_page_len_); } @@ -463,8 +467,8 @@ Status BufferedTupleStreamV2::NextReadPage() { // No rows read yet - start reading at first page. If the stream is unpinned, we can // use the reservation saved in PrepareForReadWrite() to pin the first page. read_page_ = pages_.begin(); - if (NeedReadReservation(pinned_, pages_.size(), true, false) - && !NeedReadReservation(pinned_, pages_.size(), true, true)) { + if (NeedReadReservation(pinned_, num_pages_, true, false) + && !NeedReadReservation(pinned_, num_pages_, true, true)) { buffer_pool_client_->RestoreReservation(&read_page_reservation_, default_page_len_); } } else if (delete_on_read_) { @@ -474,6 +478,7 @@ Status BufferedTupleStreamV2::NextReadPage() { bytes_pinned_ -= pages_.front().len(); buffer_pool_->DestroyPage(buffer_pool_client_, &pages_.front().handle); pages_.pop_front(); + --num_pages_; read_page_ = pages_.begin(); } else { // Unpin pages after reading them if needed. @@ -515,9 +520,9 @@ Status BufferedTupleStreamV2::NextReadPage() { // We may need to save reservation for the write page in the case when the write page // became a read/write page. - if (!NeedWriteReservation(pinned_, pages_.size(), has_write_iterator(), + if (!NeedWriteReservation(pinned_, num_pages_, has_write_iterator(), write_page_ != nullptr, false) - && NeedWriteReservation(pinned_, pages_.size(), has_write_iterator(), + && NeedWriteReservation(pinned_, num_pages_, has_write_iterator(), write_page_ != nullptr, has_read_write_page())) { buffer_pool_client_->SaveReservation(&write_page_reservation_, default_page_len_); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/7761692f/be/src/runtime/buffered-tuple-stream-v2.h ---------------------------------------------------------------------- diff --git a/be/src/runtime/buffered-tuple-stream-v2.h b/be/src/runtime/buffered-tuple-stream-v2.h index 1f21235..26f2113 100644 --- a/be/src/runtime/buffered-tuple-stream-v2.h +++ b/be/src/runtime/buffered-tuple-stream-v2.h @@ -431,6 +431,9 @@ class BufferedTupleStreamV2 { /// * before the first row has been added with AddRow() or AddRowCustom(). /// * after the stream has been destructively read in 'delete_on_read' mode std::list<Page> pages_; + // IMPALA-5629: avoid O(n) list.size() call by explicitly tracking the number of pages. + // TODO: remove when we switch to GCC5+, where list.size() is O(1). See GCC bug #49561. + int64_t num_pages_; /// Total size of pages_, including any pages already deleted in 'delete_on_read' /// mode.
