From ff768956146d7f1c38c465568d34bd048f48062d Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Wed, 17 Sep 2014 16:45:58 -0700 Subject: [PATCH] Remove some unnecessary constructors Summary: This is continuing the work done by https://github.com/facebook/rocksdb/commit/27b22f13a300c00c72de7fc826fdae21a734eb49 It's just cleaning up some unnecessary constructors. The most important change is removing Block::Block(const BlockContents& contents) constructor. It was only used from the unit test. Test Plan: compiles Reviewers: sdong, yhchiang, ljin Reviewed By: ljin Subscribers: leveldb Differential Revision: https://reviews.facebook.net/D23547 --- table/block.cc | 10 ++++----- table/block.h | 5 ++--- table/block_based_filter_block.cc | 20 +++++++----------- table/block_based_filter_block.h | 3 --- table/block_based_filter_block_test.cc | 28 +++++++++++++------------- table/block_test.cc | 12 ++++++----- 6 files changed, 34 insertions(+), 44 deletions(-) diff --git a/table/block.cc b/table/block.cc index c3066cf5b..592d175b1 100644 --- a/table/block.cc +++ b/table/block.cc @@ -297,8 +297,10 @@ uint32_t Block::NumRestarts() const { return DecodeFixed32(data_ + size_ - sizeof(uint32_t)); } -Block::Block(const BlockContents& contents) - : data_(contents.data.data()), size_(contents.data.size()) { +Block::Block(BlockContents&& contents) + : contents_(std::move(contents)), + data_(contents_.data.data()), + size_(contents_.data.size()) { if (size_ < sizeof(uint32_t)) { size_ = 0; // Error marker } else { @@ -311,10 +313,6 @@ Block::Block(const BlockContents& contents) } } -Block::Block(BlockContents&& contents) : Block(contents) { - contents_ = std::move(contents); -} - Iterator* Block::NewIterator( const Comparator* cmp, BlockIter* iter, bool total_order_seek) { if (size_ < 2*sizeof(uint32_t)) { diff --git a/table/block.h b/table/block.h index b86b615bc..68b16ea1f 100644 --- a/table/block.h +++ b/table/block.h @@ -31,7 +31,6 @@ class Block { public: // Initialize the block with the specified contents. explicit Block(BlockContents&& contents); - explicit Block(const BlockContents& contents); ~Block() = default; @@ -66,8 +65,8 @@ class Block { private: BlockContents contents_; - const char* data_; - size_t size_; + const char* data_; // contents_.data.data() + size_t size_; // contents_.data.size() uint32_t restart_offset_; // Offset in data_ of restart array std::unique_ptr hash_index_; std::unique_ptr prefix_index_; diff --git a/table/block_based_filter_block.cc b/table/block_based_filter_block.cc index 05d5beb88..fea37b67f 100644 --- a/table/block_based_filter_block.cc +++ b/table/block_based_filter_block.cc @@ -137,32 +137,26 @@ void BlockBasedFilterBlockBuilder::GenerateFilter() { BlockBasedFilterBlockReader::BlockBasedFilterBlockReader( const SliceTransform* prefix_extractor, - const BlockBasedTableOptions& table_opt, const Slice& contents) + const BlockBasedTableOptions& table_opt, BlockContents&& contents) : policy_(table_opt.filter_policy.get()), prefix_extractor_(prefix_extractor), whole_key_filtering_(table_opt.whole_key_filtering), data_(nullptr), offset_(nullptr), num_(0), - base_lg_(0) { + base_lg_(0), + contents_(std::move(contents)) { assert(policy_); - size_t n = contents.size(); + size_t n = contents_.data.size(); if (n < 5) return; // 1 byte for base_lg_ and 4 for start of offset array - base_lg_ = contents[n - 1]; - uint32_t last_word = DecodeFixed32(contents.data() + n - 5); + base_lg_ = contents_.data[n - 1]; + uint32_t last_word = DecodeFixed32(contents_.data.data() + n - 5); if (last_word > n - 5) return; - data_ = contents.data(); + data_ = contents_.data.data(); offset_ = data_ + last_word; num_ = (n - 5 - last_word) / 4; } -BlockBasedFilterBlockReader::BlockBasedFilterBlockReader( - const SliceTransform* prefix_extractor, - const BlockBasedTableOptions& table_opt, BlockContents&& contents) - : BlockBasedFilterBlockReader(prefix_extractor, table_opt, contents.data) { - contents_ = std::move(contents); -} - bool BlockBasedFilterBlockReader::KeyMayMatch(const Slice& key, uint64_t block_offset) { assert(block_offset != kNotValid); diff --git a/table/block_based_filter_block.h b/table/block_based_filter_block.h index 856b88910..9621425e3 100644 --- a/table/block_based_filter_block.h +++ b/table/block_based_filter_block.h @@ -72,9 +72,6 @@ class BlockBasedFilterBlockBuilder : public FilterBlockBuilder { class BlockBasedFilterBlockReader : public FilterBlockReader { public: // REQUIRES: "contents" and *policy must stay live while *this is live. - BlockBasedFilterBlockReader(const SliceTransform* prefix_extractor, - const BlockBasedTableOptions& table_opt, - const Slice& contents); BlockBasedFilterBlockReader(const SliceTransform* prefix_extractor, const BlockBasedTableOptions& table_opt, BlockContents&& contents); diff --git a/table/block_based_filter_block_test.cc b/table/block_based_filter_block_test.cc index 4fd8c1cf5..28eea16ce 100644 --- a/table/block_based_filter_block_test.cc +++ b/table/block_based_filter_block_test.cc @@ -55,9 +55,9 @@ class FilterBlockTest { TEST(FilterBlockTest, EmptyBuilder) { BlockBasedFilterBlockBuilder builder(nullptr, table_options_); - Slice block = builder.Finish(); - ASSERT_EQ("\\x00\\x00\\x00\\x00\\x0b", EscapeString(block)); - BlockBasedFilterBlockReader reader(nullptr, table_options_, block); + BlockContents block(builder.Finish(), false, kNoCompression); + ASSERT_EQ("\\x00\\x00\\x00\\x00\\x0b", EscapeString(block.data)); + BlockBasedFilterBlockReader reader(nullptr, table_options_, std::move(block)); ASSERT_TRUE(reader.KeyMayMatch("foo", 0)); ASSERT_TRUE(reader.KeyMayMatch("foo", 100000)); } @@ -72,8 +72,8 @@ TEST(FilterBlockTest, SingleChunk) { builder.Add("box"); builder.StartBlock(300); builder.Add("hello"); - Slice block = builder.Finish(); - BlockBasedFilterBlockReader reader(nullptr, table_options_, block); + BlockContents block(builder.Finish(), false, kNoCompression); + BlockBasedFilterBlockReader reader(nullptr, table_options_, std::move(block)); ASSERT_TRUE(reader.KeyMayMatch("foo", 100)); ASSERT_TRUE(reader.KeyMayMatch("bar", 100)); ASSERT_TRUE(reader.KeyMayMatch("box", 100)); @@ -103,8 +103,8 @@ TEST(FilterBlockTest, MultiChunk) { builder.Add("box"); builder.Add("hello"); - Slice block = builder.Finish(); - BlockBasedFilterBlockReader reader(nullptr, table_options_, block); + BlockContents block(builder.Finish(), false, kNoCompression); + BlockBasedFilterBlockReader reader(nullptr, table_options_, std::move(block)); // Check first filter ASSERT_TRUE(reader.KeyMayMatch("foo", 0)); @@ -147,10 +147,10 @@ class BlockBasedFilterBlockTest { TEST(BlockBasedFilterBlockTest, BlockBasedEmptyBuilder) { FilterBlockBuilder* builder = new BlockBasedFilterBlockBuilder( nullptr, table_options_); - Slice block = builder->Finish(); - ASSERT_EQ("\\x00\\x00\\x00\\x00\\x0b", EscapeString(block)); + BlockContents block(builder->Finish(), false, kNoCompression); + ASSERT_EQ("\\x00\\x00\\x00\\x00\\x0b", EscapeString(block.data)); FilterBlockReader* reader = new BlockBasedFilterBlockReader( - nullptr, table_options_, block); + nullptr, table_options_, std::move(block)); ASSERT_TRUE(reader->KeyMayMatch("foo", 0)); ASSERT_TRUE(reader->KeyMayMatch("foo", 100000)); @@ -169,9 +169,9 @@ TEST(BlockBasedFilterBlockTest, BlockBasedSingleChunk) { builder->Add("box"); builder->StartBlock(300); builder->Add("hello"); - Slice block = builder->Finish(); + BlockContents block(builder->Finish(), false, kNoCompression); FilterBlockReader* reader = new BlockBasedFilterBlockReader( - nullptr, table_options_, block); + nullptr, table_options_, std::move(block)); ASSERT_TRUE(reader->KeyMayMatch("foo", 100)); ASSERT_TRUE(reader->KeyMayMatch("bar", 100)); ASSERT_TRUE(reader->KeyMayMatch("box", 100)); @@ -205,9 +205,9 @@ TEST(BlockBasedFilterBlockTest, BlockBasedMultiChunk) { builder->Add("box"); builder->Add("hello"); - Slice block = builder->Finish(); + BlockContents block(builder->Finish(), false, kNoCompression); FilterBlockReader* reader = new BlockBasedFilterBlockReader( - nullptr, table_options_, block); + nullptr, table_options_, std::move(block)); // Check first filter ASSERT_TRUE(reader->KeyMayMatch("foo", 0)); diff --git a/table/block_test.cc b/table/block_test.cc index c341617a7..6b82c4d93 100644 --- a/table/block_test.cc +++ b/table/block_test.cc @@ -146,13 +146,15 @@ BlockContents GetBlockContents(std::unique_ptr *builder, return contents; } -void CheckBlockContents(const BlockContents &contents, const int max_key, +void CheckBlockContents(BlockContents contents, const int max_key, const std::vector &keys, const std::vector &values) { const size_t prefix_size = 6; // create block reader - Block reader1(contents); - Block reader2(contents); + BlockContents contents_ref(contents.data, contents.cachable, + contents.compression_type); + Block reader1(std::move(contents)); + Block reader2(std::move(contents_ref)); std::unique_ptr prefix_extractor( NewFixedPrefixTransform(prefix_size)); @@ -210,7 +212,7 @@ TEST(BlockTest, SimpleIndexHash) { std::unique_ptr builder; auto contents = GetBlockContents(&builder, keys, values); - CheckBlockContents(contents, kMaxKey, keys, values); + CheckBlockContents(std::move(contents), kMaxKey, keys, values); } TEST(BlockTest, IndexHashWithSharedPrefix) { @@ -229,7 +231,7 @@ TEST(BlockTest, IndexHashWithSharedPrefix) { std::unique_ptr builder; auto contents = GetBlockContents(&builder, keys, values, kPrefixGroup); - CheckBlockContents(contents, kMaxKey, keys, values); + CheckBlockContents(std::move(contents), kMaxKey, keys, values); } } // namespace rocksdb