Skip frame owner disconnect when there's no frames
Created attachment 173059 [details] Patch
Comment on attachment 173059 [details] Patch Looks great! I'm curious to see if we'll see benefit on any of the perf tests.
(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!
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?
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.
(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.
(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?
Created attachment 173073 [details] Patch Use the frame tree instead.
Comment on attachment 173073 [details] Patch LGTM
Clever! I like it.
How will this affect frames that are in shadow trees?
Comment on attachment 173073 [details] Patch Clearing flags on attachment: 173073 Committed r133933: <http://trac.webkit.org/changeset/133933>
All reviewed patches have been landed. Closing bug.
> How will this affect frames that are in shadow trees? Are those not added to the FrameTree?
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
@esprehn: Can you add a test for this case?
(In reply to comment #16) > @esprehn: Can you add a test for this case? Sure.
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