Bug 64862 – Switch isFixedSize() virtual function to inline one.

• Component: CSS
• Status: RESOLVED DUPLICATE

[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: 33,6%
yandex.ru: 3%

[Tamas Czene]

Created attachment 101448 [details]
Switch isFixedSize virtual function to inline one.

The number of variables created during the test.
m_isFixedSize 725
The max number of variables in the memory during the test.
m_isFixedsize 57

[Dave Hyatt]

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

[Eric Seidel (no email)]

Comment on attachment 101448 [details]
Switch isFixedSize virtual function to inline one.

Please respond to Dave Hyatt.  Again, memory hit.

[David Barr]

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 and some repeatable numbers.

[David Barr]

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

Does this change result in a measurable performance progression? 
After cleaning up, try PerformanceTests/Parser/html5-full-render.html
with 20 runs on tip of tree (ToT) with and without the patch.

> Source/WebCore/css/CSSCanvasValue.h:52
> +        m_isFixedSize = true;

I believe the preference is for a protected setter.

> Source/WebCore/css/CSSImageGeneratorValue.h:31
> +#include <wtf/AlwaysInline.h>

As mentioned on the related bugs, no need for AlwaysInline because the accessors are trivial functions.

> Source/WebCore/css/CSSImageGeneratorValue.h:52
> -    virtual bool isFixedSize() const { return false; }
> +    ALWAYS_INLINE bool isFixedSize() const { return m_isFixedSize; }

Ditto.

> Source/WebCore/css/CSSImageGeneratorValue.h:71
>      RefPtr<StyleGeneratedImage> m_image;
>      bool m_accessedImage;
> +    bool m_isFixedSize;
>  

As with https://bugs.webkit.org/show_bug.cgi?id=64865 there is almost a whole word of wasted bits in this structure already.
I suggest amending this patch to ensure that the bools are part of the same bitfield.

[David Barr]

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

At ToT:
Testing 6092696 byte document in 13 500000 byte chunks.
Running 20 times
Ignoring warm-up run (18914)
18792 18919 19019 19500 19449 19532 19488 19498 19525 19518
19451 19620 19572 19560 19372 19037 18872 18780 18834 19016

avg 19267.7
median 19450
stdev 303.52728048727346
min 18780
max 19620

With amended patch:
Testing 6092696 byte document in 13 500000 byte chunks.
Running 20 times
Ignoring warm-up run (20471)
18841 18862 18914 18809 19376 19510 19461 19442 19559 19440
19457 19532 19467 19506 19492 19050 18856 18782 18874 18791

avg 19201.05
median 19408
stdev 310.9341529970614
min 18782
max 19559

Parsing speeds at ToT seem to have doubled since the last time I ran this test.
Unfortunately, the variance also seems to have increased now swamping any
minor performance increment.
The progression between ToT and this patch is just 0.3% or 0.2 stdevs, so it
is not a clear winner.

It is worth noting that the only consumer of CSSImageGeneratorValue::isFixedSize()
is CSSImageGeneratorValue::generatedImage().
I would not expect it to be called often enough to produce a measurable penalty.

[David Barr]

The action requested in this bug was taken in:
http://trac.webkit.org/changeset/99577

[David Barr]

*** This bug has been marked as a duplicate of bug 71824 ***