Move SimpleTimer registry ownership to BucketSnapshotManager

Author: drebelsky

src/bucket/BucketListSnapshotBase.cpp

@@ -170,50 +170,6 @@ BucketLevelSnapshot<BucketT>::BucketLevelSnapshot(
 {
 }
 
-template <class BucketT>
-static UnorderedMap<
-    medida::MetricsRegistry*,
-    UnorderedMap<LedgerEntryType, SimpleTimer<std::chrono::microseconds>>>
-    gPointTimers;
-
-template <class BucketT> static std::mutex gPointTimerLock;
-
-template <class BucketT>
-static UnorderedMap<LedgerEntryType, SimpleTimer<std::chrono::microseconds>>&
-getPointTimers(medida::MetricsRegistry& registry)
-{
-    std::lock_guard guard{gPointTimerLock<BucketT>};
-    auto& res = gPointTimers<BucketT>[&registry];
-    if (res.empty())
-    {
-        // Initialize point load timers for each LedgerEntry type
-        for (auto t : xdr::xdr_traits<LedgerEntryType>::enum_values())
-        {
-            auto const& label = xdr::xdr_traits<LedgerEntryType>::enum_name(
-                static_cast<LedgerEntryType>(t));
-            res.emplace(std::piecewise_construct,
-                        std::forward_as_tuple(static_cast<LedgerEntryType>(t)),
-                        std::forward_as_tuple(registry, BucketT::METRIC_STRING,
-                                              label, "",
-                                              std::chrono::seconds(30)));
-        }
-    }
-    return res;
-}
-
-void
-unregisterSimpleTimers(medida::MetricsRegistry& registry)
-{
-    {
-        std::lock_guard guard{gPointTimerLock<LiveBucket>};
-        gPointTimers<LiveBucket>.erase(&registry);
-    }
-    {
-        std::lock_guard guard{gPointTimerLock<HotArchiveBucket>};
-        gPointTimers<HotArchiveBucket>.erase(&registry);
-    }
-}
-
 template <class BucketT>
 SearchableBucketListSnapshotBase<BucketT>::SearchableBucketListSnapshotBase(
     BucketSnapshotManager const& snapshotManager, AppConnector const& app,
@@ -223,14 +179,20 @@ SearchableBucketListSnapshotBase<BucketT>::SearchableBucketListSnapshotBase(
     , mSnapshot(std::move(snapshot))
     , mHistoricalSnapshots(std::move(historicalSnapshots))
     , mAppConnector(app)
-    , mPointTimers{getPointTimers<BucketT>(app.getMetrics())}
     , mBulkLoadMeter(app.getMetrics().NewMeter(
           {BucketT::METRIC_STRING, "query", "loads"}, "query"))
     , mBloomMisses(app.getMetrics().NewMeter(
           {BucketT::METRIC_STRING, "bloom", "misses"}, "bloom"))
     , mBloomLookups(app.getMetrics().NewMeter(
           {BucketT::METRIC_STRING, "bloom", "lookups"}, "bloom"))
 {
+    for (auto t : xdr::xdr_traits<LedgerEntryType>::enum_values())
+    {
+        auto const& label = xdr::xdr_traits<LedgerEntryType>::enum_name(
+            static_cast<LedgerEntryType>(t));
+        auto& timer = mSnapshotManager.getTimer(BucketT::METRIC_STRING, label);
+        mPointTimers.emplace(static_cast<LedgerEntryType>(t), timer);
+    }
 }
 
 template <class BucketT>

src/bucket/BucketListSnapshotBase.h

@@ -93,8 +93,8 @@ class SearchableBucketListSnapshotBase : public NonMovableOrCopyable
     // Tracks the sum of point load times for each LedgerEntryType, in
     // microseconds. For point loads, Timers are too expensive to maintain, so
     // we use SimpleTimer.
-    UnorderedMap<LedgerEntryType, SimpleTimer<std::chrono::microseconds>>&
-        mPointTimers;
+    UnorderedMap<LedgerEntryType, SimpleTimer<std::chrono::microseconds>&>
+        mPointTimers{};
 
     // Bulk load timers take significantly longer, so the timer overhead is
     // comparatively negligible.
@@ -144,8 +144,4 @@ class SearchableBucketListSnapshotBase : public NonMovableOrCopyable
     std::shared_ptr<typename BucketT::LoadT const>
     load(LedgerKey const& k) const;
 };
-
-// Remove the storage holding the SimpleTimers associated with the given
-// registry
-void unregisterSimpleTimers(medida::MetricsRegistry& registry);
 }

src/bucket/BucketManager.cpp

@@ -256,7 +256,6 @@ BucketManager::getBucketDir() const
 BucketManager::~BucketManager()
 {
 
-    unregisterSimpleTimers(mApp.getMetrics());
     deleteTmpDirAndUnlockBucketDir();
 }
 

src/bucket/BucketSnapshotManager.cpp

@@ -192,4 +192,19 @@ BucketSnapshotManager::updateCurrentSnapshot(
     updateSnapshot(mCurrHotArchiveSnapshot, mHotArchiveHistoricalSnapshots,
                    hotArchiveSnapshot);
 }
-}
+
+SimpleTimer<std::chrono::microseconds>&
+BucketSnapshotManager::getTimer(const std::string& domain,
+                                const std::string& type) const
+{
+    auto key = std::make_pair(domain, type);
+    MutexLocker lock{mSimpleTimerRegistryMutex};
+    if (mSimpleTimerRegistry.find(key) == mSimpleTimerRegistry.end())
+    {
+        mSimpleTimerRegistry.try_emplace(key, mAppConnector.getMetrics(),
+                                         domain, type, "",
+                                         std::chrono::seconds{30});
+    }
+    return mSimpleTimerRegistry.find(key)->second;
+}
+}

src/bucket/BucketSnapshotManager.h

@@ -9,6 +9,7 @@
 #include "bucket/LiveBucket.h"
 #include "main/AppConnector.h"
 #include "util/NonCopyable.h"
+#include "util/SimpleTimer.h"
 #include "util/ThreadAnnotations.h"
 
 #include <map>
@@ -49,6 +50,11 @@ class BucketSnapshotManager : NonMovableOrCopyable
     // Lock must be held when accessing any member variables holding snapshots
     mutable SharedMutex mSnapshotMutex;
 
+    mutable Mutex mSimpleTimerRegistryMutex;
+    mutable std::map<std::pair<std::string, std::string>,
+                     SimpleTimer<std::chrono::microseconds>>
+        mSimpleTimerRegistry;
+
     // Snapshot that is maintained and periodically updated by BucketManager on
     // the main thread. When background threads need to generate or refresh a
     // snapshot, they will copy this snapshot.
@@ -117,5 +123,10 @@ class BucketSnapshotManager : NonMovableOrCopyable
         SearchableSnapshotConstPtr& liveSnapshot,
         SearchableHotArchiveSnapshotConstPtr& hotArchiveSnapshot)
         LOCKS_EXCLUDED(mSnapshotMutex);
+
+    // Called in SearchableBucketListSnapshotBase to get access to the same
+    // SimpleTimers across snapshots
+    SimpleTimer<std::chrono::microseconds>&
+    getTimer(const std::string& domain, const std::string& type) const;
 };
-}
+}