From ccaca59bee61ccb98c78241bf04e365957169aa9 Mon Sep 17 00:00:00 2001 From: Lei Jin Date: Fri, 25 Apr 2014 12:23:07 -0700 Subject: [PATCH] avoid calling FindFile twice in TwoLevelIterator for PlainTable Summary: this is to reclaim the regression introduced in https://reviews.facebook.net/D17853 Test Plan: make all check Reviewers: igor, haobo, sdong, dhruba, yhchiang Reviewed By: haobo CC: leveldb Differential Revision: https://reviews.facebook.net/D17985 --- db/db_test.cc | 2 ++ db/table_cache.cc | 27 --------------------------- db/table_cache.h | 7 ------- db/version_set.cc | 7 ++++--- table/block_based_table_reader.h | 2 +- table/plain_table_factory.h | 3 ++- table/table_reader.h | 8 -------- table/two_level_iterator.cc | 18 ++++++++++-------- table/two_level_iterator.h | 7 ++++--- 9 files changed, 23 insertions(+), 58 deletions(-) diff --git a/db/db_test.cc b/db/db_test.cc index acccce201..188cfff3d 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -4421,6 +4421,8 @@ TEST(DBTest, HiddenValuesAreRemoved) { TEST(DBTest, CompactBetweenSnapshots) { do { + Options options = CurrentOptions(); + options.disable_auto_compactions = true; CreateAndReopenWithCF({"pikachu"}); Random rnd(301); FillLevels("a", "z", 1); diff --git a/db/table_cache.cc b/db/table_cache.cc index 3208b4692..2321d035a 100644 --- a/db/table_cache.cc +++ b/db/table_cache.cc @@ -190,33 +190,6 @@ Status TableCache::GetTableProperties( return s; } -bool TableCache::PrefixMayMatch(const ReadOptions& options, - const InternalKeyComparator& icomparator, - const FileMetaData& file_meta, - const Slice& internal_key, bool* table_io) { - bool may_match = true; - auto table_reader = file_meta.table_reader; - Cache::Handle* table_handle = nullptr; - if (table_reader == nullptr) { - // Need to get table handle from file number - Status s = FindTable(storage_options_, icomparator, file_meta.number, - file_meta.file_size, &table_handle, table_io); - if (!s.ok()) { - return may_match; - } - table_reader = GetTableReaderFromHandle(table_handle); - } - - may_match = table_reader->PrefixMayMatch(internal_key); - - if (table_handle != nullptr) { - // Need to release handle if it is generated from here. - ReleaseHandle(table_handle); - } - - return may_match; -} - void TableCache::Evict(Cache* cache, uint64_t file_number) { cache->Erase(GetSliceForFileNumber(&file_number)); } diff --git a/db/table_cache.h b/db/table_cache.h index 97e0f6a27..e8cd7ea2e 100644 --- a/db/table_cache.h +++ b/db/table_cache.h @@ -56,13 +56,6 @@ class TableCache { const Slice&, bool), bool* table_io, void (*mark_key_may_exist)(void*) = nullptr); - // Determine whether the table may contain the specified prefix. If - // the table index or blooms are not in memory, this may cause an I/O - bool PrefixMayMatch(const ReadOptions& options, - const InternalKeyComparator& internal_comparator, - const FileMetaData& file_meta, - const Slice& internal_prefix, bool* table_io); - // Evict any entry for the specified file number static void Evict(Cache* cache, uint64_t file_number); diff --git a/db/version_set.cc b/db/version_set.cc index 815021af6..b85094d91 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -31,6 +31,7 @@ #include "table/merger.h" #include "table/two_level_iterator.h" #include "table/format.h" +#include "table/plain_table_factory.h" #include "table/meta_blocks.h" #include "util/coding.h" #include "util/logging.h" @@ -308,13 +309,13 @@ Status Version::GetPropertiesOfAllTables(TablePropertiesCollection* props) { return Status::OK(); } -void Version::AddIterators(const ReadOptions& options, +void Version::AddIterators(const ReadOptions& read_options, const EnvOptions& soptions, std::vector* iters) { // Merge all level zero files together since they may overlap for (const FileMetaData* file : files_[0]) { iters->push_back(cfd_->table_cache()->NewIterator( - options, soptions, cfd_->internal_comparator(), *file)); + read_options, soptions, cfd_->internal_comparator(), *file)); } // For levels > 0, we can use a concatenating iterator that sequentially @@ -323,7 +324,7 @@ void Version::AddIterators(const ReadOptions& options, for (int level = 1; level < num_levels_; level++) { if (!files_[level].empty()) { iters->push_back(NewTwoLevelIterator(new LevelFileIteratorState( - cfd_->table_cache(), options, soptions, + cfd_->table_cache(), read_options, soptions, cfd_->internal_comparator(), false /* for_compaction */, cfd_->options()->prefix_extractor != nullptr), new LevelFileNumIterator(cfd_->internal_comparator(), &files_[level]))); diff --git a/table/block_based_table_reader.h b/table/block_based_table_reader.h index 23f6cb149..fbe47272e 100644 --- a/table/block_based_table_reader.h +++ b/table/block_based_table_reader.h @@ -63,7 +63,7 @@ class BlockBasedTable : public TableReader { unique_ptr&& file, uint64_t file_size, unique_ptr* table_reader); - bool PrefixMayMatch(const Slice& internal_key) override; + bool PrefixMayMatch(const Slice& internal_key); // Returns a new iterator over the table contents. // The result of NewIterator() is initially invalid (caller must diff --git a/table/plain_table_factory.h b/table/plain_table_factory.h index b23620785..84af22fb9 100644 --- a/table/plain_table_factory.h +++ b/table/plain_table_factory.h @@ -2,8 +2,9 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. See the AUTHORS file for names of contributors. -#ifndef ROCKSDB_LITE #pragma once + +#ifndef ROCKSDB_LITE #include #include diff --git a/table/table_reader.h b/table/table_reader.h index 118d3100f..02a2d16dc 100644 --- a/table/table_reader.h +++ b/table/table_reader.h @@ -25,14 +25,6 @@ class TableReader { public: virtual ~TableReader() {} - // Determine whether there is a chance that the current table file - // contains the key a key starting with iternal_prefix. The specific - // table implementation can use bloom filter and/or other heuristic - // to filter out this table as a whole. - virtual bool PrefixMayMatch(const Slice& internal_prefix) { - return true; - } - // Returns a new iterator over the table contents. // The result of NewIterator() is initially invalid (caller must // call one of the Seek methods on the iterator before using it). diff --git a/table/two_level_iterator.cc b/table/two_level_iterator.cc index 7c3c99d42..990f18184 100644 --- a/table/two_level_iterator.cc +++ b/table/two_level_iterator.cc @@ -77,16 +77,18 @@ TwoLevelIterator::TwoLevelIterator(TwoLevelIteratorState* state, : state_(state), first_level_iter_(first_level_iter) {} void TwoLevelIterator::Seek(const Slice& target) { - if (state_->prefix_enabled && !state_->PrefixMayMatch(target)) { + if (state_->check_prefix_may_match && + !state_->PrefixMayMatch(target)) { SetSecondLevelIterator(nullptr); - } else { - first_level_iter_.Seek(target); - InitDataBlock(); - if (second_level_iter_.iter() != nullptr) { - second_level_iter_.Seek(target); - } - SkipEmptyDataBlocksForward(); + return; } + first_level_iter_.Seek(target); + + InitDataBlock(); + if (second_level_iter_.iter() != nullptr) { + second_level_iter_.Seek(target); + } + SkipEmptyDataBlocksForward(); } void TwoLevelIterator::SeekToFirst() { diff --git a/table/two_level_iterator.h b/table/two_level_iterator.h index ac9f3d6a0..b8083385b 100644 --- a/table/two_level_iterator.h +++ b/table/two_level_iterator.h @@ -18,14 +18,15 @@ struct ReadOptions; class InternalKeyComparator; struct TwoLevelIteratorState { - explicit TwoLevelIteratorState(bool prefix_enabled) - : prefix_enabled(prefix_enabled) {} + explicit TwoLevelIteratorState(bool check_prefix_may_match) + : check_prefix_may_match(check_prefix_may_match) {} virtual ~TwoLevelIteratorState() {} virtual Iterator* NewSecondaryIterator(const Slice& handle) = 0; virtual bool PrefixMayMatch(const Slice& internal_key) = 0; - bool prefix_enabled; + // If call PrefixMayMatch() + bool check_prefix_may_match; };