Bug 101619 – Skip frame owner disconnect when there's no frames

• Component: New Bugs
• Status: RESOLVED FIXED

[Elliott Sprehn]

Skip frame owner disconnect when there's no frames

[Elliott Sprehn]

Created attachment 173059 [details]
Patch

[Ojan Vafai]

Comment on attachment 173059 [details]
Patch

Looks great! I'm curious to see if we'll see benefit on any of the perf tests.

[Elliott Sprehn]

(In reply to comment #2)
> (From update of attachment 173059 [details])
> Looks great! I'm curious to see if we'll see benefit on any of the perf tests.

Yeah. Complicated pages seem to always have frames on them though, even the Review Patch page has an iframe for the comment form!

[Adam Barth]

Comment on attachment 173059 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173059&action=review

> Source/WebCore/dom/ContainerNodeAlgorithms.h:280
> +        // If we know there's no frames to disconnect then don't bother traversing
> +        // the tree looking for them.
> +        Page* page = root->document()->page();
> +        if (page && !page->subframeCount())
> +            return;

Why do we need to do this on a per-page basis?  Isn't it enough to know that the current document has no subframes?

[Ojan Vafai]

Comment on attachment 173059 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173059&action=review

>> Source/WebCore/dom/ContainerNodeAlgorithms.h:280
>> +            return;
> 
> Why do we need to do this on a per-page basis?  Isn't it enough to know that the current document has no subframes?

Page is what has the data at the moment. Long-term plan is to add a count of frames in the subtree to NodeRareData and only do this traversal if the subtree has any frames.

[Elliott Sprehn]

(In reply to comment #4)
> (From update of attachment 173059 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173059&action=review
> 
> > Source/WebCore/dom/ContainerNodeAlgorithms.h:280
> > +        // If we know there's no frames to disconnect then don't bother traversing
> > +        // the tree looking for them.
> > +        Page* page = root->document()->page();
> > +        if (page && !page->subframeCount())
> > +            return;
> 
> Why do we need to do this on a per-page basis?  Isn't it enough to know that the current document has no subframes?

Yeah it is, but we don't track that currently. This is the first step in a longer road to optimization.

[Elliott Sprehn]

(In reply to comment #6)
> (In reply to comment #4)
> ...
> > Why do we need to do this on a per-page basis?  Isn't it enough to know that the current document has no subframes?
> 
> Yeah it is, but we don't track that currently. This is the first step in a longer road to optimization.

Actually, maybe I can just use document()->frame()->tree()->firstChild() to detect if there's subframes?

[Elliott Sprehn]

Created attachment 173073 [details]
Patch

Use the frame tree instead.

[Adam Barth]

Comment on attachment 173073 [details]
Patch

LGTM

[Ojan Vafai]

Clever! I like it.

[Alexey Proskuryakov]

How will this affect frames that are in shadow trees?

[WebKit Review Bot]

Comment on attachment 173073 [details]
Patch

Clearing flags on attachment: 173073

Committed r133933: <http://trac.webkit.org/changeset/133933>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

> How will this affect frames that are in shadow trees?

Are those not added to the FrameTree?

[Adam Barth]

It looks like they are added to the FrameTree, but they're skipped over in the "scoped" versions of these functions by testing Frame::inScope

[Adam Barth]

@esprehn: Can you add a test for this case?

[Elliott Sprehn]

(In reply to comment #16)
> @esprehn: Can you add a test for this case?

Sure.

[Ojan Vafai]

This is a 9%-14% improvement on some of the dromaeo dom benchmarks!

http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_domcoremodify/report.html?rev=166898&graph=dom_modify_appendChild&trace=score&history=50

http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_domcoremodify/report.html?rev=166898&graph=dom_modify_appendChild&trace=score&history=50

http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_domcoremodify/report.html?rev=166898&graph=dom_modify_insertBefore&trace=score&history=50

http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_domcoremodify/report.html?rev=166898&graph=dom_modify_insertBefore&trace=score&history=50