From 44653c7b7aabe821e671946e732dda7ae6b43d1b Mon Sep 17 00:00:00 2001 From: Zhongyi Xie Date: Mon, 2 Apr 2018 19:45:31 -0700 Subject: [PATCH] =?UTF-8?q?Revert=20"Avoid=20adding=20tombstones=20of=20th?= =?UTF-8?q?e=20same=20file=20to=20RangeDelAggregato=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: …r multiple times" This reverts commit e80709a33a2fc05cf85a89ade1f17944197e5451. 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 :smiley: 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 --- db/range_del_aggregator.cc | 7 ------- db/range_del_aggregator.h | 6 ------ db/table_cache.cc | 16 +++++++--------- 3 files changed, 7 insertions(+), 22 deletions(-) diff --git a/db/range_del_aggregator.cc b/db/range_del_aggregator.cc index 10d22fbb6..fdd847a7a 100644 --- a/db/range_del_aggregator.cc +++ b/db/range_del_aggregator.cc @@ -536,11 +536,4 @@ bool RangeDelAggregator::IsEmpty() { return true; } -bool RangeDelAggregator::AddFile(uint64_t file_number) { - if (added_files_ == nullptr) { - added_files_.reset(new std::set()); - } - return added_files_->emplace(file_number).second; -} - } // namespace rocksdb diff --git a/db/range_del_aggregator.h b/db/range_del_aggregator.h index 1c1402d29..f050e8917 100644 --- a/db/range_del_aggregator.h +++ b/db/range_del_aggregator.h @@ -6,7 +6,6 @@ #pragma once #include -#include #include #include @@ -141,7 +140,6 @@ class RangeDelAggregator { CompactionIterationStats* range_del_out_stats = nullptr, bool bottommost_level = false); bool IsEmpty(); - bool AddFile(uint64_t file_number); private: // Maps tombstone user start key -> tombstone object @@ -182,10 +180,6 @@ class RangeDelAggregator { const InternalKeyComparator& icmp_; // collapse range deletions so they're binary searchable 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> added_files_; }; } // namespace rocksdb diff --git a/db/table_cache.cc b/db/table_cache.cc index a3c02fa8b..a4f2b2ceb 100644 --- a/db/table_cache.cc +++ b/db/table_cache.cc @@ -247,15 +247,13 @@ InternalIterator* TableCache::NewIterator( } } if (s.ok() && range_del_agg != nullptr && !options.ignore_range_deletions) { - if (range_del_agg->AddFile(fd.GetNumber())) { - std::unique_ptr range_del_iter( - table_reader->NewRangeTombstoneIterator(options)); - if (range_del_iter != nullptr) { - s = range_del_iter->status(); - } - if (s.ok()) { - s = range_del_agg->AddTombstones(std::move(range_del_iter)); - } + std::unique_ptr range_del_iter( + table_reader->NewRangeTombstoneIterator(options)); + if (range_del_iter != nullptr) { + s = range_del_iter->status(); + } + if (s.ok()) { + s = range_del_agg->AddTombstones(std::move(range_del_iter)); } }