1241221 - Dominator children are missing in the UI

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P1)

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: 39051.fxsnapshot

STR:

* Import this snapshot
* Switch to dominators view

ER:

retainedSize(gc roots) =
  shallowSize(gc roots) +
    sum(retainedSize(x) for x in children(gc roots))

AR:

Lots of size missing...

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: screencap of actual results

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 0: Test retained size of nodes in dominmator trees

This adds a new test for dominator trees computed from heap snapshots, to make
sure that a node's retained size matches the following:

    retainedSize(node) = shallowSize(node) + sum(retainedSize(c) for c in children(node))

This test did not find the bug described in bug 1241221, but seems like a
valuable test to have anyways.

Comment 4

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8710110 [details] [diff] [review]
Part 0: Test retained size of nodes in dominmator trees

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

Makes me wonder a little if this should be a DEBUG-mode assert within the C++, but then again, DEBUG is slow enough as it is. And this is still a good test to have in JS anyway.

It does make me a little nervous to be comparing numbers for exact equality, but I guess these are all integer values that should fit well with 2^53 or whatever the limit is.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Fix incorrect reporting of `moreChildrenAvailable`

The condition checked when reporting whether there were `moreChildrenAvailable`
when constructing the initial DominatorTreeNode tree from a DominatorTree was
backwards. This commit fixes the condition and adds a test that fails without
the condition fix.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5da848104b4

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8710141 [details] [diff] [review]
Part 1: Fix incorrect reporting of `moreChildrenAvailable`

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

Great Nick! Looks good to me.

::: devtools/shared/heapsnapshot/DominatorTreeNode.js
@@ +205,5 @@
>        const endIdx = Math.min(childNodeIds.length, maxSiblings);
>        for (let i = 0; i < endIdx; i++) {
>          DominatorTreeNode.addChild(node, dfs(childNodeIds[i], newDepth));
>        }
> +      node.moreChildrenAvailable = endIdx < childNodeIds.length;

I was a bit confused by the name endIdx here : I assumed it was the last index reached and I thought the test was incorrect. Maybe partialLength ? 
Feel free to ignore :)

::: devtools/shared/heapsnapshot/tests/unit/test_DominatorTreeNode_partialTraversal_01.js
@@ +18,5 @@
> +//   |    `- 1000
> +//   `- 400
> +//        |- 1100
> +//        |- 1200
> +//        `- 1300

For completeness, would be nice to cover the case where `number of children == maxSiblings` (so 2 children in this test).
moreChildrenAvailable should be false in this case as well.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Julian Descottes [:jdescottes] from comment #7)
> For completeness, would be nice to cover the case where `number of children
> == maxSiblings` (so 2 children in this test).
> moreChildrenAvailable should be false in this case as well.

Great idea.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Fix incorrect reporting of `moreChildrenAvailable`

The condition checked when reporting whether there were `moreChildrenAvailable`
when constructing the initial DominatorTreeNode tree from a DominatorTree was
backwards. This commit fixes the condition and adds a test that fails without
the condition fix.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/481ad00ddc82
https://hg.mozilla.org/integration/fx-team/rev/e1f6239bfa33

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/481ad00ddc82
https://hg.mozilla.org/mozilla-central/rev/e1f6239bfa33

Comment 12

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
Test partially Successful

Component: 
Name		 	Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
This part is not so much clear to me; except this previous steps worked fine

retainedSize(gc roots) =
  shallowSize(gc roots) +
    sum(retainedSize(x) for x in children(gc roots))


Actual Results: 
As expected