1226416 - Expose a method to get a node's set of immediately dominated nodes in the dominator tree

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Description

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

This adds the `getImmediatelyDominated` method to `DominatorTree` which takes a
node id and returns the set of each node ids for every node that is immediately
dominated by the node with the given id. The results are sorted by greatest to
least retained size. In conjunction with the `root` attribute, this can be used
to traverse the whole dominator tree.

Comment 1

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

Attachment: Expose a method to get a node's set of immediately dominated nodes in the dominator tree

Once again:

r?bz for webidl changes
r?sfink for the rest

Thanks for bearing with me :)

Comment 2

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

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8689794 [details] [diff] [review]
Expose a method to get a node's set of immediately dominated nodes in the dominator tree

>+static MallocSizeOf getCurrentThreadDebuggerMallocSizeOf() {

Curly on next line, please.  And function name and return type on different lines.

>+struct CompareByRetainedSize

I don't see how this works in general, if partway through the sort we start giving different answers.  There's no guarantee the sort will even converge...

Seems like we should have an array of structs that contain an ubi::Node and its retained size, grab all the sizes up front (throwing if any fail), then do the sort.

>+  auto node = mHeapSnapshot->getNodeById(id);
>+  auto range = mDominatorTree.getDominatedSet(*node);
>+  auto rootId = Root();
>+    auto id = node.identifier();

Would like actual types for those.

Comment 4

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

Attachment: Expose a method to get a node's set of immediately dominated nodes in the dominator tree

Getting the retained size before sorting was a good call, I think it is a lot
simpler now.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8690170 [details] [diff] [review]
Expose a method to get a node's set of immediately dominated nodes in the dominator tree

This file could benefit from a "using JS::ubi::Node" up at the top, I think.  Would reduce the temptation to use auto.  ;)

>+  for (const JS::ubi::Node& node : *range) {

This is shadowing the "node" above that was returned from getNodeById.  Maybe call this one dominatedNode or something?

>+  aOutResult.SetValue(nsTArray<uint64_t>(length));

Mmm, rvalue refs.  OK, this doesn't make an extra copy, good.

>+    // The root dominates itself, but we don't want to expose that to JS.

Then don't we need to size aOutResult to length - 1?  Otherwise it'll have a trailing 0, no?  Did the tests not catch this?

Or better yet, can we size dominatedNodes to length - 1, skip adding root to it to start with, then size aOutResult to dominatedNodes.Length().

r=me with the above fixed.  Thanks for updating!

Comment 6

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

Comment on attachment 8690170 [details] [diff] [review]
Expose a method to get a node's set of immediately dominated nodes in the dominator tree

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

I don't have anything beyond what bz said.

Comment 7

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

(In reply to Boris Zbarsky [:bz] from comment #5)
> >+    // The root dominates itself, but we don't want to expose that to JS.
> 
> Then don't we need to size aOutResult to length - 1?  Otherwise it'll have a
> trailing 0, no?  Did the tests not catch this?
> 
> Or better yet, can we size dominatedNodes to length - 1, skip adding root to
> it to start with, then size aOutResult to dominatedNodes.Length().

bz and I talked about this last week on IRC, but for posterity and sfink's benefit: in this case the _capacity_ of the array would be one larger than the length, but the length is unaffected by the number passed to the constructor here. This is less slop than we would have if we didn't specify an initial capacity at all (and more performant) and instead just dynamically resized as we appended elements.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/518be0ecf071

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/518be0ecf071