Bug 85004 – Cache parsed stylesheets

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

[Antti Koivisto]

CSS parsing is ~1-3% of WebCore CPU usage on average pages, more on sites with large stylesheets. We currently reparse all stylesheets from source text when they are encountered again. In many browsing scenarios we can eliminate lot of this by caching the parsed stylesheets. For example it is very common for subpages of a site to share the stylesheets.

In the future we will also be able to share the actual data structures between pages for significant memory savings.

[Antti Koivisto]

Created attachment 139068 [details]
patch

[Andreas Kling]

Comment on attachment 139068 [details]
patch

Yes!

[Antti Koivisto]

http://trac.webkit.org/changeset/115379

[Dimitri Glazkov (Google)]

Somehow, this change made http/tests/inspector/network/network-initiator.html start failing: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=http%2Ftests%2Finspector%2Fnetwork%2Fnetwork-initiator.html

[Csaba Osztrogonác]

Reopen not to forget fixing this regression. I skipped in on Qt by r115408 to paint the bot green again. Please unskip it with the proper fix.

[Dimitri Glazkov (Google)]

(In reply to comment #5)
> Reopen not to forget fixing this regression. I skipped in on Qt by r115408 to paint the bot green again. Please unskip it with the proper fix.

Filed bug 85038 to track this.

[Antti Koivisto]

In the future please file bug instead of reopening.

[Darin Adler]

Comment on attachment 139068 [details]
patch

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

I decided to do a quick pass of review on this patch even though it’s already been landed once.

> Source/WebCore/css/CSSStyleSheet.cpp:83
> +    // FIXME: This ignores the children of media and region rules.
> +    // Most rules are StyleRules.

I’m not sure this should say FIXME unless you actually think it needs to be fixed.

> Source/WebCore/css/StylePropertySet.cpp:1003
> +unsigned StylePropertySet::averageSizeInBytes()

Do we really want to pay function call overhead for this? Why not just put it inline in the header?

> Source/WebCore/html/HTMLLinkElement.cpp:305
> +        RefPtr<MediaQuerySet> media = MediaQuerySet::createAllowingDescriptionSyntax(m_media);
> +        restoredSheet->setMediaQueries(media.release());

I think this would read better without the local variable:

    restoredSheet->setMediaQueries(MediaQuerySet::createAllowingDescriptionSyntax(m_media));

> Source/WebCore/html/HTMLLinkElement.cpp:311
> +        m_sheet = CSSStyleSheet::create(restoredSheet, this);
> +        m_loading = false;
> +        sheetLoaded();
> +        notifyLoadedSheetAndAllCriticalSubresources(false);

This is the kind of code I find almost impossible to review. Are these the key lines of code to call to simulate a successfully loaded sheet? Is this now synchronous where the old code path was asynchronous? I don’t know how to tell.

> Source/WebCore/loader/cache/CachedCSSStyleSheet.h:56
> +        virtual void destroyDecodedData() OVERRIDE;

Can this be private or protected rather than public?

[Antti Koivisto]

(In reply to comment #8)
> > Source/WebCore/css/CSSStyleSheet.cpp:83
> > +    // FIXME: This ignores the children of media and region rules.
> > +    // Most rules are StyleRules.
> 
> I’m not sure this should say FIXME unless you actually think it needs to be fixed.

It might be worth fixing. Note that the two comments are really on different topic. The first one is about counting the rules, the second one is about the multiplier.

> > Source/WebCore/css/StylePropertySet.cpp:1003
> > +unsigned StylePropertySet::averageSizeInBytes()
> 
> Do we really want to pay function call overhead for this? Why not just put it inline in the header?

That would requires including headers from header that can currently be avoided, or moving the code away from the classes that we are being measured.

The function is not called often. I think other factors are more important.

> > Source/WebCore/html/HTMLLinkElement.cpp:311
> > +        m_sheet = CSSStyleSheet::create(restoredSheet, this);
> > +        m_loading = false;
> > +        sheetLoaded();
> > +        notifyLoadedSheetAndAllCriticalSubresources(false);
> 
> This is the kind of code I find almost impossible to review. Are these the key lines of code to call to simulate a successfully loaded sheet? Is this now synchronous where the old code path was asynchronous? I don’t know how to tell.

The right solution is to factor the load completion code better. It is a mess. I have refactored quite a bit to make this patch possible but I didn't get into that yet.

[Csaba Osztrogonác]

(In reply to comment #7)
> In the future please file bug instead of reopening.

In the future please run layout tests instead of introducing new regressions. 

I think reopening is much more simple than file-ing a new bug to fix
regressions. It is the simplest way to tell the author if something 
is wrong and let the author and reviewer to determine if the bug
can be fixed quickly or rollout needed or new bug report needed.

[Alexey Proskuryakov]

Antti is right. Re-opening a bug that is fixed to track something related - even if broken by its fix - is semantically incorrect and confusing. Please don't do it.

[Csaba Osztrogonác]

(In reply to comment #11)
> Antti is right. Re-opening a bug that is fixed to track something related - even if broken by its fix - is semantically incorrect and confusing. Please don't do it.

OK, sorry for reopening bug this bug. I won't do it anymore and won't notice if you introduce new regressions. :(