From 795e663df0d868754d7904025cd090998c986f5d Mon Sep 17 00:00:00 2001 From: Zhongyi Xie Date: Thu, 21 Jun 2018 21:16:49 -0700 Subject: [PATCH] option for timing measurement of non-blocking ops during compaction (#4029) Summary: For example calling CompactionFilter is always timed and gives the user no way to disable. This PR will disable the timer if `Statistics::stats_level_` (which is part of DBOptions) is `kExceptDetailedTimers` Closes https://github.com/facebook/rocksdb/pull/4029 Differential Revision: D8583670 Pulled By: miasantreble fbshipit-source-id: 913be9fe433ae0c06e88193b59d41920a532307f --- db/builder.cc | 2 ++ db/compaction_iterator.cc | 13 ++++++++----- db/compaction_iterator.h | 5 +++-- db/compaction_iterator_test.cc | 5 +++-- db/compaction_job.cc | 6 +++--- db/db_test.cc | 1 + db/merge_helper.cc | 3 ++- 7 files changed, 22 insertions(+), 13 deletions(-) diff --git a/db/builder.cc b/db/builder.cc index e57ad0208..cb45bb093 100644 --- a/db/builder.cc +++ b/db/builder.cc @@ -28,6 +28,7 @@ #include "rocksdb/options.h" #include "rocksdb/table.h" #include "table/block_based_table_builder.h" +#include "table/format.h" #include "table/internal_iterator.h" #include "util/file_reader_writer.h" #include "util/filename.h" @@ -139,6 +140,7 @@ Status BuildTable( CompactionIterator c_iter( iter, internal_comparator.user_comparator(), &merge, kMaxSequenceNumber, &snapshots, earliest_write_conflict_snapshot, snapshot_checker, env, + ShouldReportDetailedTime(env, ioptions.statistics), true /* internal key corruption is not ok */, range_del_agg.get()); c_iter.SeekToFirst(); for (; c_iter.Valid(); c_iter.Next()) { diff --git a/db/compaction_iterator.cc b/db/compaction_iterator.cc index 3d62f43a4..bf7595495 100644 --- a/db/compaction_iterator.cc +++ b/db/compaction_iterator.cc @@ -17,14 +17,15 @@ CompactionIterator::CompactionIterator( SequenceNumber last_sequence, std::vector* snapshots, SequenceNumber earliest_write_conflict_snapshot, const SnapshotChecker* snapshot_checker, Env* env, - bool expect_valid_internal_key, RangeDelAggregator* range_del_agg, + bool report_detailed_time, bool expect_valid_internal_key, + RangeDelAggregator* range_del_agg, const Compaction* compaction, const CompactionFilter* compaction_filter, const std::atomic* shutting_down, const SequenceNumber preserve_deletes_seqnum) : CompactionIterator( input, cmp, merge_helper, last_sequence, snapshots, earliest_write_conflict_snapshot, snapshot_checker, env, - expect_valid_internal_key, range_del_agg, + report_detailed_time, expect_valid_internal_key, range_del_agg, std::unique_ptr( compaction ? new CompactionProxy(compaction) : nullptr), compaction_filter, shutting_down, preserve_deletes_seqnum) {} @@ -34,7 +35,8 @@ CompactionIterator::CompactionIterator( SequenceNumber /*last_sequence*/, std::vector* snapshots, SequenceNumber earliest_write_conflict_snapshot, const SnapshotChecker* snapshot_checker, Env* env, - bool expect_valid_internal_key, RangeDelAggregator* range_del_agg, + bool report_detailed_time, bool expect_valid_internal_key, + RangeDelAggregator* range_del_agg, std::unique_ptr compaction, const CompactionFilter* compaction_filter, const std::atomic* shutting_down, @@ -46,6 +48,7 @@ CompactionIterator::CompactionIterator( earliest_write_conflict_snapshot_(earliest_write_conflict_snapshot), snapshot_checker_(snapshot_checker), env_(env), + report_detailed_time_(report_detailed_time), expect_valid_internal_key_(expect_valid_internal_key), range_del_agg_(range_del_agg), compaction_(std::move(compaction)), @@ -171,12 +174,12 @@ void CompactionIterator::InvokeFilterIfNeeded(bool* need_skip, // to get sequence number. Slice& filter_key = ikey_.type == kTypeValue ? ikey_.user_key : key_; { - StopWatchNano timer(env_, true); + StopWatchNano timer(env_, report_detailed_time_); filter = compaction_filter_->FilterV2( compaction_->level(), filter_key, value_type, value_, &compaction_filter_value_, compaction_filter_skip_until_.rep()); iter_stats_.total_filter_time += - env_ != nullptr ? timer.ElapsedNanos() : 0; + env_ != nullptr && report_detailed_time_ ? timer.ElapsedNanos() : 0; } if (filter == CompactionFilter::Decision::kRemoveAndSkipUntil && diff --git a/db/compaction_iterator.h b/db/compaction_iterator.h index 7732e08ae..71359169c 100644 --- a/db/compaction_iterator.h +++ b/db/compaction_iterator.h @@ -63,7 +63,7 @@ class CompactionIterator { std::vector* snapshots, SequenceNumber earliest_write_conflict_snapshot, const SnapshotChecker* snapshot_checker, Env* env, - bool expect_valid_internal_key, + bool report_detailed_time, bool expect_valid_internal_key, RangeDelAggregator* range_del_agg, const Compaction* compaction = nullptr, const CompactionFilter* compaction_filter = nullptr, @@ -76,7 +76,7 @@ class CompactionIterator { std::vector* snapshots, SequenceNumber earliest_write_conflict_snapshot, const SnapshotChecker* snapshot_checker, Env* env, - bool expect_valid_internal_key, + bool report_detailed_time, bool expect_valid_internal_key, RangeDelAggregator* range_del_agg, std::unique_ptr compaction, const CompactionFilter* compaction_filter = nullptr, @@ -139,6 +139,7 @@ class CompactionIterator { const SequenceNumber earliest_write_conflict_snapshot_; const SnapshotChecker* const snapshot_checker_; Env* env_; + bool report_detailed_time_; bool expect_valid_internal_key_; RangeDelAggregator* range_del_agg_; std::unique_ptr compaction_; diff --git a/db/compaction_iterator_test.cc b/db/compaction_iterator_test.cc index 1402c358e..aa929f19f 100644 --- a/db/compaction_iterator_test.cc +++ b/db/compaction_iterator_test.cc @@ -247,8 +247,9 @@ class CompactionIteratorTest : public testing::TestWithParam { c_iter_.reset(new CompactionIterator( iter_.get(), cmp_, merge_helper_.get(), last_sequence, &snapshots_, earliest_write_conflict_snapshot, snapshot_checker_.get(), - Env::Default(), false, range_del_agg_.get(), std::move(compaction), - filter, &shutting_down_)); + Env::Default(), false /* report_detailed_time */, + false, range_del_agg_.get(), std::move(compaction), filter, + &shutting_down_)); } void AddSnapshot(SequenceNumber snapshot, diff --git a/db/compaction_job.cc b/db/compaction_job.cc index 8b9fe7a97..12aa1d41b 100644 --- a/db/compaction_job.cc +++ b/db/compaction_job.cc @@ -879,9 +879,9 @@ void CompactionJob::ProcessKeyValueCompaction(SubcompactionState* sub_compact) { sub_compact->c_iter.reset(new CompactionIterator( input.get(), cfd->user_comparator(), &merge, versions_->LastSequence(), &existing_snapshots_, earliest_write_conflict_snapshot_, - snapshot_checker_, env_, false, range_del_agg.get(), - sub_compact->compaction, compaction_filter, shutting_down_, - preserve_deletes_seqnum_)); + snapshot_checker_, env_, ShouldReportDetailedTime(env_, stats_), false, + range_del_agg.get(), sub_compact->compaction, compaction_filter, + shutting_down_, preserve_deletes_seqnum_)); auto c_iter = sub_compact->c_iter.get(); c_iter->SeekToFirst(); if (c_iter->Valid() && sub_compact->compaction->output_level() != 0) { diff --git a/db/db_test.cc b/db/db_test.cc index 14215be97..afff08673 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -5099,6 +5099,7 @@ TEST_P(DBTestWithParam, FilterCompactionTimeTest) { options.disable_auto_compactions = true; options.create_if_missing = true; options.statistics = rocksdb::CreateDBStatistics(); + options.statistics->stats_level_ = kExceptTimeForMutex; options.max_subcompactions = max_subcompactions_; DestroyAndReopen(options); diff --git a/db/merge_helper.cc b/db/merge_helper.cc index a1489dedc..f2d4fa450 100644 --- a/db/merge_helper.cc +++ b/db/merge_helper.cc @@ -14,6 +14,7 @@ #include "rocksdb/comparator.h" #include "rocksdb/db.h" #include "rocksdb/merge_operator.h" +#include "table/format.h" #include "table/internal_iterator.h" namespace rocksdb { @@ -372,7 +373,7 @@ CompactionFilter::Decision MergeHelper::FilterMerge(const Slice& user_key, if (compaction_filter_ == nullptr) { return CompactionFilter::Decision::kKeep; } - if (stats_ != nullptr) { + if (stats_ != nullptr && ShouldReportDetailedTime(env_, stats_)) { filter_timer_.Start(); } compaction_filter_value_.clear();