From 91a1cd2ab1d1f040b0e7a27f00c3436889cc437b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florian=20Qu=C3=A8ze?= Date: Tue, 18 Aug 2026 10:23:28 +0200 Subject: [PATCH] profiler-cli: accept several marker handles and ranges in marker info Inspecting a sequence of markers meant one invocation per handle. `marker info` now takes a list of handles, comma-separated handles, and m-1..m-3 ranges in any combination, returning a marker-info-multi result for two or more markers. Any spelling meaning exactly one marker keeps the previous text and JSON output byte for byte. An unresolvable handle is reported in place with the rest still shown and a non-zero exit; a malformed spec, reversed range, or range wider than 256 handles fails before any lookup. Ranges expand numerically and handle numbering continues across listings, so a range overrunning its listing resolves into unrelated markers. When one lands in more than one thread the CLI warns and exits non-zero. --- profiler-cli/guide.txt | 2 + profiler-cli/schemas.txt | 12 +- profiler-cli/src/commands/marker.ts | 57 +++++- profiler-cli/src/commands/shared.ts | 5 +- profiler-cli/src/daemon.ts | 3 + profiler-cli/src/formatters.ts | 60 +++++- profiler-cli/src/output.ts | 3 + profiler-cli/src/protocol.ts | 5 + .../src/test/integration/marker-info.test.ts | 184 ++++++++++++++++++ .../src/test/unit/marker-formatting.test.ts | 158 ++++++++++++++- src/profile-query/index.ts | 60 +++++- src/profile-query/marker-map.ts | 83 ++++++++ src/profile-query/types.ts | 20 ++ .../unit/profile-query/marker-utils.test.ts | 98 +++++++++- .../profile-query/profile-querier.test.ts | 136 +++++++++++++ 15 files changed, 871 insertions(+), 15 deletions(-) create mode 100644 profiler-cli/src/test/integration/marker-info.test.ts diff --git a/profiler-cli/guide.txt b/profiler-cli/guide.txt index 4b401645db..e08c51f838 100644 --- a/profiler-cli/guide.txt +++ b/profiler-cli/guide.txt @@ -98,6 +98,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 b5d830ea88..b1844fe9c9 100644 --- a/profiler-cli/schemas.txt +++ b/profiler-cli/schemas.txt @@ -216,7 +216,7 @@ profiler-cli thread network --json context: SessionContext } -profiler-cli marker info --json +profiler-cli marker info --json { type: "marker-info", threadHandle, friendlyThreadName, markerHandle, markerIndex, name, @@ -226,6 +226,16 @@ profiler-cli marker info --json stack?: { frames: [{ name, nameWithLibrary }], truncated } } +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..45dc2210be 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 '../../../src/profile-query/marker-map'; import { addGlobalOptions, runCommand } from './shared'; export function registerMarkerCommand( @@ -17,16 +18,50 @@ 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 + ); + + // Route on the expanded handles, not the raw text, so that every spelling + // of one marker ("m-1,", "m-1..m-1") keeps the old single-marker `--json` + // shape. Malformed specs are left for the daemon to report. + let expanded: string[] | undefined; + try { + expanded = expandMarkerHandleSpecs(specs); + } catch { + expanded = undefined; + } + if (expanded && expanded.length === 1) { + await runCommand( + sessionDir, + { command: 'marker', subcommand: 'info', marker: expanded[0] }, + opts + ); + return; + } + + 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 +71,14 @@ export function registerMarkerCommand( .option('--marker ', 'Marker handle') ).action(async (handleArg: string | undefined, opts) => { const markerHandle = handleArg ?? opts.marker; + // Without this, a range reaches the daemon and comes back as "Unknown + // marker m-1..m-3", which reads like a bad handle, not bad syntax. + if (typeof markerHandle === 'string' && /\.\.|,/.test(markerHandle)) { + 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 357ff3d5da..19d5c5b823 100644 --- a/profiler-cli/src/commands/shared.ts +++ b/profiler-cli/src/commands/shared.ts @@ -11,7 +11,7 @@ import { Option } from 'commander'; import { collectStrings } from '../utils/parse'; import { sendCommand } from '../client'; import { formatOutput } from '../output'; -import type { ClientCommand } from '../protocol'; +import type { ClientCommand, CommandResult } from '../protocol'; /** * Options shared by every command action via `addGlobalOptions`. @@ -30,9 +30,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 af22fa2c4c..4a6bd9bf3f 100644 --- a/profiler-cli/src/daemon.ts +++ b/profiler-cli/src/daemon.ts @@ -465,6 +465,9 @@ export class Daemon { case 'marker': switch (command.subcommand) { case 'info': + if (command.markers && command.markers.length > 0) { + return this.querier.markerInfoMulti(command.markers); + } if (!command.marker) { throw new Error('marker handle required for marker info'); } diff --git a/profiler-cli/src/formatters.ts b/profiler-cli/src/formatters.ts index 11557e19a9..e3afb681fd 100644 --- a/profiler-cli/src/formatters.ts +++ b/profiler-cli/src/formatters.ts @@ -19,6 +19,7 @@ import type { ThreadInfoResult, MarkerStackResult, MarkerInfoResult, + MarkerInfoMultiResult, ProfileInfoResult, ProfileMetaResult, ThreadSamplesResult, @@ -339,9 +340,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 da258a7a29..e52d406990 100644 --- a/profiler-cli/src/output.ts +++ b/profiler-cli/src/output.ts @@ -18,6 +18,7 @@ import { formatThreadInfoResult, formatMarkerStackResult, formatMarkerInfoResult, + formatMarkerInfoMultiResult, formatProfileInfoResult, formatProfileMetaResult, formatThreadSamplesResult, @@ -73,6 +74,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 98aa1b4986..8e2531e9bb 100644 --- a/profiler-cli/src/protocol.ts +++ b/profiler-cli/src/protocol.ts @@ -50,6 +50,7 @@ export type { RateStats, MarkerGroupData, MarkerInfoResult, + MarkerInfoMultiResult, MarkerStackResult, StackTraceData, ProfileInfoResult, @@ -81,6 +82,7 @@ import type { ThreadInfoResult, MarkerStackResult, MarkerInfoResult, + MarkerInfoMultiResult, ProfileInfoResult, ProfileMetaResult, ThreadSamplesResult, @@ -164,6 +166,8 @@ export type ClientCommand = command: 'marker'; subcommand: 'info' | 'select' | 'stack'; marker?: string; + /** Set instead of `marker` for several handles, e.g. ["m-42", "m-50..m-53"]. */ + markers?: string[]; } | { command: 'counter'; @@ -222,6 +226,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..9087741d64 --- /dev/null +++ b/profiler-cli/src/test/integration/marker-info.test.ts @@ -0,0 +1,184 @@ +/* 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 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' + ); + // The old message read like a bad handle rather than unsupported syntax. + 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 2356cef22a..daec4cc8f0 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'; @@ -185,3 +190,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/index.ts b/src/profile-query/index.ts index c752ee4c74..790afa7153 100644 --- a/src/profile-query/index.ts +++ b/src/profile-query/index.ts @@ -62,7 +62,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'; @@ -107,6 +107,7 @@ import type { ThreadInfoResult, MarkerStackResult, MarkerInfoResult, + MarkerInfoMultiResult, ProfileInfoResult, ProfileMetaResult, ThreadSamplesResult, @@ -1320,6 +1321,63 @@ export class ProfileQuerier { return { ...result, context: this._getContext() }; } + /** + * 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. + let rangeSpansThreadsWarning: + | MarkerInfoMultiResult['rangeSpansThreadsWarning'] + | undefined; + if (ranges.length > 0) { + const threadHandles: string[] = []; + for (const marker of markers) { + if (!threadHandles.includes(marker.threadHandle)) { + threadHandles.push(marker.threadHandle); + } + } + if (threadHandles.length > 1) { + rangeSpansThreadsWarning = { ranges, threadHandles }; + } + } + + 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..c2ab5aa153 100644 --- a/src/profile-query/marker-map.ts +++ b/src/profile-query/marker-map.ts @@ -68,3 +68,86 @@ 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; +} + +/** + * As `expandMarkerHandleSpecs`, but also reports which specs were multi-element + * ranges, so a caller can check what they resolved to. + */ +export function expandMarkerHandleSpecsDetailed(specs: string[]): { + handles: string[]; + ranges: string[]; +} { + const handles: string[] = []; + const ranges: string[] = []; + 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.` + ); + } + if (end > start) { + ranges.push(trimmed); + } + for (let id = start; id <= end; id++) { + push(`m-${id}`); + } + 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 feb0d83dc8..afc5ba1c4f 100644 --- a/src/profile-query/types.ts +++ b/src/profile-query/types.ts @@ -702,6 +702,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 401333c1f7..83ecc9dff1 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,97 @@ 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. + expect(ranges).toEqual(['m-4..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 c4a8c58beb..634b47965e 100644 --- a/src/test/unit/profile-query/profile-querier.test.ts +++ b/src/test/unit/profile-query/profile-querier.test.ts @@ -755,4 +755,140 @@ describe('ProfileQuerier', function () { expect(listedNames).toEqual(['Beta']); }); }); + + 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 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(); + }); + }); + }); });