Revert "Avoid adding tombstones of the same file to RangeDelAggregato…

Summary:
…r multiple times"

This reverts commit e80709a33a.

lingbin PR https://github.com/facebook/rocksdb/pull/3635 is causing some performance regression for seekrandom workloads
I'm reverting the commit for now but feel free to submit new patches 😃

To reproduce the regression, you can run the following db_bench command
> ./db_bench --benchmarks=fillrandom,seekrandomwhilewriting --threads=1 --num=1000000 --reads=150000 --key_size=66 --value_size=1262 --statistics=0 --compression_ratio=0.5 --histogram=1 --seek_nexts=1 --stats_per_interval=1 --stats_interval_seconds=600 --max_background_flushes=4 --num_multi_db=1 --max_background_compactions=16 --seed=1522388277 -write_buffer_size=1048576 --level0_file_num_compaction_trigger=10000 --compression_type=none

write stats printed by db_bench:

Table | | | | | | | | | | |
 --- | --- | --- | --- | --- | --- | --- | --- | --- | --- | ---
revert commit | Percentiles: | P50: | 80.77  | P75: |102.94  |P99: | 1786.44 | P99.9: | 1892.39 |P99.99: 2645.10 |
keep commit | Percentiles: | P50: | 221.72 | P75: | 686.62 | P99: | 1842.57 | P99.9: | 1899.70|  P99.99: 2814.29|
Closes https://github.com/facebook/rocksdb/pull/3672

Differential Revision: D7463315

Pulled By: miasantreble

fbshipit-source-id: 8e779c87591127f2c3694b91a56d9b459011959d
main
Zhongyi Xie 7 years ago committed by Facebook Github Bot
parent 8917eee962
commit 44653c7b7a
  1. 7
      db/range_del_aggregator.cc
  2. 6
      db/range_del_aggregator.h
  3. 16
      db/table_cache.cc

@ -536,11 +536,4 @@ bool RangeDelAggregator::IsEmpty() {
return true; return true;
} }
bool RangeDelAggregator::AddFile(uint64_t file_number) {
if (added_files_ == nullptr) {
added_files_.reset(new std::set<uint64_t>());
}
return added_files_->emplace(file_number).second;
}
} // namespace rocksdb } // namespace rocksdb

@ -6,7 +6,6 @@
#pragma once #pragma once
#include <map> #include <map>
#include <set>
#include <string> #include <string>
#include <vector> #include <vector>
@ -141,7 +140,6 @@ class RangeDelAggregator {
CompactionIterationStats* range_del_out_stats = nullptr, CompactionIterationStats* range_del_out_stats = nullptr,
bool bottommost_level = false); bool bottommost_level = false);
bool IsEmpty(); bool IsEmpty();
bool AddFile(uint64_t file_number);
private: private:
// Maps tombstone user start key -> tombstone object // Maps tombstone user start key -> tombstone object
@ -182,10 +180,6 @@ class RangeDelAggregator {
const InternalKeyComparator& icmp_; const InternalKeyComparator& icmp_;
// collapse range deletions so they're binary searchable // collapse range deletions so they're binary searchable
const bool collapse_deletions_; const bool collapse_deletions_;
// Record files whose tombstones have been added, to avoid duplicate adding.
// Same as rep_, we initializes it lazily.
std::unique_ptr<std::set<uint64_t>> added_files_;
}; };
} // namespace rocksdb } // namespace rocksdb

@ -247,15 +247,13 @@ InternalIterator* TableCache::NewIterator(
} }
} }
if (s.ok() && range_del_agg != nullptr && !options.ignore_range_deletions) { if (s.ok() && range_del_agg != nullptr && !options.ignore_range_deletions) {
if (range_del_agg->AddFile(fd.GetNumber())) { std::unique_ptr<InternalIterator> range_del_iter(
std::unique_ptr<InternalIterator> range_del_iter( table_reader->NewRangeTombstoneIterator(options));
table_reader->NewRangeTombstoneIterator(options)); if (range_del_iter != nullptr) {
if (range_del_iter != nullptr) { s = range_del_iter->status();
s = range_del_iter->status(); }
} if (s.ok()) {
if (s.ok()) { s = range_del_agg->AddTombstones(std::move(range_del_iter));
s = range_del_agg->AddTombstones(std::move(range_del_iter));
}
} }
} }

Loading…
Cancel
Save