From aef763b6d6c215e9e914da410ef273de1d40406d Mon Sep 17 00:00:00 2001 From: Siying Dong Date: Fri, 1 Mar 2019 10:39:00 -0800 Subject: [PATCH] Make statistics's stats_level change thread-safe (#5030) Summary: Right now, users can change statistics.stats_level while DB is running, but TSAN may report data race. We make stats_level_ to be atomic, and access them using accessors. Pull Request resolved: https://github.com/facebook/rocksdb/pull/5030 Differential Revision: D14267519 Pulled By: siying fbshipit-source-id: 37d7ebeff7a43a406230143422a16af899163f73 --- HISTORY.md | 3 +++ db/db_statistics_test.cc | 4 ++-- db/db_test.cc | 2 +- include/rocksdb/statistics.h | 11 +++++++++-- java/rocksjni/statistics.cc | 4 ++-- monitoring/instrumented_mutex.cc | 2 +- monitoring/statistics.cc | 2 +- table/format.cc | 2 +- table/table_test.cc | 4 ++-- tools/db_bench_tool.cc | 2 +- util/stop_watch.h | 3 ++- 11 files changed, 25 insertions(+), 14 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index ed37c3355..1fe0335e1 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -3,6 +3,9 @@ ### New Features * Introduce two more stats levels, kExceptHistogramOrTimers and kExceptTimers. +### Public API Change +* statistics.stats_level_ becomes atomic. It is preferred to use statistics.set_stats_level() and statistics.get_stats_level() to access it. + ## 6.0.0 (2/19/2019) ### New Features * Enabled checkpoint on readonly db (DBImplReadOnly). diff --git a/db/db_statistics_test.cc b/db/db_statistics_test.cc index 237a2c681..31396a7bf 100644 --- a/db/db_statistics_test.cc +++ b/db/db_statistics_test.cc @@ -46,7 +46,7 @@ TEST_F(DBStatisticsTest, CompressionStatsTest) { Options options = CurrentOptions(); options.compression = type; options.statistics = rocksdb::CreateDBStatistics(); - options.statistics->stats_level_ = StatsLevel::kExceptTimeForMutex; + options.statistics->set_stats_level(StatsLevel::kExceptTimeForMutex); DestroyAndReopen(options); int kNumKeysWritten = 100000; @@ -105,7 +105,7 @@ TEST_F(DBStatisticsTest, MutexWaitStats) { Options options = CurrentOptions(); options.create_if_missing = true; options.statistics = rocksdb::CreateDBStatistics(); - options.statistics->stats_level_ = StatsLevel::kAll; + options.statistics->set_stats_level(StatsLevel::kAll); CreateAndReopenWithCF({"pikachu"}, options); const uint64_t kMutexWaitDelay = 100; ThreadStatusUtil::TEST_SetStateDelay(ThreadStatus::STATE_MUTEX_WAIT, diff --git a/db/db_test.cc b/db/db_test.cc index 940391ef3..60e66c6c3 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -5102,7 +5102,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.statistics->set_stats_level(kExceptTimeForMutex); options.max_subcompactions = max_subcompactions_; DestroyAndReopen(options); diff --git a/include/rocksdb/statistics.h b/include/rocksdb/statistics.h index 254da1dc4..fce40bcc0 100644 --- a/include/rocksdb/statistics.h +++ b/include/rocksdb/statistics.h @@ -474,7 +474,7 @@ class Statistics { virtual void setTickerCount(uint32_t tickerType, uint64_t count) = 0; virtual uint64_t getAndResetTickerCount(uint32_t tickerType) = 0; virtual void reportTimeToHistogram(uint32_t histogramType, uint64_t time) { - if (stats_level_ <= StatsLevel::kExceptTimers) { + if (get_stats_level() <= StatsLevel::kExceptTimers) { return; } recordInHistogram(histogramType, time); @@ -514,8 +514,15 @@ class Statistics { virtual bool HistEnabledForType(uint32_t type) const { return type < HISTOGRAM_ENUM_MAX; } + void set_stats_level(StatsLevel sl) { + stats_level_.store(sl, std::memory_order_relaxed); + } + StatsLevel get_stats_level() const { + return stats_level_.load(std::memory_order_relaxed); + } - StatsLevel stats_level_ = kExceptDetailedTimers; + private: + std::atomic stats_level_{kExceptDetailedTimers}; }; // Create a concrete DBStatistics object diff --git a/java/rocksjni/statistics.cc b/java/rocksjni/statistics.cc index 355b90bbf..ae7ad5352 100644 --- a/java/rocksjni/statistics.cc +++ b/java/rocksjni/statistics.cc @@ -118,7 +118,7 @@ jbyte Java_org_rocksdb_Statistics_statsLevel( reinterpret_cast*>(jhandle); assert(pSptr_statistics != nullptr); return rocksdb::StatsLevelJni::toJavaStatsLevel( - pSptr_statistics->get()->stats_level_); + pSptr_statistics->get()->get_stats_level()); } /* @@ -132,7 +132,7 @@ void Java_org_rocksdb_Statistics_setStatsLevel( reinterpret_cast*>(jhandle); assert(pSptr_statistics != nullptr); auto stats_level = rocksdb::StatsLevelJni::toCppStatsLevel(jstats_level); - pSptr_statistics->get()->stats_level_ = stats_level; + pSptr_statistics->get()->set_stats_level(stats_level); } /* diff --git a/monitoring/instrumented_mutex.cc b/monitoring/instrumented_mutex.cc index 2255b35ae..7b61bcf4f 100644 --- a/monitoring/instrumented_mutex.cc +++ b/monitoring/instrumented_mutex.cc @@ -12,7 +12,7 @@ namespace rocksdb { namespace { Statistics* stats_for_report(Env* env, Statistics* stats) { if (env != nullptr && stats != nullptr && - stats->stats_level_ > kExceptTimeForMutex) { + stats->get_stats_level() > kExceptTimeForMutex) { return stats; } else { return nullptr; diff --git a/monitoring/statistics.cc b/monitoring/statistics.cc index 9c8c6ee8c..adb8cbfed 100644 --- a/monitoring/statistics.cc +++ b/monitoring/statistics.cc @@ -324,7 +324,7 @@ void StatisticsImpl::recordTick(uint32_t tickerType, uint64_t count) { void StatisticsImpl::recordInHistogram(uint32_t histogramType, uint64_t value) { assert(histogramType < HISTOGRAM_ENUM_MAX); - if (stats_level_ <= StatsLevel::kExceptHistogramOrTimers) { + if (get_stats_level() <= StatsLevel::kExceptHistogramOrTimers) { return; } per_core_stats_.Access()->histograms_[histogramType].Add(value); diff --git a/table/format.cc b/table/format.cc index e0fbc3363..5bf7b09a0 100644 --- a/table/format.cc +++ b/table/format.cc @@ -45,7 +45,7 @@ const uint64_t kPlainTableMagicNumber = 0; bool ShouldReportDetailedTime(Env* env, Statistics* stats) { return env != nullptr && stats != nullptr && - stats->stats_level_ > kExceptDetailedTimers; + stats->get_stats_level() > kExceptDetailedTimers; } void BlockHandle::EncodeTo(std::string* dst) const { diff --git a/table/table_test.cc b/table/table_test.cc index c13c6d582..04e11de8e 100644 --- a/table/table_test.cc +++ b/table/table_test.cc @@ -1128,7 +1128,7 @@ TEST_P(BlockBasedTableTest, BasicBlockBasedTableProperties) { Options options; options.compression = kNoCompression; options.statistics = CreateDBStatistics(); - options.statistics->stats_level_ = StatsLevel::kAll; + options.statistics->set_stats_level(StatsLevel::kAll); BlockBasedTableOptions table_options = GetBlockBasedTableOptions(); table_options.block_restart_interval = 1; options.table_factory.reset(NewBlockBasedTableFactory(table_options)); @@ -1176,7 +1176,7 @@ uint64_t BlockBasedTableTest::IndexUncompressedHelper(bool compressed) { Options options; options.compression = kSnappyCompression; options.statistics = CreateDBStatistics(); - options.statistics->stats_level_ = StatsLevel::kAll; + options.statistics->set_stats_level(StatsLevel::kAll); BlockBasedTableOptions table_options = GetBlockBasedTableOptions(); table_options.block_restart_interval = 1; table_options.enable_index_compression = compressed; diff --git a/tools/db_bench_tool.cc b/tools/db_bench_tool.cc index 16ce30736..1b2e5c208 100644 --- a/tools/db_bench_tool.cc +++ b/tools/db_bench_tool.cc @@ -6082,7 +6082,7 @@ int db_bench_tool(int argc, char** argv) { dbstats = rocksdb::CreateDBStatistics(); } if (dbstats) { - dbstats->stats_level_ = static_cast(FLAGS_stats_level); + dbstats->set_stats_level(static_cast(FLAGS_stats_level)); } FLAGS_compaction_pri_e = (rocksdb::CompactionPri)FLAGS_compaction_pri; diff --git a/util/stop_watch.h b/util/stop_watch.h index fe353b526..afa708e37 100644 --- a/util/stop_watch.h +++ b/util/stop_watch.h @@ -23,7 +23,8 @@ class StopWatch { elapsed_(elapsed), overwrite_(overwrite), stats_enabled_(statistics && - statistics->stats_level_ >= StatsLevel::kExceptTimers && + statistics->get_stats_level() >= + StatsLevel::kExceptTimers && statistics->HistEnabledForType(hist_type)), delay_enabled_(delay_enabled), total_delay_(0),