Split CallTreeNodes that are non-contiguous (#123)

This PR exposed some bugs in the Firefox importer, which is also fixed in this PR.

Fixes #86

src/import/firefox.ts

@@ -158,7 +158,7 @@ export function importFromFirefox(firefoxProfile: FirefoxProfile): Profile {
       ? cpuProfile.threads[0]
       : cpuProfile.threads.filter(t => t.name === 'GeckoMain')[0]
 
-  const frameIdToFrameInfo = new Map<number, FrameInfo>()
+  const frameKeyToFrameInfo = new Map<string, FrameInfo>()
 
   function extractStack(sample: Sample): FrameInfo[] {
     let stackFrameId: number | null = sample[0]
@@ -185,7 +185,7 @@ export function importFromFirefox(firefoxProfile: FirefoxProfile): Profile {
           return null
         }
 
-        return getOrInsert(frameIdToFrameInfo, f, () => ({
+        return getOrInsert(frameKeyToFrameInfo, location, () => ({
           key: location,
           name: match[1]!,
           file: match[2]!,
@@ -205,24 +205,22 @@ export function importFromFirefox(firefoxProfile: FirefoxProfile): Profile {
     // Find lowest common ancestor of the current stack and the previous one
     let lca: FrameInfo | null = null
 
-    // This is O(n^2), but n should be relatively small here (stack height),
-    // so hopefully this isn't much of a problem
-    for (let i = stack.length - 1; i >= 0 && prevStack.indexOf(stack[i]) === -1; i--) {}
+    for (let i = 0; i < Math.min(stack.length, prevStack.length); i++) {
+      if (prevStack[i] !== stack[i]) {
+        break
+      }
+      lca = stack[i]
+    }
 
     // Close frames that are no longer open
-    while (prevStack.length > 0 && lastOf(prevStack) != lca) {
+    while (prevStack.length > 0 && (!lca || lastOf(prevStack) !== lca)) {
       const closingFrame = prevStack.pop()!
       profile.leaveFrame(closingFrame, value)
     }
 
     // Open frames that are now becoming open
-    const toOpen: FrameInfo[] = []
-    for (let i = stack.length - 1; i >= 0 && stack[i] != lca; i--) {
-      toOpen.push(stack[i])
-    }
-    toOpen.reverse()
-
-    for (let frame of toOpen) {
+    for (let i = lca ? stack.indexOf(lca) + 1 : 0; i < stack.length; i++) {
+      const frame = stack[i]
       profile.enterFrame(frame, value)
     }
 

src/lib/__snapshots__/profile.test.ts.snap

@@ -0,0 +1,47 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`CallTreeProfileBuilder separates non-contiguous: append order 1`] = `
+"((speedscope root):0:5
+  (a:1:5
+    (b:1:2
+      (c:1:1)
+    )
+    (b:1:2
+      (c:1:1)
+    )
+  )
+)"
+`;
+
+exports[`CallTreeProfileBuilder separates non-contiguous: grouped 1`] = `
+"((speedscope root):0:5
+  (a:1:5
+    (b:2:4
+      (c:2:2)
+    )
+  )
+)"
+`;
+
+exports[`StackListProfileBuilder separates non-contiguous: append order 1`] = `
+"((speedscope root):4:0
+  (a:1:5
+    (b:1:2
+      (c:1:1)
+    )
+    (b:1:2
+      (c:1:1)
+    )
+  )
+)"
+`;
+
+exports[`StackListProfileBuilder separates non-contiguous: grouped 1`] = `
+"((speedscope root):4:0
+  (a:1:5
+    (b:2:4
+      (c:2:2)
+    )
+  )
+)"
+`;

src/lib/profile.test.ts

@@ -50,6 +50,30 @@ function toStackList(profile: Profile, grouped: boolean): string[] {
   return stackList
 }
 
+function flatten<T>(ts: T[][]): T[] {
+  let ret: T[] = []
+  return ret.concat(...ts)
+}
+
+function toTreeString(profile: Profile, grouped: boolean): string {
+  function visit(node: CallTreeNode): string[] {
+    const childLines = flatten(node.children.map(child => visit(child))).map(l => `  ${l}`)
+    const nodeStr = `${node.frame.key}:${node.getSelfWeight()}:${node.getTotalWeight()}`
+
+    if (childLines.length > 0) {
+      return [`(${nodeStr}`].concat(childLines).concat(')')
+    } else {
+      return [`(${nodeStr})`]
+    }
+  }
+
+  if (grouped) {
+    return visit(profile.getGroupedCalltreeRoot()).join('\n')
+  } else {
+    return visit(profile.getAppendOrderCalltreeRoot()).join('\n')
+  }
+}
+
 function verifyProfile(profile: Profile) {
   const allFrameKeys = new Set([fa, fb, fc, fd, fe].map(f => f.key))
   const framesInProfile = new Set<string | number>()
@@ -133,6 +157,28 @@ test('StackListProfileBuilder', () => {
   verifyProfile(profile)
 })
 
+test('StackListProfileBuilder separates non-contiguous', () => {
+  const b = new StackListProfileBuilder()
+
+  const samples = [
+    // prettier-ignore
+    [fa, fb, fc],
+    [fa, fb],
+    [fa],
+    [fa, fb],
+    [fa, fb, fc],
+  ]
+
+  samples.forEach(stack => {
+    b.appendSample(stack, 1)
+  })
+  b.appendSample([], 4)
+  const profile = b.build()
+
+  expect(toTreeString(profile, true)).toMatchSnapshot('grouped')
+  expect(toTreeString(profile, false)).toMatchSnapshot('append order')
+})
+
 test('CallTreeProfileBuilder', () => {
   const b = new CallTreeProfileBuilder()
 
@@ -167,6 +213,30 @@ test('CallTreeProfileBuilder', () => {
   verifyProfile(profile)
 })
 
+test('CallTreeProfileBuilder separates non-contiguous', () => {
+  const b = new CallTreeProfileBuilder()
+
+  b.enterFrame(fa, 0)
+  b.enterFrame(fb, 0)
+  b.enterFrame(fc, 0)
+
+  b.leaveFrame(fc, 1)
+
+  b.leaveFrame(fb, 2)
+
+  b.enterFrame(fb, 3)
+
+  b.enterFrame(fc, 4)
+
+  b.leaveFrame(fc, 5)
+  b.leaveFrame(fb, 5)
+  b.leaveFrame(fa, 5)
+
+  const profile = b.build()
+  expect(toTreeString(profile, true)).toMatchSnapshot('grouped')
+  expect(toTreeString(profile, false)).toMatchSnapshot('append order')
+})
+
 test('getInvertedProfileForCallersOf', () => {
   const b = new StackListProfileBuilder()
 

src/lib/profile.ts

@@ -89,6 +89,15 @@ export class CallTreeNode extends HasWeights {
     return this.frame === Frame.root
   }
 
+  // If a node is "frozen", it means it should no longer be mutated.
+  private frozen = false
+  isFrozen() {
+    return this.frozen
+  }
+  freeze() {
+    this.frozen = true
+  }
+
   constructor(readonly frame: Frame, readonly parent: CallTreeNode | null) {
     super()
   }
@@ -100,9 +109,17 @@ export class Profile {
   protected totalWeight: number
 
   protected frames = new KeyedSet<Frame>()
+
   protected appendOrderCalltreeRoot = new CallTreeNode(Frame.root, null)
   protected groupedCalltreeRoot = new CallTreeNode(Frame.root, null)
 
+  public getAppendOrderCalltreeRoot() {
+    return this.appendOrderCalltreeRoot
+  }
+  public getGroupedCalltreeRoot() {
+    return this.groupedCalltreeRoot
+  }
+
   // List of references to CallTreeNodes at the top of the
   // stack at the time of the sample.
   protected samples: CallTreeNode[] = []
@@ -395,7 +412,7 @@ export class StackListProfileBuilder extends Profile {
       const last = useAppendOrder
         ? lastOf(node.children)
         : node.children.find(c => c.frame === frame)
-      if (last && last.frame == frame) {
+      if (last && !last.isFrozen() && last.frame == frame) {
         node = last
       } else {
         const parent = node
@@ -414,6 +431,12 @@ export class StackListProfileBuilder extends Profile {
     }
     node.addToSelfWeight(weight)
 
+    if (useAppendOrder) {
+      for (let child of node.children) {
+        child.freeze()
+      }
+    }
+
     if (useAppendOrder) {
       node.frame.addToSelfWeight(weight)
 
@@ -493,7 +516,7 @@ export class CallTreeProfileBuilder extends Profile {
         ? lastOf(prevTop.children)
         : prevTop.children.find(c => c.frame === frame)
       let node: CallTreeNode
-      if (last && last.frame == frame) {
+      if (last && !last.isFrozen() && last.frame == frame) {
         node = last
       } else {
         node = new CallTreeNode(frame, prevTop)
@@ -520,10 +543,17 @@ export class CallTreeProfileBuilder extends Profile {
 
     if (useAppendOrder) {
       const leavingStackTop = this.appendOrderStack.pop()
-      const delta = value - this.lastValue!
+      if (leavingStackTop == null) {
+        throw new Error(`Trying to leave ${frame.key} when stack is empty`)
+      }
+      if (this.lastValue == null) {
+        throw new Error(`Trying to leave a ${frame.key} before any have been entered`)
+      }
+      leavingStackTop.freeze()
+      const delta = value - this.lastValue
       if (delta > 0) {
-        this.samples.push(leavingStackTop!)
-        this.weights.push(value - this.lastValue!)
+        this.samples.push(leavingStackTop)
+        this.weights.push(value - this.lastValue)
       } else if (delta < 0) {
         throw new Error(
           `Samples must be provided in increasing order of cumulative value. Last sample was ${this