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.
Created attachment 139068 [details] patch
Comment on attachment 139068 [details] patch Yes!
http://trac.webkit.org/changeset/115379
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
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.
(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.
In the future please file bug instead of reopening.
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?
(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.
(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.
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.
(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. :(