Closed Bug 1226416 Opened 4 years ago Closed 4 years ago

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

Categories

(DevTools :: Memory, defect)

defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Once again:

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

Thanks for bearing with me :)
Attachment #8689794 - Flags: review?(sphink)
Attachment #8689794 - Flags: review?(bzbarsky)
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
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.
Attachment #8689794 - Flags: review?(bzbarsky) → review-
Getting the retained size before sorting was a good call, I think it is a lot
simpler now.
Attachment #8690170 - Flags: review?(sphink)
Attachment #8690170 - Flags: review?(bzbarsky)
Attachment #8689794 - Attachment is obsolete: true
Attachment #8689794 - Flags: review?(sphink)
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!
Attachment #8690170 - Flags: review?(bzbarsky) → review+
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.
Attachment #8690170 - Flags: review?(sphink) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/518be0ecf071
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.