Bug 1424760 (Part 6) - Fixup the regression in part 1 r=Dexter
authorPaul Bone <pbone@mozilla.com>
Thu, 25 Jan 2018 16:17:54 +1100
changeset 401485 1dbd8efeedbfcf774474a2eaacf89ca1aa03aa69
parent 401484 0defe55f6d7659e5017b90018a289c56b88f2a47
child 401486 3f2f569d74cc277fb439f5fc96907ced06ecfbff
push id33345
push userncsoregi@mozilla.com
push dateTue, 30 Jan 2018 16:18:22 +0000
treeherdermozilla-central@fd995039d897 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersDexter
bugs1424760
milestone60.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 1424760 (Part 6) - Fixup the regression in part 1 r=Dexter I thought that I fixed an off-by-one error in part 1 of this patch series, but I didn't, I introduced one. I'm not sure why the original code didn't work. This patch also adds more testing (to avoid off-by-ones) and better logging that may help if the original problem comes back.
toolkit/components/telemetry/GCTelemetry.jsm
toolkit/components/telemetry/tests/unit/test_TelemetryGC.js
--- a/toolkit/components/telemetry/GCTelemetry.jsm
+++ b/toolkit/components/telemetry/GCTelemetry.jsm
@@ -117,35 +117,42 @@ const MAX_SLICE_KEYS = 12;
 const MAX_PHASES = 65;
 const LOGGER_NAME = "Toolkit.Telemetry";
 
 function limitProperties(name, obj, count, log) {
   log.trace("limitProperties");
 
   // If there are too many properties delete all/most of them. We don't
   // expect this ever to happen.
-  if (Object.keys(obj).length >= count) {
+  let num_properties = Object.keys(obj).length;
+  if (num_properties > count) {
     for (let key of Object.keys(obj)) {
       // If this is the main GC object then save some of the critical
       // properties.
       if (name === "data" && (
           key === "max_pause" ||
           key === "num_slices" ||
           key === "slices_list" ||
           key === "status" ||
           key === "timestamp" ||
           key === "total_time" ||
           key === "totals")) {
         continue;
       }
 
       delete obj[key];
     }
-    log.warn("Number of properties exceeded in the GC telemetry " +
-        name + " ping");
+    let log_fn;
+    if ((name === "slice.times") || (name === "data.totals")) {
+        // This is a bit more likely, but is mostly-okay.
+        log_fn = s => log.info(s);
+    } else {
+        log_fn = s => log.warn(s);
+    }
+    log_fn(`Number of properties exceeded in the GC telemetry ${name} ping, expected ${count} got ${num_properties}`);
   }
 }
 
 /*
  * Reduce the size of the object by limiting the number of slices or times
  * etc.
  */
 function limitSize(data, log) {
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryGC.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryGC.js
@@ -32,22 +32,43 @@ function run_test() {
   assert_num_entries(1, false);
   Assert.equal(20, Object.keys(get_entry()).length);
   // "true" will cause the entry to be clared.
   assert_num_entries(1, true);
   // There are currently no entries.
   assert_num_entries(0, false);
 
   // Test too many fields
-  let my_gc = make_gc();
+  let my_big_gc = make_gc();
   for (let i = 0; i < 100; i++) {
-      my_gc["new_property_" + i] = "Data";
+      my_big_gc["new_property_" + i] = "Data";
+  }
+  GCTelemetry.observeRaw(my_big_gc);
+  // Assert that it was recorded but has only 7 fields.
+  Assert.equal(7, Object.keys(get_entry()).length);
+  assert_num_entries(1, true);
+  assert_num_entries(0, false);
+
+  // Exactly the limit of fields.
+  let my_gc_24 = make_gc();
+  for (let i = 0; i < 4; i++) {
+      my_gc_24["new_property_" + i] = "Data";
   }
+  GCTelemetry.observeRaw(my_gc_24);
+  // Assert that it was recorded but has only 7 fields.
+  Assert.equal(24, Object.keys(get_entry()).length);
+  assert_num_entries(1, true);
+  assert_num_entries(0, false);
 
-  GCTelemetry.observeRaw(my_gc);
+  // Exactly too many fields.
+  let my_gc_25 = make_gc();
+  for (let i = 0; i < 5; i++) {
+      my_gc_25["new_property_" + i] = "Data";
+  }
+  GCTelemetry.observeRaw(my_gc_25);
   // Assert that it was recorded but has only 7 fields.
   Assert.equal(7, Object.keys(get_entry()).length);
   assert_num_entries(1, true);
   assert_num_entries(0, false);
 }
 
 function assert_num_entries(expect, clear) {
   let entries = GCTelemetry.entries("main", clear);