Buffer pool: Add basic counters to buffer pool client Change-Id: I9a5a57b7cfccf67ee498e68964f1e077075ee325 Reviewed-on: http://gerrit.cloudera.org:8080/4714 Reviewed-by: Tim Armstrong <tarmstr...@cloudera.com> Tested-by: Internal 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/e3a08914 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/e3a08914 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/e3a08914 Branch: refs/heads/master Commit: e3a08914451a63fe65e8f66afc743739f4570ba4 Parents: 07da767 Author: Tim Armstrong <tarmstr...@cloudera.com> Authored: Fri Oct 7 09:23:59 2016 -0700 Committer: Internal Jenkins <cloudera-hud...@gerrit.cloudera.org> Committed: Tue Oct 18 04:48:43 2016 +0000 ---------------------------------------------------------------------- be/src/bufferpool/buffer-pool-counters.h | 47 ++++++++++++++++++++++ be/src/bufferpool/buffer-pool-test.cc | 30 +++++++------- be/src/bufferpool/buffer-pool.cc | 33 ++++++++++++--- be/src/bufferpool/buffer-pool.h | 15 +++++-- be/src/bufferpool/reservation-tracker-test.cc | 9 +---- 5 files changed, 102 insertions(+), 32 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e3a08914/be/src/bufferpool/buffer-pool-counters.h ---------------------------------------------------------------------- diff --git a/be/src/bufferpool/buffer-pool-counters.h b/be/src/bufferpool/buffer-pool-counters.h new file mode 100644 index 0000000..6f3801e --- /dev/null +++ b/be/src/bufferpool/buffer-pool-counters.h @@ -0,0 +1,47 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#ifndef IMPALA_BUFFER_POOL_COUNTERS_H +#define IMPALA_BUFFER_POOL_COUNTERS_H + +#include "util/runtime-profile.h" + +namespace impala { + +/// A set of counters for each buffer pool client. +struct BufferPoolClientCounters { + public: + /// Amount of time spent trying to get a buffer. + RuntimeProfile::Counter* get_buffer_time; + + /// Amount of time spent waiting for reads from disk to complete. + RuntimeProfile::Counter* read_wait_time; + + /// Amount of time spent waiting for writes to disk to complete. + RuntimeProfile::Counter* write_wait_time; + + /// The peak total size of unpinned buffers. + RuntimeProfile::HighWaterMarkCounter* peak_unpinned_bytes; + + /// The total bytes of data unpinned. Every time a page's pin count goes from 1 to 0, + /// this counter is incremented by the page size. + RuntimeProfile::Counter* total_unpinned_bytes; +}; + +} + +#endif http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e3a08914/be/src/bufferpool/buffer-pool-test.cc ---------------------------------------------------------------------- diff --git a/be/src/bufferpool/buffer-pool-test.cc b/be/src/bufferpool/buffer-pool-test.cc index 16cf12c..793bcb9 100644 --- a/be/src/bufferpool/buffer-pool-test.cc +++ b/be/src/bufferpool/buffer-pool-test.cc @@ -15,7 +15,6 @@ // specific language governing permissions and limitations // under the License. -#include <gtest/gtest.h> #include <boost/bind.hpp> #include <boost/scoped_ptr.hpp> #include <boost/thread/thread.hpp> @@ -29,7 +28,7 @@ #include "common/init.h" #include "common/object-pool.h" #include "testutil/death-test-util.h" -#include "testutil/test-macros.h" +#include "testutil/gtest-util.h" #include "common/names.h" @@ -125,7 +124,8 @@ void BufferPoolTest::RegisterQueriesAndClients(BufferPool* pool, int query_id_hi EXPECT_TRUE( client_reservations[i][j].IncreaseReservationToFit(initial_client_reservation)); string name = Substitute("Client $0 for query $1", j, query_id); - EXPECT_OK(pool->RegisterClient(name, &client_reservations[i][j], &clients[i][j])); + EXPECT_OK(pool->RegisterClient( + name, &client_reservations[i][j], NewProfile(), &clients[i][j])); } for (int j = 0; j < clients_per_query; ++j) { @@ -209,7 +209,7 @@ TEST_F(BufferPoolTest, PageCreation) { client_tracker->InitChildTracker(NewProfile(), &global_reservations_, NULL, total_mem); ASSERT_TRUE(client_tracker->IncreaseReservation(total_mem)); BufferPool::Client client; - ASSERT_OK(pool.RegisterClient("test client", client_tracker, &client)); + ASSERT_OK(pool.RegisterClient("test client", client_tracker, NewProfile(), &client)); vector<BufferPool::PageHandle> handles(num_pages); @@ -256,7 +256,7 @@ TEST_F(BufferPoolTest, BufferAllocation) { client_tracker->InitChildTracker(NewProfile(), &global_reservations_, NULL, total_mem); ASSERT_TRUE(client_tracker->IncreaseReservationToFit(total_mem)); BufferPool::Client client; - ASSERT_OK(pool.RegisterClient("test client", client_tracker, &client)); + ASSERT_OK(pool.RegisterClient("test client", client_tracker, NewProfile(), &client)); vector<BufferPool::BufferHandle> handles(num_buffers); @@ -302,7 +302,8 @@ TEST_F(BufferPoolTest, BufferTransfer) { client_trackers[i].InitChildTracker( NewProfile(), &global_reservations_, NULL, TEST_BUFFER_LEN); ASSERT_TRUE(client_trackers[i].IncreaseReservationToFit(TEST_BUFFER_LEN)); - ASSERT_OK(pool.RegisterClient("test client", &client_trackers[i], &clients[i])); + ASSERT_OK(pool.RegisterClient( + "test client", &client_trackers[i], NewProfile(), &clients[i])); } // Transfer the page around between the clients repeatedly in a circle. @@ -344,7 +345,7 @@ TEST_F(BufferPoolTest, Pin) { NewProfile(), &global_reservations_, NULL, child_reservation); ASSERT_TRUE(client_tracker->IncreaseReservationToFit(child_reservation)); BufferPool::Client client; - ASSERT_OK(pool.RegisterClient("test client", client_tracker, &client)); + ASSERT_OK(pool.RegisterClient("test client", client_tracker, NewProfile(), &client)); BufferPool::PageHandle handle1, handle2; @@ -395,7 +396,7 @@ TEST_F(BufferPoolTest, PinWithoutReservation) { client_tracker->InitChildTracker( NewProfile(), &global_reservations_, NULL, TEST_BUFFER_LEN); BufferPool::Client client; - ASSERT_OK(pool.RegisterClient("test client", client_tracker, &client)); + ASSERT_OK(pool.RegisterClient("test client", client_tracker, NewProfile(), &client)); BufferPool::PageHandle handle; IMPALA_ASSERT_DEBUG_DEATH(pool.CreatePage(&client, TEST_BUFFER_LEN, &handle), ""); @@ -423,7 +424,7 @@ TEST_F(BufferPoolTest, ExtractBuffer) { NewProfile(), &global_reservations_, NULL, child_reservation); ASSERT_TRUE(client_tracker->IncreaseReservationToFit(child_reservation)); BufferPool::Client client; - ASSERT_OK(pool.RegisterClient("test client", client_tracker, &client)); + ASSERT_OK(pool.RegisterClient("test client", client_tracker, NewProfile(), &client)); BufferPool::PageHandle page; BufferPool::BufferHandle buffer; @@ -499,7 +500,7 @@ void BufferPoolTest::CreatePageLoop( ReservationTracker client_tracker; client_tracker.InitChildTracker(NewProfile(), parent_tracker, NULL, TEST_BUFFER_LEN); BufferPool::Client client; - ASSERT_OK(pool->RegisterClient("test client", &client_tracker, &client)); + ASSERT_OK(pool->RegisterClient("test client", &client_tracker, NewProfile(), &client)); for (int i = 0; i < num_ops; ++i) { BufferPool::PageHandle handle; ASSERT_TRUE(client_tracker.IncreaseReservation(TEST_BUFFER_LEN)); @@ -525,7 +526,8 @@ TEST_F(BufferPoolTest, CapacityExhausted) { BufferPool::PageHandle handle1, handle2, handle3; BufferPool::Client client; - ASSERT_OK(pool.RegisterClient("test client", &global_reservations_, &client)); + ASSERT_OK( + pool.RegisterClient("test client", &global_reservations_, NewProfile(), &client)); ASSERT_TRUE(global_reservations_.IncreaseReservation(TEST_BUFFER_LEN)); ASSERT_OK(pool.CreatePage(&client, TEST_BUFFER_LEN, &handle1)); @@ -549,8 +551,4 @@ TEST_F(BufferPoolTest, CapacityExhausted) { } } -int main(int argc, char** argv) { - ::testing::InitGoogleTest(&argc, argv); - impala::InitCommonRuntime(argc, argv, true, impala::TestInfo::BE_TEST); - return RUN_ALL_TESTS(); -} +IMPALA_TEST_MAIN(); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e3a08914/be/src/bufferpool/buffer-pool.cc ---------------------------------------------------------------------- diff --git a/be/src/bufferpool/buffer-pool.cc b/be/src/bufferpool/buffer-pool.cc index eaa4262..3035694 100644 --- a/be/src/bufferpool/buffer-pool.cc +++ b/be/src/bufferpool/buffer-pool.cc @@ -25,6 +25,7 @@ #include "common/names.h" #include "gutil/strings/substitute.h" #include "util/bit-util.h" +#include "util/runtime-profile-counters.h" #include "util/uid-util.h" namespace impala { @@ -176,14 +177,26 @@ BufferPool::~BufferPool() { } Status BufferPool::RegisterClient( - const string& name, ReservationTracker* reservation, Client* client) { + const string& name, ReservationTracker* reservation, RuntimeProfile* profile, + Client* client) { DCHECK(!client->is_registered()); DCHECK(reservation != NULL); + client->InitCounters(profile); client->reservation_ = reservation; client->name_ = name; return Status::OK(); } +void BufferPool::Client::InitCounters(RuntimeProfile* profile) { + counters_.get_buffer_time = ADD_TIMER(profile, "BufferPoolGetBufferTime"); + counters_.read_wait_time = ADD_TIMER(profile, "BufferPoolReadWaitTime"); + counters_.write_wait_time = ADD_TIMER(profile, "BufferPoolWriteWaitTime"); + counters_.peak_unpinned_bytes = + profile->AddHighWaterMarkCounter("BufferPoolPeakUnpinnedBytes", TUnit::BYTES); + counters_.total_unpinned_bytes = + ADD_COUNTER(profile, "BufferPoolTotalUnpinnedBytes", TUnit::BYTES); +} + void BufferPool::DeregisterClient(Client* client) { if (!client->is_registered()) return; client->reservation_->Close(); @@ -256,13 +269,16 @@ Status BufferPool::Pin(Client* client, PageHandle* handle) { Page* page = handle->page_; { lock_guard<SpinLock> pl(page->lock); // Lock page while we work on its state. - if (!page->buffer.is_open()) { - // No changes have been made to state yet, so we can cleanly return on error. - RETURN_IF_ERROR(AllocateBufferInternal(client, page->len, &page->buffer)); + if (page->pin_count == 0) { + if (!page->buffer.is_open()) { + // No changes have been made to state yet, so we can cleanly return on error. + RETURN_IF_ERROR(AllocateBufferInternal(client, page->len, &page->buffer)); + + // TODO: will need to initiate/wait for read if the page is not in-memory. + } + COUNTER_ADD(client->counters_.peak_unpinned_bytes, -handle->len()); } page->IncrementPinCount(handle); - - // TODO: will need to initiate/wait for read if the page is not in-memory. } client->reservation_->AllocateFrom(page->len); @@ -286,12 +302,16 @@ void BufferPool::UnpinLocked(Client* client, PageHandle* handle) { page->DecrementPinCount(handle); client->reservation_->ReleaseTo(page->len); + COUNTER_ADD(client->counters_.total_unpinned_bytes, handle->len()); + COUNTER_ADD(client->counters_.peak_unpinned_bytes, handle->len()); + // TODO: can evict now. Only need to preserve contents if 'page->dirty' is true. } void BufferPool::ExtractBuffer( Client* client, PageHandle* page_handle, BufferHandle* buffer_handle) { DCHECK(page_handle->is_pinned()); + DCHECK_EQ(page_handle->client_, client); Page* page = page_handle->page_; @@ -316,6 +336,7 @@ Status BufferPool::AllocateBufferInternal( DCHECK(!buffer->is_open()); DCHECK_GE(len, min_buffer_len_); DCHECK_EQ(len, BitUtil::RoundUpToPowerOfTwo(len)); + SCOPED_TIMER(client->counters_.get_buffer_time); // If there is headroom in 'buffer_bytes_remaining_', we can just allocate a new buffer. if (TryDecreaseBufferBytesRemaining(len)) { http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e3a08914/be/src/bufferpool/buffer-pool.h ---------------------------------------------------------------------- diff --git a/be/src/bufferpool/buffer-pool.h b/be/src/bufferpool/buffer-pool.h index 44b5574..6a9641d 100644 --- a/be/src/bufferpool/buffer-pool.h +++ b/be/src/bufferpool/buffer-pool.h @@ -24,6 +24,7 @@ #include <string> #include "bufferpool/buffer-allocator.h" +#include "bufferpool/buffer-pool-counters.h" #include "common/atomic.h" #include "common/status.h" #include "gutil/macros.h" @@ -167,10 +168,11 @@ class BufferPool { /// Register a client. Returns an error status and does not register the client if the /// arguments are invalid. 'name' is an arbitrary name used to identify the client in - /// any errors messages or logging. 'client' is the client to register. 'client' should - /// not already be registered. + /// any errors messages or logging. Counters for this client are added to the (non-NULL) + /// 'profile'. 'client' is the client to register. 'client' should not already be + /// registered. Status RegisterClient(const std::string& name, ReservationTracker* reservation, - Client* client); + RuntimeProfile* profile, Client* client); /// Deregister 'client' if it is registered. Idempotent. void DeregisterClient(Client* client); @@ -305,12 +307,19 @@ class BufferPool::Client { friend class BufferPool; DISALLOW_COPY_AND_ASSIGN(Client); + /// Initialize 'counters_' and add the counters to 'profile'. + void InitCounters(RuntimeProfile* profile); + /// A name identifying the client. std::string name_; /// The reservation tracker for the client. NULL means the client isn't registered. /// All pages pinned by the client count as usage against 'reservation_'. ReservationTracker* reservation_; + + /// The RuntimeProfile counters for this client. All non-NULL if is_registered() + /// is true. + BufferPoolClientCounters counters_; }; /// A handle to a buffer allocated from the buffer pool. Each BufferHandle should only http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/e3a08914/be/src/bufferpool/reservation-tracker-test.cc ---------------------------------------------------------------------- diff --git a/be/src/bufferpool/reservation-tracker-test.cc b/be/src/bufferpool/reservation-tracker-test.cc index 93bf7b8..66ce287 100644 --- a/be/src/bufferpool/reservation-tracker-test.cc +++ b/be/src/bufferpool/reservation-tracker-test.cc @@ -15,7 +15,6 @@ // specific language governing permissions and limitations // under the License. -#include <gtest/gtest.h> #include <limits> #include <string> #include <vector> @@ -24,7 +23,7 @@ #include "common/init.h" #include "common/object-pool.h" #include "runtime/mem-tracker.h" -#include "testutil/test-macros.h" +#include "testutil/gtest-util.h" #include "common/names.h" @@ -376,8 +375,4 @@ TEST_F(ReservationTrackerTest, MemTrackerIntegrationMultiLevel) { } } -int main(int argc, char** argv) { - ::testing::InitGoogleTest(&argc, argv); - impala::InitCommonRuntime(argc, argv, true, impala::TestInfo::BE_TEST); - return RUN_ALL_TESTS(); -} +IMPALA_TEST_MAIN();
