Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/components/timeline/TrackCustomMarkerGraph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import type {
IndexIntoStringTable,
MarkerSchema,
CollectedCustomMarkerSamples,
ValueBounds,
MarkerGraphType,
MarkerIndex,
Marker,
Expand All @@ -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;
Expand Down Expand Up @@ -101,6 +103,7 @@ class TrackCustomMarkerCanvas extends React.PureComponent<CanvasProps> {
markerSchema,
markerSampleRanges,
collectedSamples,
valueBounds,
height,
width,
getMarker,
Expand Down Expand Up @@ -136,7 +139,7 @@ class TrackCustomMarkerCanvas extends React.PureComponent<CanvasProps> {
throw new Error('No lines for marker ' + name);
}

const { minNumber, maxNumber } = collectedSamples;
const { minNumber, maxNumber } = valueBounds;
const [sampleStart, sampleEnd] = markerSampleRanges;

{
Expand Down Expand Up @@ -336,6 +339,7 @@ type StateProps = {
readonly rangeEnd: Milliseconds;
readonly markerSampleRanges: [IndexIntoSamplesTable, IndexIntoSamplesTable];
readonly collectedSamples: CollectedCustomMarkerSamples;
readonly valueBounds: ValueBounds;
readonly getMarker: (param: MarkerIndex) => Marker;
};

Expand Down Expand Up @@ -484,6 +488,7 @@ class TrackCustomMarkerGraphImpl extends React.PureComponent<Props, State> {
graphHeight,
width,
collectedSamples,
valueBounds,
getMarker,
} = this.props;

Expand All @@ -506,7 +511,8 @@ class TrackCustomMarkerGraphImpl extends React.PureComponent<Props, State> {

const left = (width * (sampleTime - rangeStart)) / rangeLength;

const { minNumber, maxNumber, numbersPerLine } = collectedSamples;
const { minNumber, maxNumber } = valueBounds;
const { numbersPerLine } = collectedSamples;

const dots = [];

Expand Down Expand Up @@ -572,6 +578,7 @@ class TrackCustomMarkerGraphImpl extends React.PureComponent<Props, State> {
graphHeight,
width,
collectedSamples,
valueBounds,
getMarker,
} = this.props;

Expand All @@ -589,6 +596,7 @@ class TrackCustomMarkerGraphImpl extends React.PureComponent<Props, State> {
height={graphHeight}
width={width}
collectedSamples={collectedSamples}
valueBounds={valueBounds}
getMarker={getMarker}
/>
{hoveredCounter === null ? null : (
Expand Down Expand Up @@ -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),
Expand Down
52 changes: 42 additions & 10 deletions src/selectors/per-thread/markers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import type {
ThreadsKey,
Tid,
CollectedCustomMarkerSamples,
ValueBounds,
IndexIntoSamplesTable,
IndexIntoStringTable,
State,
Expand Down Expand Up @@ -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) => {
Expand All @@ -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,
};
Expand All @@ -695,9 +686,50 @@ export function getMarkerSelectorsPerThread(
)
);

const getCommittedRangeMarkerSampleValueBounds: Selector<ValueBounds> =
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,
};
}

Expand Down
106 changes: 100 additions & 6 deletions src/test/components/TrackCustomMarker.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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)
Comment thread
fqueze marked this conversation as resolved.
) {
const { profile, stringTable } = getProfileFromTextSamples(
Array(SAMPLE_COUNT).fill('A').join(' ')
Array(values.length).fill('A').join(' ')
);
const markerStringIndex = stringTable.indexForString('Marker');
const threadIndex = 0;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -142,6 +149,7 @@ function setup() {
flushRafCalls,
getMarkerDot,
getContextDrawCalls,
markerStringIndex,
};
}

Expand Down Expand Up @@ -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
});
});
7 changes: 5 additions & 2 deletions src/types/profile-derived.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down