Compute retained sizes in dominator trees and expose them to JavaScript

RESOLVED FIXED in Firefox 45

Status

defect
RESOLVED FIXED
4 years ago
Last year

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)

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];

Comment 8

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6ae1b5c32e52
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45

Updated

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