From 49fbd5531b575a22419811437f20932be72c4f55 Mon Sep 17 00:00:00 2001 From: Haobo Xu Date: Mon, 22 Apr 2013 23:47:56 -0700 Subject: [PATCH] [RocksDB] Refactor table.cc to reduce code duplication and improve readability. Summary: In table.cc, the code section that reads in BlockContent and then put it into a Block, appears at least 4 times. This is too much duplication. BlockReader is much shorter after the change and reads way better. D10077 attempted that for index block read. This is a complete cleanup. Test Plan: make check; ./db_stress Reviewers: dhruba, sheki Reviewed By: dhruba CC: leveldb Differential Revision: https://reviews.facebook.net/D10527 --- table/block.cc | 3 ++- table/block.h | 2 ++ table/format.cc | 8 +++--- table/format.h | 8 +++--- table/table.cc | 68 +++++++++++++++++++++++++++---------------------- 5 files changed, 50 insertions(+), 39 deletions(-) diff --git a/table/block.cc b/table/block.cc index 6289a33e80..75772eceba 100644 --- a/table/block.cc +++ b/table/block.cc @@ -23,7 +23,8 @@ inline uint32_t Block::NumRestarts() const { Block::Block(const BlockContents& contents) : data_(contents.data.data()), size_(contents.data.size()), - owned_(contents.heap_allocated) { + owned_(contents.heap_allocated), + cachable_(contents.cachable) { if (size_ < sizeof(uint32_t)) { size_ = 0; // Error marker } else { diff --git a/table/block.h b/table/block.h index 2493eb9f9f..12938a6652 100644 --- a/table/block.h +++ b/table/block.h @@ -22,6 +22,7 @@ class Block { ~Block(); size_t size() const { return size_; } + bool isCachable() const { return cachable_; } Iterator* NewIterator(const Comparator* comparator); private: @@ -31,6 +32,7 @@ class Block { size_t size_; uint32_t restart_offset_; // Offset in data_ of restart array bool owned_; // Block owns data_[] + bool cachable_; // No copying allowed Block(const Block&); diff --git a/table/format.cc b/table/format.cc index 6c3f5910cd..7110f294da 100644 --- a/table/format.cc +++ b/table/format.cc @@ -66,10 +66,10 @@ Status Footer::DecodeFrom(Slice* input) { return result; } -Status ReadBlock(RandomAccessFile* file, - const ReadOptions& options, - const BlockHandle& handle, - BlockContents* result) { +Status ReadBlockContents(RandomAccessFile* file, + const ReadOptions& options, + const BlockHandle& handle, + BlockContents* result) { result->data = Slice(); result->cachable = false; result->heap_allocated = false; diff --git a/table/format.h b/table/format.h index 6c0b80c017..7d72fa2402 100644 --- a/table/format.h +++ b/table/format.h @@ -91,10 +91,10 @@ struct BlockContents { // Read the block identified by "handle" from "file". On failure // return non-OK. On success fill *result and return OK. -extern Status ReadBlock(RandomAccessFile* file, - const ReadOptions& options, - const BlockHandle& handle, - BlockContents* result); +extern Status ReadBlockContents(RandomAccessFile* file, + const ReadOptions& options, + const BlockHandle& handle, + BlockContents* result); // Implementation details follow. Clients should ignore, diff --git a/table/table.cc b/table/table.cc index 9bf72f76dd..b9971f464d 100644 --- a/table/table.cc +++ b/table/table.cc @@ -64,6 +64,32 @@ void Table::SetupCacheKeyPrefix(Rep* rep) { } } +namespace { // anonymous namespace, not visible externally + +// Read the block identified by "handle" from "file". +// The only relevant option is options.verify_checksums for now. +// Set *didIO to true if didIO is not null. +// On failure return non-OK. +// On success fill *result and return OK - caller owns *result +Status ReadBlock(RandomAccessFile* file, + const ReadOptions& options, + const BlockHandle& handle, + Block** result, + bool* didIO = nullptr) { + BlockContents contents; + Status s = ReadBlockContents(file, options, handle, &contents); + if (s.ok()) { + *result = new Block(contents); + } + + if (didIO) { + *didIO = true; + } + return s; +} + +} // end of anonymous namespace + Status Table::Open(const Options& options, const EnvOptions& soptions, unique_ptr&& file, @@ -91,15 +117,9 @@ Status Table::Open(const Options& options, s = footer.DecodeFrom(&footer_input); if (!s.ok()) return s; - // Read the index block - BlockContents contents; Block* index_block = nullptr; - if (s.ok()) { - s = ReadBlock(file.get(), ReadOptions(), footer.index_handle(), &contents); - if (s.ok()) { - index_block = new Block(contents); - } - } + // TODO: we never really verify check sum for index block + s = ReadBlock(file.get(), ReadOptions(), footer.index_handle(), &index_block); if (s.ok()) { // We've successfully read the footer and the index block: we're @@ -128,14 +148,13 @@ void Table::ReadMeta(const Footer& footer) { // TODO(sanjay): Skip this if footer.metaindex_handle() size indicates // it is an empty block. - ReadOptions opt; - BlockContents contents; - if (!ReadBlock(rep_->file.get(), opt, footer.metaindex_handle(), - &contents).ok()) { + // TODO: we never really verify check sum for meta index block + Block* meta = nullptr; + if (!ReadBlock(rep_->file.get(), ReadOptions(), footer.metaindex_handle(), + &meta).ok()) { // Do not propagate errors since meta info is not needed for operation return; } - Block* meta = new Block(contents); Iterator* iter = meta->NewIterator(BytewiseComparator()); std::string key = "filter."; @@ -155,11 +174,11 @@ void Table::ReadFilter(const Slice& filter_handle_value) { return; } - // We might want to unify with ReadBlock() if we start + // TODO: We might want to unify with ReadBlock() if we start // requiring checksum verification in Table::Open. ReadOptions opt; BlockContents block; - if (!ReadBlock(rep_->file.get(), opt, filter_handle, &block).ok()) { + if (!ReadBlockContents(rep_->file.get(), opt, filter_handle, &block).ok()) { return; } if (block.heap_allocated) { @@ -206,7 +225,6 @@ Iterator* Table::BlockReader(void* arg, // can add more features in the future. if (s.ok()) { - BlockContents contents; if (block_cache != nullptr) { char cache_key[kMaxCacheKeyPrefixSize + kMaxVarint64Length]; const size_t cache_key_prefix_size = table->rep_->cache_key_prefix_size; @@ -223,28 +241,18 @@ Iterator* Table::BlockReader(void* arg, RecordTick(statistics, BLOCK_CACHE_HIT); } else { - s = ReadBlock(table->rep_->file.get(), options, handle, &contents); + s = ReadBlock(table->rep_->file.get(), options, handle, &block, didIO); if (s.ok()) { - block = new Block(contents); - if (contents.cachable && options.fill_cache) { + if (block->isCachable() && options.fill_cache) { cache_handle = block_cache->Insert( - key, block, block->size(), &DeleteCachedBlock); + key, block, block->size(), &DeleteCachedBlock); } } - if (didIO != nullptr) { - *didIO = true; // we did some io from storage - } RecordTick(statistics, BLOCK_CACHE_MISS); } } else { - s = ReadBlock(table->rep_->file.get(), options, handle, &contents); - if (s.ok()) { - block = new Block(contents); - } - if (didIO != nullptr) { - *didIO = true; // we did some io from storage - } + s = ReadBlock(table->rep_->file.get(), options, handle, &block, didIO); } }