Bug 36303 - Inserting CSS rules one at a time via js causes slow synchronous style recalculation
Summary: Inserting CSS rules one at a time via js causes slow synchronous style recalc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-03-18 10:49 PDT by Tom Stanis
Modified: 2010-10-06 12:27 PDT (History)
13 users (show)

See Also:


Attachments
HTML page illustrating the bug (68.33 KB, text/html)
2010-03-18 10:49 PDT, Tom Stanis
no flags Details
patch to fix bug (3.60 KB, patch)
2010-03-18 17:23 PDT, Tom Stanis
no flags Details | Formatted Diff | Diff
Patch (19.31 KB, patch)
2010-08-25 17:30 PDT, Dave Hyatt
hyatt: review-
Details | Formatted Diff | Diff
Patch (19.54 KB, patch)
2010-08-26 10:41 PDT, Dave Hyatt
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Stanis 2010-03-18 10:49:41 PDT
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.
Comment 1 Tom Stanis 2010-03-18 17:23:06 PDT
Created attachment 51113 [details]
patch to fix bug

Still trying to wok through some test failures, but this patch solves the performance problem.
Comment 2 James Robinson 2010-03-18 17:47:13 PDT
+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.
Comment 3 mitz 2010-03-18 17:56:02 PDT
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.
Comment 4 James Robinson 2010-03-18 18:30:21 PDT
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().
Comment 5 James Robinson 2010-03-19 15:13:04 PDT
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 6 Dave Hyatt 2010-05-19 14:15:09 PDT
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 7 Dave Hyatt 2010-05-19 14:16:20 PDT
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 8 Dave Hyatt 2010-05-19 14:18:36 PDT
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.
Comment 9 Dave Hyatt 2010-05-19 14:36:10 PDT
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.
Comment 10 mitz 2010-05-25 15:13:33 PDT
See also bug 39647.
Comment 11 Dave Hyatt 2010-08-25 17:30:53 PDT
Created attachment 65505 [details]
Patch
Comment 12 Simon Fraser (smfr) 2010-08-25 17:34:10 PDT
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
Comment 13 James Robinson 2010-08-25 17:38:17 PDT
Awesome!  What still calls Document::updateStyleSelector()?
Comment 14 Early Warning System Bot 2010-08-25 17:54:50 PDT
Attachment 65505 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3731592
Comment 15 Dave Hyatt 2010-08-25 18:14:20 PDT
Comment on attachment 65505 [details]
Patch

Ran across a doctype bug in the Html5 parser that I have to fix.
Comment 16 Eric Seidel (no email) 2010-08-25 23:31:12 PDT
Oh, do tell! :)
Comment 17 Dimitri Glazkov (Google) 2010-08-26 07:16:39 PDT
This looks great!
Comment 18 Dave Hyatt 2010-08-26 10:41:53 PDT
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.
Comment 19 Early Warning System Bot 2010-08-26 11:05:46 PDT
Attachment 65578 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3761660
Comment 20 Dave Hyatt 2010-08-26 11:20:56 PDT
Fixed in r66115.
Comment 21 WebKit Review Bot 2010-08-26 11:32:31 PDT
http://trac.webkit.org/changeset/66115 might have broken Qt Linux ARMv5 Release
Comment 22 Tony Chang 2010-08-27 18:36:46 PDT
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.
Comment 23 Tony Chang 2010-09-01 12:38:08 PDT
(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.
Comment 24 David Kilzer (:ddkilzer) 2010-10-06 12:27:29 PDT
<rdar://problem/8302217>