| Summary: | Switch isFixedSize() virtual function to inline one. | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tamas Czene <tczene> | ||||
| Component: | CSS | Assignee: | David Barr <davidbarr> | ||||
| Status: | RESOLVED DUPLICATE | ||||||
| Severity: | Normal | CC: | davidbarr, hyatt, loki, shanestephens | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 64262 | ||||||
| Attachments: |
|
||||||
|
Description
Tamas Czene
2011-07-20 04:32:40 PDT
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
See my comments in https://bugs.webkit.org/show_bug.cgi?id=64262. Comment on attachment 101448 [details]
Switch isFixedSize virtual function to inline one.
Please respond to Dave Hyatt. Again, memory hit.
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. 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. 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. The action requested in this bug was taken in: http://trac.webkit.org/changeset/99577 |