Make statistics's stats_level change thread-safe (facebook#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: facebook#5030 Differential Revision: D14267519 Pulled By: siying fbshipit-source-id: 37d7ebeff7a43a406230143422a16af899163f73
vagogte · Mar 1, 2019 · 16c59ee · 16c59ee
1 parent cb228bd
commit 16c59ee
Show file tree

Hide file tree

Showing 11 changed files with 25 additions and 14 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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<StatsLevel> stats_level_{kExceptDetailedTimers};
 };
 
 // Create a concrete DBStatistics object

diff --git a/java/rocksjni/statistics.cc b/java/rocksjni/statistics.cc
@@ -118,7 +118,7 @@ jbyte Java_org_rocksdb_Statistics_statsLevel(
       reinterpret_cast<std::shared_ptr<rocksdb::Statistics>*>(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<std::shared_ptr<rocksdb::Statistics>*>(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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -6082,7 +6082,7 @@ int db_bench_tool(int argc, char** argv) {
     dbstats = rocksdb::CreateDBStatistics();
   }
   if (dbstats) {
-    dbstats->stats_level_ = static_cast<StatsLevel>(FLAGS_stats_level);
+    dbstats->set_stats_level(static_cast<StatsLevel>(FLAGS_stats_level));
   }
   FLAGS_compaction_pri_e = (rocksdb::CompactionPri)FLAGS_compaction_pri;
 

diff --git 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),