Bug 1468761 - Always serialize histograms in packed format r=chutten
☠☠ backed out by a67cc39bf78a ☠ ☠
authorJan-Erik Rediger <jrediger@mozilla.com>
Mon, 22 Oct 2018 15:21:45 +0000
changeset 442343 65bfae07e0f4fdcc22bbf909ffbe92f7224f546b
parent 442342 6c8bbe5f25a750a29707105cc1d57871af9e1fae
child 442344 9ee8406cf1d3585cb4090101ec48aa7fc696bc97
push id71461
push userjrediger@mozilla.com
push dateMon, 22 Oct 2018 15:27:17 +0000
treeherderautoland@8c08dcec61d8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerschutten
bugs1468761
milestone64.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 1468761 - Always serialize histograms in packed format r=chutten Differential Revision: https://phabricator.services.mozilla.com/D9235
toolkit/components/telemetry/app/TelemetryUtils.jsm
toolkit/components/telemetry/core/TelemetryHistogram.cpp
toolkit/components/telemetry/core/nsITelemetry.idl
--- a/toolkit/components/telemetry/app/TelemetryUtils.jsm
+++ b/toolkit/components/telemetry/app/TelemetryUtils.jsm
@@ -21,67 +21,16 @@ const PREF_TELEMETRY_ENABLED = "toolkit.
 const IS_CONTENT_PROCESS = (function() {
   // We cannot use Services.appinfo here because in telemetry xpcshell tests,
   // appinfo is initially unavailable, and becomes available only later on.
   // eslint-disable-next-line mozilla/use-services
   let runtime = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime);
   return runtime.processType == Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT;
 })();
 
-/**
- * When reflecting a histogram into JS, Telemetry hands us an object
- * with the following properties:
- *
- * - min, max, histogram_type, sum, sum_squares_{lo,hi}: simple integers;
- * - counts: array of counts for histogram buckets;
- * - ranges: array of calculated bucket sizes.
- *
- * This format is not straightforward to read and potentially bulky
- * with lots of zeros in the counts array.  Packing histograms makes
- * raw histograms easier to read and compresses the data a little bit.
- *
- * Returns an object:
- * { range: [min, max], bucket_count: <number of buckets>,
- *   histogram_type: <histogram_type>, sum: <sum>,
- *   values: { bucket1: count1, bucket2: count2, ... } }
- */
-function packHistogram(hgram) {
-  let r = hgram.ranges;
-  let c = hgram.counts;
-  let retgram = {
-    range: [r[1], r[r.length - 1]],
-    bucket_count: r.length,
-    histogram_type: hgram.histogram_type,
-    values: {},
-    sum: hgram.sum,
-  };
-
-  let first = true;
-  let last = 0;
-
-  for (let i = 0; i < c.length; i++) {
-    let value = c[i];
-    if (!value)
-      continue;
-
-    // add a lower bound
-    if (i && first) {
-      retgram.values[r[i - 1]] = 0;
-    }
-    first = false;
-    last = i + 1;
-    retgram.values[r[i]] = value;
-  }
-
-  // add an upper bound
-  if (last && last < c.length)
-    retgram.values[r[last]] = 0;
-  return retgram;
-}
-
 var TelemetryUtils = {
   Preferences: Object.freeze({
     // General Preferences
     ArchiveEnabled: "toolkit.telemetry.archive.enabled",
     CachedClientId: "toolkit.telemetry.cachedClientID",
     FirstRun: "toolkit.telemetry.reportingpolicy.firstRun",
     FirstShutdownPingEnabled: "toolkit.telemetry.firstShutdownPing.enabled",
     HealthPingEnabled: "toolkit.telemetry.healthping.enabled",
@@ -319,17 +268,17 @@ var TelemetryUtils = {
    */
   packHistograms(snapshot, testingMode = false) {
     let ret = {};
 
     for (let [process, histograms] of Object.entries(snapshot)) {
       ret[process] = {};
       for (let [name, value] of Object.entries(histograms)) {
         if (testingMode || !name.startsWith("TELEMETRY_TEST_")) {
-          ret[process][name] = packHistogram(value);
+          ret[process][name] = value;
         }
       }
     }
 
     return ret;
   },
 
   /**
@@ -368,17 +317,17 @@ var TelemetryUtils = {
         if (testingMode || !name.startsWith("TELEMETRY_TEST_")) {
           let keys = Object.keys(value);
           if (keys.length == 0) {
             // Skip empty keyed histogram
             continue;
           }
           ret[process][name] = {};
           for (let [key, hgram] of Object.entries(value)) {
-            ret[process][name][key] = packHistogram(hgram);
+            ret[process][name][key] = hgram;
           }
         }
       }
     }
 
     return ret;
   },
 
--- a/toolkit/components/telemetry/core/TelemetryHistogram.cpp
+++ b/toolkit/components/telemetry/core/TelemetryHistogram.cpp
@@ -742,63 +742,97 @@ internal_GetHistogramAndSamples(const St
   }
 
   // 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;
 }
 
+/**
+ * Reflect a histogram snapshot into a JavaScript object.
+ * The returned histogram object will have the following properties:
+ *
+ *   bucket_count - Number of buckets of this histogram
+ *   histogram_type - HISTOGRAM_EXPONENTIAL, HISTOGRAM_LINEAR, HISTOGRAM_BOOLEAN,
+ *                    HISTOGRAM_FLAG, HISTOGRAM_COUNT, or HISTOGRAM_CATEGORICAL
+ *   sum - sum of the bucket contents
+ *   range - A 2-item array of minimum and maximum bucket size
+ *   values - Map from bucket start to the bucket's count
+ */
 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)
+  if (!(JS_DefineProperty(cx, obj, "bucket_count",
+                             aHistogramInfo.bucketCount, 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)) {
+  // Create the "range" property and add it to the final object.
+  JS::Rooted<JSObject*> rarray(cx, JS_NewArrayObject(cx, 2));
+  if (rarray == nullptr
+      || !JS_DefineProperty(cx, obj, "range", rarray, JSPROP_ENUMERATE)) {
+    return NS_ERROR_FAILURE;
+  }
+  // Add [min, max] into the range array
+  if (!JS_DefineElement(cx, rarray, 0, aHistogramInfo.min, JSPROP_ENUMERATE)) {
+    return NS_ERROR_FAILURE;
+  }
+  if (!JS_DefineElement(cx, rarray, 1, aHistogramInfo.max, JSPROP_ENUMERATE)) {
+    return NS_ERROR_FAILURE;
+  }
+
+  JS::Rooted<JSObject*> values(cx, JS_NewPlainObject(cx));
+  if (values == nullptr
+      || !JS_DefineProperty(cx, obj, "values", values, JSPROP_ENUMERATE)) {
     return NS_ERROR_FAILURE;
   }
 
-  // Fill the "ranges" property.
+  bool first = true;
+  size_t last = 0;
+
   for (size_t i = 0; i < count; i++) {
-    if (!JS_DefineElement(cx, rarray, i, aSnapshot.mBucketRanges[i], JSPROP_ENUMERATE)) {
+    auto value = aSnapshot.mBucketCounts[i];
+    if (value == 0) {
+      continue;
+    }
+
+    if (i > 0 && first) {
+      auto range = aSnapshot.mBucketRanges[i - 1];
+      if (!JS_DefineProperty(cx, values, nsPrintfCString("%d", range).get(), 0, JSPROP_ENUMERATE)) {
+        return NS_ERROR_FAILURE;
+      }
+    }
+
+    first = false;
+    last = i + 1;
+
+    auto range = aSnapshot.mBucketRanges[i];
+    if (!JS_DefineProperty(cx, values, nsPrintfCString("%d", range).get(), value, 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)) {
+  if (last > 0 && last < count) {
+    auto range = aSnapshot.mBucketRanges[last];
+    if (!JS_DefineProperty(cx, values, nsPrintfCString("%d", range).get(), 0, JSPROP_ENUMERATE)) {
       return NS_ERROR_FAILURE;
     }
   }
 
   return NS_OK;
 }
 
 bool
--- a/toolkit/components/telemetry/core/nsITelemetry.idl
+++ b/toolkit/components/telemetry/core/nsITelemetry.idl
@@ -51,25 +51,23 @@ interface nsITelemetry : nsISupports
   const unsigned long DATASET_RELEASE_CHANNEL_OPTIN = 1;
 
 
   /**
    * Serializes the histograms from the given store to a JSON-style object.
    * The returned structure looks like:
    *   { "process": { "name1": histogramData1, "name2": histogramData2 }, ... }
    *
-   * Where histogramDataN has the following properties:
-   *   min - minimum bucket size
-   *   max - maximum bucket size
+   * Each histogram is represented in a packed format and has the following properties:
+   *   bucket_count - Number of buckets of this histogram
    *   histogram_type - HISTOGRAM_EXPONENTIAL, HISTOGRAM_LINEAR, HISTOGRAM_BOOLEAN,
    *                    HISTOGRAM_FLAG, HISTOGRAM_COUNT, or HISTOGRAM_CATEGORICAL
-   *   counts - array representing contents of the buckets in the histogram
-   *   ranges - array with calculated bucket sizes
    *   sum - sum of the bucket contents
-   * TODO(bug 1468761): Return packed histograms.
+   *   range - A 2-item array of minimum and maximum bucket size
+   *   values - Map from bucket to the bucket's count
    *
    * @param aStoreName The name of the store to snapshot. Ignored at the moment.
    * @param aClearStore Whether to clear out the histograms in the named store after snapshotting.
    */
   [implicit_jscontext]
   jsval getSnapshotForHistograms(in ACString aStoreName, in boolean aClearStore);
 
   /**
@@ -103,26 +101,25 @@ interface nsITelemetry : nsISupports
    * @param aClearStore Whether to clear out the keyed scalars in the named store after snapshotting.
    */
   [implicit_jscontext]
   jsval getSnapshotForKeyedScalars(in ACString aStoreName, in boolean aClearStore);
 
   /**
    * Serializes the histograms from the given dataset to a JSON-style object.
    * The returned structure looks like:
-   *   { process1: {name1: {histogramData1}, name2:{histogramData2}...}}
+   *   { "process": { "name1": histogramData1, "name2": histogramData2 }, ... }
    *
-   * Where histogramDataN has the following properties:
-   *   min - minimum bucket size
-   *   max - maximum bucket size
+   * Each histogram is represented in a packed format and has the following properties:
+   *   bucket_count - Number of buckets of this histogram
    *   histogram_type - HISTOGRAM_EXPONENTIAL, HISTOGRAM_LINEAR, HISTOGRAM_BOOLEAN,
    *                    HISTOGRAM_FLAG, HISTOGRAM_COUNT, or HISTOGRAM_CATEGORICAL
-   *   counts - array representing contents of the buckets in the histogram
-   *   ranges - array with calculated bucket sizes
    *   sum - sum of the bucket contents
+   *   range - A 2-item array of minimum and maximum bucket size
+   *   values - Map from bucket to the bucket's count
    *
    * @param aDataset DATASET_RELEASE_CHANNEL_OPTOUT or DATASET_RELEASE_CHANNEL_OPTIN.
    * @param aClear Whether to clear out the histograms after snapshotting
    */
   [implicit_jscontext]
   jsval snapshotHistograms(in uint32_t aDataset, in boolean aClear);
 
   /**