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

RESOLVED FIXED in Firefox 45

Status

RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 45
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 8689794 [details] [diff] [review]
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 :)
Attachment #8689794 - Flags: review?(sphink)
Attachment #8689794 - Flags: review?(bzbarsky)
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Blocks: 961331
Depends on: 1226176
Blocks: 1226440
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-
Created attachment 8690170 [details] [diff] [review]
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.
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.
Blocks: 1229229

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/518be0ecf071
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.