From ac02b3258fc5bfc5cebcbd89a9413ea2f6fbade5 Mon Sep 17 00:00:00 2001 From: Kosie van der Merwe Date: Fri, 15 Feb 2013 12:03:30 -0800 Subject: [PATCH] Hotcold: Removed Table from metrics flow path and now only create BlockMetrics in Block::MetricsIter Summary: Metrics no longer pass through `Table` to the cache and instead go directly from `Block::MetricsIter`. Additionally `BlockMetrics` is now only created when a `Seek()` is performed. Test Plan: make check Reviewers: vamsi, dhruba Reviewed By: vamsi CC: leveldb Differential Revision: https://reviews.facebook.net/D8427 --- db/db_impl.cc | 6 +-- table/block.cc | 101 ++++++++++++++++++++++++++++--------------------- table/block.h | 17 +++++---- table/table.cc | 21 ++-------- 4 files changed, 71 insertions(+), 74 deletions(-) diff --git a/db/db_impl.cc b/db/db_impl.cc index 14ef2ede..8d8a2440 100644 --- a/db/db_impl.cc +++ b/db/db_impl.cc @@ -2196,11 +2196,7 @@ Status DBImpl::Get(const ReadOptions& options, Iterator* DBImpl::NewIterator(const ReadOptions& options) { SequenceNumber latest_snapshot; - ReadOptions read_options = options; - if (read_options.record_accesses && is_hotcold_) { - read_options.metrics_handler = this; - } - Iterator* internal_iter = NewInternalIterator(read_options, &latest_snapshot); + Iterator* internal_iter = NewInternalIterator(options, &latest_snapshot); return NewDBIterator( &dbname_, env_, user_comparator(), internal_iter, (options.snapshot != NULL diff --git a/table/block.cc b/table/block.cc index 25ee5c7c..18d021f8 100644 --- a/table/block.cc +++ b/table/block.cc @@ -80,6 +80,8 @@ static inline const char* DecodeEntry(const char* p, const char* limit, class Block::Iter : public Iterator { protected: const Comparator* const comparator_; + uint64_t file_number_; + uint64_t block_offset_; const char* const data_; // underlying block contents uint32_t const restarts_; // Offset of restart array (list of fixed32) uint32_t const num_restarts_; // Number of uint32_t entries in restart array @@ -120,10 +122,14 @@ class Block::Iter : public Iterator { public: Iter(const Comparator* comparator, + uint64_t file_number, + uint64_t block_offset, const char* data, uint32_t restarts, uint32_t num_restarts) : comparator_(comparator), + file_number_(file_number), + block_offset_(block_offset), data_(data), restarts_(restarts), num_restarts_(num_restarts), @@ -227,6 +233,8 @@ class Block::Iter : public Iterator { } private: + friend class Block; + void CorruptionError() { current_ = restarts_; restart_index_ = num_restarts_; @@ -274,8 +282,9 @@ class Block::Iter : public Iterator { // This is the iterator returned by Block::NewMetricsIterator() on success. class Block::MetricsIter : public Block::Iter { private: - uint64_t file_number_; - uint64_t block_offset_; + Cache* cache_; + Cache::Handle* cache_handle_; + void* metrics_handler_; BlockMetrics* metrics_; public: @@ -285,24 +294,24 @@ class Block::MetricsIter : public Block::Iter { const char* data, uint32_t restarts, uint32_t num_restarts, - BlockMetrics* metrics) - : Block::Iter(comparator, data, restarts, num_restarts), - file_number_(file_number), - block_offset_(block_offset), - metrics_(metrics) { + Cache* cache, + Cache::Handle* cache_handle, + void* metrics_handler) + : Block::Iter(comparator, file_number, block_offset, data, restarts, + num_restarts), + cache_(cache), + cache_handle_(cache_handle), + metrics_handler_(metrics_handler), + metrics_(NULL) { } virtual ~MetricsIter() { - } - - virtual void Next() { - Block::Iter::Next(); - RecordAccess(); - } - - virtual void Prev() { - Block::Iter::Prev(); - RecordAccess(); + if (metrics_) { + cache_->ReleaseAndRecordMetrics(cache_handle_, metrics_handler_, + metrics_); + } else { + cache_->Release(cache_handle_); + } } virtual void Seek(const Slice& target) { @@ -310,21 +319,13 @@ class Block::MetricsIter : public Block::Iter { RecordAccess(); } - virtual void SeekToFirst() { - Block::Iter::SeekToFirst(); - RecordAccess(); - } - - virtual void SeekToLast() { - Block::Iter::SeekToLast(); - RecordAccess(); - } - private: - friend class Block; - void RecordAccess() { - if (metrics_ != NULL && Valid()) { + if (Valid()) { + if (metrics_ == nullptr) { + metrics_ = new BlockMetrics(file_number_, block_offset_, + num_restarts_, kBytesPerRestart); + } metrics_->RecordAccess(restart_index_, restart_offset_); } } @@ -338,7 +339,7 @@ Iterator* Block::NewIterator(const Comparator* cmp) { if (num_restarts == 0) { return NewEmptyIterator(); } else { - return new Iter(cmp, data_, restart_offset_, num_restarts); + return new Iter(cmp, 0, 0, data_, restart_offset_, num_restarts); } } @@ -353,30 +354,42 @@ Iterator* Block::NewIterator(const Comparator* cmp, if (num_restarts == 0) { return NewEmptyIterator(); } else { - return new MetricsIter(cmp, file_number, block_offset, data_, - restart_offset_, num_restarts, NULL); + return new Iter(cmp, file_number, block_offset, data_, + restart_offset_, num_restarts); } } Iterator* Block::NewMetricsIterator(const Comparator* cmp, uint64_t file_number, uint64_t block_offset, - BlockMetrics** metrics) { - assert(metrics != NULL); - - *metrics = NULL; + Cache* cache, + Cache::Handle* cache_handle, + void* metrics_handler) { + assert(cache != nullptr); + assert(cache_handle != nullptr); + assert(metrics_handler != nullptr); + + Iterator* iter = nullptr; if (size_ < 2*sizeof(uint32_t)) { - return NewErrorIterator(Status::Corruption("bad block contents")); + iter = NewErrorIterator(Status::Corruption("bad block contents")); } const uint32_t num_restarts = NumRestarts(); if (num_restarts == 0) { - return NewEmptyIterator(); + iter = NewEmptyIterator(); + } + + if (iter == nullptr) { + // MetricsIter takes ownership of the cache handle and will delete it. + iter = new MetricsIter(cmp, file_number, block_offset, data_, + restart_offset_, num_restarts, + cache, cache_handle, metrics_handler); } else { - *metrics = new BlockMetrics(file_number, block_offset, num_restarts, - kBytesPerRestart); - return new MetricsIter(cmp, file_number, block_offset, data_, - restart_offset_, num_restarts, *metrics); + // Release the cache handle as neither the error iterator nor the empty + // iterator need it's contents. + cache->Release(cache_handle); } + + return iter; } bool Block::GetBlockIterInfo(const Iterator* iter, @@ -384,7 +397,7 @@ bool Block::GetBlockIterInfo(const Iterator* iter, uint64_t& block_offset, uint32_t& restart_index, uint32_t& restart_offset) { - const MetricsIter* biter = dynamic_cast(iter); + const Iter* biter = dynamic_cast(iter); if (biter == NULL) { return false; diff --git a/table/block.h b/table/block.h index 8193a067..0a055008 100644 --- a/table/block.h +++ b/table/block.h @@ -5,11 +5,15 @@ #ifndef STORAGE_LEVELDB_TABLE_BLOCK_H_ #define STORAGE_LEVELDB_TABLE_BLOCK_H_ +#include #include #include +#include "leveldb/cache.h" #include "leveldb/iterator.h" #include "leveldb/slice.h" +using std::unique_ptr; + namespace leveldb { struct BlockContents; @@ -32,16 +36,15 @@ class Block { uint64_t file_number, uint64_t block_offset); - // Creates a new iterator that keeps track of accesses. - // - // Creates a BlockMetrics object on the heap and sets metrics to it. - // The caller is responsible for freeing this object. *metrics may be set to - // NULL. - // REQUIRES: metrics must be non-NULL + // Creates a new iterator that keeps track of accesses. When this iterator is + // deleted it frees the cache handle and passes the metrics to the cache specified. + // REQUIRES: cache, cache_handle, metrics_handler must be non-NULL Iterator* NewMetricsIterator(const Comparator* comparator, uint64_t file_number, uint64_t block_offset, - BlockMetrics** metrics); + Cache* cache, + Cache::Handle* cache_handle, + void* metrics_handler); // Returns true if iter is a Block iterator and also knows that which file // and block it belongs to. diff --git a/table/table.cc b/table/table.cc index eb2f5a7f..818c22c1 100644 --- a/table/table.cc +++ b/table/table.cc @@ -197,14 +197,6 @@ struct CacheMetricsInfo { : handler(handler), handle(handle), metrics(metrics) { } }; -// Releases the block's cache handle and flushes the metrics from the iterator. -static void ReleaseBlockAndRecordMetrics(void* arg, void* cmi) { - Cache* cache = reinterpret_cast(arg); - CacheMetricsInfo* cmiptr = reinterpret_cast(cmi); - cache->ReleaseAndRecordMetrics(cmiptr->handle, cmiptr->handler, - cmiptr->metrics); - delete cmiptr; -} // Convert an index iterator value (i.e., an encoded BlockHandle) // into an iterator over the contents of the corresponding block. @@ -280,19 +272,12 @@ Iterator* Table::BlockReader(void* arg, iter->RegisterCleanup(&ReleaseBlock, block_cache, cache_handle); } } else { - BlockMetrics* metrics = NULL; iter = block->NewMetricsIterator(table->rep_->options.comparator, table->rep_->file_number, handle.offset(), - &metrics); - - if (metrics == NULL) { - iter->RegisterCleanup(&ReleaseBlock, block_cache, cache_handle); - } else { - CacheMetricsInfo* cmi = new CacheMetricsInfo(options.metrics_handler, - cache_handle, metrics); - iter->RegisterCleanup(&ReleaseBlockAndRecordMetrics, block_cache, cmi); - } + block_cache, + cache_handle, + options.metrics_handler); } } else { iter = NewErrorIterator(s); -- 2.47.3