Closed Bug 1226176 Opened 5 years ago Closed 5 years ago

Compute retained sizes in dominator trees and expose them to JavaScript

Categories

(DevTools :: Memory, defect)

defect
Not set
normal

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)

No description provided.
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.
Attachment #8689515 - Flags: review?(sphink)
Attachment #8689515 - Flags: review?(bzbarsky)
(Forgot to `git add` new test file...)
Attachment #8689516 - Flags: review?(sphink)
Attachment #8689516 - Flags: review?(bzbarsky)
Attachment #8689515 - Attachment is obsolete: true
Attachment #8689515 - Flags: review?(sphink)
Attachment #8689515 - Flags: review?(bzbarsky)
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
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.
Attachment #8689516 - Flags: review?(bzbarsky) → review+
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?
Attachment #8689516 - Flags: review?(sphink) → review+
(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];
https://hg.mozilla.org/mozilla-central/rev/6ae1b5c32e52
Status: ASSIGNED → RESOLVED
Closed: 5 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.