Bug 71835 - Move CSSPrimitiveValue bitfields up into CSSValue.
Summary: Move CSSPrimitiveValue bitfields up into CSSValue.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-08 10:38 PST by Andreas Kling
Modified: 2011-11-09 03:12 PST (History)
4 users (show)

See Also:


Attachments
Palatial patch (55.19 KB, patch)
2011-11-08 10:42 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch without double ChangeLog.. (53.21 KB, patch)
2011-11-08 10:52 PST, Andreas Kling
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2011-11-08 10:38:01 PST
We can shrink CSSPrimitiveValue by one CPU word if we move the bitfields up into CSSValue.
Comment 1 Andreas Kling 2011-11-08 10:42:54 PST
Created attachment 114113 [details]
Palatial patch
Comment 2 Andreas Kling 2011-11-08 10:49:24 PST
Comment on attachment 114113 [details]
Palatial patch

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

> Source/WebCore/ChangeLog:55
> +2011-11-08  Andreas Kling  <kling@webkit.org>

Herp derp double ChangeLog.
Comment 3 Andreas Kling 2011-11-08 10:52:09 PST
Created attachment 114118 [details]
Patch without double ChangeLog..
Comment 4 Darin Adler 2011-11-08 13:48:19 PST
Comment on attachment 114118 [details]
Patch without double ChangeLog..

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

We should figure out where to put the kind of comment or compile time assertion that will prevent people from adding virtual functions back into CSSValue or adding data members to CSSPrimitiveValue.

> Source/WebCore/css/CSSValue.h:136
>      CSSValue(ClassType classType)

I think this constructor should be explicit.

> Source/WebCore/css/CSSValue.h:137
> +        : m_primitiveUnitType(0)

Might be nice to have some assertions that this is still 0 somewhere.

> Source/WebCore/css/CSSValue.h:197
> +    // These bits are only used by CSSPrimitiveValue but kept here
> +    // to maximize struct packing.

Great comment!

> Source/WebCore/css/CSSValue.h:198
> +    signed m_primitiveUnitType : 8; // CSSPrimitiveValue::UnitTypes

Why signed instead of unsigned?
Comment 5 Darin Adler 2011-11-08 13:48:50 PST
I wonder if there’s an optimization possible where we don’t even allocate separate value objects on the heap until websites start using the CSS DOM.
Comment 6 Antti Koivisto 2011-11-08 15:58:59 PST
(In reply to comment #5)
> I wonder if there’s an optimization possible where we don’t even allocate separate value objects on the heap until websites start using the CSS DOM.

Yes, that's really the goal. Stylesheet data structures should be separated from the CSSOM objects and the latter should only be constructed on demand. We can have faster and more compact data structures (no parent pointers, values allocated from array) and gain ability to cache and share parsed stylesheets.
Comment 7 Andreas Kling 2011-11-09 03:12:51 PST
Committed r99679: <http://trac.webkit.org/changeset/99679>