diff --git a/docs-developer/CHANGELOG-formats.md b/docs-developer/CHANGELOG-formats.md index 6d1ed2b5c8..01de815fb3 100644 --- a/docs-developer/CHANGELOG-formats.md +++ b/docs-developer/CHANGELOG-formats.md @@ -6,6 +6,14 @@ Note that this is not an exhaustive list. Processed profile format upgraders can ## Processed profile format +### Version 70 + +The `CompositorScreenshot` marker payload's `windowWidth` and `windowHeight` fields were replaced with a single `windowSize` field of the form `{ width, height }`. + +These markers are now stored as pairs of start and end markers instead of instant markers. The window ID is part of the marker name (`CompositorScreenshot `), so starts and ends are matched by name like any other marker pair. The `CompositorScreenshotWindowDestroyed` marker is gone: it is now the end marker of that window's last screenshot. The last screenshot of a window that is never destroyed has no end marker, and is extended to the end of the thread. + +Two marker schema field formats were added to describe these markers: `screenshot-size`, whose value is a `{ width, height }` object, and `screenshot-data-url`, an object format `{ type: "screenshot-data-url", sizeFieldForAspectRatio }` whose value is a string table index holding an image data URL. Profiles containing `CompositorScreenshot` markers don't necessarily carry a schema for them, so the front end supplies one. + ### Version 69 A new marker schema display location, `timeline-network`, was added. A marker schema can list `timeline-network` in its `display` array to have markers of that type surfaced in the Network track. diff --git a/src/app-logic/constants.ts b/src/app-logic/constants.ts index 1f6b47f2d5..f8a4d98330 100644 --- a/src/app-logic/constants.ts +++ b/src/app-logic/constants.ts @@ -12,7 +12,7 @@ export const GECKO_PROFILE_VERSION = 36; // 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 = 69; +export const PROCESSED_PROFILE_VERSION = 70; // 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/components/timeline/TrackScreenshots.tsx b/src/components/timeline/TrackScreenshots.tsx index 4ec906de4e..282a55015d 100644 --- a/src/components/timeline/TrackScreenshots.tsx +++ b/src/components/timeline/TrackScreenshots.tsx @@ -244,19 +244,17 @@ class HoverPreview extends PureComponent { return null; } - if (payload.url === undefined) { + const { url, windowSize } = payload; + if (url === undefined || windowSize === undefined) { return null; } - const { url } = payload; - const maximumHoverSize = isMakingPreviewSelection ? MAXIMUM_HOVER_SIZE_WHEN_SELECTING_RANGE : MAXIMUM_HOVER_SIZE; - // Type guard: payload.url !== undefined means it has windowWidth and windowHeight const { width: hoverWidth, height: hoverHeight } = computeScreenshotSize( - payload as { windowWidth: number; windowHeight: number }, + windowSize, maximumHoverSize ); @@ -354,18 +352,12 @@ class ScreenshotStrip extends PureComponent { // Coerce the payload into a screenshot one. const payload: ScreenshotPayload = screenshots[screenshotIndex] .data as any; - if (payload.url === undefined) { + const { url: urlStringIndex, windowSize } = payload; + if (urlStringIndex === undefined || windowSize === undefined) { continue; } - const { - url: urlStringIndex, - windowWidth, - windowHeight, - } = payload as ScreenshotPayload & { - windowWidth: number; - windowHeight: number; - }; - const scaledImageWidth = (trackHeight * windowWidth) / windowHeight; + const scaledImageWidth = + (trackHeight * windowSize.width) / windowSize.height; images.push(
{ {/* The following image is centered and cropped by the outer container. */} { value, thread.stringTable, threadIdToNameMap, - processIdToNameMap + processIdToNameMap, + data )} ); @@ -352,67 +354,6 @@ class MarkerTooltipContents extends React.PureComponent { ); break; } - case 'CompositorScreenshot': { - if ( - data.url !== undefined && - 'windowWidth' in data && - 'windowHeight' in data - ) { - const { width, height } = computeScreenshotSize( - data, - MAXIMUM_IMAGE_SIZE - ); - details.push( - - - , - - <> - {data.windowWidth}px × {data.windowHeight}px - - , - - This marker spans the time between each composite of a window - and shows the window contents during that time. - , - - {data.windowID} - - ); - } else if (marker.name === 'CompositorScreenshotWindowDestroyed') { - details.push( - - This marker shows the moment a window has been destroyed. - , - - {data.windowID} - - ); - } - break; - } default: // Do nothing } @@ -587,7 +528,7 @@ const URL_REGEXP = /^(https?:\/\/)\S+$/; /** * This function may return structured markup for some types suchs as table, - * list, or urls. For other types this falls back to formatFromMarkerSchema + * list, urls, or images. For other types this falls back to formatFromMarkerSchema * above. */ export function renderMarkerFieldValue( @@ -596,7 +537,9 @@ export function renderMarkerFieldValue( value: any, stringTable: StringTable, threadIdToNameMap?: Map, - processIdToNameMap?: Map + processIdToNameMap?: Map, + // The payload the value comes from, for formats that refer to a sibling field. + payload?: MarkerPayload | null ): React.ReactElement | string { if (value === undefined || value === null) { console.warn(`Formatting ${value} for ${JSON.stringify(markerType)}`); @@ -653,7 +596,8 @@ export function renderMarkerFieldValue( cell, stringTable, threadIdToNameMap, - processIdToNameMap + processIdToNameMap, + payload )} ); @@ -665,6 +609,23 @@ export function renderMarkerFieldValue( ); } + case 'screenshot-data-url': { + const size = (payload as any)?.[format.sizeFieldForAspectRatio]; + return ( + + ); + } default: throw new Error( `Unknown format type ${JSON.stringify(format as never)}` diff --git a/src/profile-logic/import/chrome.ts b/src/profile-logic/import/chrome.ts index 0a448666e0..2c1d5ee2f4 100644 --- a/src/profile-logic/import/chrome.ts +++ b/src/profile-logic/import/chrome.ts @@ -6,7 +6,10 @@ import type { Profile, RawThread, IndexIntoStackTable, + MarkerPhase, + Milliseconds, MixedObject, + ScreenshotPayload, } from 'firefox-profiler/types'; import { @@ -29,6 +32,7 @@ import { } from 'firefox-profiler/app-logic/constants'; import { getTimeRangeForThread } from '../profile-data'; +import { getScreenshotMarkerName } from '../marker-data'; import { GlobalDataCollector } from '../global-data-collector'; // Chrome Tracing Event Spec: @@ -878,6 +882,29 @@ async function extractScreenshots( ); } + // Chrome's Screenshot events don't say which window they belong to, so they + // all share one made-up window ID. + const windowID = 'id'; + const nameIndex = stringTable.indexForString( + getScreenshotMarkerName(windowID) + ); + + function pushScreenshotMarker( + data: ScreenshotPayload, + startTime: Milliseconds | null, + endTime: Milliseconds | null, + phase: MarkerPhase + ) { + markers.data.push(data); + markers.name.push(nameIndex); + markers.startTime.push(startTime); + markers.endTime.push(endTime); + markers.phase.push(phase); + markers.category.push(graphicsIndex); + markers.length++; + } + + let hasOpenScreenshot = false; for (const screenshot of screenshots) { const urlString = 'data:image/jpg;base64,' + screenshot.args.snapshot; const size = await getImageSize(urlString); @@ -885,19 +912,30 @@ async function extractScreenshots( // The image could not be processed, do not add it. continue; } - markers.data.push({ - type: 'CompositorScreenshot', - url: stringTable.indexForString(urlString), - windowID: 'id', - windowWidth: size.width, - windowHeight: size.height, - }); - markers.name.push(stringTable.indexForString('CompositorScreenshot')); - markers.startTime.push(screenshot.ts / 1000); - markers.endTime.push(null); - markers.phase.push(INSTANT); - markers.category.push(graphicsIndex); - markers.length++; + const startTime = screenshot.ts / 1000; + + // Each screenshot is valid until the next one. + if (hasOpenScreenshot) { + pushScreenshotMarker( + { type: 'CompositorScreenshot', windowID }, + null, + startTime, + INTERVAL_END + ); + } + + pushScreenshotMarker( + { + type: 'CompositorScreenshot', + url: stringTable.indexForString(urlString), + windowID, + windowSize: { width: size.width, height: size.height }, + }, + startTime, + null, + INTERVAL_START + ); + hasOpenScreenshot = true; } } diff --git a/src/profile-logic/marker-data.ts b/src/profile-logic/marker-data.ts index 5c196abd14..bdc882cdeb 100644 --- a/src/profile-logic/marker-data.ts +++ b/src/profile-logic/marker-data.ts @@ -559,7 +559,6 @@ export function correlateIPCMarkers( * the thread range. For the reverse situation, it's set to the start. * * There is also some special handling of different markers. - * - CompositorScreenshot - They are turned from Instant markers to Interval markers * - IPC - They are matched up. * - Network - They have different network phases. * @@ -614,13 +613,9 @@ export function deriveMarkersFromRawMarkerTable( return { ...startData, ...endData, - }; + } as MarkerPayload; } - // We don't add a screenshot marker as we find it, because to know its - // duration we need to wait until the next one or the end of the profile. So - // we keep it here. - const previousScreenshotMarkers: Map = new Map(); for ( let rawMarkerIndex = 0; rawMarkerIndex < rawMarkers.length; @@ -728,51 +723,6 @@ export function deriveMarkersFromRawMarkerTable( continue; } - case 'CompositorScreenshot': { - // Screenshot markers are already ordered. In the raw marker table, - // they're Instant markers, but since they're valid until the following - // raw marker of the same type and the same window, we convert them to - // Interval markers with a a start and end time. - - const { windowID } = data; - const previousScreenshotMarker = - previousScreenshotMarkers.get(windowID); - if (previousScreenshotMarker !== undefined) { - previousScreenshotMarkers.delete(windowID); - const previousStartTime = ensureExists( - rawMarkers.startTime[previousScreenshotMarker], - 'Expected to find a start time for a screenshot marker.' - ); - const thisStartTime = ensureExists( - maybeStartTime, - 'The CompositorScreenshot is assumed to have a start time.' - ); - const data = rawMarkers.data[previousScreenshotMarker]; - const markerThreadId = rawMarkers.threadId - ? rawMarkers.threadId[previousScreenshotMarker] - : null; - addMarker([previousScreenshotMarker], { - start: previousStartTime, - end: thisStartTime, - name: 'CompositorScreenshot', - category, - threadId: markerThreadId, - data, - }); - } - if (stringArray[name] === 'CompositorScreenshotWindowDestroyed') { - // This marker is added when a window is destroyed. In this case we - // don't want to store it as the start of the next compositor - // marker. But we do want to keep it, so we break out of the - // switch/case so that the standard processing happens. - break; - } else { - previousScreenshotMarkers.set(windowID, rawMarkerIndex); - } - - continue; - } - case 'IPC': { const sharedData = ipcCorrelations.get( // Older profiles don't have a tid, but they also don't have the IPC markers. @@ -990,24 +940,6 @@ export function deriveMarkersFromRawMarkerTable( }); } - // And we also need to add the "last screenshot markers". - for (const previousScreenshotMarker of previousScreenshotMarkers.values()) { - const start = ensureExists( - rawMarkers.startTime[previousScreenshotMarker], - 'Expected to find a CompositorScreenshot marker with a start time.' - ); - addMarker([previousScreenshotMarker], { - start, - end: Math.max(endOfThread, start), - name: 'CompositorScreenshot', - category: rawMarkers.category[previousScreenshotMarker], - threadId: rawMarkers.threadId - ? rawMarkers.threadId[previousScreenshotMarker] - : null, - data: rawMarkers.data[previousScreenshotMarker], - }); - } - return { markers, markerIndexToRawMarkerIndexes }; } @@ -1434,6 +1366,10 @@ export function getColorClassNameForMimeType( } } +export function getScreenshotMarkerName(windowID: number | string): string { + return `CompositorScreenshot ${windowID}`; +} + export function groupScreenshotsById( getMarker: (markerIndex: MarkerIndex) => Marker, markerIndexes: MarkerIndex[] @@ -1443,10 +1379,11 @@ export function groupScreenshotsById( const marker = getMarker(markerIndex); const { data } = marker; if (data && data.type === 'CompositorScreenshot') { - let markers = idToScreenshotMarkers.get(data.windowID); + const windowID = String(data.windowID); + let markers = idToScreenshotMarkers.get(windowID); if (markers === undefined) { markers = []; - idToScreenshotMarkers.set(data.windowID, markers); + idToScreenshotMarkers.set(windowID, markers); } markers.push(marker); @@ -1639,10 +1576,10 @@ export function filterMarkerByDisplayLocation( * Compute the Screenshot image's thumbnail size. */ export function computeScreenshotSize( - payload: { windowWidth: number; windowHeight: number }, + windowSize: { width: number; height: number }, maximumSize: number ): { readonly width: number; readonly height: number } { - const { windowWidth, windowHeight } = payload; + const { width: windowWidth, height: windowHeight } = windowSize; // Coefficient should be according to bigger side. const coefficient = diff --git a/src/profile-logic/marker-schema.ts b/src/profile-logic/marker-schema.ts index bb56860d7d..8c726f1d6a 100644 --- a/src/profile-logic/marker-schema.ts +++ b/src/profile-logic/marker-schema.ts @@ -85,6 +85,28 @@ export const markerSchemaFrontEndOnly: MarkerSchema[] = [ }, ], }, + { + // A profile can contain these markers without carrying a schema for them, + // so the front end defines one. + name: 'CompositorScreenshot', + display: ['marker-chart', 'marker-table'], + fields: [ + { + key: 'url', + label: 'Image', + format: { + type: 'screenshot-data-url', + sizeFieldForAspectRatio: 'windowSize', + }, + }, + { key: 'windowSize', label: 'Window Size', format: 'screenshot-size' }, + { key: 'windowID', label: 'Window ID', format: 'string' }, + ], + description: oneLine` + This marker spans the time between each composite of a window and shows + the window contents during that time. + `, + }, ]; /** @@ -554,9 +576,12 @@ export function formatFromMarkerSchema( rows.push(...cellRows); return rows.map((row) => `(${row.join(', ')})`).join(','); } + case 'screenshot-data-url': + // Don't expand a base64-encoded image into text output. + return '(screenshot)'; default: throw new Error( - `Unknown format type ${JSON.stringify(format.type as never)}` + `Unknown format type ${JSON.stringify(format as never)}` ); } } @@ -609,6 +634,8 @@ export function formatFromMarkerSchema( formatFromMarkerSchema(markerType, 'string', v, stringTable) ) .join(', '); + case 'screenshot-size': + return `${value.width}px × ${value.height}px`; default: console.warn( `A marker schema of type "${markerType}" had an unknown format ${JSON.stringify( @@ -642,7 +669,16 @@ export function markerPayloadMatchesSearch( continue; } - if (isStringIndexFormat(payloadField.format)) { + const { format } = payloadField; + if (typeof format === 'object' && format.type === 'screenshot-data-url') { + // Searching a base64-encoded image isn't useful. + continue; + } + if (format === 'screenshot-size') { + value = formatFromMarkerSchema(data.type, format, value, stringTable); + } + + if (isStringIndexFormat(format)) { if (typeof value !== 'number') { console.warn( `In marker ${marker.name}, the key ${payloadField.key} has an invalid value "${value}" as a unique string, it isn't a number.` @@ -673,6 +709,9 @@ export function markerPayloadMatchesSearch( export function isStringIndexFormat( format: MarkerFormatType | undefined ): boolean { + if (typeof format === 'object') { + return format.type === 'screenshot-data-url'; + } return ( format === 'unique-string' || format === 'flow-id' || @@ -684,17 +723,15 @@ export function isStringIndexFormat( * Returns a map of marker schema name -> array of field keys, listing any fields * that contain indexes into the string table. If a marker schema has no such * fields, then we don't put an entry for it in the returned map. + * The front-end-only schemas are always covered, whether or not they're in + * `markerSchemas`. */ export function computeStringIndexMarkerFieldsByDataType( markerSchemas: MarkerSchema[] ): Map { const stringIndexMarkerFieldsByDataType = new Map(); - // 'CompositorScreenshot' markers currently don't have a schema (#5303), - // hardcode the url field (which is a string index) until they do. - stringIndexMarkerFieldsByDataType.set('CompositorScreenshot', ['url']); - - for (const schema of markerSchemas) { + for (const schema of [...markerSchemas, ...markerSchemaFrontEndOnly]) { const { name, fields } = schema; const stringIndexFields = []; for (const field of fields) { diff --git a/src/profile-logic/process-profile.ts b/src/profile-logic/process-profile.ts index 69c4712e7a..e41b02d9b4 100644 --- a/src/profile-logic/process-profile.ts +++ b/src/profile-logic/process-profile.ts @@ -42,6 +42,7 @@ import { isArtTraceFormat, convertArtTraceProfile } from './import/art-trace'; import { PROCESSED_PROFILE_VERSION, INTERVAL, + INTERVAL_START, INTERVAL_END, INSTANT, } from '../app-logic/constants'; @@ -49,6 +50,7 @@ import { getFriendlyThreadName, nudgeReturnAddresses, } from '../profile-logic/profile-data'; +import { getScreenshotMarkerName } from './marker-data'; import { toInt32Array, toUint8Array, @@ -907,12 +909,20 @@ function _processMarkerPayload( // here, and then to `MarkerPayload` as the return value for this function. // This doesn't provide type safety but it shows the intent of going from an // object without much type safety, to a specific type definition. - const data: MarkerPayload = payload as any; + let data: MarkerPayload = payload as any; if (!data.type) { return data; } + if (data.type === 'CompositorScreenshot') { + const { windowWidth, windowHeight, ...rest } = data as any; + data = + windowWidth === undefined || windowHeight === undefined + ? rest + : { ...rest, windowSize: { width: windowWidth, height: windowHeight } }; + } + const stringIndexMarkerFields = stringIndexMarkerFieldsByDataType.get( data.type ); @@ -933,6 +943,91 @@ function _processMarkerPayload( return newData; } +/** + * Gecko emits one instant marker per composite of a window, plus a + * CompositorScreenshotWindowDestroyed marker when the window goes away. Each + * screenshot is valid until the next one for the same window, so rewrite them + * into start / end marker pairs. A window that is never destroyed keeps its last + * screenshot open, so that marker gets extended to the end of the thread. + */ +function _convertScreenshotMarkersToStartEnd( + markers: RawMarkerTable, + stringTable: StringTable +): RawMarkerTable { + const hasScreenshots = markers.data.some( + (data) => data !== null && data.type === 'CompositorScreenshot' + ); + if (!hasScreenshots) { + return markers; + } + + const newMarkers = getEmptyRawMarkerTable(); + const openMarkerPerWindow = new Map< + string, + { name: IndexIntoStringTable; index: number } + >(); + + function push( + name: IndexIntoStringTable, + startTime: Milliseconds | null, + endTime: Milliseconds | null, + phase: MarkerPhase, + sourceIndex: number, + data: MarkerPayload | null + ) { + newMarkers.name.push(name); + newMarkers.startTime.push(startTime); + newMarkers.endTime.push(endTime); + newMarkers.phase.push(phase); + newMarkers.category.push(markers.category[sourceIndex]); + newMarkers.data.push(data); + newMarkers.length++; + } + + for (let i = 0; i < markers.length; i++) { + const data = markers.data[i]; + if (data === null || data.type !== 'CompositorScreenshot') { + push( + markers.name[i], + markers.startTime[i], + markers.endTime[i], + markers.phase[i], + i, + data + ); + continue; + } + + const windowID = String(data.windowID); + const time = markers.startTime[i]; + const openMarker = openMarkerPerWindow.get(windowID); + if (openMarker !== undefined) { + openMarkerPerWindow.delete(windowID); + // The end marker only needs to identify the window: the start marker's + // payload is what the derived marker keeps. + push(openMarker.name, null, time, INTERVAL_END, openMarker.index, { + type: 'CompositorScreenshot', + windowID: data.windowID, + }); + } + + if ( + stringTable.getString(markers.name[i]) === + 'CompositorScreenshotWindowDestroyed' + ) { + continue; + } + + const name = stringTable.indexForString( + getScreenshotMarkerName(data.windowID) + ); + push(name, time, null, INTERVAL_START, i, data); + openMarkerPerWindow.set(windowID, { name, index: i }); + } + + return newMarkers; +} + function _timeColumnToCompactTimeDeltas(time: Milliseconds[]): Milliseconds[] { const NS_PER_MS = 1000000; @@ -1349,13 +1444,21 @@ function _processThread( frameIndexOffset ); - const { markers, jsAllocations, nativeAllocations } = _processMarkers( + const { + markers: processedMarkers, + jsAllocations, + nativeAllocations, + } = _processMarkers( geckoMarkers, thread.stringTable, stringIndexMarkerFieldsByDataType, globalDataCollector, stackIndexOffset ); + const markers = _convertScreenshotMarkersToStartEnd( + processedMarkers, + globalDataCollector.getStringTable() + ); const samples = _processSamples(geckoSamples, stackIndexOffset); // Compute usedInnerWindowIDs from the geckoFrameStruct and the thread markers. diff --git a/src/profile-logic/processed-profile-versioning.ts b/src/profile-logic/processed-profile-versioning.ts index 0999f5c078..f660f2e843 100644 --- a/src/profile-logic/processed-profile-versioning.ts +++ b/src/profile-logic/processed-profile-versioning.ts @@ -3310,6 +3310,103 @@ const _upgraders: { } } }, + [70]: (profile: any) => { + // The CompositorScreenshot marker payload's `windowWidth` and + // `windowHeight` fields were replaced with a single `windowSize` field, and + // these markers are now stored as start / end marker pairs whose name + // carries the window ID, instead of instant markers. The + // CompositorScreenshotWindowDestroyed marker becomes the end marker of that + // window's last screenshot. + const INTERVAL_START = 2; + const INTERVAL_END = 3; + const stringTable = StringTable.withBackingArray( + profile.shared.stringArray + ); + + for (const thread of profile.threads) { + const { markers } = thread; + const newMarkers: any = { + data: [], + name: [], + startTime: [], + endTime: [], + phase: [], + category: [], + length: 0, + }; + const hasThreadId = Boolean(markers.threadId); + if (hasThreadId) { + newMarkers.threadId = []; + } + + const push = ( + name: number, + startTime: number | null, + endTime: number | null, + phase: number, + sourceIndex: number, + data: any + ) => { + newMarkers.name.push(name); + newMarkers.startTime.push(startTime); + newMarkers.endTime.push(endTime); + newMarkers.phase.push(phase); + newMarkers.category.push(markers.category[sourceIndex]); + newMarkers.data.push(data); + if (hasThreadId) { + newMarkers.threadId.push(markers.threadId[sourceIndex]); + } + newMarkers.length++; + }; + + const openMarkerPerWindow = new Map(); + for (let i = 0; i < markers.length; i++) { + const data = markers.data[i]; + if (!data || data.type !== 'CompositorScreenshot') { + push( + markers.name[i], + markers.startTime[i], + markers.endTime[i], + markers.phase[i], + i, + data + ); + continue; + } + + const windowID = String(data.windowID); + const time = markers.startTime[i]; + const openMarker = openMarkerPerWindow.get(windowID); + if (openMarker !== undefined) { + openMarkerPerWindow.delete(windowID); + push(openMarker.name, null, time, INTERVAL_END, openMarker.index, { + type: 'CompositorScreenshot', + windowID: data.windowID, + }); + } + + if ( + stringTable.getString(markers.name[i]) === + 'CompositorScreenshotWindowDestroyed' + ) { + continue; + } + + const { windowWidth, windowHeight, ...startData } = data; + if (windowWidth !== undefined && windowHeight !== undefined) { + startData.windowSize = { width: windowWidth, height: windowHeight }; + } + + const name = stringTable.indexForString( + `CompositorScreenshot ${windowID}` + ); + push(name, time, null, INTERVAL_START, i, startData); + openMarkerPerWindow.set(windowID, { name, index: i }); + } + + thread.markers = newMarkers; + } + }, // If you add a new upgrader here, please document the change in // `docs-developer/CHANGELOG-formats.md`. }; diff --git a/src/profile-logic/tracks.ts b/src/profile-logic/tracks.ts index 4d1fe7ff09..eb304ddfe3 100644 --- a/src/profile-logic/tracks.ts +++ b/src/profile-logic/tracks.ts @@ -2,7 +2,6 @@ * 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/. */ import type { - ScreenshotPayload, Profile, RawProfileSharedData, RawThread, @@ -29,7 +28,6 @@ import { getMarkerTypesForDisplay, } from './marker-data'; import { intersectSets, subtractSets } from '../utils/set'; -import { StringTable } from '../utils/string-table'; import { splitSearchString, stringsToRegExp } from '../utils/string'; import { ensureExists, assertExhaustiveCheck } from '../utils/types'; @@ -717,12 +715,6 @@ export function computeGlobalTracks( let globalTracks: GlobalTrack[] = []; // Create the global tracks. - const { stringArray } = profile.shared; - const stringTable = StringTable.withBackingArray(stringArray); - const screenshotNameIndex = stringTable.hasString('CompositorScreenshot') - ? stringTable.indexForString('CompositorScreenshot') - : null; - for ( let threadIndex = 0; threadIndex < profile.threads.length; @@ -762,20 +754,20 @@ export function computeGlobalTracks( } } - // Check for screenshots. + // Check for screenshots. Their marker name carries the window ID, so match + // on the payload type instead. Windows must keep being added in the order + // their first screenshot was taken: shared URLs refer to global tracks by + // index, so a different order would change what an existing URL selects. const ids: Set = new Set(); - if (screenshotNameIndex !== null) { - for (let markerIndex = 0; markerIndex < markers.length; markerIndex++) { - if (markers.name[markerIndex] === screenshotNameIndex) { - // Coerce the payload to a screenshot one. Don't do a runtime check that - // this is correct. - const data = markers.data[markerIndex] as ScreenshotPayload; - ids.add(data.windowID); - } - } - for (const id of ids) { - globalTracks.push({ type: 'screenshots', id, threadIndex }); + for (let markerIndex = 0; markerIndex < markers.length; markerIndex++) { + const data = markers.data[markerIndex]; + if (data === null || data.type !== 'CompositorScreenshot') { + continue; } + ids.add(String(data.windowID)); + } + for (const id of ids) { + globalTracks.push({ type: 'screenshots', id, threadIndex }); } } diff --git a/src/test/components/TooltipMarker.test.tsx b/src/test/components/TooltipMarker.test.tsx index 938a5bc361..05518e6d64 100644 --- a/src/test/components/TooltipMarker.test.tsx +++ b/src/test/components/TooltipMarker.test.tsx @@ -1156,8 +1156,7 @@ describe('TooltipMarker', function () { type: 'CompositorScreenshot', url: screenshotUrlIndex, windowID: 'XXX', - windowWidth: 600, - windowHeight: 300, + windowSize: { width: 600, height: 300 }, }, ], ]); @@ -1180,23 +1179,6 @@ describe('TooltipMarker', function () { expect(container.firstChild).toMatchSnapshot(); }); - it('renders the tooltip of CompositorScreenshotWindowDestroyed', () => { - setupWithPayload([ - [ - 'CompositorScreenshotWindowDestroyed', - 1, - 2, - { - type: 'CompositorScreenshot', - windowID: 'XXX', - url: undefined, - }, - ], - ]); - - expect(document.body).toMatchSnapshot(); - }); - it('shows the source thread for markers from a merged thread', function () { // We construct a profile that has 2 threads from 2 different tabs. const tab1Domain = 'https://mozilla.org'; @@ -1276,8 +1258,7 @@ describe('TooltipMarker', function () { type: 'CompositorScreenshot', url: screenshotUrlIndex, windowID: 'XXX', - windowWidth: 600, - windowHeight: 300, + windowSize: { width: 600, height: 300 }, }, ], ] diff --git a/src/test/components/TrackContextMenu.test.tsx b/src/test/components/TrackContextMenu.test.tsx index 89807f1bdc..813a5bd347 100644 --- a/src/test/components/TrackContextMenu.test.tsx +++ b/src/test/components/TrackContextMenu.test.tsx @@ -36,7 +36,7 @@ import { getScreenshotTrackProfile, getNetworkTrackProfile, addIPCMarkerPairToThreads, - getThreadWithMarkers, + getThreadWithRawMarkers, getScreenshotMarkersForWindowId, } from '../fixtures/profiles/processed-profile'; @@ -1229,14 +1229,14 @@ describe('timeline/TrackContextMenu', function () { // add a couple of global screenshots tracks profile.threads.push({ - ...getThreadWithMarkers( + ...getThreadWithRawMarkers( profile.shared, getScreenshotMarkersForWindowId('0', 5) ), tid: profile.threads.length, }); profile.threads.push({ - ...getThreadWithMarkers( + ...getThreadWithRawMarkers( profile.shared, getScreenshotMarkersForWindowId('1', 5) ), diff --git a/src/test/components/TrackScreenshots.test.tsx b/src/test/components/TrackScreenshots.test.tsx index 43ccf90be8..8cfa6bdee1 100644 --- a/src/test/components/TrackScreenshots.test.tsx +++ b/src/test/components/TrackScreenshots.test.tsx @@ -204,8 +204,10 @@ describe('timeline/TrackScreenshots', function () { const profile = getScreenshotTrackProfile(); const { shared, threads } = profile; const [thread] = threads; - const markerIndexA = thread.markers.length - 3; - const markerIndexB = thread.markers.length - 2; + // Screenshots are stored as start / end pairs, so these are the start + // markers of the second to last and third to last screenshots. + const markerIndexA = thread.markers.length - 5; + const markerIndexB = thread.markers.length - 3; // We keep the last marker so that the profile's root range is correct. _setScreenshotMarkersToUnknown(thread, shared, markerIndexA, markerIndexB); @@ -232,8 +234,9 @@ describe('timeline/TrackScreenshots', function () { const { shared, threads } = profile; const [thread] = threads; + // The start markers of the first two screenshots. const markerIndexA = 0; - const markerIndexB = 1; + const markerIndexB = 2; _setScreenshotMarkersToUnknown(thread, shared, markerIndexA, markerIndexB); @@ -396,15 +399,11 @@ function _setScreenshotMarkersToUnknown( shared: RawProfileSharedData, ...markerIndexes: IndexIntoRawMarkerTable[] ) { - // Remove off the last few screenshot markers const stringTable = StringTable.withBackingArray(shared.stringArray); const unknownStringIndex = stringTable.indexForString('Unknown'); - const screenshotStringIndex = stringTable.indexForString( - 'CompositorScreenshot' - ); for (const markerIndex of markerIndexes) { // Double check that we've actually got screenshot markers: - if (thread.markers.name[markerIndex] !== screenshotStringIndex) { + if (thread.markers.data[markerIndex]?.type !== 'CompositorScreenshot') { throw new Error('This is not a screenshot marker.'); } thread.markers.name[markerIndex] = unknownStringIndex; diff --git a/src/test/components/__snapshots__/TooltipMarker.test.tsx.snap b/src/test/components/__snapshots__/TooltipMarker.test.tsx.snap index 011f064231..f5fda6d905 100644 --- a/src/test/components/__snapshots__/TooltipMarker.test.tsx.snap +++ b/src/test/components/__snapshots__/TooltipMarker.test.tsx.snap @@ -1994,70 +1994,6 @@ exports[`TooltipMarker renders properly redirect network markers without additio
`; -exports[`TooltipMarker renders the tooltip of CompositorScreenshotWindowDestroyed 1`] = ` - -
-
-
-
-
- 1ms -
-
- - CompositorScreenshotWindowDestroyed - -
-
-
-
-
- Description - : -
- This marker shows the moment a window has been destroyed. -
- Window ID - : -
- XXX -
- Track - : -
- Empty -
-
-
- -`; - exports[`TooltipMarker renders tooltips for various markers: Bailout_ShapeGuard after getelem on line 3666 of resource://foo.js -> resource://bar.js:3662-10 1`] = `
- 2ms + 1ms
+
+ Description + : +
+
+ This marker spans the time between each composite of a window and shows the window contents during that time. +
@@ -5206,17 +5153,7 @@ exports[`TooltipMarker shows image of CompositorScreenshot markers 1`] = ` Window Size :
- 600 - px × - 300 - px -
- Description - : -
- This marker spans the time between each composite of a window and shows the window contents during that time. + 600px × 300px
diff --git a/src/test/components/__snapshots__/TrackScreenshots.test.tsx.snap b/src/test/components/__snapshots__/TrackScreenshots.test.tsx.snap index bb50f9aa2b..2f97ffbe4c 100644 --- a/src/test/components/__snapshots__/TrackScreenshots.test.tsx.snap +++ b/src/test/components/__snapshots__/TrackScreenshots.test.tsx.snap @@ -14,7 +14,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` >
@@ -24,7 +24,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` >
@@ -34,7 +34,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > @@ -44,7 +44,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > @@ -54,7 +54,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > @@ -64,7 +64,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > @@ -74,7 +74,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > @@ -84,7 +84,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > @@ -94,7 +94,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > @@ -104,7 +104,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > @@ -114,7 +114,7 @@ exports[`timeline/TrackScreenshots matches the component snapshot 1`] = ` > diff --git a/src/test/fixtures/profiles/processed-profile.ts b/src/test/fixtures/profiles/processed-profile.ts index dc886ee205..5c9a368e4e 100644 --- a/src/test/fixtures/profiles/processed-profile.ts +++ b/src/test/fixtures/profiles/processed-profile.ts @@ -64,6 +64,7 @@ import type { } from 'firefox-profiler/types'; import { deriveMarkersFromRawMarkerTable, + getScreenshotMarkerName, IPCMarkerCorrelations, } from '../../../profile-logic/marker-data'; import { @@ -369,20 +370,33 @@ export function makeIntervalMarker( * A utility to make TestDefinedRawMarker */ export function makeCompositorScreenshot( - startTime: Milliseconds + startTime: Milliseconds, + windowID: string = '0' ): TestDefinedRawMarker { return { - ...makeInstantMarker('CompositorScreenshot', startTime), + ...makeStartMarker(getScreenshotMarkerName(windowID), startTime), data: { type: 'CompositorScreenshot', url: 0, - windowID: '', - windowWidth: 100, - windowHeight: 100, + windowID, + windowSize: { width: 300, height: 150 }, }, }; } +/** + * A utility to make TestDefinedRawMarker + */ +export function makeCompositorScreenshotEnd( + endTime: Milliseconds, + windowID: string = '0' +): TestDefinedRawMarker { + return { + ...makeEndMarker(getScreenshotMarkerName(windowID), endTime), + data: { type: 'CompositorScreenshot', windowID }, + }; +} + export function getUserTiming( name: string, startTime: Milliseconds, @@ -421,6 +435,19 @@ export function getProfileWithMarkers( return profile; } +export function getProfileWithRawMarkers( + ...markersPerThread: TestDefinedRawMarker[][] +): Profile { + const profile = getEmptyProfile(); + profile.meta.markerSchema = markerSchemaForTests; + + profile.threads = markersPerThread.map((markers, i) => ({ + ...getThreadWithRawMarkers(profile.shared, markers), + tid: i, + })); + return profile; +} + /** * This profile is useful for marker table tests. The markers were taken from * real-world values. @@ -1335,41 +1362,33 @@ export function getIPCTrackProfile() { return getProfileWithMarkers(arrayOfIPCMarkers); } +/** + * One screenshot per millisecond, starting at 0. Each one ends where the next + * one starts; the last one is left open unless `destroyTime` is given. + */ export function getScreenshotMarkersForWindowId( windowID: string, - count: number -): TestDefinedMarker[] { - return Array(count) - .fill(undefined) - .map((_, i) => [ - 'CompositorScreenshot', - i, - null, - { - type: 'CompositorScreenshot', - url: 0, // Some arbitrary string. - windowID, - windowWidth: 300, - windowHeight: 150, - }, - ]); + count: number, + destroyTime: Milliseconds | null = null +): TestDefinedRawMarker[] { + const markers: TestDefinedRawMarker[] = []; + for (let i = 0; i < count; i++) { + if (i > 0) { + markers.push(makeCompositorScreenshotEnd(i, windowID)); + } + markers.push(makeCompositorScreenshot(i, windowID)); + } + if (destroyTime !== null) { + markers.push(makeCompositorScreenshotEnd(destroyTime, windowID)); + } + return markers; } export function getScreenshotTrackProfile() { - return getProfileWithMarkers([ + return getProfileWithRawMarkers([ ...getScreenshotMarkersForWindowId('0', 5), // This window isn't closed, so we should repeat the last screenshot - ...getScreenshotMarkersForWindowId('1', 5), // This window is closed after screenshot 6. + ...getScreenshotMarkersForWindowId('1', 5, 6), // This window is closed after screenshot 6. ...getScreenshotMarkersForWindowId('2', 10), // This window isn't closed and define the profile length - [ - 'CompositorScreenshotWindowDestroyed', - 6, - null, - { - type: 'CompositorScreenshot', - windowID: '1', - url: undefined, - }, - ], ]); } diff --git a/src/test/fixtures/upgrades/processed-3.json b/src/test/fixtures/upgrades/processed-3.json index df3eb93eaa..0e865bd71f 100644 --- a/src/test/fixtures/upgrades/processed-3.json +++ b/src/test/fixtures/upgrades/processed-3.json @@ -347,30 +347,36 @@ ] }, "markers": { - "length": 10, + "length": 13, "name": [ 4, 5, 10, 10, + 18, 5, 17, 8, 8, 9, - 14 + 18, + 14, + 19 ], "time": [ 0, 2, 4, 5, + 6, 8, 8, 9, 10, 11, - 13 + 12, + 13, + 14 ], "data": [ { @@ -396,6 +402,13 @@ "interval": "end", "type": "tracing" }, + { + "type": "CompositorScreenshot", + "url": 20, + "windowID": "0x136888400", + "windowWidth": 1280, + "windowHeight": 1000 + }, { "category": "Paint", "interval": "end", @@ -428,6 +441,13 @@ "startTime": 11, "endTime": 12 }, + { + "type": "CompositorScreenshot", + "url": 20, + "windowID": "0x136888400", + "windowWidth": 1280, + "windowHeight": 1000 + }, { "type": "Network", "startTime": 3, @@ -446,6 +466,10 @@ "requestStart": 10, "responseStart": 11, "responseEnd": 12 + }, + { + "type": "CompositorScreenshot", + "windowID": "0x136888400" } ], "category": [ @@ -455,10 +479,13 @@ 4, 4, 4, + 4, 5, 5, 1, - 7 + 4, + 7, + 4 ] }, "samples": { @@ -527,7 +554,10 @@ "Load 32: https://github.com/rustwasm/wasm-bindgen/issues/3", "DiskIO", "FileIO", - "TextureCacheFree" + "TextureCacheFree", + "CompositorScreenshot", + "CompositorScreenshotWindowDestroyed", + "data:image/jpeg;base64,/9j/4AAQSkZJRgABAQAAAQABAAD/2wBDAAUD" ], "pid": "Unknown Process 1" }, diff --git a/src/test/integration/profiler-edit/__snapshots__/profiler-edit.test.ts.snap b/src/test/integration/profiler-edit/__snapshots__/profiler-edit.test.ts.snap index 8df0ad49ac..69a5ffdc23 100644 --- a/src/test/integration/profiler-edit/__snapshots__/profiler-edit.test.ts.snap +++ b/src/test/integration/profiler-edit/__snapshots__/profiler-edit.test.ts.snap @@ -87,7 +87,7 @@ Object { "markerSchema": Array [], "oscpu": "macOS 14.6.1", "pausedRanges": Array [], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "processType": 0, "product": "a.out", "sampleUnits": Object { @@ -1452,7 +1452,7 @@ Object { "markerSchema": Array [], "oscpu": "macOS 14.6.1", "pausedRanges": Array [], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "processType": 0, "product": "a.out", "sampleUnits": Object { @@ -2817,7 +2817,7 @@ Object { "markerSchema": Array [], "oscpu": "macOS 14.6.1", "pausedRanges": Array [], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "processType": 0, "product": "a.out", "sampleUnits": Object { @@ -4182,7 +4182,7 @@ Object { "markerSchema": Array [], "oscpu": "macOS 14.6.1", "pausedRanges": Array [], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "processType": 0, "product": "a.out", "sampleUnits": Object { diff --git a/src/test/store/__snapshots__/profile-view.test.ts.snap b/src/test/store/__snapshots__/profile-view.test.ts.snap index c16fdb16e5..afb8dd010c 100644 --- a/src/test/store/__snapshots__/profile-view.test.ts.snap +++ b/src/test/store/__snapshots__/profile-view.test.ts.snap @@ -428,7 +428,7 @@ Object { "oscpu": "", "physicalCPUs": 0, "platform": "", - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "processType": 0, "product": "Firefox", "sourceURL": "", diff --git a/src/test/store/profile-view.test.ts b/src/test/store/profile-view.test.ts index 127d1bca69..b22f5006d8 100644 --- a/src/test/store/profile-view.test.ts +++ b/src/test/store/profile-view.test.ts @@ -2079,9 +2079,9 @@ describe('actions/ProfileView', function () { const screenshots = screenshotMarkersById.get('0'); expect(screenshots?.length).toEqual(5); for (const screenshot of screenshots ?? []) { - expect(screenshot.name).toEqual('CompositorScreenshot'); + expect(screenshot.name).toEqual('CompositorScreenshot 0'); } - expect(screenshotMarkersById.get('1')?.length).toEqual(6); + expect(screenshotMarkersById.get('1')?.length).toEqual(5); expect(screenshotMarkersById.get('2')?.length).toEqual(10); }); @@ -2090,9 +2090,9 @@ describe('actions/ProfileView', function () { const [{ markers }] = profile.threads; const { dispatch, getState } = storeWithProfile(profile); - // Double check that there are 21 markers in the test data, and commit a - // subsection of that range. - expect(markers.length).toBe(21); + // Double check that there are 38 raw markers in the test data, and commit + // a subsection of that range. + expect(markers.length).toBe(38); dispatch(ProfileView.commitRange(3.1, 7.5)); // Get out the markers. diff --git a/src/test/store/receive-profile.test.ts b/src/test/store/receive-profile.test.ts index afc2309460..a768f16605 100644 --- a/src/test/store/receive-profile.test.ts +++ b/src/test/store/receive-profile.test.ts @@ -1757,8 +1757,7 @@ describe('actions/receive-profile', function () { type: 'CompositorScreenshot', url: 0, // Some arbitrary string. windowID: '0', - windowWidth: 300, - windowHeight: 150, + windowSize: { width: 300, height: 150 }, }, ], ]); @@ -1771,8 +1770,7 @@ describe('actions/receive-profile', function () { type: 'CompositorScreenshot', url: 0, // Some arbitrary string. windowID: '1', - windowWidth: 300, - windowHeight: 150, + windowSize: { width: 300, height: 150 }, }, ], ]); diff --git a/src/test/unit/__snapshots__/marker-data.test.ts.snap b/src/test/unit/__snapshots__/marker-data.test.ts.snap index df135ea0a7..47bf7283f3 100644 --- a/src/test/unit/__snapshots__/marker-data.test.ts.snap +++ b/src/test/unit/__snapshots__/marker-data.test.ts.snap @@ -179,12 +179,15 @@ Array [ "data": Object { "type": "CompositorScreenshot", "url": 21, - "windowHeight": 1000, "windowID": "0x136888400", - "windowWidth": 1280, + "windowSize": Object { + "height": 1000, + "width": 1280, + }, }, "end": 25, - "name": "CompositorScreenshot", + "incomplete": true, + "name": "CompositorScreenshot 0x136888400", "start": 25, "threadId": null, }, diff --git a/src/test/unit/__snapshots__/marker-schema.test.ts.snap b/src/test/unit/__snapshots__/marker-schema.test.ts.snap index 9f60bce48e..50216e9a4f 100644 --- a/src/test/unit/__snapshots__/marker-schema.test.ts.snap +++ b/src/test/unit/__snapshots__/marker-schema.test.ts.snap @@ -278,6 +278,33 @@ Array [ , "a, b", ], + Array [ + "screenshot-size", + Object { + "height": 1000, + "width": 1280, + }, + "1280px × 1000px", + "1280px × 1000px", + ], + Array [ + Object { + "sizeFieldForAspectRatio": "windowSize", + "type": "screenshot-data-url", + }, + 0, + , + "(screenshot)", + ], ] `; diff --git a/src/test/unit/__snapshots__/profile-conversion.test.ts.snap b/src/test/unit/__snapshots__/profile-conversion.test.ts.snap index a3831da47a..892fc08ddb 100644 --- a/src/test/unit/__snapshots__/profile-conversion.test.ts.snap +++ b/src/test/unit/__snapshots__/profile-conversion.test.ts.snap @@ -42,7 +42,7 @@ Object { "RefreshDriverTick", "Network", ], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "ART Trace (Android)", "symbolicated": true, "version": 36, @@ -1022,7 +1022,7 @@ Object { "RefreshDriverTick", "Network", ], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "ART Trace (Android)", "symbolicated": true, "version": 36, @@ -2305,7 +2305,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -2323,11 +2323,11 @@ Object { Object { "isMainThread": true, "jsAllocationCount": 0, - "markerCount": 483, + "markerCount": 512, "markerNamesTop": Array [ "MessageLoop::RunTask (376)", "BrowserCrApplication::sendEvent (74)", - "CompositorScreenshot (30)", + "CompositorScreenshot id (59)", "LatencyInfo.Flow (2)", "TracingStartedInBrowser (1)", ], @@ -2697,7 +2697,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -2715,11 +2715,11 @@ Object { Object { "isMainThread": true, "jsAllocationCount": 0, - "markerCount": 483, + "markerCount": 512, "markerNamesTop": Array [ "MessageLoop::RunTask (376)", "BrowserCrApplication::sendEvent (74)", - "CompositorScreenshot (30)", + "CompositorScreenshot id (59)", "LatencyInfo.Flow (2)", "TracingStartedInBrowser (1)", ], @@ -3086,7 +3086,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3187,7 +3187,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3540,7 +3540,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3605,7 +3605,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3759,7 +3759,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3817,7 +3817,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "Firefox", "symbolicated": true, "version": 36, @@ -4207,7 +4207,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "Firefox", "symbolicated": true, "version": 36, @@ -4265,7 +4265,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "Firefox", "symbolicated": true, "version": 36, @@ -4323,7 +4323,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "Firefox", "symbolicated": true, "version": 36, @@ -4643,7 +4643,7 @@ Object { "importedFrom": "Simpleperf", "interval": 0, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "com.example.sampleapplication", "symbolicated": undefined, "version": 30, @@ -5019,7 +5019,7 @@ Object { "importedFrom": "Simpleperf", "interval": 0, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "com.example.sampleapplication", "symbolicated": undefined, "version": 30, @@ -5319,7 +5319,7 @@ Object { "importedFrom": "dhat", "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "target/debug/examples/work_log (dhat)", "symbolicated": true, "version": 36, @@ -5452,7 +5452,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "Flamegraph", "symbolicated": true, "version": 36, @@ -5510,7 +5510,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "product": "Flamegraph", "symbolicated": true, "version": 36, diff --git a/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap b/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap index 394eaa6d2e..99c7339e87 100644 --- a/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap +++ b/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap @@ -40,7 +40,7 @@ Object { "oscpu": undefined, "physicalCPUs": undefined, "platform": undefined, - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "processType": 0, "product": "Firefox", "sampleUnits": undefined, @@ -7644,7 +7644,7 @@ Object { "misc": "rv:48.0", "oscpu": "Intel Mac OS X 10.11", "platform": "Macintosh", - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "processType": 0, "product": "Firefox", "stackwalk": 1, @@ -9021,7 +9021,7 @@ Object { "misc": "rv:48.0", "oscpu": "Intel Mac OS X 10.11", "platform": "Macintosh", - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "processType": 0, "product": "Firefox", "stackwalk": 1, @@ -10566,7 +10566,7 @@ Object { "misc": "rv:48.0", "oscpu": "Intel Mac OS X 10.11", "platform": "Macintosh", - "preprocessedProfileVersion": 69, + "preprocessedProfileVersion": 70, "processType": 0, "product": "Firefox", "stackwalk": 1, @@ -10890,15 +10890,15 @@ Object { null, ], "name": Array [ - 7, - 8, - 9, 10, 11, - 8, - 9, - 15, - 16, + 12, + 13, + 14, + 11, + 12, + 18, + 19, ], "originalLocation": Array [ null, @@ -10966,10 +10966,10 @@ Object { 1, ], "name": Array [ - 8, - 9, - 8, - 9, + 11, + 12, + 11, + 12, ], }, "resourceTable": Object { @@ -10985,9 +10985,9 @@ Object { 1, ], "name": Array [ - 13, - 12, - 14, + 16, + 15, + 17, ], "type": Array [ 1, @@ -11006,7 +11006,7 @@ Object { null, ], "filename": Array [ - 12, + 15, ], "id": Array [ null, @@ -11085,10 +11085,13 @@ Object { "VsyncTimestamp", "Reflow", "Rasterize", + "CompositorScreenshot", + "data:image/jpeg;base64,/9j/4AAQSkZJRgABAQAAAQABAAD/2wBDAAUD", "TextureCacheFree", "DOMEvent", "MinorGC", "Load 32: https://github.com/rustwasm/wasm-bindgen/issues/3", + "CompositorScreenshotWindowDestroyed", "(root)", "0x100000f84", "0x100001a45", @@ -11099,6 +11102,7 @@ Object { "firefox-webcontent", "setTimeout handler", "Element.getBoundingClientRect", + "CompositorScreenshot 0x136888400", ], }, "threads": Array [ @@ -11112,10 +11116,14 @@ Object { 4, 4, 4, + 4, 5, 5, 1, + 4, + 4, 7, + 4, ], "data": Array [ Object { @@ -11141,6 +11149,15 @@ Object { "interval": "end", "type": "tracing", }, + Object { + "type": "CompositorScreenshot", + "url": 4, + "windowID": "0x136888400", + "windowSize": Object { + "height": 1000, + "width": 1280, + }, + }, Object { "category": "Paint", "interval": "end", @@ -11168,6 +11185,19 @@ Object { "type": "DOMEvent", }, Object {}, + Object { + "type": "CompositorScreenshot", + "windowID": "0x136888400", + }, + Object { + "type": "CompositorScreenshot", + "url": 4, + "windowID": "0x136888400", + "windowSize": Object { + "height": 1000, + "width": 1280, + }, + }, Object { "URI": "https://github.com/rustwasm/wasm-bindgen/issues/3", "connectEnd": 9, @@ -11187,55 +11217,75 @@ Object { "tcpConnectEnd": 7, "type": "Network", }, + Object { + "type": "CompositorScreenshot", + "windowID": "0x136888400", + }, ], "endTime": Array [ null, null, null, 5, + null, 8, null, null, 10, 12, + 12, + null, 13, + 14, ], - "length": 10, + "length": 14, "name": Array [ 0, 1, 2, 2, + 20, 1, - 3, - 4, - 4, 5, 6, + 6, + 7, + 20, + 20, + 8, + 20, ], "phase": Array [ 0, 2, 2, 3, + 2, 3, 0, 2, 3, 1, + 3, + 2, 1, + 3, ], "startTime": Array [ 0, 2, 4, null, + 6, null, 8, 9, null, 11, + null, + 12, 3, + null, ], }, "name": "GeckoMain", @@ -11377,9 +11427,9 @@ Object { 2, 2, 1, - 4, - 4, - 5, + 6, + 6, + 7, ], "phase": Array [ 0, @@ -11563,10 +11613,10 @@ Object { 2, 2, 1, - 4, - 4, - 5, - 14, + 6, + 6, + 7, + 17, ], "phase": Array [ 0, diff --git a/src/test/unit/marker-data.test.ts b/src/test/unit/marker-data.test.ts index 9a5d06beff..c2f5687b53 100644 --- a/src/test/unit/marker-data.test.ts +++ b/src/test/unit/marker-data.test.ts @@ -31,6 +31,7 @@ import { makeIntervalMarker, makeInstantMarker, makeCompositorScreenshot, + makeCompositorScreenshotEnd, makeStartMarker, makeEndMarker, } from '../fixtures/profiles/processed-profile'; @@ -325,12 +326,11 @@ describe('Derive markers from Gecko phase markers', function () { ]); }); - it('has special handling for CompositorScreenshot', function () { + it('turns the instant CompositorScreenshot markers into intervals', function () { const basePayload = { type: 'CompositorScreenshot' as const, url: 16, - windowWidth: 1280, - windowHeight: 1000, + windowSize: { width: 1280, height: 1000 }, }; const payloadsForWindowA: ScreenshotPayload[] = [ { @@ -339,7 +339,7 @@ describe('Derive markers from Gecko phase markers', function () { }, { ...basePayload, - windowWidth: 500, + windowSize: { width: 500, height: 1000 }, windowID: '0xAAAAAAAAA', }, ]; @@ -351,34 +351,33 @@ describe('Derive markers from Gecko phase markers', function () { const startTimesForWindowA = [2, 5]; const startTimesForWindowB = [3, 6]; + const windowBDestroyedTime = 8; + + const screenshot = ( + startTime: number, + data: ScreenshotPayload + ): TestDefinedGeckoMarker => ({ + name: 'CompositorScreenshot', + startTime, + endTime: null, + phase: INSTANT, + data, + }); + const { markers, getState } = setupWithTestDefinedMarkers([ + screenshot(startTimesForWindowA[0], payloadsForWindowA[0]), + screenshot(startTimesForWindowB[0], payloadsForWindowB[0]), + screenshot(startTimesForWindowA[1], payloadsForWindowA[1]), + screenshot(startTimesForWindowB[1], payloadsForWindowB[1]), { - name: 'CompositorScreenshot', - startTime: startTimesForWindowA[0], - endTime: null, - phase: INTERVAL_START, - data: payloadsForWindowA[0], - }, - { - name: 'CompositorScreenshot', - startTime: startTimesForWindowB[0], - endTime: null, - phase: INTERVAL_START, - data: payloadsForWindowB[0], - }, - { - name: 'CompositorScreenshot', - startTime: startTimesForWindowA[1], + name: 'CompositorScreenshotWindowDestroyed', + startTime: windowBDestroyedTime, endTime: null, - phase: INTERVAL_START, - data: payloadsForWindowA[1], - }, - { - name: 'CompositorScreenshot', - startTime: startTimesForWindowB[1], - endTime: null, - phase: INTERVAL_START, - data: payloadsForWindowB[1], + phase: INSTANT, + data: { + type: 'CompositorScreenshot', + windowID: payloadsForWindowB[0].windowID, + }, }, ]); @@ -388,7 +387,7 @@ describe('Derive markers from Gecko phase markers', function () { // The two firsts have a duration from the first screenshot to the next in // the same window. { - name: 'CompositorScreenshot', + name: 'CompositorScreenshot 0xAAAAAAAAA', data: { ...payloadsForWindowA[0], url: expect.anything(), @@ -399,7 +398,7 @@ describe('Derive markers from Gecko phase markers', function () { threadId: null, }, { - name: 'CompositorScreenshot', + name: 'CompositorScreenshot 0xBBBBBBBBB', data: { ...payloadsForWindowB[0], url: expect.anything(), @@ -410,9 +409,10 @@ describe('Derive markers from Gecko phase markers', function () { threadId: null, }, - // The 2 lasts have a duration until the end of the thread range. + // Window A is still open, so its last screenshot is extended to the end + // of the thread range. { - name: 'CompositorScreenshot', + name: 'CompositorScreenshot 0xAAAAAAAAA', data: { ...payloadsForWindowA[1], url: expect.anything(), @@ -421,15 +421,18 @@ describe('Derive markers from Gecko phase markers', function () { end: threadRange.end, category: 0, threadId: null, + incomplete: true, }, + + // Window B was destroyed, so its last screenshot ends there. { - name: 'CompositorScreenshot', + name: 'CompositorScreenshot 0xBBBBBBBBB', data: { ...payloadsForWindowB[1], url: expect.anything(), }, start: startTimesForWindowB[1], - end: threadRange.end, + end: windowBDestroyedTime, category: 0, threadId: null, }, @@ -501,7 +504,7 @@ describe('deriveMarkersFromRawMarkerTable', function () { 'tracing:ArbitraryName', 'Network:Load 32: https://github.com/rustwasm/wasm-bindgen/issues/5', 'FileIO:FileIO', - 'CompositorScreenshot:CompositorScreenshot', + 'CompositorScreenshot:CompositorScreenshot 0x136888400', 'PreferenceRead:PreferenceRead', 'Text:RefreshDriverTick', 'NoPayloadUserData:Navigation::Start', @@ -812,12 +815,12 @@ describe('deriveMarkersFromRawMarkerTable', function () { type: 'CompositorScreenshot', url: expect.anything(), windowID: '0x136888400', - windowWidth: 1280, - windowHeight: 1000, + windowSize: { width: 1280, height: 1000 }, }, - name: 'CompositorScreenshot', + name: 'CompositorScreenshot 0x136888400', start: 25, end: 25, + incomplete: true, }); }); }); @@ -955,12 +958,17 @@ describe('filterRawMarkerTableToRange', () => { end: 5.6, markers: [ makeCompositorScreenshot(0), + makeCompositorScreenshotEnd(3), makeCompositorScreenshot(3), + makeCompositorScreenshotEnd(7), makeCompositorScreenshot(7), ], }); - expect(rawMarkerTable.startTime).toEqual([0, 3]); + // Both markers of a screenshot's pair are kept, and the screenshot taken + // after the range is dropped. + expect(rawMarkerTable.startTime).toEqual([0, null, 3, null]); + expect(rawMarkerTable.endTime).toEqual([null, 3, null, 7]); }); it('keeps a screenshot markers happening before the range if there is no other marker', () => { @@ -973,7 +981,7 @@ describe('filterRawMarkerTableToRange', () => { makeInstantMarker('EndMarkerOutOfRange', 8), ], }); - expect(processedMarkerNames).toEqual(['CompositorScreenshot']); + expect(processedMarkerNames).toEqual(['CompositorScreenshot 0']); }); it('filters network markers', () => { diff --git a/src/test/unit/marker-schema.test.ts b/src/test/unit/marker-schema.test.ts index 31f7ed8dd2..d53f23f747 100644 --- a/src/test/unit/marker-schema.test.ts +++ b/src/test/unit/marker-schema.test.ts @@ -6,6 +6,8 @@ import { formatFromMarkerSchema, parseLabel, markerSchemaFrontEndOnly, + isStringIndexFormat, + computeStringIndexMarkerFieldsByDataType, } from '../../profile-logic/marker-schema'; import { renderMarkerFieldValue } from 'firefox-profiler/components/tooltip/Marker'; import type { @@ -516,8 +518,17 @@ describe('marker schema formatting', function () { ], ['list', []], ['list', ['a', 'b']], + ['screenshot-size', { width: 1280, height: 1000 }], + // Without a payload there's no size field to look up, so the image falls + // back to a maximum size. + [ + { type: 'screenshot-data-url', sizeFieldForAspectRatio: 'windowSize' }, + 0, + ], ]; - const stringTable = StringTable.withBackingArray([]); + const stringTable = StringTable.withBackingArray([ + 'data:image/jpeg;base64,AAAA', + ]); expect( entries.map(([format, value]: [MarkerFormatType, any]): string[][] => [ format, @@ -529,6 +540,25 @@ describe('marker schema formatting', function () { }); }); +describe('computeStringIndexMarkerFieldsByDataType', function () { + it('treats the screenshot data URL as a string index', function () { + expect( + isStringIndexFormat({ + type: 'screenshot-data-url', + sizeFieldForAspectRatio: 'windowSize', + }) + ).toBe(true); + }); + + it('includes the front-end only schemas', function () { + // Callers may pass the profile's own schema list, which never contains the + // front-end only schemas. + expect(computeStringIndexMarkerFieldsByDataType([])).toEqual( + new Map([['CompositorScreenshot', ['url']]]) + ); + }); +}); + describe('getMarkerSchema', function () { it('combines front-end and Gecko marker schema', function () { const { profile } = getProfileFromTextSamples('A'); diff --git a/src/test/unit/merge-compare.test.ts b/src/test/unit/merge-compare.test.ts index c064168786..85d1135d1b 100644 --- a/src/test/unit/merge-compare.test.ts +++ b/src/test/unit/merge-compare.test.ts @@ -687,8 +687,7 @@ describe('mergeThreads function', function () { type: 'CompositorScreenshot', url: screenshot1UrlIndex, windowID: 'XXX', - windowWidth: 300, - windowHeight: 600, + windowSize: { width: 300, height: 600 }, }, ], ]); @@ -702,8 +701,7 @@ describe('mergeThreads function', function () { type: 'CompositorScreenshot', url: screenshot2UrlIndex, windowID: 'YYY', - windowWidth: 300, - windowHeight: 600, + windowSize: { width: 300, height: 600 }, }, ], ]); diff --git a/src/types/markers.ts b/src/types/markers.ts index 25cac35a6a..e8a49720fd 100644 --- a/src/types/markers.ts +++ b/src/types/markers.ts @@ -86,7 +86,14 @@ export type MarkerFormatType = | 'pid' | 'tid' | 'list' - | { type: 'table'; columns: TableColumnFormat[] }; + // The size of a window in pixels, as a { width, height } object. + // "Label: 1280px × 1000px" + | 'screenshot-size' + | { type: 'table'; columns: TableColumnFormat[] } + // An image data URL, stored as an index into the profile's string table. It is + // rendered at the aspect ratio of the 'screenshot-size' field named by + // `sizeFieldForAspectRatio`. + | { type: 'screenshot-data-url'; sizeFieldForAspectRatio: string }; type TableColumnFormat = { // type for formatting, default is string @@ -691,30 +698,30 @@ type VsyncTimestampPayload = { type: 'VsyncTimestamp'; }; -export type ScreenshotPayload = - | { - type: 'CompositorScreenshot'; - // This field represents the data url of the image. It is saved in the string table. - url: IndexIntoStringTable; - // A memory address that can uniquely identify a window. It has no meaning other than - // a way to identify a window. - windowID: string; - // The original dimensions of the window that was captured. The actual image that is - // stored in the string table will be scaled down from the original size. - windowWidth: number; - windowHeight: number; - } - // Markers that represent the closing of a window (name === 'CompositorScreenshotWindowDestroyed') - // only have a windowID data. - | { - type: 'CompositorScreenshot'; - // A memory address that can uniquely identify a window. It has no meaning other than - // a way to identify a window. - windowID: string; - // Having the property present but void makes it easier to deal with Flow in - // our flow version. - url: void; - }; +export type ScreenshotPayload = { + type: 'CompositorScreenshot'; + // A value that can uniquely identify a window. It has no meaning other than + // a way to identify a window. Both integers and hexadecimal strings like + // "0x136888400" occur, depending on the Firefox version that recorded the + // profile, so normalize it with String() before using it as a key. + windowID: number | string; + // This field represents the data url of the image. It is saved in the string table. + // The marker that only closes a window's last screenshot doesn't have one. + url?: IndexIntoStringTable; + // The original dimensions of the window that was captured. The actual image that is + // stored in the string table will be scaled down from the original size. + windowSize?: { width: number; height: number }; +}; + +export type ScreenshotPayload_Gecko = { + type: 'CompositorScreenshot'; + windowID: number | string; + url?: IndexIntoStringTable; + // Gecko writes the window dimensions as two separate fields. Profile + // processing folds them into a single `windowSize` field. + windowWidth?: number; + windowHeight?: number; +}; export type StyleMarkerPayload = { type: 'Styles'; @@ -887,7 +894,7 @@ export type MarkerPayload_Gecko = | GCMajorMarkerPayload_Gecko | GCSliceMarkerPayload_Gecko | VsyncTimestampPayload - | ScreenshotPayload + | ScreenshotPayload_Gecko | CcMarkerTracing | ArbitraryEventTracing | NavigationMarkerPayload