Bug 71666

Summary: Devirtualize CSSValue
Product: WebKit Reporter: Andreas Kling <kling>
Component: CSSAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: koivisto, macpherson, mikelawther, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 71667, 71668, 71671, 71675, 71796, 71800, 71813, 71814, 71824    
Bug Blocks:    
Attachments:
Description Flags
Patch! koivisto: review+

Andreas Kling
Reported 2011-11-07 04:41:09 PST
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
Attachments
Patch! (21.14 KB, patch)
2011-11-08 09:44 PST, Andreas Kling 		koivisto: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Andreas Kling
Comment 1 2011-11-08 09:44:50 PST
Created attachment 114097 [details] Patch!
Antti Koivisto
Comment 2 2011-11-08 09:55:10 PST
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
Comment 3 2011-11-08 10:18:07 PST
(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
Comment 4 2011-11-08 10:27:23 PST
Committed r99592: <http://trac.webkit.org/changeset/99592>
Mike Lawther
Comment 5 2011-11-15 21:03:25 PST
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
Comment 6 2011-11-21 07:00:09 PST
(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
Comment 7 2011-11-24 16:53:53 PST
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
Comment 8 2011-11-25 06:11:47 PST
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
Comment 9 2011-11-27 04:51:22 PST
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
Comment 10 2011-11-27 05:08:04 PST
(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.
Note You need to log in before you can comment on or make changes to this bug.