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
--- 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.");
+ }
+});