Bug 1463742 - Remove separate dynamic-builtin event storage. r?chutten draft
authorJan-Erik Rediger <jrediger@mozilla.com>
Thu, 24 May 2018 12:51:15 +0200
changeset 800501 34c25d87a5c207638e022314ea6e4889902a4913
parent 798688 a4ad0a192ce80752be35441ca10afa64ff82d813
child 800502 80f3dc2f7e7fa0542605a095f3643595c5ade398
child 800830 acba02f501fd4c9bcf07020d14b98be29fd4b6b2
push id111386
push userbmo:jrediger@mozilla.com
push dateMon, 28 May 2018 12:15:29 +0000
reviewerschutten
bugs1463742
milestone62.0a1
Bug 1463742 - Remove separate dynamic-builtin event storage. r?chutten Using a separate storage just for dynamic-builtin events was adopted from the Scalar implementation of dynamic builtins, where the second storage was needed because of the way IDs are constructed. Events however use "category#method#object" for the key, making separate storage unnecessary. Having that additional storage leads to problems along the way. One of them is the snapshotter failing to properly accumulate from both storages into a single array. In addition, even if those are merged, an additional sort step would be necessary to keep events ordered by time of occurence. And even then it can't guarantee exact order for events from different storages but same millisecond-time. MozReview-Commit-ID: AefVyDRDqQB
toolkit/components/telemetry/TelemetryEvent.cpp
toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js
--- a/toolkit/components/telemetry/TelemetryEvent.cpp
+++ b/toolkit/components/telemetry/TelemetryEvent.cpp
@@ -317,19 +317,16 @@ nsTHashtable<nsCStringHashKey> gEnabledC
 
 // The main event storage. Events are inserted here, keyed by process id and
 // in recording order.
 typedef nsUint32HashKey ProcessIDHashKey;
 typedef nsTArray<EventRecord> EventRecordArray;
 typedef nsClassHashtable<ProcessIDHashKey, EventRecordArray> EventRecordsMapType;
 
 EventRecordsMapType gEventRecords;
-// Provide separate storage for "dynamic builtin" events needed to
-// support "build faster" in local developer builds.
-EventRecordsMapType gBuiltinEventRecords;
 
 // The details on dynamic events that are recorded from addons are registered here.
 StaticAutoPtr<nsTArray<DynamicEventInfo>> gDynamicEventInfo;
 
 } // namespace
 
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
@@ -399,27 +396,22 @@ CanRecordEvent(const StaticMutexAutoLock
 bool
 IsExpired(const EventKey& key)
 {
   return key.id == kExpiredEventId;
 }
 
 EventRecordArray*
 GetEventRecordsForProcess(const StaticMutexAutoLock& lock, ProcessID processType,
-                          const EventKey& eventKey, bool isDynamicBuiltin)
+                          const EventKey& eventKey)
 {
-  // Put dynamic-builtin events (used to support "build faster") in a
-  // separate storage.
-  EventRecordsMapType& processStorage =
-    isDynamicBuiltin ? gBuiltinEventRecords : gEventRecords;
-
   EventRecordArray* eventRecords = nullptr;
-  if (!processStorage.Get(uint32_t(processType), &eventRecords)) {
+  if (!gEventRecords.Get(uint32_t(processType), &eventRecords)) {
     eventRecords = new EventRecordArray();
-    processStorage.Put(uint32_t(processType), eventRecords);
+    gEventRecords.Put(uint32_t(processType), eventRecords);
   }
   return eventRecords;
 }
 
 EventKey*
 GetEventKey(const StaticMutexAutoLock& lock, const nsACString& category,
            const nsACString& method, const nsACString& object)
 {
@@ -478,19 +470,18 @@ RecordEvent(const StaticMutexAutoLock& l
 
   // Fixup the process id only for non-builtin (e.g. supporting build faster)
   // dynamic events.
   if (eventKey->dynamic &&
       !(*gDynamicEventInfo)[eventKey->id].builtin) {
     processType = ProcessID::Dynamic;
   }
 
-  bool isDynamicBuiltin = eventKey->dynamic && (*gDynamicEventInfo)[eventKey->id].builtin;
   EventRecordArray* eventRecords =
-    GetEventRecordsForProcess(lock, processType, *eventKey, isDynamicBuiltin);
+    GetEventRecordsForProcess(lock, processType, *eventKey);
 
   // Apply hard limit on event count in storage.
   if (eventRecords->Length() >= kMaxEventRecords) {
     return RecordEventResult::StorageLimitReached;
   }
 
   // Check whether the extra keys passed are valid.
   if (!CheckExtraKeysValid(*eventKey, extra)) {
@@ -751,17 +742,16 @@ TelemetryEvent::DeInitializeGlobalState(
 
   gCanRecordBase = false;
   gCanRecordExtended = false;
 
   gEventNameIDMap.Clear();
   gCategoryNames.Clear();
   gEnabledCategories.Clear();
   gEventRecords.Clear();
-  gBuiltinEventRecords.Clear();
 
   gDynamicEventInfo = nullptr;
 
   gInitDone = false;
 }
 
 void
 TelemetryEvent::SetCanRecordBase(bool b)
@@ -1163,21 +1153,19 @@ TelemetryEvent::CreateSnapshots(uint32_t
           const char* processName = GetNameForProcessID(ProcessID(iter.Key()));
           processEvents.AppendElement(mozilla::MakePair(processName, Move(events)));
         }
       }
     };
 
     // Take a snapshot of the plain and dynamic builtin events.
     snapshotter(gEventRecords);
-    snapshotter(gBuiltinEventRecords);
 
     if (aClear) {
       gEventRecords.Clear();
-      gBuiltinEventRecords.Clear();
     }
   }
 
   // (2) Serialize the events to a JS object.
   JS::RootedObject rootObj(cx, JS_NewPlainObject(cx));
   if (!rootObj) {
     return NS_ERROR_FAILURE;
   }
@@ -1207,17 +1195,16 @@ TelemetryEvent::ClearEvents()
 {
   StaticMutexAutoLock lock(gTelemetryEventsMutex);
 
   if (!gInitDone) {
     return;
   }
 
   gEventRecords.Clear();
-  gBuiltinEventRecords.Clear();
 }
 
 void
 TelemetryEvent::SetEventRecordingEnabled(const nsACString& category, bool enabled)
 {
   StaticMutexAutoLock locker(gTelemetryEventsMutex);
 
   if (!gCategoryNames.Contains(category)) {
@@ -1250,17 +1237,16 @@ TelemetryEvent::SizeOfIncludingThis(mozi
       for (uint32_t i = 0; i < len; ++i) {
         partial += (*eventRecords)[i].SizeOfExcludingThis(aMallocSizeOf);
       }
     }
     return partial;
   };
 
   n += getSizeOfRecords(gEventRecords);
-  n += getSizeOfRecords(gBuiltinEventRecords);
 
   n += gEventNameIDMap.ShallowSizeOfExcludingThis(aMallocSizeOf);
   for (auto iter = gEventNameIDMap.ConstIter(); !iter.Done(); iter.Next()) {
     n += iter.Key().SizeOfExcludingThisIfUnshared(aMallocSizeOf);
   }
 
   n += gCategoryNames.ShallowSizeOfExcludingThis(aMallocSizeOf);
   n += gEnabledCategories.ShallowSizeOfExcludingThis(aMallocSizeOf);
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js
@@ -190,8 +190,50 @@ add_task(async function test_dynamicBuil
   ];
   let events = snapshot.parent;
   Assert.equal(events.length, expected.length, "Should have recorded the right amount of events.");
   for (let i = 0; i < expected.length; ++i) {
     Assert.deepEqual(events[i].slice(1), expected[i],
                      "Should have recorded the expected event data.");
   }
 });
+
+add_task(async function test_dynamicBuiltinDontOverwriteStaticData() {
+  Telemetry.clearEvents();
+  Telemetry.canRecordExtended = true;
+
+  const TEST_STATIC_EVENT_NAME = "telemetry.test";
+  const TEST_EVENT_NAME = "telemetry.test.nooverwrite";
+
+  // Register some dynamic builtin test events.
+  Telemetry.registerBuiltinEvents(TEST_EVENT_NAME, {
+    "dynamic": {
+      methods: ["dynamic"],
+      objects: ["builtin", "anotherone"],
+    }
+  });
+
+  // First enable the categories we're using
+  Telemetry.setEventRecordingEnabled(TEST_STATIC_EVENT_NAME, true);
+  Telemetry.setEventRecordingEnabled(TEST_EVENT_NAME, true);
+
+  // Now record some dynamic-builtin and static events
+  Telemetry.recordEvent(TEST_EVENT_NAME, "dynamic", "builtin");
+  Telemetry.recordEvent(TEST_STATIC_EVENT_NAME, "test1", "object1");
+  Telemetry.recordEvent(TEST_EVENT_NAME, "dynamic", "anotherone");
+
+  let snapshot =
+    Telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
+  Assert.ok(("parent" in snapshot), "Should have parent events in the snapshot.");
+
+  // All events should now be recorded in the right order
+  let expected = [
+    [TEST_EVENT_NAME, "dynamic", "builtin"],
+    [TEST_STATIC_EVENT_NAME, "test1", "object1"],
+    [TEST_EVENT_NAME, "dynamic", "anotherone"],
+  ];
+  let events = snapshot.parent;
+  Assert.equal(events.length, expected.length, "Should have recorded the right amount of events.");
+  for (let i = 0; i < expected.length; ++i) {
+    Assert.deepEqual(events[i].slice(1), expected[i],
+                     "Should have recorded the expected event data.");
+  }
+});