Bug 93712

Summary: CSS: Shrink RuleData by storing selector as index rather than pointer.
Product: WebKit Reporter: Andreas Kling <kling>
Component: CSSAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, fmalita, macpherson, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
koivisto: review+
Latch for panding none

Description Andreas Kling 2012-08-10 05:15:23 PDT
CSS: Shrink RuleData by storing selector as index rather than pointer.
Comment 1 Andreas Kling 2012-08-10 05:15:54 PDT
Created attachment 157711 [details]
Patch
Comment 2 Antti Koivisto 2012-08-10 05:45:05 PDT
Comment on attachment 157711 [details]
Patch

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

r=me

Better keep eye on performance bots after landing.

> Source/WebCore/css/CSSSelectorList.h:53
> +    size_t indexAfter(size_t index) const
> +    {

This could use a more descriptive name. No great suggestions though.

> Source/WebCore/css/StyleResolver.cpp:229
> +    unsigned m_selectorIndex : 12;
>      // This number was picked fairly arbitrarily. We can probably lower it if we need to.
>      // Some simple testing showed <100,000 RuleData's on large sites.
> -    unsigned m_position : 24;
> +    unsigned m_position : 20;

So we now support 4k selectors per rule and 1M active style rules per document. That sounds more than sufficient but we probably shouldn't shave many more bit here.

If the limits are overrun we get semi-random matching behavior (wrong selectors gets tested, rules applied in wrong order,...). I wonder we should add overflow testing and simply ignore overflowing rules/selectors.
Comment 3 Antti Koivisto 2012-08-10 06:01:06 PDT
Comment on attachment 157711 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:230
> +    unsigned m_specificity : 24;

Specificity shouldn't really need nearly this many bits. The computation is just not implemented very space-efficiently.
Comment 4 Andreas Kling 2012-08-10 06:04:26 PDT
Created attachment 157723 [details]
Latch for panding
Comment 5 WebKit Review Bot 2012-08-10 10:00:55 PDT
Comment on attachment 157723 [details]
Latch for panding

Clearing flags on attachment: 157723

Committed r125294: <http://trac.webkit.org/changeset/125294>
Comment 6 WebKit Review Bot 2012-08-10 10:01:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Florin Malita 2012-08-10 11:22:58 PDT
Looks like this broke the Android & Qt builds:

Source/WebCore/css/StyleResolver.cpp:248: error: size of array 'RuleData_should_stay_small' is negative
Comment 8 Florin Malita 2012-08-10 11:56:13 PDT
I've committed http://trac.webkit.org/changeset/125305 to get the builds going, but that's probably wasting sizeof(unsigned) on 32-bit platforms due to uint64 alignment.

I'll post a patch shortly to address that.