Bug 36303 – Inserting CSS rules one at a time via js causes slow synchronous style recalculation

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Tom Stanis]

Created attachment 51049 [details]
HTML page illustrating the bug

Using sheet.insertRule() causes webkit to re-evaluate all of the style rules synchronously.  This means that if you have a bunch of rules that are added one at a time, styles will be recalculated multiple times.  Instead, we should start the relayout timer and evaluate all of these styles once.

Attached is an HTML page that clearly illustrates how slow this can be.  In my tests this takes ~3000ms whereas firefox completes much quicker.

I have a fix in progress that brings this down to ~100ms.

[Tom Stanis]

Created attachment 51113 [details]
patch to fix bug

Still trying to wok through some test failures, but this patch solves the performance problem.

[James Robinson]

+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.

[James Robinson]

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().

[James Robinson]

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?

[Dave Hyatt]

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.

[Dave Hyatt]

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.

[Dave Hyatt]

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.

[Dave Hyatt]

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.

[Dave Hyatt]

Created attachment 65505 [details]
Patch

[Simon Fraser (smfr)]

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

[James Robinson]

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

[Early Warning System Bot]

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

[Dave Hyatt]

Comment on attachment 65505 [details]
Patch

Ran across a doctype bug in the Html5 parser that I have to fix.

[Eric Seidel (no email)]

Oh, do tell! :)

[Dimitri Glazkov (Google)]

This looks great!

[Dave Hyatt]

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.

[Early Warning System Bot]

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

[Dave Hyatt]

Fixed in r66115.

[WebKit Review Bot]

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

[Tony Chang]

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.

[Tony Chang]

(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.

[David Kilzer (:ddkilzer)]

<rdar://problem/8302217>