From bd36a5473123824e090e367e0ab7e0735ec65e85 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/README.md | 2 +- profiler-cli/guide.txt | 2 + profiler-cli/schemas.txt | 12 +- profiler-cli/src/commands/marker.ts | 51 ++++- profiler-cli/src/commands/shared.ts | 9 +- profiler-cli/src/daemon.ts | 16 +- profiler-cli/src/formatters.ts | 60 +++++- profiler-cli/src/output.ts | 3 + profiler-cli/src/protocol.ts | 6 + .../src/test/integration/marker-info.test.ts | 204 ++++++++++++++++++ .../src/test/unit/marker-formatting.test.ts | 158 +++++++++++++- src/profile-query/formatters/marker-info.ts | 14 +- src/profile-query/index.ts | 82 ++++++- src/profile-query/marker-map.ts | 93 ++++++++ src/profile-query/types.ts | 20 ++ .../unit/profile-query/marker-utils.test.ts | 102 ++++++++- .../profile-query/profile-querier.test.ts | 152 +++++++++++++ 17 files changed, 957 insertions(+), 29 deletions(-) create mode 100644 profiler-cli/src/test/integration/marker-info.test.ts 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(); + }); + }); + }); });