Bug 1430531 - Refactor histogram code to allow for JS-free snapshotting. r=chutten
authorAlessio Placitelli <alessio.placitelli@gmail.com>
Mon, 14 May 2018 14:09:47 +0200
changeset 418076 4589edd72ebb49b5e4468f4daa166e6f760e45d4
parent 418075 44e8b4f4dfa6753a9ca19979ce7ed8e061878fe5
child 418077 bd328282d668761f63dd934aa40ad15fb38fa959
push id63934
push useralessio.placitelli@gmail.com
push dateMon, 14 May 2018 12:55:19 +0000
treeherderautoland@bd328282d668 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerschutten
bugs1430531
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 1430531 - Refactor histogram code to allow for JS-free snapshotting. r=chutten This patch introduces a couple of new functions to copy histogram data to Mozilla-friendly arrays. This solves the problem of passing Histogram pointers around and makes working away from JS functions easier. MozReview-Commit-ID: BIg3FXBzxfT
toolkit/components/telemetry/TelemetryHistogram.cpp
--- a/toolkit/components/telemetry/TelemetryHistogram.cpp
+++ b/toolkit/components/telemetry/TelemetryHistogram.cpp
@@ -139,16 +139,24 @@ struct HistogramInfo {
   bool allows_key(const nsACString& key) const;
 };
 
 enum reflectStatus {
   REFLECT_OK,
   REFLECT_FAILURE
 };
 
+// Struct used to keep information about the histograms for which a
+// snapshot should be created.
+struct HistogramSnapshotData {
+  nsTArray<Histogram::Sample> mBucketRanges;
+  nsTArray<Histogram::Count> mBucketCounts;
+  int64_t mSampleSum; // Same type as Histogram::SampleSet::sum_
+};
+
 class KeyedHistogram {
 public:
   KeyedHistogram(HistogramID id, const HistogramInfo& info);
   ~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);
@@ -617,16 +625,110 @@ internal_HistogramAdd(Histogram& histogr
 
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
 //
 // PRIVATE: Histogram reflection helpers
 
 namespace {
 
+/**
+ * Copy histograms and samples to Mozilla-friendly structures.
+ * Please note that this version does not make use of JS contexts.
+ *
+ * @param {StaticMutexAutoLock} the proof we hold the mutex.
+ * @param {Histogram} the histogram to reflect.
+ * @return {nsresult} NS_ERROR_FAILURE if we fail to allocate memory for the snapshot.
+ */
+nsresult
+internal_GetHistogramAndSamples(const StaticMutexAutoLock& aLock,
+                                const Histogram *h,
+                                HistogramSnapshotData& aSnapshot)
+{
+  MOZ_ASSERT(h);
+
+  // Convert the ranges of the buckets to a nsTArray.
+  const size_t bucketCount = h->bucket_count();
+  for (size_t i = 0; i < bucketCount; i++) {
+    if (!aSnapshot.mBucketRanges.AppendElement(h->ranges(i))) {
+      return NS_ERROR_FAILURE;
+    }
+  }
+
+  // Get a snapshot of the samples.
+  Histogram::SampleSet ss;
+  h->SnapshotSample(&ss);
+
+  // Get the number of samples in each bucket.
+  for (size_t i = 0; i < bucketCount; i++) {
+    if (!aSnapshot.mBucketCounts.AppendElement(ss.counts(i))) {
+      return NS_ERROR_FAILURE;
+    }
+  }
+
+  // Finally, save the |sum|. We don't need to reflect declared_min, declared_max and
+  // histogram_type as they are in gHistogramInfo.
+  aSnapshot.mSampleSum = ss.sum();
+  return NS_OK;
+}
+
+nsresult
+internal_ReflectHistogramAndSamples(JSContext *cx,
+                                    JS::Handle<JSObject*> obj,
+                                    const HistogramInfo& aHistogramInfo,
+                                    const HistogramSnapshotData& aSnapshot)
+{
+  if (!(JS_DefineProperty(cx, obj, "min",
+                          aHistogramInfo.min, JSPROP_ENUMERATE)
+        && JS_DefineProperty(cx, obj, "max",
+                             aHistogramInfo.max, JSPROP_ENUMERATE)
+        && JS_DefineProperty(cx, obj, "histogram_type",
+                             aHistogramInfo.histogramType, JSPROP_ENUMERATE)
+        && JS_DefineProperty(cx, obj, "sum",
+                             double(aSnapshot.mSampleSum), JSPROP_ENUMERATE))) {
+    return NS_ERROR_FAILURE;
+  }
+
+  // Don't rely on the bucket counts from "aHistogramInfo": it may
+  // differ from the length of aSnapshot.mBucketCounts due to expired
+  // histograms.
+  const size_t count = aSnapshot.mBucketCounts.Length();
+  MOZ_ASSERT(count == aSnapshot.mBucketRanges.Length(),
+             "The number of buckets and the number of counts must match.");
+
+  // Create the "ranges" property and add it to the final object.
+  JS::Rooted<JSObject*> rarray(cx, JS_NewArrayObject(cx, count));
+  if (!rarray
+      || !JS_DefineProperty(cx, obj, "ranges", rarray, JSPROP_ENUMERATE)) {
+    return NS_ERROR_FAILURE;
+  }
+
+  // Fill the "ranges" property.
+  for (size_t i = 0; i < count; i++) {
+    if (!JS_DefineElement(cx, rarray, i, aSnapshot.mBucketRanges[i], JSPROP_ENUMERATE)) {
+      return NS_ERROR_FAILURE;
+    }
+  }
+
+  JS::Rooted<JSObject*> counts_array(cx, JS_NewArrayObject(cx, count));
+  if (!counts_array
+      || !JS_DefineProperty(cx, obj, "counts", counts_array, JSPROP_ENUMERATE)) {
+    return NS_ERROR_FAILURE;
+  }
+
+  // Fill the "counts" property.
+  for (size_t i = 0; i < count; i++) {
+    if (!JS_DefineElement(cx, counts_array, i, aSnapshot.mBucketCounts[i], JSPROP_ENUMERATE)) {
+      return NS_ERROR_FAILURE;
+    }
+  }
+
+  return NS_OK;
+}
+
 bool
 internal_FillRanges(JSContext *cx, JS::Handle<JSObject*> array, Histogram *h)
 {
   JS::Rooted<JS::Value> range(cx);
   for (size_t i = 0; i < h->bucket_count(); i++) {
     range.setInt32(h->ranges(i));
     if (!JS_DefineElement(cx, array, i, range, JSPROP_ENUMERATE))
       return false;
@@ -1223,49 +1325,45 @@ internal_JSHistogram_Snapshot(JSContext 
     return false;
   }
 
   JSObject* obj = &args.thisv().toObject();
   JSHistogramData* data = static_cast<JSHistogramData*>(JS_GetPrivate(obj));
   MOZ_ASSERT(data);
   HistogramID id = data->histogramId;
 
-  Histogram* h = nullptr;
-  Histogram::SampleSet ss;
+  HistogramSnapshotData dataSnapshot;
   {
     StaticMutexAutoLock locker(gTelemetryHistogramMutex);
     MOZ_ASSERT(internal_IsHistogramEnumId(id));
 
     // This is not good standard behavior given that we have histogram instances
     // covering multiple processes.
     // However, changing this requires some broader changes to callers.
-    h = internal_GetHistogramById(id, ProcessID::Parent);
-    // Take a snapshot of Histogram::SampleSet here, protected by the lock,
-    // and then, outside of the lock protection, mirror it to a JS structure
-    MOZ_ASSERT(h);
-    h->SnapshotSample(&ss);
+    Histogram* h = internal_GetHistogramById(id, ProcessID::Parent);
+    // Take a snapshot of the data here, protected by the lock, and then,
+    // outside of the lock protection, mirror it to a JS structure
+    if (NS_FAILED(internal_GetHistogramAndSamples(locker, h, dataSnapshot))) {
+      return false;
+    }
   }
 
   JS::Rooted<JSObject*> snapshot(cx, JS_NewPlainObject(cx));
   if (!snapshot) {
     return false;
   }
 
-  reflectStatus status = internal_ReflectHistogramAndSamples(cx, snapshot, h, ss);
-
-  switch (status) {
-  case REFLECT_FAILURE:
+  if (NS_FAILED(internal_ReflectHistogramAndSamples(cx,
+                                                    snapshot,
+                                                    gHistogramInfos[id],
+                                                    dataSnapshot))) {
     return false;
-  case REFLECT_OK:
-    args.rval().setObject(*snapshot);
-    return true;
-  default:
-    MOZ_ASSERT_UNREACHABLE("Unhandled reflection status.");
   }
 
+  args.rval().setObject(*snapshot);
   return true;
 }
 
 bool
 internal_JSHistogram_Clear(JSContext *cx, unsigned argc, JS::Value *vp)
 {
   JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
 
@@ -2135,24 +2233,22 @@ TelemetryHistogram::CreateHistogramSnaps
   // to launch a process for it.
   bool includeGPUProcess = false;
   if (auto gpm = mozilla::gfx::GPUProcessManager::Get()) {
     includeGPUProcess = gpm->AttemptedGPUProcess();
   }
 
   // Struct used to keep information about the histograms for which a
   // snapshot should be created
-  struct MOZ_NON_MEMMOVABLE HistogramProcessInfo {
-    Histogram* h;
-    Histogram::SampleSet ss;
-    size_t index;
+  struct HistogramProcessInfo {
+    HistogramSnapshotData data;
+    HistogramID histogramID;
   };
 
-  mozilla::Vector<mozilla::Vector<HistogramProcessInfo>>
-    processHistArray;
+  mozilla::Vector<mozilla::Vector<HistogramProcessInfo>> processHistArray;
   {
     if (!processHistArray.resize(static_cast<uint32_t>(ProcessID::Count))) {
       return NS_ERROR_OUT_OF_MEMORY;
     }
 
     StaticMutexAutoLock locker(gTelemetryHistogramMutex);
     for (uint32_t process = 0; process < static_cast<uint32_t>(ProcessID::Count); ++process) {
       mozilla::Vector<HistogramProcessInfo>& hArray = processHistArray[process];
@@ -2177,19 +2273,22 @@ TelemetryHistogram::CreateHistogramSnaps
         bool shouldInstantiate =
           info.histogramType == nsITelemetry::HISTOGRAM_FLAG;
         Histogram* h = internal_GetHistogramById(id, ProcessID(process),
                                                  shouldInstantiate);
         if (!h || internal_IsExpired(h) || !internal_ShouldReflectHistogram(h, id)) {
           continue;
         }
 
-        Histogram::SampleSet ss;
-        h->SnapshotSample(&ss);
-        if (!hArray.emplaceBack(HistogramProcessInfo{h, ss, i})) {
+        HistogramSnapshotData snapshotData;
+        if (NS_FAILED(internal_GetHistogramAndSamples(locker, h, snapshotData))) {
+          continue;
+        }
+
+        if (!hArray.emplaceBack(HistogramProcessInfo{snapshotData, id})) {
           return NS_ERROR_OUT_OF_MEMORY;
         }
 
         if (aClearSubsession) {
           h->Clear();
         }
       }
     }
@@ -2205,36 +2304,33 @@ TelemetryHistogram::CreateHistogramSnaps
                            GetNameForProcessID(ProcessID(process)),
                            processObject, JSPROP_ENUMERATE)) {
       return NS_ERROR_FAILURE;
     }
 
     const mozilla::Vector<HistogramProcessInfo>& hArray = processHistArray[process];
     for (size_t i = 0; i < hArray.length(); ++i) {
       const HistogramProcessInfo& hData = hArray[i];
-      uint32_t histogramIndex = hData.index;
-
-      HistogramID id = HistogramID(histogramIndex);
+      HistogramID id = hData.histogramID;
 
       JS::Rooted<JSObject*> hobj(aCx, JS_NewPlainObject(aCx));
       if (!hobj) {
         return NS_ERROR_FAILURE;
       }
 
-      Histogram* h = hData.h;
-      reflectStatus status =  internal_ReflectHistogramAndSamples(aCx, hobj, h,
-                                                                  hData.ss);
-      switch (status) {
-      case REFLECT_FAILURE:
+      if (NS_FAILED(internal_ReflectHistogramAndSamples(aCx,
+                                                        hobj,
+                                                        gHistogramInfos[id],
+                                                        hData.data))) {
         return NS_ERROR_FAILURE;
-      case REFLECT_OK:
-        if (!JS_DefineProperty(aCx, processObject, gHistogramInfos[id].name(),
-                               hobj, JSPROP_ENUMERATE)) {
+      }
+
+      if (!JS_DefineProperty(aCx, processObject, gHistogramInfos[id].name(),
+                             hobj, JSPROP_ENUMERATE)) {
           return NS_ERROR_FAILURE;
-        }
       }
     }
   }
   return NS_OK;
 }
 
 nsresult
 TelemetryHistogram::GetKeyedHistogramSnapshots(JSContext* aCx,