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;