From 8e78cfd644d599a5907f318703e58ff7544553fa Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Mon, 6 Jan 2025 11:02:18 +0100 Subject: [PATCH 1/9] Remove unused ProfilerMarkerPayload type. --- src/types/profile.js | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/types/profile.js b/src/types/profile.js index 03bfe096ab..719dadcd6f 100644 --- a/src/types/profile.js +++ b/src/types/profile.js @@ -227,18 +227,6 @@ export type NativeAllocationsTable = | UnbalancedNativeAllocationsTable | BalancedNativeAllocationsTable; -/** - * This is the base abstract class that marker payloads inherit from. This probably isn't - * used directly in profiler.firefox.com, but is provided here for mainly documentation - * purposes. - */ -export type ProfilerMarkerPayload = { - type: string, - startTime?: Milliseconds, - endTime?: Milliseconds, - stack?: Thread, -}; - /** * Markers represent arbitrary events that happen within the browser. They have a * name, time, and potentially a JSON data payload. These can come from all over the From b72025c2342a7f735baedf107fc549364d0416ff Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Mon, 6 Jan 2025 11:56:28 +0100 Subject: [PATCH 2/9] Remove useless markerSchemaByName check. This variable is always non-null. We likely wanted to check markerSchemaName but that's also always non-null at the moment. --- src/profile-logic/tracks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/profile-logic/tracks.js b/src/profile-logic/tracks.js index 4b69c15577..5af38558b9 100644 --- a/src/profile-logic/tracks.js +++ b/src/profile-logic/tracks.js @@ -328,7 +328,7 @@ export function computeLocalTracksByPid( thread.stringTable.getString(markerNameIndex), markerData ); - if (markerData && markerSchemaByName) { + if (markerData) { const mapEntry = markerTracksBySchemaName.get(markerSchemaName); if (mapEntry && mapEntry.keys.every((k) => k in markerData)) { mapEntry.markerNames.add(markerNameIndex); From 8d2e3f62a5859f74421e4223bfc0572a3f10b272 Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Mon, 6 Jan 2025 12:11:22 +0100 Subject: [PATCH 3/9] Allow getMarkerSchemaName to return null. --- src/profile-logic/marker-data.js | 7 +++++-- src/profile-logic/marker-schema.js | 13 +++++++------ src/profile-logic/tracks.js | 2 +- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/profile-logic/marker-data.js b/src/profile-logic/marker-data.js index fa1aa55edf..d4c978ba34 100644 --- a/src/profile-logic/marker-data.js +++ b/src/profile-logic/marker-data.js @@ -1528,9 +1528,12 @@ export function filterMarkerByDisplayLocation( return additionalResult; } - return markerTypes.has( - getMarkerSchemaName(markerSchemaByName, marker.name, marker.data) + const schemaName = getMarkerSchemaName( + markerSchemaByName, + marker.name, + marker.data ); + return schemaName !== null && markerTypes.has(schemaName); }); } diff --git a/src/profile-logic/marker-schema.js b/src/profile-logic/marker-schema.js index 91046ffbd6..ea5790c19c 100644 --- a/src/profile-logic/marker-schema.js +++ b/src/profile-logic/marker-schema.js @@ -91,7 +91,7 @@ export function getMarkerSchemaName( markerSchemaByName: MarkerSchemaByName, markerName: string, markerData: MarkerPayload | null -): string { +): string | null { if (!markerData) { // Fall back to using the name if no payload exists. return markerName; @@ -127,11 +127,12 @@ export function getSchemaFromMarker( markerName: string, markerData: MarkerPayload | null ): MarkerSchema | null { - return ( - markerSchemaByName[ - getMarkerSchemaName(markerSchemaByName, markerName, markerData) - ] || null + const schemaName = getMarkerSchemaName( + markerSchemaByName, + markerName, + markerData ); + return schemaName !== null ? (markerSchemaByName[schemaName] ?? null) : null; } /** @@ -390,7 +391,7 @@ export function getLabelGetter( marker.name, marker.data ); - const applyLabel = labelFns.get(schemaName); + const applyLabel = schemaName !== null ? labelFns.get(schemaName) : null; label = applyLabel ? // A label function is available, apply it. diff --git a/src/profile-logic/tracks.js b/src/profile-logic/tracks.js index 5af38558b9..3f35669d9c 100644 --- a/src/profile-logic/tracks.js +++ b/src/profile-logic/tracks.js @@ -328,7 +328,7 @@ export function computeLocalTracksByPid( thread.stringTable.getString(markerNameIndex), markerData ); - if (markerData) { + if (markerData && markerSchemaName) { const mapEntry = markerTracksBySchemaName.get(markerSchemaName); if (mapEntry && mapEntry.keys.every((k) => k in markerData)) { mapEntry.markerNames.add(markerNameIndex); From 50f792e7b7452c73b8019b226ca92022e7c3a290 Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Fri, 20 Dec 2024 10:39:07 -0500 Subject: [PATCH 4/9] For markers without payload, don't fall back to the marker name when looking up the schema. The usefulness of this special case appears to have been limited to writing tests: Our tests contained many markers which only match a schema due to their name. So this patch moves the special case to the handling of TestDefinedMarkers: When the payload is absent, we synthesize a { type: } payload for convenience. Markers from Firefox never needed this special case. And the special handling caused some trouble when markers were mistakenly matched against a schema with a custom label that was referring to fields which weren't present on the marker (because the marker doesn't have a payload). I found two cases of this in the wild, with "CPUSpeed" and "Awake" markers: The first CPUSpeed marker on https://share.firefox.dev/4gmLaif has a null data field. Before this patch, it matched a CPUSpeed schema. This gave it a broken tableLabel ("CPUSpeed Speed = GHz"). For Awake markers (e.g. on https://share.firefox.dev/403suOD ), the code in Firefox which emits these markers uses the typed marker API for the IntervalStart marker, and the payload-less API for the IntervalEnd marker: https://searchfox.org/mozilla-central/rev/43ce3de32b3a946bceac74c3badf442af9f245c0/tools/profiler/core/platform.cpp#7496-7497 So unpaired end markers got a broken tableLabel from the Awake schema before this patch ("Awake - CPU Id = " on https://share.firefox.dev/4fx8cSf ). These two cases are fixed by this patch - the two markers get the default tableLabel, which does not refer to these absent fields. --- src/profile-logic/marker-schema.js | 4 +- .../fixtures/profiles/processed-profile.js | 17 +++- .../store/__snapshots__/markers.test.js.snap | 8 +- .../__snapshots__/profile-view.test.js.snap | 96 ++++++++++++++----- 4 files changed, 92 insertions(+), 33 deletions(-) diff --git a/src/profile-logic/marker-schema.js b/src/profile-logic/marker-schema.js index ea5790c19c..7990862268 100644 --- a/src/profile-logic/marker-schema.js +++ b/src/profile-logic/marker-schema.js @@ -93,8 +93,8 @@ export function getMarkerSchemaName( markerData: MarkerPayload | null ): string | null { if (!markerData) { - // Fall back to using the name if no payload exists. - return markerName; + // No payload - no schema. + return null; } const { type } = markerData; diff --git a/src/test/fixtures/profiles/processed-profile.js b/src/test/fixtures/profiles/processed-profile.js index c3e9de110c..fe96acb72a 100644 --- a/src/test/fixtures/profiles/processed-profile.js +++ b/src/test/fixtures/profiles/processed-profile.js @@ -62,17 +62,23 @@ type MarkerName = string; type MarkerTime = Milliseconds; // These markers can create an Instant or a complete Interval marker, depending -// on if an end time is passed in. The definition uses a union, becaus as far +// on if an end time is passed in. +// +// If the data field is left out (undefined), a default value { type: MarkerName } +// is used. If the data field is manually set to null, a null data is used. +// +// The definition uses a union, becaus as far // as I can tell, Flow doesn't support multiple arity tuples. export type TestDefinedMarkers = Array< - // Instant marker: + // Instant marker, payload defaulting to { type: MarkerName }: | [MarkerName, MarkerTime] - // No payload: + // Interval marker: | [ MarkerName, MarkerTime, // start time MarkerTime | null, // end time ] + // Marker with manual payload: | [ MarkerName, MarkerTime, // start time @@ -165,7 +171,8 @@ export function addMarkersToThreadWithCorrespondingSamples( const startTime = tuple[1]; // Flow doesn't support variadic tuple types. const maybeEndTime = (tuple: any)[2] || null; - const payload: MarkerPayload | null = (tuple: any)[3] || null; + const maybePayload: MarkerPayload | null | void = (tuple: any)[3]; + const payload = maybePayload === undefined ? { type: name } : maybePayload; markersTable.name.push(stringTable.indexForString(name)); if (maybeEndTime === null) { @@ -184,7 +191,7 @@ export function addMarkersToThreadWithCorrespondingSamples( markerSchemaForTests, stringTable ); - markersTable.data.push(payload); + markersTable.data.push((payload: any)); markersTable.category.push(0); markersTable.length++; }); diff --git a/src/test/store/__snapshots__/markers.test.js.snap b/src/test/store/__snapshots__/markers.test.js.snap index 7ecb6b42d7..b66d251669 100644 --- a/src/test/store/__snapshots__/markers.test.js.snap +++ b/src/test/store/__snapshots__/markers.test.js.snap @@ -17,7 +17,9 @@ Array [ }, Object { "category": 0, - "data": null, + "data": Object { + "type": "TTI", + }, "end": null, "name": "TTI", "start": 6, @@ -49,7 +51,9 @@ Array [ }, Object { "category": 0, - "data": null, + "data": Object { + "type": "FirstContentfulPaint", + }, "end": 8, "name": "FirstContentfulPaint", "start": 7, diff --git a/src/test/store/__snapshots__/profile-view.test.js.snap b/src/test/store/__snapshots__/profile-view.test.js.snap index 138d5480f8..fda6bd7654 100644 --- a/src/test/store/__snapshots__/profile-view.test.js.snap +++ b/src/test/store/__snapshots__/profile-view.test.js.snap @@ -996,12 +996,24 @@ Object { 0, ], "data": Array [ - null, - null, - null, - null, - null, - null, + Object { + "type": "A", + }, + Object { + "type": "B", + }, + Object { + "type": "C", + }, + Object { + "type": "D", + }, + Object { + "type": "E", + }, + Object { + "type": "F", + }, Object { "URI": "https://mozilla.org", "endTime": 6.5, @@ -1678,12 +1690,24 @@ Array [ 0, ], "data": Array [ - null, - null, - null, - null, - null, - null, + Object { + "type": "A", + }, + Object { + "type": "B", + }, + Object { + "type": "C", + }, + Object { + "type": "D", + }, + Object { + "type": "E", + }, + Object { + "type": "F", + }, Object { "URI": "https://mozilla.org", "endTime": 6.5, @@ -1841,7 +1865,9 @@ exports[`snapshots of selectors/profile matches the last stored run of markerThr Array [ Object { "category": 0, - "data": null, + "data": Object { + "type": "D", + }, "end": null, "name": "D", "start": 3, @@ -1849,7 +1875,9 @@ Array [ }, Object { "category": 0, - "data": null, + "data": Object { + "type": "E", + }, "end": null, "name": "E", "start": 4, @@ -1857,7 +1885,9 @@ Array [ }, Object { "category": 0, - "data": null, + "data": Object { + "type": "F", + }, "end": null, "name": "F", "start": 5, @@ -1908,7 +1938,9 @@ exports[`snapshots of selectors/profile matches the last stored run of markerThr Array [ Object { "category": 0, - "data": null, + "data": Object { + "type": "A", + }, "end": null, "name": "A", "start": 0, @@ -1916,7 +1948,9 @@ Array [ }, Object { "category": 0, - "data": null, + "data": Object { + "type": "B", + }, "end": null, "name": "B", "start": 1, @@ -1924,7 +1958,9 @@ Array [ }, Object { "category": 0, - "data": null, + "data": Object { + "type": "C", + }, "end": null, "name": "C", "start": 2, @@ -1932,7 +1968,9 @@ Array [ }, Object { "category": 0, - "data": null, + "data": Object { + "type": "D", + }, "end": null, "name": "D", "start": 3, @@ -1940,7 +1978,9 @@ Array [ }, Object { "category": 0, - "data": null, + "data": Object { + "type": "E", + }, "end": null, "name": "E", "start": 4, @@ -1948,7 +1988,9 @@ Array [ }, Object { "category": 0, - "data": null, + "data": Object { + "type": "F", + }, "end": null, "name": "F", "start": 5, @@ -2068,7 +2110,9 @@ exports[`snapshots of selectors/profile matches the last stored run of markerThr Array [ Object { "category": 0, - "data": null, + "data": Object { + "type": "D", + }, "end": null, "name": "D", "start": 3, @@ -2076,7 +2120,9 @@ Array [ }, Object { "category": 0, - "data": null, + "data": Object { + "type": "E", + }, "end": null, "name": "E", "start": 4, @@ -2084,7 +2130,9 @@ Array [ }, Object { "category": 0, - "data": null, + "data": Object { + "type": "F", + }, "end": null, "name": "F", "start": 5, From af20273b88ec3f0e7e3cfeeddf6a30aff981ac3e Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Mon, 6 Jan 2025 12:36:41 +0100 Subject: [PATCH 5/9] Make this marker test more realistic. RefreshDriverTick markers from Firefox are Text markers. Firefox (as of Jan 2025) has never emitted a 'RefreshDriverTick' schema. Only pre-schema profiles have a 'RefreshDriverTick' schema, which they get from the v33 upgrader. Post-schema profiles don't display RefreshDriverTick markers in the timeline-overview area, because Text markers aren't displayed there. This commit makes the marker test consistent with what Firefox outputs. Now it no longer relies on a special case in getMarkerSchemaName which looks up schemas for Text markers based on the marker name. To test timeline-overview filtering, a new marker is introduced, with schema name 'VisibleInTimelineOverview'. --- .../components/__snapshots__/MenuButtons.test.js.snap | 6 +++--- src/test/fixtures/profiles/marker-schema.js | 11 ++--------- .../store/__snapshots__/profile-view.test.js.snap | 11 ++--------- src/test/store/markers.test.js | 7 +++++-- 4 files changed, 12 insertions(+), 23 deletions(-) diff --git a/src/test/components/__snapshots__/MenuButtons.test.js.snap b/src/test/components/__snapshots__/MenuButtons.test.js.snap index 97eefd2356..9e176d7d58 100644 --- a/src/test/components/__snapshots__/MenuButtons.test.js.snap +++ b/src/test/components/__snapshots__/MenuButtons.test.js.snap @@ -2211,7 +2211,7 @@ exports[`app/MenuButtons matches the snapshot for the menu buttons and class="menuButtonsDownloadSize" > ( - 1.65 kB + 1.64 kB ) @@ -2329,7 +2329,7 @@ exports[`app/MenuButtons matches the snapshot for the opened panel for class="menuButtonsDownloadSize" > ( - 1.63 kB + 1.62 kB ) @@ -2442,7 +2442,7 @@ exports[`app/MenuButtons matches the snapshot for the opened panel for class="menuButtonsDownloadSize" > ( - 1.65 kB + 1.64 kB ) diff --git a/src/test/fixtures/profiles/marker-schema.js b/src/test/fixtures/profiles/marker-schema.js index 8a783bd024..a19320fea9 100644 --- a/src/test/fixtures/profiles/marker-schema.js +++ b/src/test/fixtures/profiles/marker-schema.js @@ -185,16 +185,9 @@ export const markerSchemaForTests: MarkerSchema[] = [ ], }, { - name: 'RefreshDriverTick', + name: 'VisibleInTimelineOverview', display: ['marker-chart', 'marker-table', 'timeline-overview'], - data: [ - { - key: 'name', - label: 'Tick Reasons', - format: 'string', - searchable: true, - }, - ], + data: [], }, { name: 'StringTesting', diff --git a/src/test/store/__snapshots__/profile-view.test.js.snap b/src/test/store/__snapshots__/profile-view.test.js.snap index fda6bd7654..d36ad80eb1 100644 --- a/src/test/store/__snapshots__/profile-view.test.js.snap +++ b/src/test/store/__snapshots__/profile-view.test.js.snap @@ -403,20 +403,13 @@ Object { "tooltipLabel": "IPC — {marker.data.niceDirection}", }, Object { - "data": Array [ - Object { - "format": "string", - "key": "name", - "label": "Tick Reasons", - "searchable": true, - }, - ], + "data": Array [], "display": Array [ "marker-chart", "marker-table", "timeline-overview", ], - "name": "RefreshDriverTick", + "name": "VisibleInTimelineOverview", }, Object { "data": Array [ diff --git a/src/test/store/markers.test.js b/src/test/store/markers.test.js index 61450e3768..623e3918fe 100644 --- a/src/test/store/markers.test.js +++ b/src/test/store/markers.test.js @@ -419,7 +419,8 @@ describe('Marker schema filtering', function () { const profile = getProfileWithMarkers([ ['no payload', 0, null, null], ['payload no schema', 0, null, { type: 'no schema marker' }], - ['RefreshDriverTick', 0, null, { type: 'Text', name: 'RefreshDriverTick' }], + ['RefreshDriverTick', 0, null, { type: 'Text', name: 'Tick with 1 observer' }], + ['VisibleInTimelineOverview', 0, null], ['UserTiming', 5, 6, { type: 'UserTiming', name: 'name', entryType: 'mark' }], // The following is a tracing marker without a schema attached, this was a // regression reported in Bug 1678698. @@ -459,6 +460,7 @@ describe('Marker schema filtering', function () { 'no payload', 'payload no schema', 'RefreshDriverTick', + 'VisibleInTimelineOverview', 'Load 0: https://mozilla.org', 'UserTiming', 'RandomTracingMarker', @@ -473,6 +475,7 @@ describe('Marker schema filtering', function () { 'no payload', 'payload no schema', 'RefreshDriverTick', + 'VisibleInTimelineOverview', 'Load 0: https://mozilla.org', 'UserTiming', 'RandomTracingMarker', @@ -664,7 +667,7 @@ describe('Marker schema filtering', function () { const { getMarkerNames } = setup(getProfileForMarkerSchema()); expect( getMarkerNames(selectedThreadSelectors.getTimelineOverviewMarkerIndexes) - ).toEqual(['RefreshDriverTick']); + ).toEqual(['VisibleInTimelineOverview']); }); }); From 05530d4f7432c74891920a282391eae076e79e5f Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Mon, 6 Jan 2025 12:36:41 +0100 Subject: [PATCH 6/9] Don't look up the schema for Text markers based on the marker name. Just use data.type, like for other normal markers. This mostly just affects the RefreshDriverTick marker from upgraded profiles: - Pre-schema profiles which get their schema from the v33 upgrader have a schema with name 'RefreshDriverTick' with 'timeline-overview' in the display array. - Post-schema profiles from Firefox do not have a 'RefreshDriverTick' schema. The RefreshDriverTick markers in such profiles is a regular Text marker, and Text markers do not show up in the 'timeline-overview' area. This change makes it so that the 'RefreshDriverTick' schema from upgraded pre-schema profiles becomes unused, and the markers vanish from the timeline-overview area. This is consistent with post-schema profiles. --- src/profile-logic/gecko-profile-versioning.js | 8 ++++++++ src/profile-logic/marker-schema.js | 6 ------ src/profile-logic/processed-profile-versioning.js | 8 ++++++++ 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/profile-logic/gecko-profile-versioning.js b/src/profile-logic/gecko-profile-versioning.js index d7f86f6d0d..99a23bd60a 100644 --- a/src/profile-logic/gecko-profile-versioning.js +++ b/src/profile-logic/gecko-profile-versioning.js @@ -1288,6 +1288,14 @@ const _upgraders = { ], }, { + // An unused schema for RefreshDriverTick markers. + // This schema is not consistent with what post-schema Firefox would + // output. Firefox (as of Jan 2026) is still using Text markers and does + // not have a RefreshDriverTick schema. Furthermore, upgraded profiles + // which get this schema do not have any { type: 'RefreshDriverTick' } + // markers - in the past they picked up this schema due to a compat hack, + // but this hack is now removed. So this schema is unused. It is kept + // here for historical accuracy. name: 'RefreshDriverTick', display: ['marker-chart', 'marker-table', 'timeline-overview'], data: [{ key: 'name', label: 'Tick Reasons', format: 'string' }], diff --git a/src/profile-logic/marker-schema.js b/src/profile-logic/marker-schema.js index 7990862268..505fb69bd8 100644 --- a/src/profile-logic/marker-schema.js +++ b/src/profile-logic/marker-schema.js @@ -109,12 +109,6 @@ export function getMarkerSchemaName( : // If so, use the category as the schema name. markerData.category; } - if (type === 'Text') { - // Text markers are a cheap and easy way to create markers with - // a category. Check for schema if it exists, if not, fallback to - // a Text type marker. - return markerSchemaByName[markerName] === undefined ? 'Text' : markerName; - } return type; } diff --git a/src/profile-logic/processed-profile-versioning.js b/src/profile-logic/processed-profile-versioning.js index 6dc2eec8ca..6ec10c2cd1 100644 --- a/src/profile-logic/processed-profile-versioning.js +++ b/src/profile-logic/processed-profile-versioning.js @@ -1676,6 +1676,14 @@ const _upgraders = { ], }, { + // An unused schema for RefreshDriverTick markers. + // This schema is not consistent with what post-schema Firefox would + // output. Firefox (as of Jan 2025) is still using Text markers and does + // not have a RefreshDriverTick schema. Furthermore, upgraded profiles + // which get this schema do not have any { type: 'RefreshDriverTick' } + // markers - in the past they picked up this schema due to a compat hack, + // but this hack is now removed. So this schema is unused. It is kept + // here for historical accuracy. name: 'RefreshDriverTick', display: ['marker-chart', 'marker-table', 'timeline-overview'], data: [{ key: 'name', label: 'Tick Reasons', format: 'string' }], From dd29459145c89d6132b1732475cc2901ad3c18a4 Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Mon, 6 Jan 2025 12:32:42 +0100 Subject: [PATCH 7/9] Make marker tests more realistic. This adjusts some markers and schemas to be more consistent with what Firefox outputs: Markers with { type: "Paint" / "Navigation" / "Layout" } aren't present in any existing profiles, they were neither emitted by Firefox nor generated by an upgrader. Instead, these markers have { type: "tracing" } in their payload. DOMEvent markers were tracing markers in the past, and have their own schema today. The test now uses both variants. CC markers were tracing markers in the past, and have their own schema today. This test now uses the former. Post-schema profiles from Firefox have schema with name 'tracing'. This makes tracing markers visible in the 'timeline-overview' area. This patch adds the tracing schema to the test profile. This affects a test which was expecting that 'RandomTracingMarker' was not visible in the timeline-overview area. The test expectation is adjusted. Upgraded pre-schema profiles do not have a 'tracing' schema. This will be fixed in the next commit. --- src/test/components/TimelineMarkers.test.js | 24 +++++++++---------- .../__snapshots__/MenuButtons.test.js.snap | 6 ++--- src/test/fixtures/profiles/marker-schema.js | 15 +----------- .../__snapshots__/profile-view.test.js.snap | 18 +------------- src/test/store/markers.test.js | 8 +++---- 5 files changed, 21 insertions(+), 50 deletions(-) diff --git a/src/test/components/TimelineMarkers.test.js b/src/test/components/TimelineMarkers.test.js index b275bf5775..000780baff 100644 --- a/src/test/components/TimelineMarkers.test.js +++ b/src/test/components/TimelineMarkers.test.js @@ -179,14 +179,14 @@ describe('TimelineMarkers', function () { window.devicePixelRatio = 1; const { container } = setupWithMarkers({ rangeStart: 0, rangeEnd: 15 }, [ - ['DOMEvent', 0, 10], - ['DOMEvent', 0, 10], - ['DOMEvent', 5, 15], - ['Paint', 2, 13], - ['Navigation', 2, 6], - ['Layout', 6, 8], + ['DOMEvent', 0, 10, { type: 'tracing', category: 'JS' }], + ['DOMEvent', 0, 10, { type: 'tracing', category: 'JS' }], + ['DOMEvent', 5, 15, { type: 'DOMEvent', latency: 3.7, eventType: 'msg' }], + ['Paint', 2, 13, { type: 'tracing', category: 'Paint' }], + ['Navigation', 2, 6, { type: 'tracing', category: 'Navigation' }], + ['Layout', 6, 8, { type: 'tracing', category: 'Layout' }], // These 2 will be ignored. - ['CC', 0, 5], + ['CC', 0, 5, { type: 'tracing', category: 'CC' }], ['GCMajor', 5, 10], ]); @@ -205,8 +205,8 @@ describe('TimelineMarkers', function () { { rangeStart: 0, rangeEnd: 15, component: TimelineMarkersMemory }, [ // The first one will be ignored. - ['DOMEvent', 0, 10], - ['CC', 0, 5], + ['DOMEvent', 0, 10, { type: 'tracing', category: 'JS' }], + ['CC', 0, 5, { type: 'tracing', category: 'CC' }], ['GCMajor', 5, 10], ] ); @@ -253,7 +253,7 @@ describe('TimelineMarkers', function () { { rangeStart: 0, rangeEnd: 10 }, [ ['DOMEvent', 0, 3], - ['Navigation', 6, 10], + ['Load', 6, 10, { type: 'tracing', category: 'Navigation' }], ] ); @@ -300,7 +300,7 @@ describe('TimelineMarkers', function () { { rangeStart: 0, rangeEnd: 10 }, [ ['DOMEvent', 0, 3], - ['Navigation', 6, 10], + ['Load', 6, 10, { type: 'tracing', category: 'Navigation' }], ] ); @@ -315,7 +315,7 @@ describe('TimelineMarkers', function () { expect(getContextMenu()).toHaveClass('react-contextmenu--visible'); clickOnMenuItem('Copy description'); - expect(copy).toHaveBeenLastCalledWith('Navigation'); + expect(copy).toHaveBeenLastCalledWith('Load'); expect(getContextMenu()).not.toHaveClass('react-contextmenu--visible'); diff --git a/src/test/components/__snapshots__/MenuButtons.test.js.snap b/src/test/components/__snapshots__/MenuButtons.test.js.snap index 9e176d7d58..0e79a8879f 100644 --- a/src/test/components/__snapshots__/MenuButtons.test.js.snap +++ b/src/test/components/__snapshots__/MenuButtons.test.js.snap @@ -2211,7 +2211,7 @@ exports[`app/MenuButtons matches the snapshot for the menu buttons and class="menuButtonsDownloadSize" > ( - 1.64 kB + 1.63 kB ) @@ -2329,7 +2329,7 @@ exports[`app/MenuButtons matches the snapshot for the opened panel for class="menuButtonsDownloadSize" > ( - 1.62 kB + 1.61 kB ) @@ -2442,7 +2442,7 @@ exports[`app/MenuButtons matches the snapshot for the opened panel for class="menuButtonsDownloadSize" > ( - 1.64 kB + 1.63 kB ) diff --git a/src/test/fixtures/profiles/marker-schema.js b/src/test/fixtures/profiles/marker-schema.js index a19320fea9..4a1ddf7e23 100644 --- a/src/test/fixtures/profiles/marker-schema.js +++ b/src/test/fixtures/profiles/marker-schema.js @@ -144,26 +144,13 @@ export const markerSchemaForTests: MarkerSchema[] = [ ], }, { - // TODO - Note that this marker is a "tracing" marker currently. - // See issue #2749 - name: 'Paint', + name: 'tracing', display: ['marker-chart', 'marker-table', 'timeline-overview'], data: [ { key: 'category', label: 'Type', format: 'string', searchable: true }, ], }, { - // TODO - Note that this marker is a "tracing" marker currently. - // See issue #2749 - name: 'Navigation', - display: ['marker-chart', 'marker-table', 'timeline-overview'], - data: [ - { key: 'category', label: 'Type', format: 'string', searchable: true }, - ], - }, - { - // TODO - Note that this marker is a "tracing" marker currently. - // See issue #2749 name: 'Layout', display: ['marker-chart', 'marker-table', 'timeline-overview'], data: [ diff --git a/src/test/store/__snapshots__/profile-view.test.js.snap b/src/test/store/__snapshots__/profile-view.test.js.snap index d36ad80eb1..9af0dc9e79 100644 --- a/src/test/store/__snapshots__/profile-view.test.js.snap +++ b/src/test/store/__snapshots__/profile-view.test.js.snap @@ -335,23 +335,7 @@ Object { "marker-table", "timeline-overview", ], - "name": "Paint", - }, - Object { - "data": Array [ - Object { - "format": "string", - "key": "category", - "label": "Type", - "searchable": true, - }, - ], - "display": Array [ - "marker-chart", - "marker-table", - "timeline-overview", - ], - "name": "Navigation", + "name": "tracing", }, Object { "data": Array [ diff --git a/src/test/store/markers.test.js b/src/test/store/markers.test.js index 623e3918fe..03de917204 100644 --- a/src/test/store/markers.test.js +++ b/src/test/store/markers.test.js @@ -227,9 +227,9 @@ describe('memory markers', function () { return storeWithProfile( getProfileWithMarkers([ - ['DOMEvent', 0, null], - ['Navigation', 1, null], - ['Paint', 2, null], + ['DOMEvent', 0, null, { type: 'tracing', category: 'JS' }], + ['Navigation', 1, null, { type: 'tracing', category: 'Navigation' }], + ['Paint', 2, null, { type: 'tracing', category: 'Paint' }], ['IdleForgetSkippable', 3, 4, { type: 'tracing', category: 'CC' }], ['GCMinor', 5, null, { type: 'GCMinor', nursery: any }], ['GCMajor', 6, null, { type: 'GCMajor', timings: any }], @@ -667,7 +667,7 @@ describe('Marker schema filtering', function () { const { getMarkerNames } = setup(getProfileForMarkerSchema()); expect( getMarkerNames(selectedThreadSelectors.getTimelineOverviewMarkerIndexes) - ).toEqual(['VisibleInTimelineOverview']); + ).toEqual(['VisibleInTimelineOverview', 'RandomTracingMarker']); }); }); From 53059b000cc38b8842fd63bbe229f0b6322e2cdb Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Mon, 6 Jan 2025 12:32:42 +0100 Subject: [PATCH 8/9] Remove special handling of tracing markers when looking up marker schemas. This adds an upgrader so that old profiles still get their CC markers displayed in the memory track and non-CC tracing markers displayed in the timeline-overview area. Newer profiles don't need this special case because their CC markers have a CC schema which correctly puts them in the memory track, and because they include a 'tracing' schema which puts non-CC tracing markers into the timeline-overview area. --- docs-developer/CHANGELOG-formats.md | 6 ++ src/app-logic/constants.js | 2 +- src/profile-logic/gecko-profile-versioning.js | 12 +-- src/profile-logic/marker-schema.js | 23 +----- .../processed-profile-versioning.js | 81 +++++++++++++++++-- src/test/components/TimelineMarkers.test.js | 4 +- .../symbolicator-cli.test.js.snap | 2 +- .../__snapshots__/profile-view.test.js.snap | 2 +- src/test/store/markers.test.js | 16 +--- .../profile-conversion.test.js.snap | 30 +++---- .../profile-upgrading.test.js.snap | 53 +++++++++++- 11 files changed, 161 insertions(+), 70 deletions(-) diff --git a/docs-developer/CHANGELOG-formats.md b/docs-developer/CHANGELOG-formats.md index 918c5b23b4..7d35fa46c7 100644 --- a/docs-developer/CHANGELOG-formats.md +++ b/docs-developer/CHANGELOG-formats.md @@ -6,6 +6,12 @@ Note that this is not an exhaustive list. Processed profile format upgraders can ## Processed profile format +### Version 52 + +No format changes, but a front-end behavior change: The schema for a marker is now looked up purely based on its `data.type`. In the past there were some special cases when `data` was `null`, or when `data.type` was `tracing` or `Text`. These special cases have been removed. The new behavior is simpler and more predictable, and was probably what you expected anyway. + +This change came with a new version because we needed to upgrade old profiles from Firefox which were relying on the more complex behavior. + ### Version 51 Two new marker schema field format types have been added: `flow-id` and `terminating-flow-id`, with string index values (like `unique-string`). diff --git a/src/app-logic/constants.js b/src/app-logic/constants.js index f80c2fdad8..10b8dc0375 100644 --- a/src/app-logic/constants.js +++ b/src/app-logic/constants.js @@ -14,7 +14,7 @@ export const GECKO_PROFILE_VERSION = 31; // The current version of the "processed" profile format. // Please don't forget to update the processed profile format changelog in // `docs-developer/CHANGELOG-formats.md`. -export const PROCESSED_PROFILE_VERSION = 51; +export const PROCESSED_PROFILE_VERSION = 52; // The following are the margin sizes for the left and right of the timeline. Independent // components need to share these values. diff --git a/src/profile-logic/gecko-profile-versioning.js b/src/profile-logic/gecko-profile-versioning.js index 99a23bd60a..9520905b59 100644 --- a/src/profile-logic/gecko-profile-versioning.js +++ b/src/profile-logic/gecko-profile-versioning.js @@ -1252,27 +1252,27 @@ const _upgraders = { // eventType is in the payload as well. ], }, + + // The following three schemas should have just been a single schema named + // "tracing". They are kept here for historical accuracy. + // There is a processed profile format upgrader (version 52) which adds the + // "tracing" schema for profiles which don't have it. { - // TODO - Note that this marker is a "tracing" marker currently. - // See issue #2749 name: 'Paint', display: ['marker-chart', 'marker-table', 'timeline-overview'], data: [{ key: 'category', label: 'Type', format: 'string' }], }, { - // TODO - Note that this marker is a "tracing" marker currently. - // See issue #2749 name: 'Navigation', display: ['marker-chart', 'marker-table', 'timeline-overview'], data: [{ key: 'category', label: 'Type', format: 'string' }], }, { - // TODO - Note that this marker is a "tracing" marker currently. - // See issue #2749 name: 'Layout', display: ['marker-chart', 'marker-table', 'timeline-overview'], data: [{ key: 'category', label: 'Type', format: 'string' }], }, + { name: 'IPC', tooltipLabel: 'IPC — {marker.data.niceDirection}', diff --git a/src/profile-logic/marker-schema.js b/src/profile-logic/marker-schema.js index 505fb69bd8..6c68449153 100644 --- a/src/profile-logic/marker-schema.js +++ b/src/profile-logic/marker-schema.js @@ -82,34 +82,17 @@ export const markerSchemaFrontEndOnly: MarkerSchema[] = [ }, ]; -/** - * For the most part, schema is matched up by the Payload's "type" field, - * but for practical purposes, there are a few other options, see the - * implementation of this function for details. - */ export function getMarkerSchemaName( markerSchemaByName: MarkerSchemaByName, markerName: string, markerData: MarkerPayload | null ): string | null { - if (!markerData) { - // No payload - no schema. + if (!markerData || !markerData.type) { + // No schema. return null; } - const { type } = markerData; - if (type === 'tracing' && markerData.category) { - // TODO - Tracing markers have a duplicate "category" field. - // See issue #2749 - - // Does a marker schema for the "category" exist? - return markerSchemaByName[markerData.category] === undefined - ? // If not, default back to tracing - 'tracing' - : // If so, use the category as the schema name. - markerData.category; - } - return type; + return markerData.type; } /** diff --git a/src/profile-logic/processed-profile-versioning.js b/src/profile-logic/processed-profile-versioning.js index 6ec10c2cd1..c50b1067ca 100644 --- a/src/profile-logic/processed-profile-versioning.js +++ b/src/profile-logic/processed-profile-versioning.js @@ -1640,27 +1640,26 @@ const _upgraders = { // eventType is in the payload as well. ], }, + + // The following three schemas should have just been a single schema named + // "tracing". They are kept here for historical accuracy. + // The upgrader for version 52 adds the missing "tracing" schema. { - // TODO - Note that this marker is a "tracing" marker currently. - // See issue #2749 name: 'Paint', display: ['marker-chart', 'marker-table', 'timeline-overview'], data: [{ key: 'category', label: 'Type', format: 'string' }], }, { - // TODO - Note that this marker is a "tracing" marker currently. - // See issue #2749 name: 'Navigation', display: ['marker-chart', 'marker-table', 'timeline-overview'], data: [{ key: 'category', label: 'Type', format: 'string' }], }, { - // TODO - Note that this marker is a "tracing" marker currently. - // See issue #2749 name: 'Layout', display: ['marker-chart', 'marker-table', 'timeline-overview'], data: [{ key: 'category', label: 'Type', format: 'string' }], }, + { name: 'IPC', tooltipLabel: 'IPC — {marker.data.niceDirection}', @@ -2278,6 +2277,76 @@ const _upgraders = { // marker data with the new field types data, and no modification is needed in the // frontend to display older formats. }, + [52]: (profile) => { + // This version simplifies how markers are mapped to their schema. + // The schema is now purely determined by data.type. The marker's name is ignored. + // If a marker has a null data, then it has no schema. + // + // In earlier versions, there were special cases for mapping markers with type + // "tracing" and "Text", and for markers with a null data property. These + // special cases have been removed. + // + // The upgrader for version 52 makes it so that existing profiles appear the + // same with the simplified logic. Specifically: + // - Some old profiles have markers with data.type === 'tracing' but no schema + // with the name 'tracing'. To ensure that the tracing markers from these + // profile still show up in the 'timeline-overview' area, this upgrader adds + // a schema to such profiles. + // - Some old profiles have CC markers which only showed up in the memory track + // because of special treatment of 'tracing' markers - the markers would have + // data.type === 'tracing' and data.category === 'CC', and there would be a + // 'CC' schema with 'timeline-memory'. This upgrader moves these tracing CC + // markers to a new 'tracingCCFrom52Upgrader' schema. + // + // Profiles from modern versions of Firefox already include a 'tracing' schema. + // And they don't use tracing markers for CC markers. + + const schemaNames = new Set(profile.meta.markerSchema.map((s) => s.name)); + const kTracingCCSchemaName = 'tracingCCFrom52Upgrader'; + const shouldMigrateTracingCCMarkers = schemaNames.has('CC'); + let hasTracingMarkers = false; + let hasMigratedTracingCCMarkers = false; + for (const thread of profile.threads) { + const { markers } = thread; + for (let i = 0; i < markers.length; i++) { + const data = markers.data[i]; + if (data && data.type === 'tracing' && data.category) { + hasTracingMarkers = true; + if (shouldMigrateTracingCCMarkers && data.category === 'CC') { + data.type = kTracingCCSchemaName; + hasMigratedTracingCCMarkers = true; + if (data.interval) { + // Also delete the interval property. This is present on old + // profiles where marker phase information was represented in the + // payload, i.e. you'd have interval: "start" / "end" on the + // data object. + // Our kTracingCCSchemaName schema does not list the interval field, + // so we shouldn't have this field on the payload either. + delete data.interval; + } + } + } + } + } + if (hasTracingMarkers && !schemaNames.has('tracing')) { + // Make sure that tracing markers still show up in the timeline-overview area. + profile.meta.markerSchema.push({ + name: 'tracing', + display: ['marker-chart', 'marker-table', 'timeline-overview'], + data: [{ key: 'category', label: 'Type', format: 'string' }], + }); + } + + if (hasMigratedTracingCCMarkers) { + // Add the kTracingCCSchemaName schema for migrated tracing CC markers, to + // make sure that these markers still show up in the timeline-memory area. + profile.meta.markerSchema.push({ + name: kTracingCCSchemaName, + display: ['marker-chart', 'marker-table', 'timeline-memory'], + data: [{ key: 'category', label: 'Type', format: 'string' }], + }); + } + }, // If you add a new upgrader here, please document the change in // `docs-developer/CHANGELOG-formats.md`. }; diff --git a/src/test/components/TimelineMarkers.test.js b/src/test/components/TimelineMarkers.test.js index 000780baff..12cd5aa21b 100644 --- a/src/test/components/TimelineMarkers.test.js +++ b/src/test/components/TimelineMarkers.test.js @@ -186,7 +186,7 @@ describe('TimelineMarkers', function () { ['Navigation', 2, 6, { type: 'tracing', category: 'Navigation' }], ['Layout', 6, 8, { type: 'tracing', category: 'Layout' }], // These 2 will be ignored. - ['CC', 0, 5, { type: 'tracing', category: 'CC' }], + ['CC', 0, 5], ['GCMajor', 5, 10], ]); @@ -206,7 +206,7 @@ describe('TimelineMarkers', function () { [ // The first one will be ignored. ['DOMEvent', 0, 10, { type: 'tracing', category: 'JS' }], - ['CC', 0, 5, { type: 'tracing', category: 'CC' }], + ['CC', 0, 5], ['GCMajor', 5, 10], ] ); diff --git a/src/test/integration/symbolicator-cli/__snapshots__/symbolicator-cli.test.js.snap b/src/test/integration/symbolicator-cli/__snapshots__/symbolicator-cli.test.js.snap index de1ddacb60..9f03640ed2 100644 --- a/src/test/integration/symbolicator-cli/__snapshots__/symbolicator-cli.test.js.snap +++ b/src/test/integration/symbolicator-cli/__snapshots__/symbolicator-cli.test.js.snap @@ -88,7 +88,7 @@ Object { "markerSchema": Array [], "oscpu": "macOS 14.6.1", "pausedRanges": Array [], - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "a.out", "sampleUnits": Object { diff --git a/src/test/store/__snapshots__/profile-view.test.js.snap b/src/test/store/__snapshots__/profile-view.test.js.snap index 9af0dc9e79..0ce6dd13ba 100644 --- a/src/test/store/__snapshots__/profile-view.test.js.snap +++ b/src/test/store/__snapshots__/profile-view.test.js.snap @@ -434,7 +434,7 @@ Object { "oscpu": "", "physicalCPUs": 0, "platform": "", - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "Firefox", "sourceURL": "", diff --git a/src/test/store/markers.test.js b/src/test/store/markers.test.js index 03de917204..b14784ee37 100644 --- a/src/test/store/markers.test.js +++ b/src/test/store/markers.test.js @@ -2,10 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ // @flow -import { - selectedThreadSelectors, - getMarkerSchemaByName, -} from 'firefox-profiler/selectors'; +import { selectedThreadSelectors } from 'firefox-profiler/selectors'; import { changeTimelineTrackOrganization } from 'firefox-profiler/actions/receive-profile'; import { unserializeProfileOfArbitraryFormat } from 'firefox-profiler/profile-logic/process-profile'; import { ensureExists } from 'firefox-profiler/utils/flow'; @@ -230,7 +227,7 @@ describe('memory markers', function () { ['DOMEvent', 0, null, { type: 'tracing', category: 'JS' }], ['Navigation', 1, null, { type: 'tracing', category: 'Navigation' }], ['Paint', 2, null, { type: 'tracing', category: 'Paint' }], - ['IdleForgetSkippable', 3, 4, { type: 'tracing', category: 'CC' }], + ['IdleForgetSkippable', 3, 4, { type: 'CC' }], ['GCMinor', 5, null, { type: 'GCMinor', nursery: any }], ['GCMajor', 6, null, { type: 'GCMajor', timings: any }], ['GCSlice', 7, null, { type: 'GCSlice', timings: any }], @@ -433,15 +430,6 @@ describe('Marker schema filtering', function () { function setup(profile) { const { getState } = storeWithProfile(profile); const getMarker = selectedThreadSelectors.getMarkerGetter(getState()); - const markerSchemaByName = getMarkerSchemaByName(getState()); - - if (markerSchemaByName.RandomTracingMarker) { - throw new Error( - 'This test assumes that the RandomTracingMarker marker has no schema. If this ' + - 'schema were added somewhere else, then rename RandomTracingMarker to ' + - 'something else. ' - ); - } function getMarkerNames(selector): string[] { return selector(getState()) diff --git a/src/test/unit/__snapshots__/profile-conversion.test.js.snap b/src/test/unit/__snapshots__/profile-conversion.test.js.snap index 59355ebd9d..6d0f6a837f 100644 --- a/src/test/unit/__snapshots__/profile-conversion.test.js.snap +++ b/src/test/unit/__snapshots__/profile-conversion.test.js.snap @@ -469,7 +469,7 @@ Object { "oscpu": undefined, "physicalCPUs": undefined, "platform": undefined, - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "ART Trace (Android)", "sampleUnits": undefined, @@ -100220,7 +100220,7 @@ Object { "oscpu": undefined, "physicalCPUs": undefined, "platform": undefined, - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "ART Trace (Android)", "sampleUnits": undefined, @@ -382162,7 +382162,7 @@ Object { "oscpu": "", "physicalCPUs": 0, "platform": "", - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "Chrome Trace", "profilingEndTime": 119159778.026, @@ -430868,7 +430868,7 @@ Object { "oscpu": "", "physicalCPUs": 0, "platform": "", - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "Chrome Trace", "profilingEndTime": 119159778.026, @@ -479574,7 +479574,7 @@ Object { "oscpu": "", "physicalCPUs": 0, "platform": "", - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "Chrome Trace", "profilingEndTime": 66155012.423, @@ -482616,7 +482616,7 @@ Object { "oscpu": "", "physicalCPUs": 0, "platform": "", - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "Chrome Trace", "sourceURL": "", @@ -485734,7 +485734,7 @@ Object { "oscpu": "", "physicalCPUs": 0, "platform": "", - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "Chrome Trace", "profilingEndTime": 355035987.653, @@ -489802,7 +489802,7 @@ Object { "oscpu": "", "physicalCPUs": 0, "platform": "", - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "Chrome Trace", "profilingEndTime": 66155012.423, @@ -491002,7 +491002,7 @@ Object { "oscpu": undefined, "physicalCPUs": undefined, "platform": undefined, - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "Firefox", "sampleUnits": undefined, @@ -498527,7 +498527,7 @@ Object { "oscpu": undefined, "physicalCPUs": undefined, "platform": undefined, - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "Firefox", "sampleUnits": undefined, @@ -517940,7 +517940,7 @@ Object { "oscpu": undefined, "physicalCPUs": undefined, "platform": undefined, - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "Firefox", "sampleUnits": undefined, @@ -525458,7 +525458,7 @@ Object { "oscpu": undefined, "physicalCPUs": undefined, "platform": undefined, - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "Firefox", "sampleUnits": undefined, @@ -549052,7 +549052,7 @@ Object { "keepProfileThreadOrder": true, "markerSchema": Array [], "platform": "Android", - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "com.example.sampleapplication", "sourceCodeIsNotOnSearchfox": true, @@ -640488,7 +640488,7 @@ Object { "keepProfileThreadOrder": true, "markerSchema": Array [], "platform": "Android", - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "com.example.sampleapplication", "sourceCodeIsNotOnSearchfox": true, @@ -740767,7 +740767,7 @@ Object { "oscpu": "", "physicalCPUs": 0, "platform": "", - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "target/debug/examples/work_log (dhat)", "sourceURL": "", diff --git a/src/test/unit/__snapshots__/profile-upgrading.test.js.snap b/src/test/unit/__snapshots__/profile-upgrading.test.js.snap index f45bdfb44e..badcd1800b 100644 --- a/src/test/unit/__snapshots__/profile-upgrading.test.js.snap +++ b/src/test/unit/__snapshots__/profile-upgrading.test.js.snap @@ -40,7 +40,7 @@ Object { "oscpu": undefined, "physicalCPUs": undefined, "platform": undefined, - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "Firefox", "sampleUnits": undefined, @@ -10967,11 +10967,26 @@ Object { ], "name": "Network", }, + Object { + "data": Array [ + Object { + "format": "string", + "key": "category", + "label": "Type", + }, + ], + "display": Array [ + "marker-chart", + "marker-table", + "timeline-overview", + ], + "name": "tracing", + }, ], "misc": "rv:48.0", "oscpu": "Intel Mac OS X 10.11", "platform": "Macintosh", - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "Firefox", "stackwalk": 1, @@ -12621,11 +12636,26 @@ Object { ], "name": "Network", }, + Object { + "data": Array [ + Object { + "format": "string", + "key": "category", + "label": "Type", + }, + ], + "display": Array [ + "marker-chart", + "marker-table", + "timeline-overview", + ], + "name": "tracing", + }, ], "misc": "rv:48.0", "oscpu": "Intel Mac OS X 10.11", "platform": "Macintosh", - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "Firefox", "stackwalk": 1, @@ -14418,11 +14448,26 @@ Object { ], "name": "Network", }, + Object { + "data": Array [ + Object { + "format": "string", + "key": "category", + "label": "Type", + }, + ], + "display": Array [ + "marker-chart", + "marker-table", + "timeline-overview", + ], + "name": "tracing", + }, ], "misc": "rv:48.0", "oscpu": "Intel Mac OS X 10.11", "platform": "Macintosh", - "preprocessedProfileVersion": 51, + "preprocessedProfileVersion": 52, "processType": 0, "product": "Firefox", "stackwalk": 1, From 82272682ff1a30aa678a657cfc7360fc88368807 Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Fri, 27 Dec 2024 11:31:16 +0100 Subject: [PATCH 9/9] Remove getMarkerSchemaName and the now-unused marker name argument from getSchemaFromMarker. --- src/actions/receive-profile.js | 13 ++----------- src/components/tooltip/Marker.js | 6 +----- src/profile-logic/marker-data.js | 25 +++++------------------- src/profile-logic/marker-schema.js | 30 ++++------------------------- src/profile-logic/sanitize.js | 3 --- src/profile-logic/tracks.js | 11 ++--------- src/selectors/per-thread/markers.js | 11 ++--------- 7 files changed, 16 insertions(+), 83 deletions(-) diff --git a/src/actions/receive-profile.js b/src/actions/receive-profile.js index af83bc8053..84ee8b7521 100644 --- a/src/actions/receive-profile.js +++ b/src/actions/receive-profile.js @@ -38,7 +38,6 @@ import { getRelevantPagesForActiveTab, getSymbolServerUrl, getActiveTabID, - getMarkerSchemaByName, } from 'firefox-profiler/selectors'; import { getSelectedTab, @@ -321,11 +320,7 @@ export function finalizeFullProfileView( tabFilter, tabToThreadIndexesMap ); - const localTracksByPid = computeLocalTracksByPid( - profile, - globalTracks, - getMarkerSchemaByName(getState()) - ); + const localTracksByPid = computeLocalTracksByPid(profile, globalTracks); const legacyThreadOrder = getLegacyThreadOrder(getState()); const globalTrackOrder = initializeGlobalTrackOrder( @@ -1835,11 +1830,7 @@ export function changeTabFilter(tabID: TabID | null): ThunkAction { tabID, tabToThreadIndexesMap ); - const localTracksByPid = computeLocalTracksByPid( - profile, - globalTracks, - getMarkerSchemaByName(getState()) - ); + const localTracksByPid = computeLocalTracksByPid(profile, globalTracks); const legacyThreadOrder = getLegacyThreadOrder(getState()); const globalTrackOrder = initializeGlobalTrackOrder( diff --git a/src/components/tooltip/Marker.js b/src/components/tooltip/Marker.js index 916f224037..df3797ae67 100644 --- a/src/components/tooltip/Marker.js +++ b/src/components/tooltip/Marker.js @@ -239,11 +239,7 @@ class MarkerTooltipContents extends React.PureComponent { if (data) { // Add the details for the markers based on their Marker schema. - const schema = getSchemaFromMarker( - markerSchemaByName, - marker.name, - marker.data - ); + const schema = getSchemaFromMarker(markerSchemaByName, marker.data); if (schema) { for (const schemaData of schema.data) { // Check for a schema that is looking up and formatting a value from diff --git a/src/profile-logic/marker-data.js b/src/profile-logic/marker-data.js index d4c978ba34..98f8278181 100644 --- a/src/profile-logic/marker-data.js +++ b/src/profile-logic/marker-data.js @@ -14,7 +14,6 @@ import { INTERVAL_END, } from 'firefox-profiler/app-logic/constants'; import { - getMarkerSchemaName, getSchemaFromMarker, markerPayloadMatchesSearch, } from './marker-schema'; @@ -226,11 +225,7 @@ function positiveFilterMarker( } // Now check the schema for the marker payload for searchable - const markerSchema = getSchemaFromMarker( - markerSchemaByName, - marker.name, - marker.data - ); + const markerSchema = getSchemaFromMarker(markerSchemaByName, marker.data); if ( markerSchema && markerPayloadMatchesSearch(markerSchema, marker, stringTable, test) @@ -292,11 +287,7 @@ function negativeFilterMarker( } // Now check the schema for the marker payload for searchable - const markerSchema = getSchemaFromMarker( - markerSchemaByName, - marker.name, - marker.data - ); + const markerSchema = getSchemaFromMarker(markerSchemaByName, marker.data); if ( markerSchema && @@ -1287,14 +1278,12 @@ export function getAllowMarkersWithNoSchema( const { data } = marker; if (!data) { - // Keep the marker if there is payload. + // Keep the marker if there is no payload. return true; } if (!markerSchemaByName[data.type]) { - // Keep the marker if there is no schema. Note that this function doesn't use - // the getMarkerSchemaName function, as that function attempts to find a - // marker schema name in a very permissive manner. In the marker chart + // Keep the marker if there is no schema. In the marker chart // and marker table, most likely we want to show everything. return true; } @@ -1528,11 +1517,7 @@ export function filterMarkerByDisplayLocation( return additionalResult; } - const schemaName = getMarkerSchemaName( - markerSchemaByName, - marker.name, - marker.data - ); + const schemaName = marker.data ? (marker.data.type ?? null) : null; return schemaName !== null && markerTypes.has(schemaName); }); } diff --git a/src/profile-logic/marker-schema.js b/src/profile-logic/marker-schema.js index 6c68449153..18b17c2dec 100644 --- a/src/profile-logic/marker-schema.js +++ b/src/profile-logic/marker-schema.js @@ -82,34 +82,16 @@ export const markerSchemaFrontEndOnly: MarkerSchema[] = [ }, ]; -export function getMarkerSchemaName( - markerSchemaByName: MarkerSchemaByName, - markerName: string, - markerData: MarkerPayload | null -): string | null { - if (!markerData || !markerData.type) { - // No schema. - return null; - } - - return markerData.type; -} - /** * This function takes the intended marker schema for a marker field, and applies * the appropriate formatting function. */ export function getSchemaFromMarker( markerSchemaByName: MarkerSchemaByName, - markerName: string, markerData: MarkerPayload | null ): MarkerSchema | null { - const schemaName = getMarkerSchemaName( - markerSchemaByName, - markerName, - markerData - ); - return schemaName !== null ? (markerSchemaByName[schemaName] ?? null) : null; + const schemaName = markerData ? markerData.type : null; + return schemaName ? (markerSchemaByName[schemaName] ?? null) : null; } /** @@ -363,12 +345,8 @@ export function getLabelGetter( // No label exists, it will have to be generated for the first time. if (label === undefined) { const marker = getMarker(markerIndex); - const schemaName = getMarkerSchemaName( - markerSchemaByName, - marker.name, - marker.data - ); - const applyLabel = schemaName !== null ? labelFns.get(schemaName) : null; + const schemaName = marker.data ? marker.data.type : null; + const applyLabel = schemaName ? labelFns.get(schemaName) : null; label = applyLabel ? // A label function is available, apply it. diff --git a/src/profile-logic/sanitize.js b/src/profile-logic/sanitize.js index 2ffc6d35fb..2b364959d8 100644 --- a/src/profile-logic/sanitize.js +++ b/src/profile-logic/sanitize.js @@ -299,11 +299,8 @@ function sanitizeThreadPII( if (currentMarker && PIIToBeRemoved.shouldRemoveUrls) { // Use the schema to find some properties that need to be sanitized. - const markerNameIndex = markerTable.name[i]; - const markerName = thread.stringTable.getString(markerNameIndex); const markerSchema = getSchemaFromMarker( markerSchemaByName, - markerName, currentMarker ); if (markerSchema) { diff --git a/src/profile-logic/tracks.js b/src/profile-logic/tracks.js index 3f35669d9c..9047854d0d 100644 --- a/src/profile-logic/tracks.js +++ b/src/profile-logic/tracks.js @@ -15,7 +15,6 @@ import type { Counter, Tid, TrackReference, - MarkerSchemaByName, TabID, } from 'firefox-profiler/types'; @@ -23,7 +22,6 @@ import { defaultThreadOrder, getFriendlyThreadName } from './profile-data'; import { intersectSets, subtractSets } from '../utils/set'; import { splitSearchString, stringsToRegExp } from '../utils/string'; import { ensureExists, assertExhaustiveCheck } from '../utils/flow'; -import { getMarkerSchemaName } from './marker-schema'; export type TracksWithOrder = {| +globalTracks: GlobalTrack[], @@ -258,8 +256,7 @@ export function initializeLocalTrackOrderByPid( */ export function computeLocalTracksByPid( profile: Profile, - availableGlobalTracks: GlobalTrack[], - markerSchemaByName: MarkerSchemaByName + availableGlobalTracks: GlobalTrack[] ): Map { const localTracksByPid = new Map(); @@ -323,11 +320,7 @@ export function computeLocalTracksByPid( for (let i = 0; i < markers.length; ++i) { const markerNameIndex = markers.name[i]; const markerData = markers.data[i]; - const markerSchemaName = getMarkerSchemaName( - markerSchemaByName, - thread.stringTable.getString(markerNameIndex), - markerData - ); + const markerSchemaName = markerData ? markerData.type : null; if (markerData && markerSchemaName) { const mapEntry = markerTracksBySchemaName.get(markerSchemaName); if (mapEntry && mapEntry.keys.every((k) => k in markerData)) { diff --git a/src/selectors/per-thread/markers.js b/src/selectors/per-thread/markers.js index 393ee3f168..d64ab7e706 100644 --- a/src/selectors/per-thread/markers.js +++ b/src/selectors/per-thread/markers.js @@ -11,10 +11,7 @@ import * as MarkerData from '../../profile-logic/marker-data'; import * as MarkerTimingLogic from '../../profile-logic/marker-timing'; import * as ProfileSelectors from '../profile'; import { getRightClickedMarkerInfo } from '../right-clicked-marker'; -import { - getLabelGetter, - getMarkerSchemaName, -} from '../../profile-logic/marker-schema'; +import { getLabelGetter } from '../../profile-logic/marker-schema'; import { getInclusiveSampleIndexRangeForSelection } from '../../profile-logic/profile-data'; import type { BasicThreadSelectorsPerThread } from './thread'; @@ -682,11 +679,7 @@ export function getMarkerSelectorsPerThread( if ( data && marker.name === name && - getMarkerSchemaName( - ProfileSelectors.getMarkerSchemaByName, - marker.name, - data - ) === schemaName && + data.type === schemaName && keys.every((key) => key in data) ) { markerIndexes.push(index);