From c9d3de5b3fd3d347a10a951a2d497f0217b3e28a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florian=20Qu=C3=A8ze?= Date: Wed, 3 Sep 2025 23:26:49 +0200 Subject: [PATCH] Scale custom marker graphs on values within the committed range. --- .../timeline/TrackCustomMarkerGraph.tsx | 14 ++- src/selectors/per-thread/markers.ts | 52 +++++++-- .../components/TrackCustomMarker.test.tsx | 106 +++++++++++++++++- src/types/profile-derived.ts | 7 +- 4 files changed, 159 insertions(+), 20 deletions(-) diff --git a/src/components/timeline/TrackCustomMarkerGraph.tsx b/src/components/timeline/TrackCustomMarkerGraph.tsx index 95351f1a15..db5d254e00 100644 --- a/src/components/timeline/TrackCustomMarkerGraph.tsx +++ b/src/components/timeline/TrackCustomMarkerGraph.tsx @@ -29,6 +29,7 @@ import type { IndexIntoStringTable, MarkerSchema, CollectedCustomMarkerSamples, + ValueBounds, MarkerGraphType, MarkerIndex, Marker, @@ -51,6 +52,7 @@ type CanvasProps = { readonly markerSchema: MarkerSchema; readonly markerSampleRanges: [IndexIntoSamplesTable, IndexIntoSamplesTable]; readonly collectedSamples: CollectedCustomMarkerSamples; + readonly valueBounds: ValueBounds; readonly width: CssPixels; readonly height: CssPixels; readonly getMarker: (param: MarkerIndex) => Marker; @@ -101,6 +103,7 @@ class TrackCustomMarkerCanvas extends React.PureComponent { markerSchema, markerSampleRanges, collectedSamples, + valueBounds, height, width, getMarker, @@ -136,7 +139,7 @@ class TrackCustomMarkerCanvas extends React.PureComponent { throw new Error('No lines for marker ' + name); } - const { minNumber, maxNumber } = collectedSamples; + const { minNumber, maxNumber } = valueBounds; const [sampleStart, sampleEnd] = markerSampleRanges; { @@ -336,6 +339,7 @@ type StateProps = { readonly rangeEnd: Milliseconds; readonly markerSampleRanges: [IndexIntoSamplesTable, IndexIntoSamplesTable]; readonly collectedSamples: CollectedCustomMarkerSamples; + readonly valueBounds: ValueBounds; readonly getMarker: (param: MarkerIndex) => Marker; }; @@ -484,6 +488,7 @@ class TrackCustomMarkerGraphImpl extends React.PureComponent { graphHeight, width, collectedSamples, + valueBounds, getMarker, } = this.props; @@ -506,7 +511,8 @@ class TrackCustomMarkerGraphImpl extends React.PureComponent { const left = (width * (sampleTime - rangeStart)) / rangeLength; - const { minNumber, maxNumber, numbersPerLine } = collectedSamples; + const { minNumber, maxNumber } = valueBounds; + const { numbersPerLine } = collectedSamples; const dots = []; @@ -572,6 +578,7 @@ class TrackCustomMarkerGraphImpl extends React.PureComponent { graphHeight, width, collectedSamples, + valueBounds, getMarker, } = this.props; @@ -589,6 +596,7 @@ class TrackCustomMarkerGraphImpl extends React.PureComponent { height={graphHeight} width={width} collectedSamples={collectedSamples} + valueBounds={valueBounds} getMarker={getMarker} /> {hoveredCounter === null ? null : ( @@ -620,6 +628,8 @@ export const TrackCustomMarkerGraph = explicitConnect< markerTrackSelectors.getCommittedRangeMarkerSampleRange(state), collectedSamples: markerTrackSelectors.getCollectedCustomMarkerSamples(state), + valueBounds: + markerTrackSelectors.getCommittedRangeMarkerSampleValueBounds(state), rangeStart: start, rangeEnd: end, getMarker: selectors.getMarkerGetter(state), diff --git a/src/selectors/per-thread/markers.ts b/src/selectors/per-thread/markers.ts index 9f28f07094..afb5b47ed6 100644 --- a/src/selectors/per-thread/markers.ts +++ b/src/selectors/per-thread/markers.ts @@ -27,6 +27,7 @@ import type { ThreadsKey, Tid, CollectedCustomMarkerSamples, + ValueBounds, IndexIntoSamplesTable, IndexIntoStringTable, State, @@ -638,8 +639,6 @@ export function getMarkerSelectorsPerThread( ); } const markerIndexes: MarkerIndex[] = []; - let minNumber = Infinity; - let maxNumber = -Infinity; const numbersPerLine: number[][] = []; const { graphs, name: schemaName } = markerSchema; const keys = graphs.map((graph) => { @@ -659,19 +658,11 @@ export function getMarkerSelectorsPerThread( for (let i = 0; i < keys.length; ++i) { const val = (data as any)[keys[i]]; numbersPerLine[i].push(val); - if (val < minNumber) { - minNumber = val; - } - if (val > maxNumber) { - maxNumber = val; - } } } }); return { - minNumber, - maxNumber, numbersPerLine, markerIndexes, }; @@ -695,9 +686,50 @@ export function getMarkerSelectorsPerThread( ) ); + const getCommittedRangeMarkerSampleValueBounds: Selector = + createSelector( + getCollectedCustomMarkerSamples, + getCommittedRangeMarkerSampleRange, + (collectedSamples, sampleRange) => { + const [sampleStart, sampleEnd] = sampleRange; + const { numbersPerLine } = collectedSamples; + + // Handle edge case where there are no samples in range + if (sampleStart >= sampleEnd) { + return { minNumber: 0, maxNumber: 0 }; + } + + let minNumber = Infinity; + let maxNumber = -Infinity; + + for ( + let sampleIndex = sampleStart; + sampleIndex < sampleEnd; + sampleIndex++ + ) { + for ( + let graphIndex = 0; + graphIndex < numbersPerLine.length; + graphIndex++ + ) { + const val = numbersPerLine[graphIndex][sampleIndex]; + if (val < minNumber) { + minNumber = val; + } + if (val > maxNumber) { + maxNumber = val; + } + } + } + + return { minNumber, maxNumber }; + } + ); + return { getCollectedCustomMarkerSamples, getCommittedRangeMarkerSampleRange, + getCommittedRangeMarkerSampleValueBounds, }; } diff --git a/src/test/components/TrackCustomMarker.test.tsx b/src/test/components/TrackCustomMarker.test.tsx index 04cd23f8df..608433586d 100644 --- a/src/test/components/TrackCustomMarker.test.tsx +++ b/src/test/components/TrackCustomMarker.test.tsx @@ -6,9 +6,11 @@ import type { CssPixels } from 'firefox-profiler/types'; import { Provider } from 'react-redux'; -import { fireEvent } from '@testing-library/react'; +import { fireEvent, act } from '@testing-library/react'; import { render } from 'firefox-profiler/test/fixtures/testing-library'; +import { commitRange } from 'firefox-profiler/actions/profile-view'; +import { getThreadSelectors } from 'firefox-profiler/selectors/per-thread'; import { TrackCustomMarker } from '../../components/timeline/TrackCustomMarker'; import { ensureExists } from '../../utils/types'; @@ -46,9 +48,13 @@ function getMarkerPixelPosition(time: number): CssPixels { return (time * GRAPH_WIDTH) / SAMPLE_COUNT; } -function setup() { +function setup( + values: number[] = Array(SAMPLE_COUNT) + .fill(0) + .map((_, i) => i) +) { const { profile, stringTable } = getProfileFromTextSamples( - Array(SAMPLE_COUNT).fill('A').join(' ') + Array(values.length).fill('A').join(' ') ); const markerStringIndex = stringTable.indexForString('Marker'); const threadIndex = 0; @@ -83,9 +89,10 @@ function setup() { thread.markers.category.push(4); thread.markers.length++; }; - for (let i = 0; i < SAMPLE_COUNT; i++) { - addMarker(i, i, i * 2); - } + + values.forEach((value, index) => { + addMarker(index, value, value * 2); + }); const store = storeWithProfile(profile); const { getState, dispatch } = store; const flushRafCalls = mockRaf(); @@ -142,6 +149,7 @@ function setup() { flushRafCalls, getMarkerDot, getContextDrawCalls, + markerStringIndex, }; } @@ -300,3 +308,89 @@ describe('TrackCustomMarker with intersection observer', function () { ); }); }); + +describe('TrackCustomMarker with committed range scaling', function () { + autoMockCanvasContext(); + autoMockElementSize({ width: GRAPH_WIDTH, height: GRAPH_HEIGHT }); + autoMockIntersectionObserver(); + beforeEach(addRootOverlayElement); + afterEach(removeRootOverlayElement); + + it('uses the committed range scaling selector correctly', function () { + // Create markers with values: [1, 100, 2, 3] at times 0, 1, 2, 3 + // Full range min/max would be 1-100, but committed range 2-3 should be 2-3 + const { profile, dispatch, getState, markerStringIndex } = setup([ + 1, 100, 2, 3, + ]); + + // Get the selectors to test them directly + const state = getState(); + const threadSelectors = getThreadSelectors(0); + const markerSchema = ensureExists( + profile.meta.markerSchema.find((schema) => schema.name === 'Marker') + ); + const markerTrackSelectors = threadSelectors.getMarkerTrackSelectors( + markerSchema, + markerStringIndex + ); + + // Get initial samples (full range) - should have min=1, max=200 (because second line = first * 2) + const fullRangeSamples = + markerTrackSelectors.getCollectedCustomMarkerSamples(state); + const fullRangeValueBounds = + markerTrackSelectors.getCommittedRangeMarkerSampleValueBounds(state); + expect(fullRangeValueBounds.minNumber).toBe(1); + expect(fullRangeValueBounds.maxNumber).toBe(200); // 100 * 2 from second line + + // Commit range from 2ms to 3.5ms to include only the markers at times 2ms and 3ms. + // The 2.1 value is to exclude from the range the end of the marker at 1ms. + act(() => { + dispatch(commitRange(2.1, 3.5)); + }); + + // Get committed range samples - should include markers at 2ms[2,4] and 3ms[3,6] + const newState = getState(); + const committedRangeSamples = + markerTrackSelectors.getCollectedCustomMarkerSamples(newState); + const committedRangeValueBounds = + markerTrackSelectors.getCommittedRangeMarkerSampleValueBounds(newState); + expect(committedRangeValueBounds.minNumber).toBe(2); + expect(committedRangeValueBounds.maxNumber).toBe(6); + + // Verify the arrays are the same length (same structure) + expect(committedRangeSamples.numbersPerLine).toHaveLength( + fullRangeSamples.numbersPerLine.length + ); + expect(committedRangeSamples.markerIndexes).toHaveLength( + fullRangeSamples.markerIndexes.length + ); + }); + + it('handles edge case where committed range has identical values', function () { + // Create markers with identical values in the committed range + const { profile, dispatch, getState, markerStringIndex } = setup([ + 1, 100, 5, 5, + ]); + + const threadSelectors = getThreadSelectors(0); + const markerSchema = ensureExists( + profile.meta.markerSchema.find((schema) => schema.name === 'Marker') + ); + const markerTrackSelectors = threadSelectors.getMarkerTrackSelectors( + markerSchema, + markerStringIndex + ); + + // Focus on the range with identical values at times 2ms and 3ms (values [5,10] and [5,10]) + act(() => { + dispatch(commitRange(2.1, 3.5)); + }); + + // Should not crash when min === max - both markers have first=5, second=10 + const state = getState(); + const committedRangeValueBounds = + markerTrackSelectors.getCommittedRangeMarkerSampleValueBounds(state); + expect(committedRangeValueBounds.minNumber).toBe(5); + expect(committedRangeValueBounds.maxNumber).toBe(10); // 5 * 2 from second line + }); +}); diff --git a/src/types/profile-derived.ts b/src/types/profile-derived.ts index d83a9a1e9d..b2e444deea 100644 --- a/src/types/profile-derived.ts +++ b/src/types/profile-derived.ts @@ -584,14 +584,17 @@ export type AccumulatedCounterSamples = { * A collection of the data for all configured lines for a given marker */ export type CollectedCustomMarkerSamples = { - readonly minNumber: number; - readonly maxNumber: number; // This value holds the number per configured line // selection. The array will share the indexes of the range filtered marker samples. readonly numbersPerLine: number[][]; readonly markerIndexes: MarkerIndex[]; }; +export type ValueBounds = { + readonly minNumber: number; + readonly maxNumber: number; +}; + export type StackType = 'js' | 'native' | 'unsymbolicated'; export type GlobalTrack =