Skip to content

Commit 9a8edfd

Browse files
Karakatiza666Karakatiza666
committed
[profiler-lib] Determine root node id dynamically instead of assuming it's matches the hard-coded constant
Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
1 parent 749a1ad commit 9a8edfd

9 files changed

Lines changed: 54 additions & 25 deletions

File tree

‎js-packages/profiler-layout/src/lib/components/MetricsView.svelte‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<script lang="ts" module>
2-
import { type NodeAttributes, TOP_NODE_ID } from 'profiler-lib'
2+
import type { NodeAttributes } from 'profiler-lib'
33
44
export type MetricsMode = 'overview' | 'node' | 'top-nodes'
55
@@ -10,9 +10,13 @@
1010
]
1111
1212
/** The toplevel node represents the whole circuit (the overview) rather than a single operator.
13-
* Reused wherever we need to distinguish overview data from a single node. */
14-
export function isOverviewAttributes(nodeAttributes: NodeAttributes): boolean {
15-
return nodeAttributes.nodeId === TOP_NODE_ID
13+
* `rootNodeId` is the loaded profile's actual toplevel id; while it is `undefined` (no profile
14+
* yet) nothing counts as the overview. */
15+
export function isOverviewAttributes(
16+
nodeAttributes: NodeAttributes,
17+
rootNodeId: string | undefined
18+
): boolean {
19+
return rootNodeId !== undefined && nodeAttributes.nodeId === rootNodeId
1620
}
1721
1822
/** The node id is what `search()` matches against, so it's the query that links back to the
@@ -31,6 +35,8 @@
3135
interface Props {
3236
mode: MetricsMode
3337
tooltipData: TooltipData | null
38+
/** The loaded profile's toplevel node id, used to recognise overview data. */
39+
rootNodeId: string | undefined
3440
/** When true, metrics flagged `advanced` in the profile metadata are included. */
3541
showAdvanced: boolean
3642
/** Lookup coordinator; the view registers an imperative handler so each Enter on the
@@ -41,13 +47,15 @@
4147
onSearchNode?: (query: string) => void
4248
}
4349
44-
const { mode, tooltipData, showAdvanced, lookup, onSearchNode }: Props = $props()
50+
const { mode, tooltipData, rootNodeId, showAdvanced, lookup, onSearchNode }: Props = $props()
4551
4652
const nodeAttributes = $derived(
4753
tooltipData && 'nodeAttributes' in tooltipData ? tooltipData.nodeAttributes : null
4854
)
4955
// Single-node data (a specific operator) as opposed to the whole-circuit overview.
50-
const isNodeView = $derived(nodeAttributes ? !isOverviewAttributes(nodeAttributes) : false)
56+
const isNodeView = $derived(
57+
nodeAttributes ? !isOverviewAttributes(nodeAttributes, rootNodeId) : false
58+
)
5159
const identityRows = $derived(
5260
nodeAttributes && isNodeView
5361
? idAttributes.flatMap((r) => {

‎js-packages/profiler-layout/src/lib/components/SupportBundleViewerLayout.svelte‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,8 @@
7373
}: Props = $props()
7474
7575
let profilerDiagram: ProfilerDiagram | undefined = $state()
76+
// The loaded profile's toplevel node id, so the analysis panel can recognise overview data.
77+
const diagramRootNodeId = $derived(profilerDiagram?.getProfile()?.rootNodeId)
7678
let tooltipData: TooltipData | null = $state(null)
7779
// Most recently inspected operator; the "Node" segment restores it via `setMetricsMode('node')`.
7880
let lastNodeData: NodeAttributes | null = $state(null)
@@ -139,7 +141,7 @@
139141
onNodeClick: (nodeId) => {
140142
// The only path outside `setMetricsMode` that switches to "Node"; `displayNodeAttributes`
141143
// fires right after with the payload.
142-
analysisView = { node: nodeId }
144+
analysisView = { nodeId }
143145
currentTab = 'Metrics'
144146
},
145147
displayTopNodes(data, _isSticky) {
@@ -221,7 +223,7 @@
221223
analysisView = 'top-nodes'
222224
profilerDiagram?.showTopNodes(true)
223225
} else if (mode === 'node' && lastNodeData) {
224-
analysisView = { node: lastNodeData.nodeId }
226+
analysisView = { nodeId: lastNodeData.nodeId }
225227
tooltipData = { nodeAttributes: lastNodeData }
226228
}
227229
}
@@ -258,6 +260,7 @@
258260
const analysisTabProps = $derived<AnalysisTabProps>({
259261
metricsMode,
260262
tooltipData,
263+
rootNodeId: diagramRootNodeId,
261264
showAdvancedMetrics,
262265
lookup,
263266
logText,

‎js-packages/profiler-layout/src/lib/components/metrics/dispatch.test.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ function row(metric: string): TooltipRow {
1111
function makeAttrs(metrics: string[]): NodeAttributes {
1212
return {
1313
title: 'n op',
14+
nodeId: 'n',
1415
columns: [],
1516
rows: metrics.map(row),
1617
attributes: new Map()

‎js-packages/profiler-layout/src/lib/components/tabs/MetricsTab.svelte‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@
99
export type AnalysisTabProps = {
1010
metricsMode: MetricsMode
1111
tooltipData: TooltipData | null
12+
/** Id of the loaded profile's toplevel node, so the Metrics view can tell the overview from a
13+
* single operator. `undefined` until a profile is loaded. */
14+
rootNodeId: string | undefined
1215
/** When true, metrics flagged `advanced` in the profile metadata are shown too. */
1316
showAdvancedMetrics: boolean
1417
lookup: LookupCoordinator
@@ -30,6 +33,7 @@
3033
let {
3134
metricsMode,
3235
tooltipData,
36+
rootNodeId,
3337
showAdvancedMetrics,
3438
lookup,
3539
onSearchNode
@@ -39,6 +43,7 @@
3943
<MetricsView
4044
mode={metricsMode}
4145
{tooltipData}
46+
{rootNodeId}
4247
showAdvanced={showAdvancedMetrics}
4348
{lookup}
4449
{onSearchNode}

‎js-packages/profiler-layout/src/lib/functions/metricsMode.test.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,24 +8,24 @@ describe('metricsModeOf', () => {
88
})
99

1010
it("maps any `{ node }` to 'node' regardless of the operator id", () => {
11-
expect(metricsModeOf({ node: '42' })).toBe('node')
12-
expect(metricsModeOf({ node: 'persistent-id-xyz' })).toBe('node')
11+
expect(metricsModeOf({ nodeId: '42' })).toBe('node')
12+
expect(metricsModeOf({ nodeId: 'persistent-id-xyz' })).toBe('node')
1313
})
1414

1515
// Regression for the old title-based guess: the mode used to be inferred from
1616
// `tooltipData.title === 'n region'`, so any profile whose root id wasn't `n` kept showing
1717
// "Node" by mistake. Reading the mode straight from the `AnalysisView` removes that guesswork.
1818
it('reads the mode from the `AnalysisView` for any root id', () => {
19-
const seq: AnalysisView[] = ['overview', { node: 'root-42' }, 'top-nodes', 'overview']
19+
const seq: AnalysisView[] = ['overview', { nodeId: 'root-42' }, 'top-nodes', 'overview']
2020
expect(seq.map(metricsModeOf)).toEqual(['overview', 'node', 'top-nodes', 'overview'])
2121
})
2222
})
2323

2424
describe('isNodeView', () => {
2525
it('narrows to `{ node }` and returns false for string variants', () => {
26-
const v: AnalysisView = { node: 'op-7' }
26+
const v: AnalysisView = { nodeId: 'op-7' }
2727
if (isNodeView(v)) {
28-
expect(v.node).toBe('op-7')
28+
expect(v.nodeId).toBe('op-7')
2929
} else {
3030
throw new Error('expected isNodeView to narrow')
3131
}

‎js-packages/profiler-layout/src/lib/functions/metricsMode.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,13 @@ import type { MetricsMode } from '../components/MetricsView.svelte'
66
/** Which view the analysis panel is currently showing: the whole-circuit overview, the ranked
77
* top nodes, or a single operator's metrics. When one operator is selected, its id is stored so the
88
* panel can return to that same operator later. */
9-
export type AnalysisView = 'overview' | 'top-nodes' | { node: string }
9+
export type AnalysisView = 'overview' | 'top-nodes' | { nodeId: string }
1010

1111
export function metricsModeOf(view: AnalysisView): MetricsMode {
1212
return typeof view === 'string' ? view : 'node'
1313
}
1414

15-
/** Accepts `null` so "is there a node to restore?" is a single call. */
16-
export function isNodeView(view: AnalysisView | null): view is { node: string } {
15+
/** Accepts `null` so "is there a nodeId to restore?" is a single call. */
16+
export function isNodeView(view: AnalysisView | null): view is { nodeId: string } {
1717
return view !== null && typeof view === 'object'
1818
}

‎js-packages/profiler-lib/src/index.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ export {
1919
CircuitProfile,
2020
PropertyValue,
2121
MissingValue,
22-
TOP_NODE_ID,
2322
type JsonProfiles
2423
} from './profile.js';
2524
export { type Dataflow, type SourcePositionRange } from './dataflow.js';

‎js-packages/profiler-lib/src/profile.test.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest'
22
import {
33
BooleanValue,
44
BytesValue,
5+
CircuitProfile,
56
CountValue,
67
MissingValue,
78
PercentValue,
@@ -10,6 +11,22 @@ import {
1011
TimeValue
1112
} from './profile.js'
1213

14+
describe('CircuitProfile.isTop', () => {
15+
it('recognises the toplevel node by the parsed root id', () => {
16+
const profile = new CircuitProfile(1, 'n')
17+
expect(profile.isTop('n')).toBe(true)
18+
expect(profile.isTop('n0')).toBe(false)
19+
expect(profile.isTop('42')).toBe(false)
20+
})
21+
22+
it('does not assume the root id is "n"', () => {
23+
// The format conventionally emits "n", but isTop must follow whatever the profile carries.
24+
const profile = new CircuitProfile(1, 'root-7')
25+
expect(profile.isTop('root-7')).toBe(true)
26+
expect(profile.isTop('n')).toBe(false)
27+
})
28+
})
29+
1330
describe('CountValue', () => {
1431
it('toString formats whole numbers with K/M/B suffixes', () => {
1532
expect(new CountValue(0).toString()).toBe('0')

‎js-packages/profiler-lib/src/profile.ts‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,6 @@ import { type MirNode, SourcePositionRanges, SourcePositionRange, Sources, type
66
type JsonMeasurement = Array<any>;
77
export type NodeId = string;
88

9-
/** Id of the toplevel profile graph node. Every profile graph is a single node containing the
10-
* whole circuit; its metrics are the circuit-wide overview rather than one operator's. The id is
11-
* fixed by the profile format. */
12-
export const TOP_NODE_ID: NodeId = "n";
13-
149
export type CircuitMetricCategory = string;
1510

1611
export interface ProfileMetricDescription {
@@ -1336,7 +1331,7 @@ export class CircuitProfile {
13361331
* Profile graphs are always a single node containing everything else inside.
13371332
* That node is pretty much ignored everywhere else in this code after parsing. */
13381333
isTop(node: NodeId): boolean {
1339-
return node === TOP_NODE_ID;
1334+
return node === this.rootNodeId;
13401335
}
13411336

13421337
addNode(n: JsonSimpleNodeWrapper | JsonClusterWrapper, parent: Option<NodeId>) {
@@ -1534,7 +1529,8 @@ export class CircuitProfile {
15341529
return Option.some(ranges[0]!);
15351530
}
15361531

1537-
constructor(readonly worker_count: number) { }
1532+
/** @param rootNodeId Id of the toplevel graph node, taken from the parsed profile. */
1533+
constructor(readonly worker_count: number, readonly rootNodeId: NodeId) { }
15381534

15391535
// Scan the nodes and compute the range of each property
15401536
computePropertyRanges() {
@@ -1607,7 +1603,7 @@ export class CircuitProfile {
16071603
// Decode the graph structure and create the nodes.
16081604
// The graph itself is always a complex node.
16091605
let rootNodeId = json.graph.nodes.id;
1610-
let result = new CircuitProfile(worker_count);
1606+
let result = new CircuitProfile(worker_count, rootNodeId);
16111607
result.complexNodes.set(rootNodeId,
16121608
new ComplexNode(rootNodeId, json.graph.nodes.label, worker_count));
16131609
for (const nodeWrapper of json.graph.nodes.nodes) {

0 commit comments

Comments
 (0)