Give flame graph siblings with the same function name a defined order

src/profile-logic/flame-graph.ts

@@ -6,15 +6,19 @@ 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,
   CallTreeTimingsNonInverted,
 } 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
         );

src/selectors/per-thread/stack-sample.ts

@@ -597,6 +597,9 @@ export function getStackAndSampleSelectorsPerThread(
   const getFlameGraphRows: Selector<FlameGraph.FlameGraphRows> = 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
   );

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
     );
   });

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),
+        ],
+      ]);
+    });
   });
 
   /**