diff --git a/profiler-cli/README.md b/profiler-cli/README.md index dee091c641..60066bf6e6 100644 --- a/profiler-cli/README.md +++ b/profiler-cli/README.md @@ -43,7 +43,7 @@ profiler-cli thread markers # List markers with aggregated statis profiler-cli thread functions # List all functions with CPU percentages profiler-cli thread network # Show network requests with timing phases [--search] [--min-duration] [--max-duration] [--limit] [--sort] profiler-cli thread page-load # Show page load summary (navigation timing, resources, CPU, jank) -profiler-cli marker info # Show detailed marker information (e.g., m-1234) +profiler-cli marker info ... # Show detailed marker information; accepts several handles and ranges (e.g., m-1234, m-1234..m-1240) profiler-cli marker stack # Show full stack trace for a marker profiler-cli function expand # Show full untruncated function name (e.g., f-123) profiler-cli function info # Show detailed function information and category breakdown diff --git a/profiler-cli/guide.txt b/profiler-cli/guide.txt index 39ba3569a7..4c8ad3ce8c 100644 --- a/profiler-cli/guide.txt +++ b/profiler-cli/guide.txt @@ -103,6 +103,8 @@ CORE WORKFLOW Step 6: Drill into specifics profiler-cli marker info m-1234 Full details for a marker (from handles in marker list) + profiler-cli marker info m-1234 m-1240 Several markers in one call (one record per handle) + profiler-cli marker info m-1234..m-1240 Inclusive range of handles, e.g. consecutive list rows profiler-cli marker stack m-1234 Full stack trace at the time of a marker profiler-cli function info f-12 Function details (source location, library) profiler-cli function expand f-12 Show full untruncated function name diff --git a/profiler-cli/schemas.txt b/profiler-cli/schemas.txt index 528fd57404..4479283264 100644 --- a/profiler-cli/schemas.txt +++ b/profiler-cli/schemas.txt @@ -267,7 +267,7 @@ profiler-cli function info --json context: SessionContext } -profiler-cli marker info --json +profiler-cli marker info --json { type: "marker-info", threadHandle, friendlyThreadName, markerHandle, markerIndex, name, @@ -286,6 +286,16 @@ profiler-cli strategy --json context: SessionContext } +profiler-cli marker info ... --json + { + type: "marker-info-multi", + requested: [markerHandle], + markers: [MarkerInfoResult], + errors: [{ markerHandle, error }], + rangeSpansThreadsWarning?: { ranges: [spec], threadHandles: [threadHandle] }, + context: SessionContext + } + profiler-cli status --json { type: "status", diff --git a/profiler-cli/src/commands/marker.ts b/profiler-cli/src/commands/marker.ts index 14a52be7ca..2b8d6bb2eb 100644 --- a/profiler-cli/src/commands/marker.ts +++ b/profiler-cli/src/commands/marker.ts @@ -7,6 +7,7 @@ */ import type { Command } from 'commander'; +import { expandMarkerHandleSpecs } from 'firefox-profiler/profile-query/marker-map'; import { addGlobalOptions, runCommand } from './shared'; export function registerMarkerCommand( @@ -17,16 +18,32 @@ export function registerMarkerCommand( addGlobalOptions( marker - .command('info [handle]') - .description('Show detailed marker information (e.g. m-1234)') - .option('--marker ', 'Marker handle') - ).action(async (handleArg: string | undefined, opts) => { - const markerHandle = handleArg ?? opts.marker; - await runCommand( + .command('info [handles...]') + .description( + 'Show detailed marker information for one or more markers ' + + '(e.g. m-1234, m-1234 m-1240, m-1234..m-1240)' + ) + .option( + '--marker ', + 'Marker handle(s) or range(s); a range covers at most 256 handles' + ) + ).action(async (handleArgs: string[], opts) => { + const specs = (handleArgs.length > 0 ? handleArgs : [opts.marker]).filter( + (spec): spec is string => spec !== undefined + ); + + const result = await runCommand( sessionDir, - { command: 'marker', subcommand: 'info', marker: markerHandle }, + { command: 'marker', subcommand: 'info', markers: specs }, opts ); + if ( + typeof result !== 'string' && + result.type === 'marker-info-multi' && + (result.errors.length > 0 || result.rangeSpansThreadsWarning) + ) { + process.exitCode = 1; + } }); addGlobalOptions( @@ -36,6 +53,26 @@ export function registerMarkerCommand( .option('--marker ', 'Marker handle') ).action(async (handleArg: string | undefined, opts) => { const markerHandle = handleArg ?? opts.marker; + // Expanding with the real grammar keeps this from being a second, drifting + // definition of it. A range reaching the daemon would come back as "Unknown + // marker m-1..m-3", which reads like a bad handle rather than bad syntax. + if (typeof markerHandle === 'string') { + let expanded: string[]; + try { + expanded = expandMarkerHandleSpecs([markerHandle]); + } catch (error) { + console.error( + `Error: ${error instanceof Error ? error.message : String(error)}` + ); + process.exit(1); + } + if (expanded.length > 1) { + console.error( + `Error: marker stack takes a single handle; ranges and lists are only supported by 'marker info'.` + ); + process.exit(1); + } + } await runCommand( sessionDir, { command: 'marker', subcommand: 'stack', marker: markerHandle }, diff --git a/profiler-cli/src/commands/shared.ts b/profiler-cli/src/commands/shared.ts index 9f8a3f7367..0119fd817f 100644 --- a/profiler-cli/src/commands/shared.ts +++ b/profiler-cli/src/commands/shared.ts @@ -12,7 +12,11 @@ import { collectStrings } from '../utils/parse'; import { sendCommand } from '../client'; import { formatOutput } from '../output'; import { CALL_TREE_SUMMARY_STRATEGIES } from 'firefox-profiler/profile-logic/profile-data'; -import type { ClientCommand, CallTreeSummaryStrategy } from '../protocol'; +import type { + ClientCommand, + CallTreeSummaryStrategy, + CommandResult, +} from '../protocol'; /** * Options shared by every command action via `addGlobalOptions`. @@ -31,9 +35,10 @@ export async function runCommand( sessionDir: string, command: ClientCommand, opts: GlobalOptions -): Promise { +): Promise { const result = await sendCommand(sessionDir, command, opts.session); console.log(formatOutput(result, opts.json ?? false)); + return result; } /** diff --git a/profiler-cli/src/daemon.ts b/profiler-cli/src/daemon.ts index d0c56efe4a..7ea6c8904d 100644 --- a/profiler-cli/src/daemon.ts +++ b/profiler-cli/src/daemon.ts @@ -36,6 +36,7 @@ import { toErrorMessage, } from './diagnostics'; import { assertExhaustiveCheck } from 'firefox-profiler/utils/types'; +import { expandMarkerHandleSpecs } from 'firefox-profiler/profile-query/marker-map'; import { BUILD_HASH, PACKAGE_NAME } from './constants'; /** @@ -470,11 +471,20 @@ export class Daemon { } case 'marker': switch (command.subcommand) { - case 'info': - if (!command.marker) { + case 'info': { + // Expand once here, so the single/multi result shape follows what + // the specs actually resolve to: every spelling of one marker + // ("m-1", "m-1,", "m-1..m-1") returns the single-marker shape. + const specs = command.markers ?? []; + const handles = expandMarkerHandleSpecs(specs); + if (handles.length === 0) { throw new Error('marker handle required for marker info'); } - return this.querier.markerInfo(command.marker); + if (handles.length === 1) { + return this.querier.markerInfo(handles[0]); + } + return this.querier.markerInfoMulti(specs); + } case 'stack': if (!command.marker) { throw new Error('marker handle required for marker stack'); diff --git a/profiler-cli/src/formatters.ts b/profiler-cli/src/formatters.ts index f3d19510fc..43726797a1 100644 --- a/profiler-cli/src/formatters.ts +++ b/profiler-cli/src/formatters.ts @@ -20,6 +20,7 @@ import type { ThreadListResult, MarkerStackResult, MarkerInfoResult, + MarkerInfoMultiResult, ProfileInfoResult, ProfileMetaResult, ThreadSamplesResult, @@ -488,9 +489,64 @@ export function formatMarkerInfoResult( result: WithContext ): string { const contextHeader = formatContextHeader(result.context); - let output = `${contextHeader} + return `${contextHeader}\n\n${formatMarkerInfoBody(result)}`; +} -Marker ${result.markerHandle}: ${result.name}`; +/** + * Format several marker info records as plain text, one per requested handle + * under a single context header. Handles that did not resolve are reported in + * place. + */ +export function formatMarkerInfoMultiResult( + result: WithContext +): string { + const contextHeader = formatContextHeader(result.context); + const total = result.markers.length + result.errors.length; + const records: string[] = []; + + // Walk the requested handles, so failed lookups keep their place in the order. + const byHandle = new Map(result.markers.map((m) => [m.markerHandle, m])); + const errorsByHandle = new Map( + result.errors.map((e) => [e.markerHandle, e.error]) + ); + let position = 0; + for (const markerHandle of result.requested) { + position++; + const prefix = `[${position}/${total}] `; + const marker = byHandle.get(markerHandle); + if (marker) { + records.push(prefix + formatMarkerInfoBody(marker).trimEnd()); + continue; + } + const error = errorsByHandle.get(markerHandle); + if (error !== undefined) { + records.push(`${prefix}Marker ${markerHandle}: error: ${error}`); + } + } + + let output = `${contextHeader}\n\n${records.join('\n\n----------\n\n')}`; + if (result.errors.length > 0) { + const verb = result.errors.length === 1 ? 'was' : 'were'; + output += `\n\n${result.errors.length} of ${total} requested markers ${verb} not found.`; + } + const spread = result.rangeSpansThreadsWarning; + if (spread) { + const rangeList = spread.ranges.join(', '); + const threadList = spread.threadHandles.join(', '); + output += + `\n\nWarning: the range ${rangeList} covers markers in more than one thread ` + + `(${threadList}). Handle ranges are numeric, so a range that runs past the end of ` + + `the listing you were reading picks up unrelated markers. Re-run the listing and ` + + `check the handles.`; + } + return output; +} + +/** + * Format one marker info record, below the context header. + */ +function formatMarkerInfoBody(result: MarkerInfoResult): string { + let output = `Marker ${result.markerHandle}: ${result.name}`; if (result.tooltipLabel) { output += ` - ${result.tooltipLabel}`; } diff --git a/profiler-cli/src/output.ts b/profiler-cli/src/output.ts index 1c4cd51b1d..ba84e3ba18 100644 --- a/profiler-cli/src/output.ts +++ b/profiler-cli/src/output.ts @@ -19,6 +19,7 @@ import { formatThreadListResult, formatMarkerStackResult, formatMarkerInfoResult, + formatMarkerInfoMultiResult, formatProfileInfoResult, formatProfileMetaResult, formatThreadSamplesResult, @@ -77,6 +78,8 @@ export function formatOutput( return formatMarkerStackResult(result); case 'marker-info': return formatMarkerInfoResult(result); + case 'marker-info-multi': + return formatMarkerInfoMultiResult(result); case 'profile-info': return formatProfileInfoResult(result); case 'profile-meta': diff --git a/profiler-cli/src/protocol.ts b/profiler-cli/src/protocol.ts index 79ef4ded0e..454659288f 100644 --- a/profiler-cli/src/protocol.ts +++ b/profiler-cli/src/protocol.ts @@ -62,6 +62,7 @@ export type { RateStats, MarkerGroupData, MarkerInfoResult, + MarkerInfoMultiResult, MarkerStackResult, StackTraceData, ProfileInfoResult, @@ -97,6 +98,7 @@ import type { ThreadListOptions, MarkerStackResult, MarkerInfoResult, + MarkerInfoMultiResult, ProfileInfoResult, ProfileMetaResult, ThreadSamplesResult, @@ -182,7 +184,10 @@ export type ClientCommand = | { command: 'marker'; subcommand: 'info' | 'select' | 'stack'; + /** Single handle, for `stack`. */ marker?: string; + /** Handle specs for `info`, e.g. ["m-42", "m-50..m-53"]. */ + markers?: string[]; } | { command: 'counter'; @@ -247,6 +252,7 @@ export type CommandResult = | WithContext | WithContext | WithContext + | WithContext | WithContext | WithContext | WithContext diff --git a/profiler-cli/src/test/integration/marker-info.test.ts b/profiler-cli/src/test/integration/marker-info.test.ts new file mode 100644 index 0000000000..e707aff4d6 --- /dev/null +++ b/profiler-cli/src/test/integration/marker-info.test.ts @@ -0,0 +1,204 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * 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/. */ + +/** + * Integration tests for `marker info` accepting several handles. These run + * through the real command layer: the single-vs-multi routing, the `--json` + * shape it selects, and the exit code are only observable from outside. + */ + +import { + createTestContext, + cleanupTestContext, + cli, + cliFail, + type CliTestContext, +} from './utils'; + +import type { + MarkerInfoResult, + MarkerInfoMultiResult, + WithContext, +} from '../../protocol'; + +const FIXTURE = 'src/test/fixtures/upgrades/processed-1.json'; + +describe('marker info with several handles', () => { + let ctx: CliTestContext; + + beforeEach(async () => { + ctx = await createTestContext(); + await cli(ctx, ['load', FIXTURE]); + // Listing the markers is what mints the m-N handles. The fixture thread has + // three markers, so this yields m-1, m-2 and m-3. + await cli(ctx, ['thread', 'markers', '--list']); + }); + + afterEach(async () => { + await cleanupTestContext(ctx); + }); + + async function markerInfoJson(args: string[]) { + const result = await cli(ctx, ['marker', 'info', ...args, '--json']); + return JSON.parse(result.stdout); + } + + it('returns the single-marker shape for one handle', async () => { + const parsed: WithContext = await markerInfoJson(['m-1']); + + // This is the back-compat contract: one handle must not get the wrapper. + expect(parsed.type).toBe('marker-info'); + expect(parsed.markerHandle).toBe('m-1'); + expect(parsed).not.toHaveProperty('markers'); + }); + + it('returns the single-marker shape for the legacy --marker flag', async () => { + const parsed: WithContext = await markerInfoJson([ + '--marker', + 'm-1', + ]); + + expect(parsed.type).toBe('marker-info'); + expect(parsed.markerHandle).toBe('m-1'); + }); + + it.each([['m-1,'], [',m-1'], ['m-1..m-1'], ['m-1..1'], [' m-1']])( + 'returns the single-marker shape for %p, which means one marker', + async (spec) => { + const parsed: WithContext = await markerInfoJson([ + spec, + ]); + + expect(parsed.type).toBe('marker-info'); + expect(parsed.markerHandle).toBe('m-1'); + } + ); + + it('returns the multi shape for several handles', async () => { + const parsed: WithContext = await markerInfoJson([ + 'm-1', + 'm-2', + ]); + + expect(parsed.type).toBe('marker-info-multi'); + expect(parsed.requested).toEqual(['m-1', 'm-2']); + expect(parsed.markers.map((m) => m.markerHandle)).toEqual(['m-1', 'm-2']); + expect(parsed.errors).toEqual([]); + }); + + it('returns the multi shape for a range', async () => { + const parsed: WithContext = await markerInfoJson([ + 'm-1..m-3', + ]); + + expect(parsed.type).toBe('marker-info-multi'); + expect(parsed.requested).toEqual(['m-1', 'm-2', 'm-3']); + }); + + it('prints one record per handle in text mode', async () => { + const result = await cli(ctx, ['marker', 'info', 'm-1', 'm-2']); + + expect(result.stdout).toContain('[1/2] Marker m-1:'); + expect(result.stdout).toContain('[2/2] Marker m-2:'); + expect(result.stdout).toContain('----------'); + // The session banner is printed once, not per record. + const banners = result.stdout + .split('\n') + .filter((line) => line.startsWith('[Thread:')); + expect(banners).toHaveLength(1); + }); + + it('reports an unknown handle per handle, keeps the rest, and exits 1', async () => { + const result = await cliFail(ctx, [ + 'marker', + 'info', + 'm-1', + 'm-9999', + 'm-2', + ]); + + expect(result.exitCode).toBe(1); + expect(result.stdout).toContain('[1/3] Marker m-1:'); + expect(result.stdout).toContain( + '[2/3] Marker m-9999: error: Unknown marker m-9999' + ); + expect(result.stdout).toContain('[3/3] Marker m-2:'); + expect(result.stdout).toContain('1 of 3 requested markers was not found.'); + }); + + it('fails the whole command on a malformed spec', async () => { + const result = await cliFail(ctx, ['marker', 'info', 'm-1', 'bogus']); + + expect(result.exitCode).toBe(1); + expect(result.stdout + result.stderr).toContain( + 'Invalid marker handle bogus' + ); + // Nothing was printed for the valid handle. + expect(result.stdout).not.toContain('Marker m-1:'); + }); + + it('fails the whole command on a reversed range', async () => { + const result = await cliFail(ctx, ['marker', 'info', 'm-3..m-1']); + + expect(result.exitCode).toBe(1); + expect(result.stdout + result.stderr).toContain( + 'end m-1 is before start m-3' + ); + }); + + it('still requires a handle', async () => { + const result = await cliFail(ctx, ['marker', 'info']); + + expect(result.exitCode).toBe(1); + expect(result.stdout + result.stderr).toContain( + 'marker handle required for marker info' + ); + }); + + it('rejects specs that expand to nothing', async () => { + // `specs` is non-empty here, so the emptiness only shows up after + // expansion; without that check the command prints just the context + // header and exits 0. + for (const args of [ + ['marker', 'info', ''], + ['marker', 'info', ','], + ]) { + const result = await cliFail(ctx, args); + expect({ args, exitCode: result.exitCode }).toEqual({ + args, + exitCode: 1, + }); + expect(result.stdout + result.stderr).toContain( + 'marker handle required for marker info' + ); + } + }); + + it('rejects an absurdly wide range instead of expanding it', async () => { + const result = await cliFail(ctx, ['marker', 'info', 'm-1..m-999999']); + + expect(result.exitCode).toBe(1); + expect(result.stdout + result.stderr).toContain('more than the maximum of'); + }); + + it('tells the user that marker stack does not take ranges', async () => { + const result = await cliFail(ctx, ['marker', 'stack', 'm-1..m-2']); + + expect(result.exitCode).toBe(1); + expect(result.stdout + result.stderr).toContain( + 'marker stack takes a single handle' + ); + // "Unknown marker m-1..m-2" would read like a bad handle instead of + // unsupported syntax, so the range must be rejected before lookup. + expect(result.stdout + result.stderr).not.toContain('Unknown marker'); + }); + + it('still accepts a single handle for marker stack', async () => { + // m-2 is the fixture's Reflow marker, the one with a stack. + const result = await cli(ctx, ['marker', 'stack', 'm-2']); + + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain('m-2'); + }); +}); diff --git a/profiler-cli/src/test/unit/marker-formatting.test.ts b/profiler-cli/src/test/unit/marker-formatting.test.ts index 2043230062..1056671764 100644 --- a/profiler-cli/src/test/unit/marker-formatting.test.ts +++ b/profiler-cli/src/test/unit/marker-formatting.test.ts @@ -2,10 +2,15 @@ * 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 { formatThreadMarkersResult } from '../../formatters'; +import { + formatThreadMarkersResult, + formatMarkerInfoMultiResult, +} from '../../formatters'; import type { ThreadMarkersResult, FlatMarkerItem, + MarkerInfoResult, + MarkerInfoMultiResult, SessionContext, WithContext, } from 'firefox-profiler/profile-query/types'; @@ -214,3 +219,154 @@ describe('formatThreadMarkersResult zoom baseline', function () { expect(output).toContain('3 markers'); }); }); + +function makeMarkerInfo( + overrides: Partial = {} +): MarkerInfoResult { + return { + type: 'marker-info', + threadHandle: 't-0', + friendlyThreadName: 'GeckoMain', + markerHandle: 'm-1', + markerIndex: 0, + name: 'DOMEvent', + markerType: 'DOMEvent', + category: { index: 0, name: 'DOM' }, + start: 100, + end: null, + ...overrides, + }; +} + +function makeMultiResult( + overrides: Partial> = {} +): WithContext { + return { + context: createContext(), + type: 'marker-info-multi', + requested: ['m-1'], + markers: [makeMarkerInfo()], + errors: [], + ...overrides, + }; +} + +describe('formatMarkerInfoMultiResult', function () { + it('prints one record per requested handle, in order', function () { + const output = formatMarkerInfoMultiResult( + makeMultiResult({ + requested: ['m-2', 'm-1'], + markers: [ + makeMarkerInfo({ markerHandle: 'm-2', name: 'Paint' }), + makeMarkerInfo({ markerHandle: 'm-1', name: 'DOMEvent' }), + ], + }) + ); + + expect(output).toContain('[1/2] Marker m-2: Paint'); + expect(output).toContain('[2/2] Marker m-1: DOMEvent'); + expect(output.indexOf('m-2')).toBeLessThan(output.indexOf('m-1')); + }); + + it('prints the session context header only once', function () { + const output = formatMarkerInfoMultiResult( + makeMultiResult({ + requested: ['m-1', 'm-2'], + markers: [ + makeMarkerInfo({ markerHandle: 'm-1' }), + makeMarkerInfo({ markerHandle: 'm-2' }), + ], + }) + ); + + const headerLines = output + .split('\n') + .filter((line) => line.startsWith('[Thread:')); + expect(headerLines).toHaveLength(1); + }); + + it('separates records with a rule', function () { + const output = formatMarkerInfoMultiResult( + makeMultiResult({ + requested: ['m-1', 'm-2'], + markers: [ + makeMarkerInfo({ markerHandle: 'm-1' }), + makeMarkerInfo({ markerHandle: 'm-2' }), + ], + }) + ); + + expect(output).toContain('\n----------\n'); + }); + + it('reports an unresolved handle in place and keeps the others', function () { + const output = formatMarkerInfoMultiResult( + makeMultiResult({ + requested: ['m-1', 'm-9999', 'm-2'], + markers: [ + makeMarkerInfo({ markerHandle: 'm-1' }), + makeMarkerInfo({ markerHandle: 'm-2' }), + ], + errors: [{ markerHandle: 'm-9999', error: 'Unknown marker m-9999' }], + }) + ); + + expect(output).toContain('[1/3] Marker m-1: DOMEvent'); + expect(output).toContain( + '[2/3] Marker m-9999: error: Unknown marker m-9999' + ); + expect(output).toContain('[3/3] Marker m-2: DOMEvent'); + expect(output).toContain('1 of 3 requested markers was not found.'); + }); + + it('does not add a not-found footer when every handle resolved', function () { + const output = formatMarkerInfoMultiResult(makeMultiResult()); + + expect(output).not.toContain('not found'); + }); + + it('warns when a range strayed into another thread', function () { + const output = formatMarkerInfoMultiResult( + makeMultiResult({ + requested: ['m-1', 'm-2'], + markers: [ + makeMarkerInfo({ markerHandle: 'm-1', threadHandle: 't-0' }), + makeMarkerInfo({ markerHandle: 'm-2', threadHandle: 't-1' }), + ], + rangeSpansThreadsWarning: { + ranges: ['m-1..m-2'], + threadHandles: ['t-0', 't-1'], + }, + }) + ); + + expect(output).toContain( + 'Warning: the range m-1..m-2 covers markers in more than one thread (t-0, t-1)' + ); + }); + + // Mirrors the single-handle guard: `start` is already profile-start-relative, + // so each record must print it verbatim. Needs a non-zero `rootRange.start`. + it('renders record times verbatim, without re-subtracting rootRange.start', function () { + const output = formatMarkerInfoMultiResult( + makeMultiResult({ + context: { ...createContext(), rootRange: { start: 9.2, end: 3000 } }, + requested: ['m-1', 'm-2'], + markers: [ + makeMarkerInfo({ markerHandle: 'm-1', start: 549.34 }), + makeMarkerInfo({ markerHandle: 'm-2', start: 700, end: 750 }), + ], + }) + ); + + expect(output).toContain('549.34ms'); + expect(output).not.toContain('540.14ms'); // 549.34 - 9.2, if subtracted twice + expect(output).toContain('700ms - 750ms'); + }); + + it('omits the range warning when the query did not set one', function () { + const output = formatMarkerInfoMultiResult(makeMultiResult()); + + expect(output).not.toContain('Warning:'); + }); +}); diff --git a/src/profile-query/formatters/marker-info.ts b/src/profile-query/formatters/marker-info.ts index bcb673a526..4dee449a7b 100644 --- a/src/profile-query/formatters/marker-info.ts +++ b/src/profile-query/formatters/marker-info.ts @@ -1058,16 +1058,10 @@ export function collectMarkerInfo( const threadHandleDisplay = threadMap.handleForThreadIndexes(threadIndexes); const zeroAt = getZeroAt(state); - // Get tooltip label - const getTooltipLabel = getLabelGetter( - (mi: MarkerIndex) => fullMarkerList[mi], - getProfile(state).meta.markerSchema, - markerSchemaByName, - categories, - stringTable, - 'tooltipLabel' - ); - const tooltipLabel = getTooltipLabel(markerIndex); + // The memoized selector, not a bare getLabelGetter: this runs once per + // marker, and building the getter parses every schema in the profile. + const tooltipLabel = + threadSelectors.getMarkerTooltipLabelGetter(state)(markerIndex); // Collect marker fields let fields: MarkerInfoResult['fields']; diff --git a/src/profile-query/index.ts b/src/profile-query/index.ts index 680132a693..0c94178678 100644 --- a/src/profile-query/index.ts +++ b/src/profile-query/index.ts @@ -65,7 +65,7 @@ import { } from 'firefox-profiler/profile-logic/source-map-matching'; import { assertExhaustiveCheck } from 'firefox-profiler/utils/types'; import { getAnyLibForFunc, getLibNameForFunc } from './function-list'; -import { MarkerMap } from './marker-map'; +import { MarkerMap, expandMarkerHandleSpecsDetailed } from './marker-map'; import { loadProfileFromFileOrUrl, type LoadOptions } from './loader'; import { collectProfileInfo } from './formatters/profile-info'; import { collectProfileMeta } from './formatters/profile-meta'; @@ -118,6 +118,7 @@ import type { ThreadListResult, MarkerStackResult, MarkerInfoResult, + MarkerInfoMultiResult, ProfileInfoResult, ProfileMetaResult, ThreadSamplesResult, @@ -1493,6 +1494,85 @@ export class ProfileQuerier { }; } + /** + * Show detailed information about several markers at once. A handle that does + * not resolve goes into `errors` rather than failing the whole query. + */ + async markerInfoMulti( + markerHandleSpecs: string[] + ): Promise> { + const { handles, ranges } = + expandMarkerHandleSpecsDetailed(markerHandleSpecs); + const markers: MarkerInfoResult[] = []; + const errors: MarkerInfoMultiResult['errors'] = []; + + for (const markerHandle of handles) { + try { + markers.push( + await collectMarkerInfo( + this._store, + this._markerMap, + this._threadMap, + markerHandle + ) + ); + } catch (error) { + errors.push({ + markerHandle, + error: error instanceof Error ? error.message : String(error), + }); + } + } + + // Only ranges are checked: a typed-out list of handles from several threads + // is a deliberate comparison, not an accident of numbering. Each range is + // judged on the markers it expanded to, so an unrelated handle elsewhere in + // the command cannot make a single-thread range look like it spans threads. + const threadHandleFor = new Map( + markers.map((marker) => [marker.markerHandle, marker.threadHandle]) + ); + let rangeSpansThreadsWarning: + | MarkerInfoMultiResult['rangeSpansThreadsWarning'] + | undefined; + const spanningRanges: string[] = []; + const spanningThreadHandles: string[] = []; + for (const range of ranges) { + const threadHandles: string[] = []; + for (const handle of range.handles) { + const threadHandle = threadHandleFor.get(handle); + if ( + threadHandle !== undefined && + !threadHandles.includes(threadHandle) + ) { + threadHandles.push(threadHandle); + } + } + if (threadHandles.length > 1) { + spanningRanges.push(range.spec); + for (const threadHandle of threadHandles) { + if (!spanningThreadHandles.includes(threadHandle)) { + spanningThreadHandles.push(threadHandle); + } + } + } + } + if (spanningRanges.length > 0) { + rangeSpansThreadsWarning = { + ranges: spanningRanges, + threadHandles: spanningThreadHandles, + }; + } + + return { + type: 'marker-info-multi', + requested: handles, + markers, + errors, + rangeSpansThreadsWarning, + context: this._getContext(), + }; + } + async markerStack( markerHandle: string ): Promise> { diff --git a/src/profile-query/marker-map.ts b/src/profile-query/marker-map.ts index 7b8ccefc64..b94a46476f 100644 --- a/src/profile-query/marker-map.ts +++ b/src/profile-query/marker-map.ts @@ -68,3 +68,96 @@ export class MarkerMap { return markerId; } } + +/** Matches a single marker handle, e.g. "m-42". */ +const MARKER_HANDLE_RE = /^m-(\d+)$/; + +/** Matches an inclusive marker handle range, e.g. "m-42..m-45" or "m-42..45". */ +const MARKER_RANGE_RE = /^m-(\d+)\.\.(?:m-)?(\d+)$/; + +/** A range wider than this is rejected as a probable typo. */ +export const MAX_MARKER_RANGE_SIZE = 256; + +/** + * Expand marker handle specs ("m-42", "m-42..m-45", "m-1,m-3..m-5") into a flat + * list of handles, dropping duplicates. Ranges expand numerically, so one that + * overruns its listing resolves into unrelated markers rather than failing. + */ +export function expandMarkerHandleSpecs(specs: string[]): string[] { + return expandMarkerHandleSpecsDetailed(specs).handles; +} + +/** A multi-element range spec, and the handles it expanded to. */ +export type MarkerHandleRange = { + spec: string; + handles: string[]; +}; + +/** + * As `expandMarkerHandleSpecs`, but also reports which specs were multi-element + * ranges and what each one expanded to, so a caller can check a range's own + * markers rather than the whole result set. + */ +export function expandMarkerHandleSpecsDetailed(specs: string[]): { + handles: string[]; + ranges: MarkerHandleRange[]; +} { + const handles: string[] = []; + const ranges: MarkerHandleRange[] = []; + const seen = new Set(); + const push = (handle: string) => { + if (!seen.has(handle)) { + seen.add(handle); + handles.push(handle); + } + }; + + for (const rawSpec of specs) { + for (const spec of rawSpec.split(',')) { + const trimmed = spec.trim(); + if (trimmed === '') { + continue; + } + + const range = MARKER_RANGE_RE.exec(trimmed); + if (range) { + const start = parseInt(range[1], 10); + const end = parseInt(range[2], 10); + if (end < start) { + throw new Error( + `Invalid marker range ${trimmed}: end m-${end} is before start m-${start}` + ); + } + const size = end - start + 1; + if (size > MAX_MARKER_RANGE_SIZE) { + throw new Error( + `Marker range ${trimmed} covers ${size} handles, more than the ` + + `maximum of ${MAX_MARKER_RANGE_SIZE}. Narrow the range, or pass ` + + `the handles you want individually.` + ); + } + const rangeHandles: string[] = []; + for (let id = start; id <= end; id++) { + const handle = `m-${id}`; + rangeHandles.push(handle); + push(handle); + } + if (end > start) { + ranges.push({ spec: trimmed, handles: rangeHandles }); + } + continue; + } + + if (MARKER_HANDLE_RE.test(trimmed)) { + push(trimmed); + continue; + } + + throw new Error( + `Invalid marker handle ${trimmed}: expected a handle like m-42 or a range like m-42..m-45` + ); + } + } + + return { handles, ranges }; +} diff --git a/src/profile-query/types.ts b/src/profile-query/types.ts index d912702799..4c9073ca7d 100644 --- a/src/profile-query/types.ts +++ b/src/profile-query/types.ts @@ -824,6 +824,26 @@ export type MarkerInfoResult = { stack?: StackTraceData; }; +/** Result of `marker info` with more than one handle. */ +export type MarkerInfoMultiResult = { + type: 'marker-info-multi'; + /** Handles requested, ranges expanded, in requested order. */ + requested: string[]; + markers: MarkerInfoResult[]; + errors: Array<{ + markerHandle: string; + error: string; + }>; + /** + * Set when a range resolved into several threads, which means it ran past the + * end of the listing the user was reading. + */ + rangeSpansThreadsWarning?: { + ranges: string[]; + threadHandles: string[]; + }; +}; + export type MarkerStackResult = { type: 'marker-stack'; threadHandle: string; diff --git a/src/test/unit/profile-query/marker-utils.test.ts b/src/test/unit/profile-query/marker-utils.test.ts index 8c911cb674..863c2feac5 100644 --- a/src/test/unit/profile-query/marker-utils.test.ts +++ b/src/test/unit/profile-query/marker-utils.test.ts @@ -11,7 +11,12 @@ import { collectThreadMarkers, collectThreadNetwork, } from 'firefox-profiler/profile-query/formatters/marker-info'; -import { MarkerMap } from 'firefox-profiler/profile-query/marker-map'; +import { + MarkerMap, + expandMarkerHandleSpecs, + expandMarkerHandleSpecsDetailed, + MAX_MARKER_RANGE_SIZE, +} from 'firefox-profiler/profile-query/marker-map'; import { ThreadMap } from 'firefox-profiler/profile-query/thread-map'; import { getCategories } from 'firefox-profiler/selectors/profile'; import { @@ -559,6 +564,101 @@ describe('collectMarkerInfo', function () { }); }); +describe('expandMarkerHandleSpecs', function () { + it('passes single handles through in order', function () { + expect(expandMarkerHandleSpecs(['m-5', 'm-1', 'm-3'])).toEqual([ + 'm-5', + 'm-1', + 'm-3', + ]); + }); + + it('expands an inclusive range', function () { + expect(expandMarkerHandleSpecs(['m-3..m-6'])).toEqual([ + 'm-3', + 'm-4', + 'm-5', + 'm-6', + ]); + }); + + it('accepts a range whose end omits the m- prefix', function () { + expect(expandMarkerHandleSpecs(['m-8..10'])).toEqual([ + 'm-8', + 'm-9', + 'm-10', + ]); + }); + + it('accepts a single-marker range', function () { + expect(expandMarkerHandleSpecs(['m-7..m-7'])).toEqual(['m-7']); + }); + + it('mixes handles, ranges and comma-separated lists', function () { + expect(expandMarkerHandleSpecs(['m-1,m-4..m-6', 'm-9'])).toEqual([ + 'm-1', + 'm-4', + 'm-5', + 'm-6', + 'm-9', + ]); + }); + + it('drops duplicates, keeping the first occurrence', function () { + expect(expandMarkerHandleSpecs(['m-2..m-4', 'm-3', 'm-4..m-5'])).toEqual([ + 'm-2', + 'm-3', + 'm-4', + 'm-5', + ]); + }); + + it('rejects a reversed range', function () { + expect(() => expandMarkerHandleSpecs(['m-9..m-4'])).toThrow( + 'end m-4 is before start m-9' + ); + }); + + it('rejects a spec that is not a handle or a range', function () { + expect(() => expandMarkerHandleSpecs(['t-3'])).toThrow( + 'Invalid marker handle t-3' + ); + }); + + it('accepts a range exactly at the size limit', function () { + const handles = expandMarkerHandleSpecs([ + `m-1..m-${MAX_MARKER_RANGE_SIZE}`, + ]); + + expect(handles).toHaveLength(MAX_MARKER_RANGE_SIZE); + }); + + it('rejects a range wider than the size limit', function () { + const end = MAX_MARKER_RANGE_SIZE + 1; + + expect(() => expandMarkerHandleSpecs([`m-1..m-${end}`])).toThrow( + `covers ${end} handles, more than the maximum of ${MAX_MARKER_RANGE_SIZE}` + ); + }); + + it('reports which specs were multi-element ranges', function () { + const { handles, ranges } = expandMarkerHandleSpecsDetailed([ + 'm-1', + 'm-4..m-6', + 'm-9..m-9', + ]); + + expect(handles).toEqual(['m-1', 'm-4', 'm-5', 'm-6', 'm-9']); + // A single-element range cannot straddle two listings, so it is not + // reported as a range needing a provenance check. Each reported range + // carries the handles it expanded to, so a caller can check that range's + // own markers rather than the whole result set. + expect(ranges).toEqual([ + { spec: 'm-4..m-6', handles: ['m-4', 'm-5', 'm-6'] }, + ]); + }); +}); + describe('collectThreadMarkers topN option', function () { it('defaults to 5 top markers per group', function () { const { store, threadMap, markerMap } = setupWithMarkers([ diff --git a/src/test/unit/profile-query/profile-querier.test.ts b/src/test/unit/profile-query/profile-querier.test.ts index 50777f8961..0adb605af1 100644 --- a/src/test/unit/profile-query/profile-querier.test.ts +++ b/src/test/unit/profile-query/profile-querier.test.ts @@ -1090,4 +1090,156 @@ describe('ProfileQuerier', function () { expect(renderer!.markerCount).not.toBe(rawMarkerCounts[1]); }); }); + + describe('markerInfoMulti', function () { + async function querierWithMarkerHandles() { + const profile = getProfileWithMarkers([ + ['Alpha', 10, null, { type: 'tracing', category: 'Test' }], + ['Beta', 20, null, { type: 'tracing', category: 'Test' }], + ['Gamma', 30, null, { type: 'tracing', category: 'Test' }], + ['Delta', 40, null, { type: 'tracing', category: 'Test' }], + ]); + const store = storeWithProfile(profile); + const rootRange = getProfileRootRange(store.getState()); + const querier = new ProfileQuerier(store, rootRange); + // Listing the markers is what hands out the m-N handles. + const list = await querier.threadMarkers('t-0', { list: true }); + return { + querier, + handles: list.flatMarkers!.map((m) => m.handle), + }; + } + + it('returns one record per handle, in the requested order', async function () { + const { querier, handles } = await querierWithMarkerHandles(); + + const result = await querier.markerInfoMulti([handles[2], handles[0]]); + + expect(result.type).toBe('marker-info-multi'); + expect(result.requested).toEqual([handles[2], handles[0]]); + expect(result.markers.map((m) => m.name)).toEqual(['Gamma', 'Alpha']); + expect(result.errors).toEqual([]); + }); + + it('expands an inclusive range of handles', async function () { + const { querier, handles } = await querierWithMarkerHandles(); + + const result = await querier.markerInfoMulti([ + `${handles[0]}..${handles[2]}`, + ]); + + expect(result.requested).toEqual(handles.slice(0, 3)); + expect(result.markers.map((m) => m.name)).toEqual([ + 'Alpha', + 'Beta', + 'Gamma', + ]); + }); + + it('reports an unknown handle per handle and still returns the others', async function () { + const { querier, handles } = await querierWithMarkerHandles(); + + const result = await querier.markerInfoMulti([ + handles[0], + 'm-9999', + handles[1], + ]); + + expect(result.markers.map((m) => m.name)).toEqual(['Alpha', 'Beta']); + expect(result.errors).toEqual([ + { markerHandle: 'm-9999', error: 'Unknown marker m-9999' }, + ]); + }); + + it('rejects a malformed handle spec outright', async function () { + const { querier } = await querierWithMarkerHandles(); + + await expect(querier.markerInfoMulti(['not-a-handle'])).rejects.toThrow( + 'Invalid marker handle not-a-handle' + ); + }); + + describe('range provenance', function () { + // Handle numbering continues across listings, so a range running off the + // end of the first thread's listing resolves into the second thread's. + async function querierWithTwoListings() { + const profile = getProfileWithMarkers( + [ + ['Alpha', 10, null, { type: 'tracing', category: 'Test' }], + ['Beta', 20, null, { type: 'tracing', category: 'Test' }], + ], + [ + ['Gamma', 30, null, { type: 'tracing', category: 'Test' }], + ['Delta', 40, null, { type: 'tracing', category: 'Test' }], + ] + ); + const store = storeWithProfile(profile); + const rootRange = getProfileRootRange(store.getState()); + const querier = new ProfileQuerier(store, rootRange); + const first = await querier.threadMarkers('t-0', { list: true }); + const second = await querier.threadMarkers('t-1', { list: true }); + return { + querier, + firstHandles: first.flatMarkers!.map((m) => m.handle), + secondHandles: second.flatMarkers!.map((m) => m.handle), + }; + } + + it('warns when a range straddles two threads', async function () { + const { querier, firstHandles, secondHandles } = + await querierWithTwoListings(); + const spec = `${firstHandles[1]}..${secondHandles[0]}`; + + const result = await querier.markerInfoMulti([spec]); + + // Every handle resolves, so this would otherwise look like a success. + expect(result.errors).toEqual([]); + expect(result.rangeSpansThreadsWarning).toEqual({ + ranges: [spec], + threadHandles: ['t-0', 't-1'], + }); + }); + + it('does not warn for a range inside one thread', async function () { + const { querier, firstHandles } = await querierWithTwoListings(); + + const result = await querier.markerInfoMulti([ + `${firstHandles[0]}..${firstHandles[1]}`, + ]); + + expect(result.rangeSpansThreadsWarning).toBeUndefined(); + }); + + it('does not warn when a single-thread range sits next to a handle on another thread', async function () { + const { querier, firstHandles, secondHandles } = + await querierWithTwoListings(); + + // The range is entirely within t-0; only the extra bare handle is on + // t-1. Judging the warning on every resolved marker rather than on the + // range's own markers made this warn about a range that never spanned + // anything. + const spec = `${firstHandles[0]}..${firstHandles[1]}`; + const result = await querier.markerInfoMulti([spec, secondHandles[0]]); + + expect(result.errors).toEqual([]); + expect(result.markers).toHaveLength(3); + expect(result.rangeSpansThreadsWarning).toBeUndefined(); + }); + + it('does not warn for an explicit list of handles from two threads', async function () { + const { querier, firstHandles, secondHandles } = + await querierWithTwoListings(); + + // Typing both handles out is a deliberate comparison, not an accident + // of numbering, so it must not be second-guessed. + const result = await querier.markerInfoMulti([ + firstHandles[0], + secondHandles[0], + ]); + + expect(result.markers).toHaveLength(2); + expect(result.rangeSpansThreadsWarning).toBeUndefined(); + }); + }); + }); });