Bug 76019 – Improve bit-packing of CSSValue, CSSPrimitiveValue and CSSValueList.

• Component: CSS
• Status: RESOLVED WONTFIX

[Luke Macpherson]

Improve bit-packing of CSSValue, CSSPrimitiveValue and CSSValueList.

[Luke Macpherson]

Created attachment 121961 [details]
Patch

[Andreas Kling]

The CSSPrimitiveValue::UnitTypes change (reassignment of enum values) is web- and API-facing, as you can retrieve the primitive value type via CSSPrimitiveValue.primitiveType.

Not sure that's something we want to do.

[Luke Macpherson]

I'll move that part into a separate CL - saving that bit isn't particularly critical anyway (due to padding).

[Luke Macpherson]

Created attachment 122092 [details]
Patch

[WebKit Review Bot]

Comment on attachment 122092 [details]
Patch

Attachment 122092 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11170556

[Early Warning System Bot]

Comment on attachment 122092 [details]
Patch

Attachment 122092 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11213166

[Gyuyoung Kim]

Comment on attachment 122092 [details]
Patch

Attachment 122092 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11219003

[Tony Chang]

Comment on attachment 122092 [details]
Patch

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

Luke, can you get some numbers on how much memory this saves?  E.g., you could instrument the code to count how many CSSValues there are at a given time.

> Source/WebCore/css/CSSPrimitiveValue.h:61
> +    unsigned char m_primitiveUnitType : 7; // CSSPrimitiveValue::UnitTypes
> +    mutable bool m_hasCachedCSSText : 1;
> +    bool m_isQuirkValue : 1;

This adds up to 9.  Is that intentional?  I didn't think the bits packed across classes.

[Luke Macpherson]

Created attachment 122112 [details]
Patch

[Luke Macpherson]

(In reply to comment #8)
> (From update of attachment 122092 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122092&action=review
> 
> Luke, can you get some numbers on how much memory this saves?  E.g., you could instrument the code to count how many CSSValues there are at a given time.
> 
> > Source/WebCore/css/CSSPrimitiveValue.h:61
> > +    unsigned char m_primitiveUnitType : 7; // CSSPrimitiveValue::UnitTypes
> > +    mutable bool m_hasCachedCSSText : 1;
> > +    bool m_isQuirkValue : 1;
> 
> This adds up to 9.  Is that intentional?  I didn't think the bits packed across classes.

Yes. It doesn't really matter here as this is going to pad out to 4-8 bytes anyway. And no, that's not a regression. Getting it down to six is something I think is doable, but I'll save that for another patch.

[Luke Macpherson]

(In reply to comment #8)
> (From update of attachment 122092 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122092&action=review
> 
> Luke, can you get some numbers on how much memory this saves?  E.g., you could instrument the code to count how many CSSValues there are at a given time.

Some quick instrumentation shows that for gmail I have 6422 CSSValues allocated simultaneously, and 5321 of those are CSSPrimitiveValue. So for gmail I would only expect this change to save 4k of memory in that case, since CSSPrimitiveValue's size is unchanged, while all other descendants of CSSValue are 4 bytes smaller.

The other value of this change is that it restores separation of concerns to the codebase - member variables of CSSPrimitiveValue no longer leak into CSSValue, whose sole purpose becomes a devirtualized dispatcher for concrete value types.

[Luke Macpherson]

Created attachment 122162 [details]
Patch

[Andreas Kling]

Comment on attachment 122162 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.h:57
> +    CSSPrimitiveValueBase() : CSSValue(PrimitiveClass), m_primitiveUnitType(0), m_hasCachedCSSText(false), m_isQuirkValue(false) { }
> +    CSSPrimitiveValueBase(ClassType type) : CSSValue(type), m_primitiveUnitType(0), m_hasCachedCSSText(false), m_isQuirkValue(false) { }

These constructors should be protected, nobody should ever construct a standalone CSSPrimitiveValueBase.

[Andreas Kling]

Comment on attachment 122162 [details]
Patch

I'm not convinced we should be introducing bitfield refcounts, it feels like we're scratching at something of a Pandora's box here:

- It introduces an arbitrary inconsistency in how many refs different objects can have.
- It's no longer possible to take the address of a ref counter. (Something the JSC JIT has used in the past, not sure if we'll need it again.)

The CSSPrimitiveValueBase change is great on its own.

[Ryosuke Niwa]

I don't think we want to make this change. In addition to the points kling pointed out, using bitfields for the ref-count will make ref/deref slower and bloats the code size where these functions are called. Also, implementing an variant makes the future refactoring harder.

[Luke Macpherson]

(In reply to comment #15)
Ok, let me respond to the points raised:
1) It introduces an arbitrary inconsistency in how many refs different objects can have.
Are there any cases where this is actually important? Code would have to be pretty poorly designed to require more than 67 million references to a single CSSValue, and even then I have ASSERTs to catch this case in debug mode.

2) It's no longer possible to take the address of a ref counter. (Something the JSC JIT has used in the past, not sure if we'll need it again.)
While I'm wary of debating what code might look like in the future, JIT code should probably be calling a code stub to access values like this, not reaching into data structures directly.

3) Using bitfields for the ref-count will make ref/deref slower and bloats the code size where these functions are called.
This is probably incorrect for modern processors, where code tends to be cache and bus limited. Packing common fields like this is likely to actually yield improved performance in practice through better cache utilization, though I suspect it would be too insignificant to measure in this case.

4). Implementing an variant makes the future refactoring harder.
RefPtr and friends are designed to use templates to handle this exact situation. They do not require classes to extend RefCounted. You can therefore re-factor RefCounted completely independently of other RefPtr-able objects.

[Antti Koivisto]

There are also plans for a wider refactoring of the stylesheet structures which may remove the need for refcounting for common CSSValues. I agree with kling and rniwa, we shouldn't do this now. Marking as WONTFIX to indicate this.