You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2017/07/11 03:37:50 UTC
[6/6] incubator-impala git commit: IMPALA-5629: avoid expensive
list::size() call
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 <dh...@cloudera.com>
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 <ta...@cloudera.com>
Authored: Fri Jul 7 17:10:23 2017 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
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.