From e4e344759f312b68eb0463fc40b1a5c06b2e972f Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Fri, 23 Oct 2026 16:53:26 -0400 Subject: [PATCH] Give flame graph siblings with the same function name a defined order. The flame graph ordered siblings by function name only. Siblings with the same name kept the order in which they appear in the call node table, which depends on the stack table. Two similar profiles could therefore show the same functions in opposite orders. Example: in the JS-only flame graph of https://share.firefox.dev/4jy20z8 vs https://share.firefox.dev/4k1tYTW, the call node for callHook in the stack below has two children named "view": view [../../src/components/Controls.js:9:5] callHook [../../node_modules/mithril/render/render.js:34:11] updateComponent [../../node_modules/mithril/render/render.js:462:11] updateNode [../../node_modules/mithril/render/render.js:396:11] updateNodes [../../node_modules/mithril/render/render.js:270:11] updateElement [../../node_modules/mithril/render/render.js:451:11] Both view functions come from the same bundle, so they have the same name and the same file name. The two profiles showed them in opposite orders. Siblings with the same function name are now compared by: 1. The generated file name. 2. The original (source-mapped) file name. 3. The line, then the column, where the function starts. This uses the original location if there is one, and the generated location otherwise. To support this, bisectionRightByStrKey is replaced with bisectionRightBy, which takes a comparison function. computeFlameGraphRows now also takes the SourceTable and the SourceLocationTable. --- src/profile-logic/flame-graph.ts | 103 +++++++++++++++++++- src/selectors/per-thread/stack-sample.ts | 3 + src/test/unit/bisect.test.ts | 44 +++++---- src/test/unit/profile-tree.test.ts | 117 +++++++++++++++++++++++ src/utils/bisect.ts | 17 ++-- 5 files changed, 254 insertions(+), 30 deletions(-) diff --git a/src/profile-logic/flame-graph.ts b/src/profile-logic/flame-graph.ts index 24f8347c5a..508344c679 100644 --- a/src/profile-logic/flame-graph.ts +++ b/src/profile-logic/flame-graph.ts @@ -6,7 +6,11 @@ import type { CallNodeTable, FuncTable, IndexIntoCallNodeTable, + IndexIntoFuncTable, + SourceTable, + SourceLocationTable, } from 'firefox-profiler/types'; +import { FuncFlag } from 'firefox-profiler/types'; import type { StringTable } from 'firefox-profiler/utils/string-table'; import type { CallTreeTimingsInverted, @@ -14,7 +18,7 @@ import type { } from './call-tree'; import type { CallNodeInfoInverted } from './call-node-info'; -import { bisectionRightByStrKey } from 'firefox-profiler/utils/bisect'; +import { bisectionRightBy } from 'firefox-profiler/utils/bisect'; export type FlameGraphDepth = number; export type IndexIntoFlameGraphTiming = number; @@ -175,7 +179,9 @@ class FlameGraphTimingNonInverted implements FlameGraphTiming { * The call nodes are ordered in the same order that they'll be displayed in the * flame graph: * - * - Siblings are ordered by function name. + * - Siblings are ordered by function name. Siblings with the same function + * name are ordered by file name, then by original (source-mapped) file + * name, then by the line and column where the function starts. * - Siblings are grouped, i.e. all nodes with the same prefix are next to each other. * - The order of these groups with respect to each other is determined by how * their prefix call nodes are ordered in the previous row. @@ -204,12 +210,24 @@ class FlameGraphTimingNonInverted implements FlameGraphTiming { */ export type FlameGraphRows = IndexIntoCallNodeTable[][]; +function compareStrings(a: string, b: string): number { + if (a < b) { + return -1; + } + if (a > b) { + return 1; + } + return 0; +} + /** * Compute the FlameGraphRows. The result is independent of timing information. */ export function computeFlameGraphRows( callNodeTable: CallNodeTable, funcTable: FuncTable, + sources: SourceTable, + sourceLocationTable: SourceLocationTable, stringTable: StringTable ): FlameGraphRows { if (callNodeTable.length === 0) { @@ -219,6 +237,72 @@ export function computeFlameGraphRows( const { func, nextSibling, subtreeRangeEnd, maxDepth } = callNodeTable; const funcTableNameColumn = funcTable.name; + // Returns the file name of a function, or the empty string if the function + // has no source. + function getFuncFileName(funcIndex: IndexIntoFuncTable): string { + if ((funcTable.flags[funcIndex] & FuncFlag.HasSource) === 0) { + return ''; + } + return stringTable.getString(sources.filename[funcTable.source[funcIndex]]); + } + + function hasOriginalLocation(funcIndex: IndexIntoFuncTable): boolean { + return (funcTable.flags[funcIndex] & FuncFlag.HasOriginalLocation) !== 0; + } + + // Returns the original (source-mapped) file name of a function, or the empty + // string if the function has no original location. + function getFuncOriginalFileName(funcIndex: IndexIntoFuncTable): string { + if (!hasOriginalLocation(funcIndex)) { + return ''; + } + const location = funcTable.originalLocation[funcIndex]; + return stringTable.getString( + sources.filename[sourceLocationTable.source[location]] + ); + } + + // Returns the line where a function starts, preferring the original + // location. Returns 0 if the line is unknown. + function getFuncStartLine(funcIndex: IndexIntoFuncTable): number { + if (hasOriginalLocation(funcIndex)) { + return sourceLocationTable.line[funcTable.originalLocation[funcIndex]]; + } + if ((funcTable.flags[funcIndex] & FuncFlag.HasLine) !== 0) { + return funcTable.lineNumber[funcIndex]; + } + return 0; + } + + // Returns the column where a function starts, preferring the original + // location. Returns 0 if the column is unknown. + function getFuncStartColumn(funcIndex: IndexIntoFuncTable): number { + if (hasOriginalLocation(funcIndex)) { + return sourceLocationTable.column[funcTable.originalLocation[funcIndex]]; + } + if ((funcTable.flags[funcIndex] & FuncFlag.HasColumn) !== 0) { + return funcTable.columnNumber[funcIndex]; + } + return 0; + } + + // Compares two functions with the same name, so that siblings with the same + // function name have a defined order. + function compareFuncsWithSameName( + funcA: IndexIntoFuncTable, + funcB: IndexIntoFuncTable + ): number { + return ( + compareStrings(getFuncFileName(funcA), getFuncFileName(funcB)) || + compareStrings( + getFuncOriginalFileName(funcA), + getFuncOriginalFileName(funcB) + ) || + getFuncStartLine(funcA) - getFuncStartLine(funcB) || + getFuncStartColumn(funcA) - getFuncStartColumn(funcB) + ); + } + // flameGraphRows is what we'll return from this function. // // Each row is conceptually partitioned into two parts: "Finished nodes" and @@ -269,13 +353,22 @@ export function computeFlameGraphRows( } else { // There are other siblings already present in rowAtThisDepth[siblingIndexRangeStart..]. // Do an ordered insert, to keep siblings ordered by function name. + // Siblings with the same function name are compared further by + // compareFuncsWithSameName. // assert(siblingIndexRangeStart < siblingIndexRangeEnd) const thisFunc = func[currentSibling]; const funcName = stringTable.getString(funcTableNameColumn[thisFunc]); - const insertionIndex = bisectionRightByStrKey( + const insertionIndex = bisectionRightBy( rowAtThisDepth, - funcName, - (cn) => stringTable.getString(funcTableNameColumn[func[cn]]), + (cn) => { + const siblingFunc = func[cn]; + return ( + compareStrings( + stringTable.getString(funcTableNameColumn[siblingFunc]), + funcName + ) || compareFuncsWithSameName(siblingFunc, thisFunc) + ); + }, siblingIndexRangeStart, siblingIndexRangeEnd ); diff --git a/src/selectors/per-thread/stack-sample.ts b/src/selectors/per-thread/stack-sample.ts index 63347c19c1..9f02a570ac 100644 --- a/src/selectors/per-thread/stack-sample.ts +++ b/src/selectors/per-thread/stack-sample.ts @@ -597,6 +597,9 @@ export function getStackAndSampleSelectorsPerThread( const getFlameGraphRows: Selector = createSelector( (state: State) => _getNonInvertedCallNodeInfo(state).getCallNodeTable(), (state: State) => threadSelectors.getFilteredThread(state).funcTable, + (state: State) => threadSelectors.getFilteredThread(state).sources, + (state: State) => + threadSelectors.getFilteredThread(state).sourceLocationTable, (state: State) => threadSelectors.getFilteredThread(state).stringTable, FlameGraph.computeFlameGraphRows ); diff --git a/src/test/unit/bisect.test.ts b/src/test/unit/bisect.test.ts index 8bdc34cbbb..3e9b38f5b2 100644 --- a/src/test/unit/bisect.test.ts +++ b/src/test/unit/bisect.test.ts @@ -6,7 +6,7 @@ import { bisectionLeft, bisectionRight, bisectionRightByKey, - bisectionRightByStrKey, + bisectionRightBy, } from '../../utils/bisect'; describe('bisectionRight', function () { @@ -114,7 +114,7 @@ describe('bisectionRightByKey', function () { }); }); -describe('bisectionRightByStrKey', function () { +describe('bisectionRightBy', function () { const array = [ { name: 'a0' }, { name: 'a1' }, @@ -131,51 +131,59 @@ describe('bisectionRightByStrKey', function () { { name: 'a9' }, ]; - function getName(x: { name: string }): string { - return x.name; + function compareNameTo(x: string): (elem: { name: string }) => number { + return (elem) => { + if (elem.name < x) { + return -1; + } + if (elem.name > x) { + return 1; + } + return 0; + }; } // The new element to be inserted into this array is referred to as x it('returns index of the first element whose key is greater than x', function () { - expect(bisectionRightByStrKey(array, 'a4', getName)).toBe(8); - expect(bisectionRightByStrKey(array, 'a3', getName)).toBe(8); + expect(bisectionRightBy(array, compareNameTo('a4'))).toBe(8); + expect(bisectionRightBy(array, compareNameTo('a3'))).toBe(8); }); it('returns index of the first element whose key is greater than x, occuring after low', function () { - expect(bisectionRightByStrKey(array, 'a4', getName, 8)).toBe(8); - expect(bisectionRightByStrKey(array, 'a3', getName, 9)).toBe(9); + expect(bisectionRightBy(array, compareNameTo('a4'), 8)).toBe(8); + expect(bisectionRightBy(array, compareNameTo('a3'), 9)).toBe(9); }); it('returns index of the first element whose key is greater than x, between low and high', function () { - expect(bisectionRightByStrKey(array, 'a4', getName, 1, 7)).toBe(7); - expect(bisectionRightByStrKey(array, 'a3', getName, 4, 6)).toBe(6); + expect(bisectionRightBy(array, compareNameTo('a4'), 1, 7)).toBe(7); + expect(bisectionRightBy(array, compareNameTo('a3'), 4, 6)).toBe(6); }); it('returns 0 if all the keys of elements are greater than x', function () { - expect(bisectionRightByStrKey(array, '_', getName)).toBe(0); + expect(bisectionRightBy(array, compareNameTo('_'))).toBe(0); }); it('returns array length if x is greater than keys of all elements', function () { - expect(bisectionRightByStrKey(array, 'b', getName)).toBe(13); + expect(bisectionRightBy(array, compareNameTo('b'))).toBe(13); }); it('throws TypeError if either low or high are outside the range of the array', function () { - expect(() => bisectionRightByStrKey(array, 'b', getName, -2)).toThrow( + expect(() => bisectionRightBy(array, compareNameTo('b'), -2)).toThrow( TypeError ); - expect(() => bisectionRightByStrKey(array, 'b', getName, 100)).toThrow( + expect(() => bisectionRightBy(array, compareNameTo('b'), 100)).toThrow( TypeError ); - expect(() => bisectionRightByStrKey(array, 'b', getName, 2, -10)).toThrow( + expect(() => bisectionRightBy(array, compareNameTo('b'), 2, -10)).toThrow( TypeError ); - expect(() => bisectionRightByStrKey(array, 'b', getName, 2, 100)).toThrow( + expect(() => bisectionRightBy(array, compareNameTo('b'), 2, 100)).toThrow( TypeError ); - expect(() => bisectionRightByStrKey(array, 'b', getName, -20, -10)).toThrow( + expect(() => bisectionRightBy(array, compareNameTo('b'), -20, -10)).toThrow( TypeError ); - expect(() => bisectionRightByStrKey(array, 'b', getName, 100, 200)).toThrow( + expect(() => bisectionRightBy(array, compareNameTo('b'), 100, 200)).toThrow( TypeError ); }); diff --git a/src/test/unit/profile-tree.test.ts b/src/test/unit/profile-tree.test.ts index 48c82e95e1..e892b1e487 100644 --- a/src/test/unit/profile-tree.test.ts +++ b/src/test/unit/profile-tree.test.ts @@ -128,6 +128,8 @@ describe('unfiltered call tree', function () { const rows = computeFlameGraphRows( callNodeInfo.getCallNodeTable(), thread.funcTable, + thread.sources, + thread.sourceLocationTable, thread.stringTable ); expect(rows).toEqual([ @@ -137,6 +139,121 @@ describe('unfiltered call tree', function () { [cnGHI, cnGHJ, cnZXW, cnZXY], ]); }); + + it('orders siblings with the same function name by file name', function () { + const { derivedThreads, defaultCategory } = getProfileFromTextSamples(` + A[file:b.js] A[file:a.js] A + B[file:b.js] B B[file:a.js] + `); + const [thread] = derivedThreads; + const { funcTable, sources, sourceLocationTable, stringTable } = thread; + const callNodeInfo = getCallNodeInfo( + thread.stackTable, + thread.frameTable, + defaultCategory + ); + const callNodeTable = callNodeInfo.getCallNodeTable(); + const rows = computeFlameGraphRows( + callNodeTable, + funcTable, + sources, + sourceLocationTable, + stringTable + ); + + // Describe each call node as "funcName fileName". + const describe = (callNodeIndex: number) => { + const funcIndex = callNodeTable.func[callNodeIndex]; + const name = stringTable.getString(funcTable.name[funcIndex]); + const fileName = + (funcTable.flags[funcIndex] & FuncFlag.HasSource) !== 0 + ? stringTable.getString( + sources.filename[funcTable.source[funcIndex]] + ) + : '(no file)'; + return `${name} ${fileName}`; + }; + expect(rows.map((row) => row.map(describe))).toEqual([ + // Functions without a file come first. + ['A (no file)', 'A a.js', 'A b.js'], + // Children are grouped by their parent, in the parent's order. + ['B a.js', 'B (no file)', 'B b.js'], + ]); + }); + + it('orders siblings from the same file by their original location', function () { + // The fixture merges functions with the same name and file, so create + // them with different names and rename them afterwards. + const { + derivedThreads, + funcNamesDictPerThread: [{ A, view1, view2, view3, view4 }], + defaultCategory, + } = getProfileFromTextSamples(` + A A A A + view1[file:bundle.js] view2[file:bundle.js] view3[file:bundle.js] view4[file:bundle.js] + `); + const [thread] = derivedThreads; + const { funcTable, sources, sourceLocationTable, stringTable } = thread; + const viewNameIndex = stringTable.indexForString('view'); + for (const funcIndex of [view1, view2, view3, view4]) { + funcTable.name[funcIndex] = viewNameIndex; + } + + function addSource(fileName: string): number { + sources.id.push(null); + sources.filename.push(stringTable.indexForString(fileName)); + sources.startLine.push(1); + sources.startColumn.push(1); + sources.sourceMapURL.push(null); + sources.content.push(null); + return sources.length++; + } + function setOriginalLocation( + funcIndex: number, + source: number, + line: number, + column: number + ) { + sourceLocationTable.source.push(source); + sourceLocationTable.line.push(line); + sourceLocationTable.column.push(column); + funcTable.originalLocation[funcIndex] = sourceLocationTable.length++; + funcTable.flags[funcIndex] |= FuncFlag.HasOriginalLocation; + } + const controls = addSource('Controls.js'); + const todoItem = addSource('TodoItem.js'); + setOriginalLocation(view1, todoItem, 3, 1); + setOriginalLocation(view2, controls, 20, 5); + setOriginalLocation(view3, controls, 9, 7); + setOriginalLocation(view4, controls, 9, 5); + + const callNodeInfo = getCallNodeInfo( + thread.stackTable, + thread.frameTable, + defaultCategory + ); + const callNodeTable = callNodeInfo.getCallNodeTable(); + const rows = computeFlameGraphRows( + callNodeTable, + funcTable, + sources, + sourceLocationTable, + stringTable + ); + const cnA = callNodeInfo.getCallNodeIndexFromPath([A]); + const cnForView = (funcIndex: number) => + callNodeInfo.getCallNodeIndexFromPath([A, funcIndex]); + expect(rows).toEqual([ + [cnA], + // Ordered by original file name, then line, then column. + [ + cnForView(view4), + cnForView(view3), + cnForView(view2), + cnForView(view1), + ], + ]); + }); }); /** diff --git a/src/utils/bisect.ts b/src/utils/bisect.ts index a2a1168af8..218d4b46ca 100644 --- a/src/utils/bisect.ts +++ b/src/utils/bisect.ts @@ -157,14 +157,17 @@ export function bisectionRightByKey( } /** - * This is the same as bisectionRightByKey but uses string as the key type. - * If you can find a way to make Flow accept a single function that handles - * both string and number keys, please remove this duplication. + * This is the same as bisectionRightByKey but uses a comparison function. + * cmp(elem) compares an element of the array to the looked-up value, which is + * usually captured by the closure: + * - rv > 0 if elem > value, + * - rv === 0 if elem == value, + * - rv < 0 if elem < value, + * The returned index is the index of the first element for which rv > 0. */ -export function bisectionRightByStrKey( +export function bisectionRightBy( array: T[], - x: string, - toKey: (arg: T) => string, + cmp: (elem: T) => number, low: number = 0, high: number = array.length ): number { @@ -175,7 +178,7 @@ export function bisectionRightByStrKey( while (low < high) { const mid = (low + high) >> 1; - if (x < toKey(array[mid])) { + if (cmp(array[mid]) > 0) { high = mid; } else { low = mid + 1;