Bug 36303

Created attachment 51113 [details] patch to fix bug Still trying to wok through some test failures, but this patch solves the performance problem.

+cc'ing a few folks. FYI, this bug has hit a lot of Google properties and likely lots of other pages on the web. Looks like a good patch to me (but I am not a reviewer). Style nit: one-line blocks do not get curly braces.

The tricky part is that attach() requires the style selector to be up to date, but by the time attach() is entered, it is too late to update the style selector and recalcStyle() for the entire document.

Could we just defer all the work in attach() until the next recalcStyle()? From a quick glance, the only work in attach() that depends on the style selector is mutating the render tree and it should be fine for the render tree to be stale as long as it's updated before entering layout().

Or to put it another way, why not always force the Node::lazyAttach() case. I'm experimenting with this locally and it seems to not be too bad. The tricky cases are plugins and shadow DOM trees. Plugins are tricky because they are instantiated through a RenderWidget and need to start up synchronously, but it should be pretty simple to force a layout pass after adding them to the DOM. Shadow DOM trees can be lazily created, there's no reason for them to even exist until layout will occur. There are a few more corner cases but they don't seem too tricky. Is there any reason why this can't work?

Comment on attachment 51113 [details] patch to fix bug You should inline updateStyleSelectorIfDirty. It's silly to do two function calls when most of the time you won't be dirty.

Comment on attachment 51113 [details] patch to fix bug You should inline updateStyleSelectorIfDirty. It's silly to do two function calls when most of the time you won't be dirty.

Also, lots of extra { } in if statements that should be removed. Also, the placement of updateStyleSelectorIfDirty is bad. It should happen before the recalcStyle, and the recalcStyle should not happen if the updateStyleSelector ran. You should make a new helper function that combines the updateStyleSelector check with this code: // Always ensure our style info is up-to-date. This can happen in situations where // the layout beats any sort of style recalc update that needs to occur. if (m_frame->needsReapplyStyles()) m_frame->reapplyStyles(); else if (document->childNeedsStyleRecalc()) document->recalcStyle(); The logic should be something like: if (m_styleSelectorDirty) updateStyleSelector(); else if (m_frame->needsReapplyStyles()) etc.

I think you could keep chugging along by using styleNotYetAvailable stuff... the same thing we do when stylesheets haven't loaded. The new objects would get parsed and attached but have no renderers. Eventually you'd yield back to the run loop and the layout would happen, and you'd get the renderers made. Note that you also need to patch the recalc style timer to update the style selector if needed. We don't want that firing and not updating the style selector first.

See also bug 39647.

Created attachment 65505 [details] Patch

Comment on attachment 65505 [details] Patch > Index: WebCore/rendering/RenderBlock.cpp > =================================================================== > --- WebCore/rendering/RenderBlock.cpp (revision 66057) > +++ WebCore/rendering/RenderBlock.cpp (working copy) > @@ -4132,8 +4132,7 @@ void RenderBlock::setDesiredColumnCountA > bool destroyColumns = !firstChild() > || (count == 1 && style()->hasAutoColumnWidth()) > || firstChild()->isAnonymousColumnsBlock() > - || firstChild()->isAnonymousColumnSpanBlock() > - || document()->settings()->paginateDuringLayoutEnabled(); > + || firstChild()->isAnonymousColumnSpanBlock(); Unrelated change here. I like it! r=me

Awesome! What still calls Document::updateStyleSelector()?

Attachment 65505 [details] did not build on qt: Build output: http://queues.webkit.org/results/3731592

Comment on attachment 65505 [details] Patch Ran across a doctype bug in the Html5 parser that I have to fix.

Oh, do tell! :)

This looks great!

Created attachment 65578 [details] Patch This patch fixes the bug. One layout test changes results to be incorrect, but it's an artifact of a later style recalc happening after the HTML5 parser altered the document to be in quirks mode without doing a style recalc of its own. The timer-based one then fixes the document up. After I land this patch, I'm going to do a big doctype/quirks mode cleanup now that I have discovered this issue.

Attachment 65578 [details] did not build on qt: Build output: http://queues.webkit.org/results/3761660

Fixed in r66115.

http://trac.webkit.org/changeset/66115 might have broken Qt Linux ARMv5 Release

This seems to have caused a perf regression in the chromium intl1 page cycler. I filed bug 44808 about this and added a test case there.

(In reply to comment #22) > This seems to have caused a perf regression in the chromium intl1 page cycler. I filed bug 44808 about this and added a test case there. Are we ok with taking the perf regression from this change mentioned in bug 44808? If so, should I just close that bug? It seems like this change made some cases faster and some cases slower.

<rdar://problem/8302217>