From f95a5b2464d0a80e8badfbacda06e567e60cc79a Mon Sep 17 00:00:00 2001 From: Zhongyi Xie Date: Fri, 20 Jul 2018 16:43:13 -0700 Subject: [PATCH] Avoid unnecessary big for-loop when reporting ticker stats stored in GetContext (#3490) Summary: Currently in `Version::Get` when reporting ticker stats stored in `GetContext`, there is a big for-loop through all `Ticker` which adds unnecessary cost to overall CPU usage. We can optimize by storing only ticker values that are used in `Get()` calls in a new struct `GetContextStats` since only a small fraction of all tickers are used in `Get()` calls. For comparison, with the new approach we only need to visit 17 values while old approach will require visiting 100+ `Ticker` Pull Request resolved: https://github.com/facebook/rocksdb/pull/3490 Differential Revision: D6969154 Pulled By: miasantreble fbshipit-source-id: fc27072965a3a94125a3e6883d20dafcf5b84029 --- db/version_set.cc | 15 ++---- table/block_based_table_reader.cc | 86 +++++++++++++++++++------------ table/get_context.cc | 70 +++++++++++++++++++++++-- table/get_context.h | 24 ++++++++- 4 files changed, 147 insertions(+), 48 deletions(-) diff --git a/db/version_set.cc b/db/version_set.cc index 49881c451..932b9d598 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -1212,12 +1212,9 @@ void Version::Get(const ReadOptions& read_options, const LookupKey& k, // report the counters before returning if (get_context.State() != GetContext::kNotFound && - get_context.State() != GetContext::kMerge) { - for (uint32_t t = 0; t < Tickers::TICKER_ENUM_MAX; t++) { - if (get_context.tickers_value[t] > 0) { - RecordTick(db_statistics_, t, get_context.tickers_value[t]); - } - } + get_context.State() != GetContext::kMerge && + db_statistics_ != nullptr) { + get_context.ReportCounters(); } switch (get_context.State()) { case GetContext::kNotFound: @@ -1251,10 +1248,8 @@ void Version::Get(const ReadOptions& read_options, const LookupKey& k, f = fp.GetNextFile(); } - for (uint32_t t = 0; t < Tickers::TICKER_ENUM_MAX; t++) { - if (get_context.tickers_value[t] > 0) { - RecordTick(db_statistics_, t, get_context.tickers_value[t]); - } + if (db_statistics_ != nullptr) { + get_context.ReportCounters(); } if (GetContext::kMerge == get_context.State()) { if (!merge_operator_) { diff --git a/table/block_based_table_reader.cc b/table/block_based_table_reader.cc index e70235c06..8b83cda53 100644 --- a/table/block_based_table_reader.cc +++ b/table/block_based_table_reader.cc @@ -137,6 +137,8 @@ Slice GetCacheKeyFromOffset(const char* cache_key_prefix, Cache::Handle* GetEntryFromCache(Cache* block_cache, const Slice& key, Tickers block_cache_miss_ticker, Tickers block_cache_hit_ticker, + uint64_t* block_cache_miss_stats, + uint64_t* block_cache_hit_stats, Statistics* statistics, GetContext* get_context) { auto cache_handle = block_cache->Lookup(key, statistics); @@ -144,12 +146,12 @@ Cache::Handle* GetEntryFromCache(Cache* block_cache, const Slice& key, PERF_COUNTER_ADD(block_cache_hit_count, 1); if (get_context != nullptr) { // overall cache hit - get_context->RecordCounters(BLOCK_CACHE_HIT, 1); + get_context->get_context_stats_.num_cache_hit++; // total bytes read from cache - get_context->RecordCounters(BLOCK_CACHE_BYTES_READ, - block_cache->GetUsage(cache_handle)); + get_context->get_context_stats_.num_cache_bytes_read += + block_cache->GetUsage(cache_handle); // block-type specific cache hit - get_context->RecordCounters(block_cache_hit_ticker, 1); + (*block_cache_hit_stats)++; } else { // overall cache hit RecordTick(statistics, BLOCK_CACHE_HIT); @@ -161,9 +163,9 @@ Cache::Handle* GetEntryFromCache(Cache* block_cache, const Slice& key, } else { if (get_context != nullptr) { // overall cache miss - get_context->RecordCounters(BLOCK_CACHE_MISS, 1); + get_context->get_context_stats_.num_cache_miss++; // block-type specific cache miss - get_context->RecordCounters(block_cache_miss_ticker, 1); + (*block_cache_miss_stats)++; } else { RecordTick(statistics, BLOCK_CACHE_MISS); RecordTick(statistics, block_cache_miss_ticker); @@ -1166,8 +1168,16 @@ Status BlockBasedTable::GetDataBlockFromCache( block->cache_handle = GetEntryFromCache( block_cache, block_cache_key, is_index ? BLOCK_CACHE_INDEX_MISS : BLOCK_CACHE_DATA_MISS, - is_index ? BLOCK_CACHE_INDEX_HIT : BLOCK_CACHE_DATA_HIT, statistics, - get_context); + is_index ? BLOCK_CACHE_INDEX_HIT : BLOCK_CACHE_DATA_HIT, + get_context + ? (is_index ? &get_context->get_context_stats_.num_cache_index_miss + : &get_context->get_context_stats_.num_cache_data_miss) + : nullptr, + get_context + ? (is_index ? &get_context->get_context_stats_.num_cache_index_hit + : &get_context->get_context_stats_.num_cache_data_hit) + : nullptr, + statistics, get_context); if (block->cache_handle != nullptr) { block->value = reinterpret_cast(block_cache->Value(block->cache_handle)); @@ -1222,24 +1232,26 @@ Status BlockBasedTable::GetDataBlockFromCache( block_cache->TEST_mark_as_data_block(block_cache_key, charge); if (s.ok()) { if (get_context != nullptr) { - get_context->RecordCounters(BLOCK_CACHE_ADD, 1); - get_context->RecordCounters(BLOCK_CACHE_BYTES_WRITE, charge); + get_context->get_context_stats_.num_cache_add++; + get_context->get_context_stats_.num_cache_bytes_write += charge; } else { RecordTick(statistics, BLOCK_CACHE_ADD); RecordTick(statistics, BLOCK_CACHE_BYTES_WRITE, charge); } if (is_index) { if (get_context != nullptr) { - get_context->RecordCounters(BLOCK_CACHE_INDEX_ADD, 1); - get_context->RecordCounters(BLOCK_CACHE_INDEX_BYTES_INSERT, charge); + get_context->get_context_stats_.num_cache_index_add++; + get_context->get_context_stats_.num_cache_index_bytes_insert += + charge; } else { RecordTick(statistics, BLOCK_CACHE_INDEX_ADD); RecordTick(statistics, BLOCK_CACHE_INDEX_BYTES_INSERT, charge); } } else { if (get_context != nullptr) { - get_context->RecordCounters(BLOCK_CACHE_DATA_ADD, 1); - get_context->RecordCounters(BLOCK_CACHE_DATA_BYTES_INSERT, charge); + get_context->get_context_stats_.num_cache_data_add++; + get_context->get_context_stats_.num_cache_data_bytes_insert += + charge; } else { RecordTick(statistics, BLOCK_CACHE_DATA_ADD); RecordTick(statistics, BLOCK_CACHE_DATA_BYTES_INSERT, charge); @@ -1321,24 +1333,25 @@ Status BlockBasedTable::PutDataBlockToCache( if (s.ok()) { assert(block->cache_handle != nullptr); if (get_context != nullptr) { - get_context->RecordCounters(BLOCK_CACHE_ADD, 1); - get_context->RecordCounters(BLOCK_CACHE_BYTES_WRITE, charge); + get_context->get_context_stats_.num_cache_add++; + get_context->get_context_stats_.num_cache_bytes_write += charge; } else { RecordTick(statistics, BLOCK_CACHE_ADD); RecordTick(statistics, BLOCK_CACHE_BYTES_WRITE, charge); } if (is_index) { if (get_context != nullptr) { - get_context->RecordCounters(BLOCK_CACHE_INDEX_ADD, 1); - get_context->RecordCounters(BLOCK_CACHE_INDEX_BYTES_INSERT, charge); + get_context->get_context_stats_.num_cache_index_add++; + get_context->get_context_stats_.num_cache_index_bytes_insert += + charge; } else { RecordTick(statistics, BLOCK_CACHE_INDEX_ADD); RecordTick(statistics, BLOCK_CACHE_INDEX_BYTES_INSERT, charge); } } else { if (get_context != nullptr) { - get_context->RecordCounters(BLOCK_CACHE_DATA_ADD, 1); - get_context->RecordCounters(BLOCK_CACHE_DATA_BYTES_INSERT, charge); + get_context->get_context_stats_.num_cache_data_add++; + get_context->get_context_stats_.num_cache_data_bytes_insert += charge; } else { RecordTick(statistics, BLOCK_CACHE_DATA_ADD); RecordTick(statistics, BLOCK_CACHE_DATA_BYTES_INSERT, charge); @@ -1463,9 +1476,13 @@ BlockBasedTable::CachableEntry BlockBasedTable::GetFilter( filter_blk_handle, cache_key); Statistics* statistics = rep_->ioptions.statistics; - auto cache_handle = - GetEntryFromCache(block_cache, key, BLOCK_CACHE_FILTER_MISS, - BLOCK_CACHE_FILTER_HIT, statistics, get_context); + auto cache_handle = GetEntryFromCache( + block_cache, key, BLOCK_CACHE_FILTER_MISS, BLOCK_CACHE_FILTER_HIT, + get_context ? &get_context->get_context_stats_.num_cache_filter_miss + : nullptr, + get_context ? &get_context->get_context_stats_.num_cache_filter_hit + : nullptr, + statistics, get_context); FilterBlockReader* filter = nullptr; if (cache_handle != nullptr) { @@ -1486,10 +1503,11 @@ BlockBasedTable::CachableEntry BlockBasedTable::GetFilter( : Cache::Priority::LOW); if (s.ok()) { if (get_context != nullptr) { - get_context->RecordCounters(BLOCK_CACHE_ADD, 1); - get_context->RecordCounters(BLOCK_CACHE_BYTES_WRITE, usage); - get_context->RecordCounters(BLOCK_CACHE_FILTER_ADD, 1); - get_context->RecordCounters(BLOCK_CACHE_FILTER_BYTES_INSERT, usage); + get_context->get_context_stats_.num_cache_add++; + get_context->get_context_stats_.num_cache_bytes_write += usage; + get_context->get_context_stats_.num_cache_filter_add++; + get_context->get_context_stats_.num_cache_filter_bytes_insert += + usage; } else { RecordTick(statistics, BLOCK_CACHE_ADD); RecordTick(statistics, BLOCK_CACHE_BYTES_WRITE, usage); @@ -1535,9 +1553,13 @@ InternalIterator* BlockBasedTable::NewIndexIterator( GetCacheKeyFromOffset(rep_->cache_key_prefix, rep_->cache_key_prefix_size, rep_->dummy_index_reader_offset, cache_key); Statistics* statistics = rep_->ioptions.statistics; - auto cache_handle = - GetEntryFromCache(block_cache, key, BLOCK_CACHE_INDEX_MISS, - BLOCK_CACHE_INDEX_HIT, statistics, get_context); + auto cache_handle = GetEntryFromCache( + block_cache, key, BLOCK_CACHE_INDEX_MISS, BLOCK_CACHE_INDEX_HIT, + get_context ? &get_context->get_context_stats_.num_cache_index_miss + : nullptr, + get_context ? &get_context->get_context_stats_.num_cache_index_hit + : nullptr, + statistics, get_context); if (cache_handle == nullptr && no_io) { if (input_iter != nullptr) { @@ -1573,8 +1595,8 @@ InternalIterator* BlockBasedTable::NewIndexIterator( if (s.ok()) { if (get_context != nullptr) { - get_context->RecordCounters(BLOCK_CACHE_ADD, 1); - get_context->RecordCounters(BLOCK_CACHE_BYTES_WRITE, charge); + get_context->get_context_stats_.num_cache_add++; + get_context->get_context_stats_.num_cache_bytes_write += charge; } else { RecordTick(statistics, BLOCK_CACHE_ADD); RecordTick(statistics, BLOCK_CACHE_BYTES_WRITE, charge); diff --git a/table/get_context.cc b/table/get_context.cc index 2a90978c3..fca16305c 100644 --- a/table/get_context.cc +++ b/table/get_context.cc @@ -91,11 +91,73 @@ void GetContext::SaveValue(const Slice& value, SequenceNumber /*seq*/) { } } -void GetContext::RecordCounters(Tickers ticker, size_t val) { - if (ticker == Tickers::TICKER_ENUM_MAX) { - return; +void GetContext::ReportCounters() { + if (get_context_stats_.num_cache_hit > 0) { + RecordTick(statistics_, BLOCK_CACHE_HIT, get_context_stats_.num_cache_hit); + } + if (get_context_stats_.num_cache_index_hit > 0) { + RecordTick(statistics_, BLOCK_CACHE_INDEX_HIT, + get_context_stats_.num_cache_index_hit); + } + if (get_context_stats_.num_cache_data_hit > 0) { + RecordTick(statistics_, BLOCK_CACHE_DATA_HIT, + get_context_stats_.num_cache_data_hit); + } + if (get_context_stats_.num_cache_filter_hit > 0) { + RecordTick(statistics_, BLOCK_CACHE_FILTER_HIT, + get_context_stats_.num_cache_filter_hit); + } + if (get_context_stats_.num_cache_index_miss > 0) { + RecordTick(statistics_, BLOCK_CACHE_INDEX_MISS, + get_context_stats_.num_cache_index_miss); + } + if (get_context_stats_.num_cache_filter_miss > 0) { + RecordTick(statistics_, BLOCK_CACHE_FILTER_MISS, + get_context_stats_.num_cache_filter_miss); + } + if (get_context_stats_.num_cache_data_miss > 0) { + RecordTick(statistics_, BLOCK_CACHE_DATA_MISS, + get_context_stats_.num_cache_data_miss); + } + if (get_context_stats_.num_cache_bytes_read > 0) { + RecordTick(statistics_, BLOCK_CACHE_BYTES_READ, + get_context_stats_.num_cache_bytes_read); + } + if (get_context_stats_.num_cache_miss > 0) { + RecordTick(statistics_, BLOCK_CACHE_MISS, + get_context_stats_.num_cache_miss); + } + if (get_context_stats_.num_cache_add > 0) { + RecordTick(statistics_, BLOCK_CACHE_ADD, get_context_stats_.num_cache_add); + } + if (get_context_stats_.num_cache_bytes_write > 0) { + RecordTick(statistics_, BLOCK_CACHE_BYTES_WRITE, + get_context_stats_.num_cache_bytes_write); + } + if (get_context_stats_.num_cache_index_add > 0) { + RecordTick(statistics_, BLOCK_CACHE_INDEX_ADD, + get_context_stats_.num_cache_index_add); + } + if (get_context_stats_.num_cache_index_bytes_insert > 0) { + RecordTick(statistics_, BLOCK_CACHE_INDEX_BYTES_INSERT, + get_context_stats_.num_cache_index_bytes_insert); + } + if (get_context_stats_.num_cache_data_add > 0) { + RecordTick(statistics_, BLOCK_CACHE_DATA_ADD, + get_context_stats_.num_cache_data_add); + } + if (get_context_stats_.num_cache_data_bytes_insert > 0) { + RecordTick(statistics_, BLOCK_CACHE_DATA_BYTES_INSERT, + get_context_stats_.num_cache_data_bytes_insert); + } + if (get_context_stats_.num_cache_filter_add > 0) { + RecordTick(statistics_, BLOCK_CACHE_FILTER_ADD, + get_context_stats_.num_cache_filter_add); + } + if (get_context_stats_.num_cache_filter_bytes_insert > 0) { + RecordTick(statistics_, BLOCK_CACHE_FILTER_BYTES_INSERT, + get_context_stats_.num_cache_filter_bytes_insert); } - tickers_value[ticker] += static_cast(val); } bool GetContext::SaveValue(const ParsedInternalKey& parsed_key, diff --git a/table/get_context.h b/table/get_context.h index 2b9135676..066be104b 100644 --- a/table/get_context.h +++ b/table/get_context.h @@ -17,6 +17,26 @@ namespace rocksdb { class MergeContext; class PinnedIteratorsManager; +struct GetContextStats { + uint64_t num_cache_hit = 0; + uint64_t num_cache_index_hit = 0; + uint64_t num_cache_data_hit = 0; + uint64_t num_cache_filter_hit = 0; + uint64_t num_cache_index_miss = 0; + uint64_t num_cache_filter_miss = 0; + uint64_t num_cache_data_miss = 0; + uint64_t num_cache_bytes_read = 0; + uint64_t num_cache_miss = 0; + uint64_t num_cache_add = 0; + uint64_t num_cache_bytes_write = 0; + uint64_t num_cache_index_add = 0; + uint64_t num_cache_index_bytes_insert = 0; + uint64_t num_cache_data_add = 0; + uint64_t num_cache_data_bytes_insert = 0; + uint64_t num_cache_filter_add = 0; + uint64_t num_cache_filter_bytes_insert = 0; +}; + class GetContext { public: enum GetState { @@ -27,7 +47,7 @@ class GetContext { kMerge, // saver contains the current merge result (the operands) kBlobIndex, }; - uint64_t tickers_value[Tickers::TICKER_ENUM_MAX] = {0}; + GetContextStats get_context_stats_; GetContext(const Comparator* ucmp, const MergeOperator* merge_operator, Logger* logger, Statistics* statistics, GetState init_state, @@ -77,7 +97,7 @@ class GetContext { return true; } - void RecordCounters(Tickers ticker, size_t val); + void ReportCounters(); private: const Comparator* ucmp_;