Bug 64865 – Switch isQuirkValue() virtual function to inline one.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Tamas Czene]

Removing the virtual functions - removing the calls - makes measurable performance progression on CSS.

We measured the following performance progressions on some very popular websites, for example:
msn.com: 32,4%
yandex.ru: 2%

The number of variables created during the test.
m_isQuirkValue 229806
The max number of variables in the memory during the test.
m_isQuirkValue 2749

[Tamas Czene]

Created attachment 101455 [details]
Switch isQuirkValue virtual function to inline one.

[WebKit Review Bot]

Comment on attachment 101455 [details]
Switch isQuirkValue virtual function to inline one.

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

New failing tests:
css1/basic/comments.html
css1/basic/grouping.html
css1/box_properties/border_bottom_width.html
css1/box_properties/border_bottom_width_inline.html
css1/box_properties/border_left_width.html
css1/basic/inheritance.html
css1/box_properties/border_color_inline.html
css1/box_properties/border_inline.html
css1/basic/class_as_selector.html
css1/box_properties/border.html
css1/box_properties/border_right.html
css1/box_properties/border_color.html
css1/box_properties/border_left_width_inline.html
css1/basic/id_as_selector.html
css1/box_properties/border_left.html
css1/box_properties/border_bottom_inline.html
css1/basic/containment.html
css1/box_properties/border_bottom.html
css1/box_properties/border_left_inline.html
css1/basic/contextual_selectors.html

[WebKit Review Bot]

Created attachment 101461 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dave Hyatt]

See my comments in https://bugs.webkit.org/show_bug.cgi?id=64262.

[Eric Seidel (no email)]

Comment on attachment 101455 [details]
Switch isQuirkValue virtual function to inline one.

Um.  This increases the size of all CSSPrimativeValues.  I suspect this could cause a large memory regression?

[David Barr]

(In reply to comment #5)
> (From update of attachment 101455 [details])
> Um.  This increases the size of all CSSPrimativeValues.  I suspect this could cause a large memory regression?

I believe an alternative approach might hit both targets, investigating.

[Antti Koivisto]

Inreasing the size of CSSPrimativeValue is definitely not acceptable.

[Antti Koivisto]

The performance claims are obviously bs. This is WONTFIX.

[Eric Seidel (no email)]

Another way of saying what Antti said:  A change like this would need to provide evidence (results from a trusted, repeatable benchmark) that this change is a perf improvement.

[Luke Macpherson]

FWIW, I've also noticed that the virtual dispatch on CSSValue shows up a lot in profiling. I wouldn't be surprised if there were some performance gains to be found here by eliminating the virtual call, but as Eric points out, there is a tradeoff with the size on these objects because there tend to be a lot of them.

[David Barr]

(In reply to comment #10)
> FWIW, I've also noticed that the virtual dispatch on CSSValue shows up a lot in profiling. I wouldn't be surprised if there were some performance gains to be found here by eliminating the virtual call, but as Eric points out, there is a tradeoff with the size on these objects because there tend to be a lot of them.

I believe there are some wasted bits that could be put to good use for a perf enhancement without increasing the memory overhead. Please give a chance to upload an RFC patch.

[Antti Koivisto]

I'm all for eliminating virtual functions. This particular bug just didn't start very well (wild, unsubstantiated performance claims, patch that doesn't actually remove a virtual function and adds a member variable to hold value "false"). 

Better continue in a new bug.

[Antti Koivisto]

Hmm, I was wrong, it does actually eliminate the virtual call. The patch is just buggy. I think this can be recovered.

[Antti Koivisto]

Comment on attachment 101455 [details]
Switch isQuirkValue virtual function to inline one.

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

> Source/WebCore/css/CSSPrimitiveValue.h:27
> +#include <wtf/AlwaysInline.h>

Not needed.

> Source/WebCore/css/CSSPrimitiveValue.h:196
> -    virtual bool isQuirkValue() { return false; }
> +    ALWAYS_INLINE bool isQuirkValue() { return m_isQuirkValue; }

Remove ALWAYS_INLINE. This will almost surely inline without it.

> Source/WebCore/css/CSSPrimitiveValue.h:206
>  
> +    bool m_isQuirkValue;

Move this to this bitfield:

    signed m_type : 31;
    mutable unsigned m_hasCachedCSSText : 1;

Take one bit out from m_type. Make all member variables protected instead of private.

> Source/WebCore/css/CSSQuirkPrimitiveValue.h:44
>      {
> +        m_isQuirkValue = false;
>      }

Should be set to true here.

[David Barr]

Created attachment 110958 [details]
Patch

[David Barr]

Created attachment 110959 [details]
Patch

[Gabor Loki]

Comment on attachment 110959 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> [phone]  David Barr  <[e-mail]>
> +
> +        Inline CSSPrimitiveValue::isQuirkValue() as non-virtual function
> +        https://bugs.webkit.org/show_bug.cgi?id=64865
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        This is expected to produce a small performance progression.
> +        Additional memory overhead is avoided by using spare bits.

David, may I ask you to mention Tamas Czene here? Basically it was his idea, and the previous patch was done by him.

[Darin Adler]

Comment on attachment 110959 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.h:249
> +    unsigned m_isQuirkValue : 1;

We should expose a protected inline setter for m_isQuirkValue instead of exposing the data member itself.

[Darin Adler]

Comment on attachment 110959 [details]
Patch

This idea is fine, but there is no reason to keep an entire class, CSSQuirkPrimitiveValue, any more given that the class has no contents. Instead we just need a new create function in CSSPrimitiveValue that creates a quirk value.

Doing it that way will remove an entire unnecessary vtable, make the code cleaner and easier to understand, and also remove the need to add new protected members.

I’m going to say review+ but I’d much prefer this be redone to eliminate the class.

[Antti Koivisto]

(In reply to comment #19)
> Doing it that way will remove an entire unnecessary vtable, make the code cleaner and easier to understand, and also remove the need to add new protected members.

Unfortunately it will take much more than that to remove vtable from CSSValues. CSSPrimitiveValue is not the base class.

> I’m going to say review+ but I’d much prefer this be redone to eliminate the class.

Yeah, definitely. I didn't notice that there was nothing else there.

[David Barr]

Created attachment 111125 [details]
Patch

[David Barr]

Comment on attachment 111125 [details]
Patch

As suggested, updated patch to remove now trivial class CSSQuirkPrimitiveValue and due credit given to Tamas Czene.

[David Barr]

Comment on attachment 111125 [details]
Patch

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

Sorry for the noise, I noticed some nits just after uploading the patch.

> Source/WebCore/WebCore.gypi:-2383
> -            'css/CSSQuirkPrimitiveValue.h',

As a developer new to WebKit, I'm not familiar with the process of propagating this change. FWIW, it doesn't appear to break the build.

> Source/WebCore/css/CSSQuirkPrimitiveValue.h:-31
> -// This value is used to handle quirky margins in reflow roots (body, td, and th) like WinIE.
> -// The basic idea is that a stylesheet can use the value __qem (for quirky em) instead of em.
> -// When the quirky value is used, if you're in quirks mode, the margin will collapse away
> -// inside a table cell.

I think this comment should have followed CSSQuirkPrimitiveValue::create() when it became CSSPrimitiveValue::createQuirk().

[Darin Adler]

Comment on attachment 111125 [details]
Patch

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

Not setting commit-queue+ because of David’s comment, and my related one.

>> Source/WebCore/css/CSSQuirkPrimitiveValue.h:-31
>> -// inside a table cell.
> 
> I think this comment should have followed CSSQuirkPrimitiveValue::create() when it became CSSPrimitiveValue::createQuirk().

I agree. I also would suggest naming the new function CSSPrimitiveValue::createAllowingMarginQuirk().

[David Barr]

Created attachment 111150 [details]
Patch

[Darin Adler]

Comment on attachment 111150 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.h:140
> +    /*
> +     * This value is used to handle quirky margins in reflow roots (body, td, and th) like WinIE.
> +     * The basic idea is that a stylesheet can use the value __qem (for quirky em) instead of em.
> +     * When the quirky value is used, if you're in quirks mode, the margin will collapse away
> +     * inside a table cell.
> +     */

We use "//" comments, not "/*" comments. Not sure why you changed this one.

> Source/WebCore/css/CSSPrimitiveValue.h:143
> +        CSSPrimitiveValue *ptr = new CSSPrimitiveValue(value, type);

The "*" goes next to the type, not the local variable name.

Instead of ptr this local variable should be named value.

[David Barr]

Comment on attachment 111150 [details]
Patch

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

>> Source/WebCore/css/CSSPrimitiveValue.h:140
>> +     */
> 
> We use "//" comments, not "/*" comments. Not sure why you changed this one.

I was matching style with the nearest comment block in the destination file, which just happened to be the only incorrectly styled comment in the whole file. Will fix.

>> Source/WebCore/css/CSSPrimitiveValue.h:143
>> +        CSSPrimitiveValue *ptr = new CSSPrimitiveValue(value, type);
> 
> The "*" goes next to the type, not the local variable name.
> 
> Instead of ptr this local variable should be named value.

Given that the parameter is also called value, I'll take the liberty of calling it quirkValue.
This has the advantage that the middle line reads quite clearly:
quirkValue->m_isQuirkValue = true;

[David Barr]

Created attachment 111152 [details]
Patch

[David Barr]

Comment on attachment 111152 [details]
Patch

All nits addressed, requesting commit-queue.

[Eric Seidel (no email)]

Comment on attachment 111152 [details]
Patch

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

Seems OK.

> Source/WebCore/css/CSSPrimitiveValue.h:257
> -    signed m_type : 31;
> +    signed m_type : 30;

Why is m_type bigger than it needs to be?

[David Barr]

(In reply to comment #30)
> (From update of attachment 111152 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111152&action=review
> 
> Seems OK.
> 
> > Source/WebCore/css/CSSPrimitiveValue.h:257
> > -    signed m_type : 31;
> > +    signed m_type : 30;
> 
> Why is m_type bigger than it needs to be?

It takes values from enum CSSPrimitiveValue::UnitType, the maximum of which is presently
CSS_COUNTER_NAME = 109, so I think it only needs 7 unsigned bits.
However it began as an int even though it's always implicitly cast to short by its accessors.
One bit was borrowed for m_cachedCSSText in r59386 to resolve a substantially larger
memory regression but the extra bits were left assigned to m_type.
It seems Sam Weinig and I were both timid when shrinking it. Principal of least change perhaps?

[Eric Seidel (no email)]

Comment on attachment 111152 [details]
Patch

Its fine to not shink it, but we should explain exacxtly what you just did.  Where it gets its size from, and how large it currently would need to be.  That way the next reviewer won't have any question when we lop another bit off it, since the comment right above will explain that it only uses 7 of the 31 bits and they can verify that by searching for the right enum.

[Eric Seidel (no email)]

Anyway, the change is fine.  ADding a comment to explain woudl be gravy.

[Eric Seidel (no email)]

Did we ever validate the profiling?

Did we ever test this with the PLT or with PerformanceTests/Parser/html5-full-render.html?  It seems at least one of those should show this change.

[David Barr]

Created attachment 111184 [details]
Patch

[David Barr]

Comment on attachment 111184 [details]
Patch

Added comment above m_type declaration for future reference.

[David Barr]

PerformanceTests/Parser/html5-full-render.html with 20 runs:

ToT:
Testing 6092696 byte document in 13 500000 byte chunks.
Running 20 times
Ignoring warm-up run (43271)
42328 42366 42265 42350 42285 42353 42270 42247 42309 42647
42622 42305 42305 42283 42286 42223 42162 42278 42375 42391

avg 42332.5
median 42305
stdev 113.7002638519366
min 42162
max 42647

With patch:
Testing 6092696 byte document in 13 500000 byte chunks.
Running 20 times
Ignoring warm-up run (42394)
42100 42123 42166 42121 42017 42113 41883 41853 42158 41996
42160 42056 42209 42098 41933 42182 41880 41806 41981 42063

avg 42044.9
median 42080.5
stdev 117.5461186088252
min 41806
max 42209

That gives a progression of 2.45 stdevs or 0.7%

[Luke Macpherson]

Comment on attachment 111184 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.h:257
> +    // This holds enum UnitType, so only 7 unsigned bits are necessary.

I would recommend adding a UnitTypeBits constant defined as 7, remove this comment, and use the constant here.
Also it would be good to add a COMPILE_ASSERT to the constructor of PrimitiveValue that asserts that its size never exceeds what you expect.

[Luke Macpherson]

Performance numbers look really good, and IMHO getting rid of the quirk value class is good too.

[David Barr]

Created attachment 111192 [details]
Patch

[Luke Macpherson]

Comment on attachment 111192 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.h:145
> +    }

Also, I'd delete this comment + code and and just add a boolean parameter that defaults to false to create(double, UnitTypes).

[WebKit Review Bot]

Comment on attachment 111192 [details]
Patch

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

[David Barr]

Created attachment 111193 [details]
Patch

[David Barr]

Comment on attachment 111193 [details]
Patch

Leaving m_type signed and 8 bits to work around 'sign mismatch  in comparison' errors. While I agree that the semantics of constructing CSSPrimitiveValues for quirky ems could be improved, I think it is beyond the scope of this change.

[Eric Seidel (no email)]

Thank you David.

[WebKit Review Bot]

Comment on attachment 111193 [details]
Patch

Clearing flags on attachment: 111193

Committed r97583: <http://trac.webkit.org/changeset/97583>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.