1226176 - Compute retained sizes in dominator trees and expose them to JavaScript

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Comment 1

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

Attachment: Compute retained sizes in dominator trees and expose them to JavaScript

r?bz for webidl and cycle collection changes. I realized that DominatorTrees
need to hold a reference to their owning heap snapshots, so I had to change the
cycle collection macros back to the verbose way.

r?sfink for the rest.

Comment 2

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

Attachment: Compute retained sizes in dominator trees and expose them to JavaScript

(Forgot to `git add` new test file...)

Comment 3

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

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8689516 [details] [diff] [review]
Compute retained sizes in dominator trees and expose them to JavaScript

> so I had to change the cycle collection macros back to the verbose way.

Why?  You're not tracing anything new, so this:

  NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(DominatorTree, mParent, mHeapSnapshot)

should work fine.  You only need the verbose thing if you need to trace things other than your wrappercache.

>+++ b/dom/webidl/DominatorTree.webidl

Might be worth documenting when this method can throw.

r=me on the DOM bits.

Comment 5

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

Comment on attachment 8689516 [details] [diff] [review]
Compute retained sizes in dominator trees and expose them to JavaScript

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

::: js/public/UbiNodeDominatorTree.h
@@ +407,5 @@
> +        MOZ_ASSERT(retainedSizes.isNothing());
> +        auto length = postOrder.length();
> +
> +        retainedSizes.emplace();
> +        if (!retainedSizes->growBy(length)) {

Is this guaranteed to zero out the vector?

Comment 6

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

(In reply to Steve Fink [:sfink, :s:] from comment #5)
> Comment on attachment 8689516 [details] [diff] [review]
> Compute retained sizes in dominator trees and expose them to JavaScript
> 
> Review of attachment 8689516 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/public/UbiNodeDominatorTree.h
> @@ +407,5 @@
> > +        MOZ_ASSERT(retainedSizes.isNothing());
> > +        auto length = postOrder.length();
> > +
> > +        retainedSizes.emplace();
> > +        if (!retainedSizes->growBy(length)) {
> 
> Is this guaranteed to zero out the vector?

No, but zeroing isn't necessary since the value at each index is never read before it is initialized. This is the invariant described in this commment:

        // Iterate in forward order so that we know all of a node's children in
        // the dominator tree have already had their retained size
        // computed.

and ensured by this assertion:

                MOZ_ASSERT(idxOfDominated < i);
                size += retainedSizes.ref()[idxOfDominated];

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ae1b5c32e52

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ae1b5c32e52