Bug 1768636 - Scope GIFFT timer maps to metrics r=TravisLong

TimerIds are only unique for a given metric, not globally.

Differential Revision: https://phabricator.services.mozilla.com/D145988

toolkit/components/glean/bindings/private/TimingDistribution.cpp

@@ -9,6 +9,7 @@
 #include "Common.h"
 #include "mozilla/Components.h"
 #include "mozilla/ResultVariant.h"
+#include "mozilla/Tuple.h"
 #include "mozilla/dom/ToJSValue.h"
 #include "mozilla/glean/bindings/HistogramGIFFTMap.h"
 #include "mozilla/glean/fog_ffi_generated.h"
@@ -24,8 +25,11 @@ extern "C" NS_EXPORT void GIFFT_TimingDistributionStart(
   auto mirrorId = mozilla::glean::HistogramIdForMetric(aMetricId);
   if (mirrorId) {
     mozilla::glean::GetTimerIdToStartsLock().apply([&](auto& lock) {
-      (void)NS_WARN_IF(lock.ref()->Remove(aTimerId));
-      lock.ref()->InsertOrUpdate(aTimerId, mozilla::TimeStamp::Now());
+      auto tuple = mozilla::MakeTuple(aMetricId, aTimerId);
+      // It should be all but impossible for anyone to have already inserted
+      // this timer for this metric given the monotonicity of timer ids.
+      (void)NS_WARN_IF(lock.ref()->Remove(tuple));
+      lock.ref()->InsertOrUpdate(tuple, mozilla::TimeStamp::Now());
     });
   }
 }
@@ -36,7 +40,10 @@ extern "C" NS_EXPORT void GIFFT_TimingDistributionStopAndAccumulate(
   auto mirrorId = mozilla::glean::HistogramIdForMetric(aMetricId);
   if (mirrorId) {
     mozilla::glean::GetTimerIdToStartsLock().apply([&](auto& lock) {
-      auto optStart = lock.ref()->Extract(aTimerId);
+      auto optStart =
+          lock.ref()->Extract(mozilla::MakeTuple(aMetricId, aTimerId));
+      // The timer might not be in the map to be removed if it's already been
+      // cancelled or stop_and_accumulate'd.
       if (!NS_WARN_IF(!optStart)) {
         AccumulateTimeDelta(mirrorId.extract(), optStart.extract());
       }
@@ -58,8 +65,12 @@ extern "C" NS_EXPORT void GIFFT_TimingDistributionCancel(
     uint32_t aMetricId, mozilla::glean::TimerId aTimerId) {
   auto mirrorId = mozilla::glean::HistogramIdForMetric(aMetricId);
   if (mirrorId) {
-    mozilla::glean::GetTimerIdToStartsLock().apply(
-        [&](auto& lock) { (void)NS_WARN_IF(!lock.ref()->Remove(aTimerId)); });
+    mozilla::glean::GetTimerIdToStartsLock().apply([&](auto& lock) {
+      // The timer might not be in the map to be removed if it's already been
+      // cancelled or stop_and_accumulate'd.
+      (void)NS_WARN_IF(
+          !lock.ref()->Remove(mozilla::MakeTuple(aMetricId, aTimerId)));
+    });
   }
 }
 

toolkit/components/glean/build_scripts/glean_parser_ext/templates/gifft.jinja2

@@ -4,12 +4,13 @@
    It is only for internal use by types in
    toolkit/components/glean/bindings/private */
 {# The rendered source is autogenerated, but this
-Jinja2 template is not. Pleas file bugs! #}
+Jinja2 template is not. Please file bugs! #}
 
 #include "mozilla/AppShutdown.h"
 #include "mozilla/ClearOnShutdown.h"
 #include "mozilla/Maybe.h"
 #include "mozilla/Telemetry.h"
+#include "mozilla/Tuple.h"
 #include "mozilla/DataMutex.h"
 {% if probe_type == "Scalar" %}
 #include "mozilla/Tuple.h"
@@ -27,8 +28,38 @@ namespace mozilla::glean {
 using Telemetry::{{ probe_type }}ID;
 
 {% if probe_type == "Histogram" %}
-typedef uint64_t TimerId; // Same as in TimingDistribution.h.
-typedef StaticDataMutex<UniquePtr<nsTHashMap<TimerId, TimeStamp>>> TimerToStampMutex;
+
+using MetricId = uint32_t; // Same type as in api/src/private/mod.rs
+using TimerId = uint64_t; // Same as in TimingDistribution.h.
+using MetricTimerTuple = Tuple<MetricId, TimerId>;
+class MetricTimerTupleHashKey : public PLDHashEntryHdr {
+ public:
+  using KeyType = const MetricTimerTuple&;
+  using KeyTypePointer = const MetricTimerTuple*;
+
+  explicit MetricTimerTupleHashKey(KeyTypePointer aKey) : mValue(*aKey) {}
+  MetricTimerTupleHashKey(MetricTimerTupleHashKey&& aOther)
+      : PLDHashEntryHdr(std::move(aOther)),
+        mValue(std::move(aOther.mValue)) {}
+  ~MetricTimerTupleHashKey() = default;
+
+  KeyType GetKey() const { return mValue; }
+  bool KeyEquals(KeyTypePointer aKey) const {
+    return Get<0>(*aKey) == Get<0>(mValue) && Get<1>(*aKey) == Get<1>(mValue);
+  }
+
+  static KeyTypePointer KeyToPointer(KeyType aKey) { return &aKey; }
+  static PLDHashNumber HashKey(KeyTypePointer aKey) {
+    // Chosen because this is how nsIntegralHashKey does it.
+    return HashGeneric(Get<0>(*aKey), Get<1>(*aKey));
+  }
+  enum { ALLOW_MEMMOVE = true };
+
+ private:
+  const MetricTimerTuple mValue;
+};
+
+typedef StaticDataMutex<UniquePtr<nsTHashMap<MetricTimerTupleHashKey, TimeStamp>>> TimerToStampMutex;
 static inline Maybe<TimerToStampMutex::AutoLock> GetTimerIdToStartsLock() {
   static TimerToStampMutex sTimerIdToStarts("sTimerIdToStarts");
   auto lock = sTimerIdToStarts.Lock();
@@ -37,7 +68,7 @@ static inline Maybe<TimerToStampMutex::AutoLock> GetTimerIdToStartsLock() {
     return Nothing();
   }
   if (!*lock) {
-    *lock = MakeUnique<nsTHashMap<TimerId, TimeStamp>>();
+    *lock = MakeUnique<nsTHashMap<MetricTimerTupleHashKey, TimeStamp>>();
     RefPtr<nsIRunnable> cleanupFn = NS_NewRunnableFunction(__func__, [&] {
       if (AppShutdown::IsInOrBeyond(ShutdownPhase::XPCOMWillShutdown)) {
         auto lock = sTimerIdToStarts.Lock();

toolkit/components/glean/tests/pytest/gifft_output_Event

@@ -8,6 +8,7 @@
 #include "mozilla/ClearOnShutdown.h"
 #include "mozilla/Maybe.h"
 #include "mozilla/Telemetry.h"
+#include "mozilla/Tuple.h"
 #include "mozilla/DataMutex.h"
 #include "nsThreadUtils.h"
 

toolkit/components/glean/tests/pytest/gifft_output_Histogram

@@ -8,6 +8,7 @@
 #include "mozilla/ClearOnShutdown.h"
 #include "mozilla/Maybe.h"
 #include "mozilla/Telemetry.h"
+#include "mozilla/Tuple.h"
 #include "mozilla/DataMutex.h"
 #include "nsThreadUtils.h"
 
@@ -18,8 +19,38 @@ namespace mozilla::glean {
 
 using Telemetry::HistogramID;
 
-typedef uint64_t TimerId; // Same as in TimingDistribution.h.
-typedef StaticDataMutex<UniquePtr<nsTHashMap<TimerId, TimeStamp>>> TimerToStampMutex;
+
+using MetricId = uint32_t; // Same type as in api/src/private/mod.rs
+using TimerId = uint64_t; // Same as in TimingDistribution.h.
+using MetricTimerTuple = Tuple<MetricId, TimerId>;
+class MetricTimerTupleHashKey : public PLDHashEntryHdr {
+ public:
+  using KeyType = const MetricTimerTuple&;
+  using KeyTypePointer = const MetricTimerTuple*;
+
+  explicit MetricTimerTupleHashKey(KeyTypePointer aKey) : mValue(*aKey) {}
+  MetricTimerTupleHashKey(MetricTimerTupleHashKey&& aOther)
+      : PLDHashEntryHdr(std::move(aOther)),
+        mValue(std::move(aOther.mValue)) {}
+  ~MetricTimerTupleHashKey() = default;
+
+  KeyType GetKey() const { return mValue; }
+  bool KeyEquals(KeyTypePointer aKey) const {
+    return Get<0>(*aKey) == Get<0>(mValue) && Get<1>(*aKey) == Get<1>(mValue);
+  }
+
+  static KeyTypePointer KeyToPointer(KeyType aKey) { return &aKey; }
+  static PLDHashNumber HashKey(KeyTypePointer aKey) {
+    // Chosen because this is how nsIntegralHashKey does it.
+    return HashGeneric(Get<0>(*aKey), Get<1>(*aKey));
+  }
+  enum { ALLOW_MEMMOVE = true };
+
+ private:
+  const MetricTimerTuple mValue;
+};
+
+typedef StaticDataMutex<UniquePtr<nsTHashMap<MetricTimerTupleHashKey, TimeStamp>>> TimerToStampMutex;
 static inline Maybe<TimerToStampMutex::AutoLock> GetTimerIdToStartsLock() {
   static TimerToStampMutex sTimerIdToStarts("sTimerIdToStarts");
   auto lock = sTimerIdToStarts.Lock();
@@ -28,7 +59,7 @@ static inline Maybe<TimerToStampMutex::AutoLock> GetTimerIdToStartsLock() {
     return Nothing();
   }
   if (!*lock) {
-    *lock = MakeUnique<nsTHashMap<TimerId, TimeStamp>>();
+    *lock = MakeUnique<nsTHashMap<MetricTimerTupleHashKey, TimeStamp>>();
     RefPtr<nsIRunnable> cleanupFn = NS_NewRunnableFunction(__func__, [&] {
       if (AppShutdown::IsInOrBeyond(ShutdownPhase::XPCOMWillShutdown)) {
         auto lock = sTimerIdToStarts.Lock();

toolkit/components/glean/tests/pytest/gifft_output_Scalar

@@ -8,6 +8,7 @@
 #include "mozilla/ClearOnShutdown.h"
 #include "mozilla/Maybe.h"
 #include "mozilla/Telemetry.h"
+#include "mozilla/Tuple.h"
 #include "mozilla/DataMutex.h"
 #include "mozilla/Tuple.h"
 #include "nsClassHashtable.h"

toolkit/components/glean/tests/test_metrics.yaml

@@ -160,6 +160,7 @@ test_only:
       - glean-team@mozilla.com
     send_in_pings:
       - test-ping
+    telemetry_mirror: TELEMETRY_TEST_MIRROR_FOR_TIMING
 
   mabels_kitchen_counters:
     type: labeled_counter

toolkit/components/glean/tests/xpcshell/test_GIFFT.js

@@ -119,7 +119,12 @@ add_task(function test_gifft_custom_dist() {
 
 add_task(async function test_gifft_timing_dist() {
   let t1 = Glean.testOnlyIpc.aTimingDist.start();
+  // Interleave some other metric's samples. bug 1768636.
+  let ot1 = Glean.testOnly.whatTimeIsIt.start();
   let t2 = Glean.testOnlyIpc.aTimingDist.start();
+  let ot2 = Glean.testOnly.whatTimeIsIt.start();
+  Glean.testOnly.whatTimeIsIt.cancel(ot1);
+  Glean.testOnly.whatTimeIsIt.cancel(ot2);
 
   await sleep(5);
 

toolkit/components/telemetry/Histograms.json

@@ -9755,6 +9755,19 @@
     "releaseChannelCollection": "opt-out",
     "description": "a testing histogram; not meant to be touched"
   },
+  "TELEMETRY_TEST_MIRROR_FOR_TIMING": {
+    "record_in_processes": ["main", "content"],
+    "products": ["firefox"],
+    "alert_emails": ["chutten@mozilla.com"],
+    "expires_in_version": "never",
+    "kind": "linear",
+    "low": 1,
+    "high": 2147483646,
+    "n_buckets": 10,
+    "bug_numbers": [1768636],
+    "releaseChannelCollection": "opt-out",
+    "description": "a testing histogram; not meant to be touched"
+  },
   "TELEMETRY_TEST_BOOLEAN": {
     "record_in_processes": ["main", "content"],
     "products": ["firefox", "fennec", "thunderbird"],