From 3fa80f0e853c1741c0d41efed3b41c38f465e439 Mon Sep 17 00:00:00 2001 From: Adam Singer Date: Tue, 27 Nov 2018 12:56:40 -0800 Subject: [PATCH] Remove enable_internal_stats (#4714) Summary: Simple patch to address comments in [statistics.h#L65](https://github.com/facebook/rocksdb/blob/master/monitoring/statistics.h#L65|statistics.h#L65) `TODO(ajkr): clean this up since there are no internal stats anymore` Pull Request resolved: https://github.com/facebook/rocksdb/pull/4714 Differential Revision: D13208093 Pulled By: ajkr fbshipit-source-id: 4468badb850592411147539f859082644f5296f6 --- java/rocksjni/statisticsjni.cc | 4 +- monitoring/statistics.cc | 80 +++++++++++++--------------------- monitoring/statistics.h | 5 +-- 3 files changed, 33 insertions(+), 56 deletions(-) diff --git a/java/rocksjni/statisticsjni.cc b/java/rocksjni/statisticsjni.cc index 3ac1e5b41..8fddc437a 100644 --- a/java/rocksjni/statisticsjni.cc +++ b/java/rocksjni/statisticsjni.cc @@ -11,11 +11,11 @@ namespace rocksdb { StatisticsJni::StatisticsJni(std::shared_ptr stats) - : StatisticsImpl(stats, false), m_ignore_histograms() { + : StatisticsImpl(stats), m_ignore_histograms() { } StatisticsJni::StatisticsJni(std::shared_ptr stats, - const std::set ignore_histograms) : StatisticsImpl(stats, false), + const std::set ignore_histograms) : StatisticsImpl(stats), m_ignore_histograms(ignore_histograms) { } diff --git a/monitoring/statistics.cc b/monitoring/statistics.cc index 4bc1a04fe..cba427ae4 100644 --- a/monitoring/statistics.cc +++ b/monitoring/statistics.cc @@ -220,12 +220,11 @@ const std::vector> HistogramsNameMap = { }; std::shared_ptr CreateDBStatistics() { - return std::make_shared(nullptr, false); + return std::make_shared(nullptr); } -StatisticsImpl::StatisticsImpl(std::shared_ptr stats, - bool enable_internal_stats) - : stats_(std::move(stats)), enable_internal_stats_(enable_internal_stats) {} +StatisticsImpl::StatisticsImpl(std::shared_ptr stats) + : stats_(std::move(stats)) {} StatisticsImpl::~StatisticsImpl() {} @@ -235,10 +234,7 @@ uint64_t StatisticsImpl::getTickerCount(uint32_t tickerType) const { } uint64_t StatisticsImpl::getTickerCountLocked(uint32_t tickerType) const { - assert( - enable_internal_stats_ ? - tickerType < INTERNAL_TICKER_ENUM_MAX : - tickerType < TICKER_ENUM_MAX); + assert(tickerType < TICKER_ENUM_MAX); uint64_t res = 0; for (size_t core_idx = 0; core_idx < per_core_stats_.Size(); ++core_idx) { res += per_core_stats_.AccessAtCore(core_idx)->tickers_[tickerType]; @@ -254,10 +250,7 @@ void StatisticsImpl::histogramData(uint32_t histogramType, std::unique_ptr StatisticsImpl::getHistogramImplLocked( uint32_t histogramType) const { - assert( - enable_internal_stats_ ? - histogramType < INTERNAL_HISTOGRAM_ENUM_MAX : - histogramType < HISTOGRAM_ENUM_MAX); + assert(histogramType < HISTOGRAM_ENUM_MAX); std::unique_ptr res_hist(new HistogramImpl()); for (size_t core_idx = 0; core_idx < per_core_stats_.Size(); ++core_idx) { res_hist->Merge( @@ -282,8 +275,7 @@ void StatisticsImpl::setTickerCount(uint32_t tickerType, uint64_t count) { } void StatisticsImpl::setTickerCountLocked(uint32_t tickerType, uint64_t count) { - assert(enable_internal_stats_ ? tickerType < INTERNAL_TICKER_ENUM_MAX - : tickerType < TICKER_ENUM_MAX); + assert(tickerType < TICKER_ENUM_MAX); for (size_t core_idx = 0; core_idx < per_core_stats_.Size(); ++core_idx) { if (core_idx == 0) { per_core_stats_.AccessAtCore(core_idx)->tickers_[tickerType] = count; @@ -297,8 +289,7 @@ uint64_t StatisticsImpl::getAndResetTickerCount(uint32_t tickerType) { uint64_t sum = 0; { MutexLock lock(&aggregate_lock_); - assert(enable_internal_stats_ ? tickerType < INTERNAL_TICKER_ENUM_MAX - : tickerType < TICKER_ENUM_MAX); + assert(tickerType < TICKER_ENUM_MAX); for (size_t core_idx = 0; core_idx < per_core_stats_.Size(); ++core_idx) { sum += per_core_stats_.AccessAtCore(core_idx)->tickers_[tickerType].exchange( @@ -312,10 +303,7 @@ uint64_t StatisticsImpl::getAndResetTickerCount(uint32_t tickerType) { } void StatisticsImpl::recordTick(uint32_t tickerType, uint64_t count) { - assert( - enable_internal_stats_ ? - tickerType < INTERNAL_TICKER_ENUM_MAX : - tickerType < TICKER_ENUM_MAX); + assert(tickerType < TICKER_ENUM_MAX); per_core_stats_.Access()->tickers_[tickerType].fetch_add( count, std::memory_order_relaxed); if (stats_ && tickerType < TICKER_ENUM_MAX) { @@ -324,10 +312,7 @@ void StatisticsImpl::recordTick(uint32_t tickerType, uint64_t count) { } void StatisticsImpl::measureTime(uint32_t histogramType, uint64_t value) { - assert( - enable_internal_stats_ ? - histogramType < INTERNAL_HISTOGRAM_ENUM_MAX : - histogramType < HISTOGRAM_ENUM_MAX); + assert(histogramType < HISTOGRAM_ENUM_MAX); per_core_stats_.Access()->histograms_[histogramType].Add(value); if (stats_ && histogramType < HISTOGRAM_ENUM_MAX) { stats_->measureTime(histogramType, value); @@ -359,41 +344,36 @@ std::string StatisticsImpl::ToString() const { std::string res; res.reserve(20000); for (const auto& t : TickersNameMap) { - if (t.first < TICKER_ENUM_MAX || enable_internal_stats_) { - char buffer[kTmpStrBufferSize]; - snprintf(buffer, kTmpStrBufferSize, "%s COUNT : %" PRIu64 "\n", - t.second.c_str(), getTickerCountLocked(t.first)); - res.append(buffer); - } + assert(t.first < TICKER_ENUM_MAX); + char buffer[kTmpStrBufferSize]; + snprintf(buffer, kTmpStrBufferSize, "%s COUNT : %" PRIu64 "\n", + t.second.c_str(), getTickerCountLocked(t.first)); + res.append(buffer); } for (const auto& h : HistogramsNameMap) { - if (h.first < HISTOGRAM_ENUM_MAX || enable_internal_stats_) { - char buffer[kTmpStrBufferSize]; - HistogramData hData; - getHistogramImplLocked(h.first)->Data(&hData); - // don't handle failures - buffer should always be big enough and arguments - // should be provided correctly - int ret = snprintf( - buffer, kTmpStrBufferSize, - "%s P50 : %f P95 : %f P99 : %f P100 : %f COUNT : %" PRIu64 " SUM : %" - PRIu64 "\n", h.second.c_str(), hData.median, hData.percentile95, - hData.percentile99, hData.max, hData.count, hData.sum); - if (ret < 0 || ret >= kTmpStrBufferSize) { - assert(false); - continue; - } - res.append(buffer); + assert(h.first < HISTOGRAM_ENUM_MAX); + char buffer[kTmpStrBufferSize]; + HistogramData hData; + getHistogramImplLocked(h.first)->Data(&hData); + // don't handle failures - buffer should always be big enough and arguments + // should be provided correctly + int ret = snprintf( + buffer, kTmpStrBufferSize, + "%s P50 : %f P95 : %f P99 : %f P100 : %f COUNT : %" PRIu64 " SUM : %" + PRIu64 "\n", h.second.c_str(), hData.median, hData.percentile95, + hData.percentile99, hData.max, hData.count, hData.sum); + if (ret < 0 || ret >= kTmpStrBufferSize) { + assert(false); + continue; } + res.append(buffer); } res.shrink_to_fit(); return res; } bool StatisticsImpl::HistEnabledForType(uint32_t type) const { - if (LIKELY(!enable_internal_stats_)) { - return type < HISTOGRAM_ENUM_MAX; - } - return true; + return type < HISTOGRAM_ENUM_MAX; } } // namespace rocksdb diff --git a/monitoring/statistics.h b/monitoring/statistics.h index 4427c8c54..dcd5f7a01 100644 --- a/monitoring/statistics.h +++ b/monitoring/statistics.h @@ -41,8 +41,7 @@ enum HistogramsInternal : uint32_t { class StatisticsImpl : public Statistics { public: - StatisticsImpl(std::shared_ptr stats, - bool enable_internal_stats); + StatisticsImpl(std::shared_ptr stats); virtual ~StatisticsImpl(); virtual uint64_t getTickerCount(uint32_t ticker_type) const override; @@ -62,8 +61,6 @@ class StatisticsImpl : public Statistics { private: // If non-nullptr, forwards updates to the object pointed to by `stats_`. std::shared_ptr stats_; - // TODO(ajkr): clean this up since there are no internal stats anymore - bool enable_internal_stats_; // Synchronizes anything that operates across other cores' local data, // such that operations like Reset() can be performed atomically. mutable port::Mutex aggregate_lock_;