Bug 1468809 - Do not snapshot expired keyed histograms. r=chutten,janerik
authorAlessio Placitelli <alessio.placitelli@gmail.com>
Fri, 15 Jun 2018 19:31:26 +0200
changeset 476936 469b11ffea7af5cf5d49dd5e1c2040d25183d05b
parent 476935 578ef56130c95c80676f2db78798b37348287e42
child 476937 be26c7e898901662063ba149f38899bd4490f888
push id9374
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:43:20 +0000
treeherdermozilla-beta@160e085dfb0b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerschutten, janerik
bugs1468809
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1468809 - Do not snapshot expired keyed histograms. r=chutten,janerik This patch changes the snapshotting code for keyed histograms so that requesting a snapshot does not report expired histograms. MozReview-Commit-ID: GDiw6yOcF8J
toolkit/components/telemetry/Histograms.json
toolkit/components/telemetry/TelemetryHistogram.cpp
toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
--- a/toolkit/components/telemetry/Histograms.json
+++ b/toolkit/components/telemetry/Histograms.json
@@ -7414,16 +7414,28 @@
   },
   "TELEMETRY_TEST_EXPIRED": {
     "record_in_processes": ["main", "content"],
     "alert_emails": ["telemetry-client-dev@mozilla.com"],
     "expires_in_version": "4",
     "kind": "flag",
     "description": "a testing histogram; not meant to be touched"
   },
+  "TELEMETRY_TEST_EXPIRED_KEYED": {
+    "record_in_processes": ["main", "content"],
+    "alert_emails": ["telemetry-client-dev@mozilla.com"],
+    "expires_in_version": "4",
+    "kind": "linear",
+    "keyed": true,
+    "low": 1,
+    "high": 2147483646,
+    "n_buckets": 10,
+    "bug_numbers": [1468809],
+    "description": "a testing histogram; not meant to be touched"
+  },
   "TELEMETRY_TEST_CONTENT_PROCESS": {
     "record_in_processes": ["content"],
     "alert_emails": ["telemetry-client-dev@mozilla.com"],
     "expires_in_version": "never",
     "kind": "linear",
     "low": 1,
     "high": 10000,
     "n_buckets": 10,
--- a/toolkit/components/telemetry/TelemetryHistogram.cpp
+++ b/toolkit/components/telemetry/TelemetryHistogram.cpp
@@ -174,17 +174,17 @@ struct KeyedHistogramSnapshotInfo {
   HistogramID histogramId;
 };
 
 typedef mozilla::Vector<KeyedHistogramSnapshotInfo> KeyedHistogramSnapshotsArray;
 typedef mozilla::Vector<KeyedHistogramSnapshotsArray> KeyedHistogramProcessSnapshotsArray;
 
 class KeyedHistogram {
 public:
-  KeyedHistogram(HistogramID id, const HistogramInfo& info);
+  KeyedHistogram(HistogramID id, const HistogramInfo& info, bool expired);
   ~KeyedHistogram();
   nsresult GetHistogram(const nsCString& name, Histogram** histogram);
   Histogram* GetHistogram(const nsCString& name);
   uint32_t GetHistogramType() const { return mHistogramInfo.histogramType; }
   nsresult GetJSKeys(JSContext* cx, JS::CallArgs& args);
   // Note: unlike other methods, GetJSSnapshot is thread safe.
   nsresult GetJSSnapshot(JSContext* cx, JS::Handle<JSObject*> obj,
                          bool clearSubsession);
@@ -193,23 +193,26 @@ public:
 
   nsresult Add(const nsCString& key, uint32_t aSample, ProcessID aProcessType);
   void Clear();
 
   HistogramID GetHistogramID() const { return mId; }
 
   bool IsEmpty() const { return mHistogramMap.IsEmpty(); }
 
+  bool IsExpired() const { return mIsExpired; }
+
 private:
   typedef nsBaseHashtableET<nsCStringHashKey, Histogram*> KeyedHistogramEntry;
   typedef AutoHashtable<KeyedHistogramEntry> KeyedHistogramMapType;
   KeyedHistogramMapType mHistogramMap;
 
   const HistogramID mId;
   const HistogramInfo& mHistogramInfo;
+  bool mIsExpired;
 };
 
 } // namespace
 
 
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
 //
@@ -232,16 +235,19 @@ Histogram** gHistogramStorage;
 KeyedHistogram** gKeyedHistogramStorage;
 
 // Cache of histogram name to a histogram id.
 StringToHistogramIdMap gNameToHistogramIDMap(HistogramCount);
 
 // To simplify logic below we use a single histogram instance for all expired histograms.
 Histogram* gExpiredHistogram = nullptr;
 
+// The single placeholder for expired keyed histograms.
+KeyedHistogram* gExpiredKeyedHistogram = nullptr;
+
 // This tracks whether recording is enabled for specific histograms.
 // To utilize C++ initialization rules, we invert the meaning to "disabled".
 bool gHistogramRecordingDisabled[HistogramCount] = {};
 
 // This is for gHistogramInfos, gHistogramStringTable
 #include "TelemetryHistogramData.inc"
 
 } // namespace
@@ -379,17 +385,31 @@ internal_GetKeyedHistogramById(Histogram
 
   KeyedHistogram* kh = internal_GetKeyedHistogramFromStorage(histogramId,
                                                              processId);
   if (kh || !instantiate) {
     return kh;
   }
 
   const HistogramInfo& info = gHistogramInfos[histogramId];
-  kh = new KeyedHistogram(histogramId, info);
+  const bool isExpired = IsExpiredVersion(info.expiration());
+
+  // If the keyed histogram is expired, set its storage to the expired
+  // keyed histogram.
+  if (isExpired) {
+    if (!gExpiredKeyedHistogram) {
+      // If we don't have an expired keyed histogram, create one.
+      gExpiredKeyedHistogram = new KeyedHistogram(histogramId, info, true /* expired */);
+      MOZ_ASSERT(gExpiredKeyedHistogram);
+    }
+    kh = gExpiredKeyedHistogram;
+  } else {
+    kh = new KeyedHistogram(histogramId, info, false /* expired */);
+  }
+
   internal_SetKeyedHistogramInStorage(histogramId, processId, kh);
 
   return kh;
 }
 
 // Look up a histogram id from a histogram name.
 nsresult
 internal_GetHistogramIdByName(const StaticMutexAutoLock& aLock,
@@ -894,20 +914,21 @@ internal_ReflectKeyedHistogram(const Key
                              histogramSnapshot, JSPROP_ENUMERATE)) {
       return NS_ERROR_FAILURE;
     }
   }
 
   return NS_OK;
 }
 
-KeyedHistogram::KeyedHistogram(HistogramID id, const HistogramInfo& info)
+KeyedHistogram::KeyedHistogram(HistogramID id, const HistogramInfo& info, bool expired)
   : mHistogramMap()
   , mId(id)
   , mHistogramInfo(info)
+  , mIsExpired(expired)
 {
 }
 
 KeyedHistogram::~KeyedHistogram()
 {
   for (auto iter = mHistogramMap.Iter(); !iter.Done(); iter.Next()) {
     Histogram* h = iter.Get()->mData;
     if (h == gExpiredHistogram) {
@@ -964,16 +985,21 @@ KeyedHistogram::Add(const nsCString& key
                                            internal_CanRecordExtended());
   // If `histogram` is a non-parent-process histogram, then recording-enabled
   // has been checked in its owner process.
   if (!canRecordDataset ||
     (aProcessType == ProcessID::Parent && !internal_IsRecordingEnabled(mId))) {
     return NS_OK;
   }
 
+  // Don't record if expired.
+  if (IsExpired()) {
+    return NS_OK;
+  }
+
   // Don't record if the current platform is not enabled
   if (!CanRecordProduct(gHistogramInfos[mId].products)) {
     return NS_OK;
   }
 
   Histogram* histogram = GetHistogram(key);
   MOZ_ASSERT(histogram);
   if (!histogram) {
@@ -1129,17 +1155,17 @@ internal_GetKeyedHistogramsSnapshot(cons
 
       if (!IsInDataset(info.dataset, aDataset)) {
         continue;
       }
 
       KeyedHistogram* keyed = internal_GetKeyedHistogramById(id,
                                                              ProcessID(process),
                                                              /* instantiate = */ false);
-      if (!keyed || (aSkipEmpty && keyed->IsEmpty())) {
+      if (!keyed || (aSkipEmpty && keyed->IsEmpty()) || keyed->IsExpired()) {
         continue;
       }
 
       // Take a snapshot of the keyed histogram data!
       KeyedHistogramSnapshotData snapshot;
       if (!NS_SUCCEEDED(keyed->GetSnapshot(aLock, snapshot, aClearSubsession))) {
         return NS_ERROR_FAILURE;
       }
@@ -1995,28 +2021,31 @@ void TelemetryHistogram::DeInitializeGlo
   gCanRecordBase = false;
   gCanRecordExtended = false;
   gNameToHistogramIDMap.Clear();
   gInitDone = false;
 
   // FactoryGet `new`s Histograms for us, but requires us to manually delete.
   if (XRE_IsParentProcess()) {
     for (size_t i = 0; i < HistogramCount * size_t(ProcessID::Count); ++i) {
-      if (i < HistogramCount * size_t(ProcessID::Count)) {
+      if (i < HistogramCount * size_t(ProcessID::Count)
+          && gKeyedHistogramStorage[i] != gExpiredKeyedHistogram) {
         delete gKeyedHistogramStorage[i];
       }
       if (gHistogramStorage[i] != gExpiredHistogram) {
         delete gHistogramStorage[i];
       }
     }
     delete[] gHistogramStorage;
     delete[] gKeyedHistogramStorage;
   }
   delete gExpiredHistogram;
   gExpiredHistogram = nullptr;
+  delete gExpiredKeyedHistogram;
+  gExpiredKeyedHistogram = nullptr;
 }
 
 #ifdef DEBUG
 bool TelemetryHistogram::GlobalStateHasBeenInitialized() {
   StaticMutexAutoLock locker(gTelemetryHistogramMutex);
   return gInitDone;
 }
 #endif
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
@@ -450,16 +450,33 @@ add_task(async function test_expired_his
     }
     Assert.equal(histograms[process].__expired__, undefined);
   }
   let parentHgrams = Telemetry.snapshotHistograms(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN,
                                                   false /* clear */).parent;
   Assert.equal(parentHgrams[test_expired_id], undefined);
 });
 
+add_task(async function test_keyed_expired_histogram() {
+  var test_expired_id = "TELEMETRY_TEST_EXPIRED_KEYED";
+  var dummy = Telemetry.getKeyedHistogramById(test_expired_id);
+  dummy.add("someKey", 1);
+
+  const histograms = Telemetry.snapshotKeyedHistograms(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN,
+                                                       false /* clear */);
+  for (let process of ["parent", "content", "gpu", "extension"]) {
+    if (!(process in histograms)) {
+      info("Nothing present for process " + process);
+      continue;
+    }
+    Assert.ok(!(test_expired_id in histograms[process]),
+              "The expired keyed histogram must not be reported");
+  }
+});
+
 add_task(async function test_keyed_histogram() {
   // Check that invalid names get rejected.
 
   let threw = false;
   try {
     Telemetry.getKeyedHistogramById("test::unknown histogram", "never", Telemetry.HISTOGRAM_BOOLEAN);
   } catch (e) {
     // This should throw as it is an unknown ID