1164325 - Uninverted tree does not calculate frames with same key uniquely

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: perfperf

In the attachment, you can see `AbstractTreeItem.p.attachTo` called twice (and more below that), with different samples/total numbers, but the self cost and time are identical. This should not be the case.

Ideally, I'd love some kind of recursion detection for patterns (I know we have flatten recursion, but that doesn't handle [A()->B()->C()]*), but absent of that, we should probably have different self costs for these frames maybe? Or have an "aggregate cost" or something like we mentioned for the flame graphs.

Not sure what the best thing to do here, just wanted to get the ball rolling.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: not recursive, but frames found elsewhere

So in this case, it's not recursive, but many of these gecko frames are found elsewhere and they're all aggregated, resulting in (in a non-inverted tree), a parent with 6.46% total cost, and it's child at 15.00%

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

This looks like only an issue for meta categories when hiding platform data

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

This happens in both content and chrome frames, only in uninverted trees. The following samples:

* A -> B
* A -> B -> C -> B
* D -> B

In each case, every B frame (all 4) should have unique self/total costs. This is not the case, and they all share the self cost, and total costs usually get weird. I believe the uninvert function just checks to see if the keys are identical -- it should also check that they share the same stack (from frame to root)

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

In samples (A->B) and (D->B), in inverted B shares the same frame node -- in uninverted, these would be two different frame nodes, so it looks like the wrong numbers only occur when the value shares a frame node in inverted form.

Shu, any ideas on how to fix this?

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1164325.patch

Failing test cases if it helps

Comment 6

[Shu-yu Guo [:shu]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4)
> In samples (A->B) and (D->B), in inverted B shares the same frame node -- in
> uninverted, these would be two different frame nodes, so it looks like the
> wrong numbers only occur when the value shares a frame node in inverted form.
> 
> Shu, any ideas on how to fix this?

Good find! This is a problem with where we keep the sample counts: on the nodes instead of on the edges. This causes loss of information: in your example, since we keep the sample count on B's frame node, we don't know what part of that count the A->B edge contributed, and what part D->B contributed.

If we move the counts to edges, we should be able to keep the counts right through round trips of inversion/uninversion. In the example, in the inverted view, instead of saying B's framenode has sample count of 2, we instead say B->A has a sample count of 1, and B->D has a count of 1. When we uninvert, we reverse edges and split nodes so we get a tree, but the edge counts stay the same. A node's count is the sum of all its edge counts.

Makes sense?

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

The edge counting makes sense, but really at a loss of how to implement that based on what we have now -- do you have anything in mind?

Comment 8

[Shu-yu Guo [:shu]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #7)
> The edge counting makes sense, but really at a loss of how to implement that
> based on what we have now -- do you have anything in mind?

I was thinking of just having a second array. Currently each node keeps its children in the .calls array. We can have a .callCounts array of the same length that holds the counts.

Comment 9

[Shu-yu Guo [:shu]]

Attachment: Fix uninverting the profiler call tree.

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8637376 [details] [diff] [review]
Fix uninverting the profiler call tree.

Review of attachment 8637376 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! This was a real serious issue, and looking at the values in bug 1167021 which has the attachment that reproduces it, looks like it's fixed

::: browser/devtools/performance/test/unit/test_tree-model-12.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Let's add a short description here of what this is testing -- also, how this test is different than tree-model-13 test

::: browser/devtools/performance/test/unit/test_tree-model-13.js
@@ +6,5 @@
> +}
> +
> +add_task(function () {
> +  let { ThreadNode } = devtools.require("devtools/performance/tree-model");
> +  let root = new ThreadNode(gThread, { invertTree: true, startTime: 0, endTime: 50 });

Is this the same test just inverted?

Comment 11

[Shu-yu Guo [:shu]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #10)

> ::: browser/devtools/performance/test/unit/test_tree-model-13.js
> @@ +6,5 @@
> > +}
> > +
> > +add_task(function () {
> > +  let { ThreadNode } = devtools.require("devtools/performance/tree-model");
> > +  let root = new ThreadNode(gThread, { invertTree: true, startTime: 0, endTime: 50 });
> 
> Is this the same test just inverted?

Yes

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0581b5897fc2

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0581b5897fc2