Devirtualize CSSValue

• Component: CSS
• Status: RESOLVED FIXED

[Andreas Kling]

We can shrink CSSValue (and more importantly CSSPrimitiveValue) by one CPU word (4 or 8 bytes) by devirtualizing it and hoisting CSSPrimitiveValue's bitfields up into CSSValue.

Current layout:

CSSValue : public RefCounted, 8/12 bytes:
* (vptr)
* m_refCount

CSSPrimitiveValue : public CSSValue, 16/24 bytes:
* (vptr)
* m_refCount
* flags for CSSPrimitiveValue (32-bit bitfield)
* m_value

Target layout:

CSSValue : public RefCounted, 8/8 bytes:
* m_refCount
* flags for CSSValue and CSSPrimitiveValue (32-bit bitfield)

CSSPrimitiveValue : public CSSValue, 12/16 bytes:
* m_refCount
* flags for CSSValue and CSSPrimitiveValue (32-bit bitfield)
* m_value

[Andreas Kling]

Created attachment 114097 [details]
Patch!

[Antti Koivisto]

Comment on attachment 114097 [details]
Patch!

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

r=me

> Source/WebCore/css/CSSValue.cpp:214
> +    case InitialClass:
> +    case ImplicitInitialClass:
> +        delete static_cast<CSSInitialValue*>(this);
> +        return;

This is bit strange.

[Andreas Kling]

(In reply to comment #2)
> (From update of attachment 114097 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114097&action=review
> 
> r=me
> 
> > Source/WebCore/css/CSSValue.cpp:214
> > +    case InitialClass:
> > +    case ImplicitInitialClass:
> > +        delete static_cast<CSSInitialValue*>(this);
> > +        return;
> 
> This is bit strange.

Indeed. As I said on IRC, this is a hack to avoid out-of-lining CSSValue::isImplicitInitialValue() since we can't dereference a CSSInitialValue pointer in CSSValue.h. I'll move this to a dedicated CSSValue bit in a future patch.

[Andreas Kling]

Committed r99592: <http://trac.webkit.org/changeset/99592>

[Mike Lawther]

Now that these have all landed - what has been the performance improvement? 

Apologies if this was mentioned/discussed on IRC - I'm not on-channel all the time.

[Andreas Kling]

(In reply to comment #5)
> Now that these have all landed - what has been the performance improvement? 
> 
> Apologies if this was mentioned/discussed on IRC - I'm not on-channel all the time.

This was done primarily as a memory optimization, so I only verified the lack of performance regressions. As an example, it saves ~100kB on current GMail.

[Mike Lawther]

OK - thanks. I asked because of the comment in CSSValue.h

// NOTE: This class is non-virtual for memory and performance reasons.
// Don't go making it virtual again unless you know exactly what you're doing!

The memory benefits of devirtualising are clear, but I wanted to understand the performance reasons. 

So it sounds like there is negligible performance delta between virtual/non-virtual in this case.

[Antti Koivisto]

Note that reducing memory usage of the core data structures can in some cases be a measurable performance win in itself due to improved locality.

[Mike Lawther]

Yep - totally (improved cache coherency etc). That was why I was interested in any performance measurements that had been done so the gains could be quantified. Or whether any gains from improved memory may have been canceled out due to the increased code size and execution time of the DIY dispatch.

[Andreas Kling]

(In reply to comment #7)
> OK - thanks. I asked because of the comment in CSSValue.h
> 
> // NOTE: This class is non-virtual for memory and performance reasons.
> // Don't go making it virtual again unless you know exactly what you're doing!
> 
> The memory benefits of devirtualising are clear, but I wanted to understand the performance reasons. 
> 
> So it sounds like there is negligible performance delta between virtual/non-virtual in this case.

That sounds about right. I discussed this with Antti, and we simply wanted a sufficiently scary-looking comment so people wouldn't accidentally make the class virtual again.

(In reply to comment #9)
> Yep - totally (improved cache coherency etc). That was why I was interested in any performance measurements that had been done so the gains could be quantified. Or whether any gains from improved memory may have been canceled out due to the increased code size and execution time of the DIY dispatch.

I suspect that if anything, the DIY dispatch is actually faster than virtual calls, as it can synergize better with CPU branch prediction.