Bug 64262

Created attachment 100268 [details] Small speed up, which switches some virtual functions to inline ones.

Comment on attachment 100268 [details] Small speed up, which switches some virtual functions to inline ones. Attachment 100268 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9012419 New failing tests: css2.1/t1202-counter-16-f.html css2.1/t1202-counters-03-b.html css2.1/t1202-counter-12-b.html css2.1/t1202-counter-08-b.html css2.1/t1202-counter-02-b.html css2.1/t1202-counter-14-b.html css2.1/t1202-counter-00-b.html css2.1/t1202-counter-09-b.html css2.1/t1202-counter-15-b.html css2.1/t1202-counter-05-b.html css2.1/t1202-counter-07-b.html css2.1/t1202-counter-06-b.html css2.1/t1202-counter-01-b.html css2.1/t1202-counter-03-b.html css2.1/t1202-counters-02-b.html css2.1/t1202-counter-04-b.html css2.1/t1202-counters-01-b.html css2.1/t1202-counter-13-b.html css2.1/t1202-counter-11-b.html css2.1/t1202-counters-00-b.html

Created attachment 100283 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 100268 [details] Small speed up, which switches some virtual functions to inline ones. View in context: https://bugs.webkit.org/attachment.cgi?id=100268&action=review > Source/WebCore/css/CSSImageGeneratorValue.h:70 > + bool m_isFixedSize; You should initialize it. > Source/WebCore/css/CSSPrimitiveValue.cpp:115 > CSSPrimitiveValue::CSSPrimitiveValue() Could we move all CSSPrimitiveValue::CSSPrimitiveValue constructors into the header file? > Source/WebCore/css/CSSPrimitiveValue.h:189 > + bool m_isQuirkValue; It should be protected. > Source/WebCore/css/CSSValue.h:77 > + bool m_isPrimitiveValue; It should be protected as well. > Source/WebCore/css/CSSValue.h:81 > + CSSValue() > + :m_isPrimitiveValue(false) > + { > + } It is not in WebKit style (the check-webkit-style should complain about it). Please, move up the constructor after 'public:' and add space between ':' and m_isPrimitiveValue

Comment on attachment 100268 [details] Small speed up, which switches some virtual functions to inline ones. View in context: https://bugs.webkit.org/attachment.cgi?id=100268&action=review > Source/WebCore/css/CSSImageGeneratorValue.h:53 > virtual IntSize fixedSize(const RenderObject*) { return IntSize(); } Such function must remain virtal, as it can be called from its base class (if exists), or from its descendants. I would not overwrite them except if we got a permission from the author of those functions, and done them one-by-one. There are several others like this.

> > virtual IntSize fixedSize(const RenderObject*) { return IntSize(); } I guess you wanted to point to 'virtual bool isFixedSize() const { return false; }' line. ;) > Such function must remain virtal, as it can be called from its base class (if exists), or from its descendants. I would not overwrite them except if we got a permission from the author of those functions, and done them one-by-one. > > There are several others like this. Well, this code is 3 years old. Since then there were no attempt to introduce new descendant or to extend this virtual function. So, I do not expect this patch will crash a rapidly changing API. However, we can ask the author about this code. David, what do you think about Tamas's change?

Besides measuring speedup, one needs to consider memory use impact. How did you measure it?

(In reply to comment #7) > Besides measuring speedup, one needs to consider memory use impact. How did you measure it? I made the measurements with Methanol, which measures page loads to locally stored sites. The measures were 5*10, and I analysed the average of these. Binary size: References: 32804896 Modified: 32800323 I will measures the change of the memory usage.

The size of modified objects is increased by 4 bytes (measured at runtime) The number of variables created during the test. m_isPrimitiveValue 261596 m_isQuirkValue 229806 m_isFixedSize 725 The max number of variables in the memory during the test. m_isPrimitiveValue 468 m_isQuirkValue 2749 m_isFixedsize 57

Created attachment 100965 [details] Small speed up, which switches some virtual functions to inline ones.

Created attachment 101148 [details] Small speed up, which switches some virtual functions to inline ones. We measured the following performance progressions on some very popular websites, for example: twitter.com: 5.5% youtube.com: 7.4% yahoo.com: 6.5% The number of variables created during the test. m_isPrimitiveValue 261596 m_isQuirkValue 229806 m_isFixedSize 725 The max number of variables in the memory during the test. m_isPrimitiveValue 468 m_isQuirkValue 2749 m_isFixedsize 57

Comment on attachment 100965 [details] Small speed up, which switches some virtual functions to inline ones. Marking obsolete, it seems to be the same as the last one. You should try "webkit-patch upload" to update your patch, it obsoletes old patches for you.

Comment on attachment 101148 [details] Small speed up, which switches some virtual functions to inline ones. View in context: https://bugs.webkit.org/attachment.cgi?id=101148&action=review You are changing from virtual function to protected attribute for 3 features: ::isFixedSize() ::isQuirkValue() ::isPrimitiveValue() In each case, you are increasing complexity of the code, and increasing the memory consumption per instance. I would prefer to see 3 patches, one for each value. With numbers for performance and memory usage (ideally 3 bugs on bugzilla). When changing something used all over the place like CSSValue, I think it is better to do small changes with a good rationale. > Source/WebCore/css/CSSCanvasValue.h:52 > + m_isFixedSize = true; This should use the constructor initialiazation: m_isFixedSize(true); > Source/WebCore/css/CSSPrimitiveValue.h:210 > + m_isPrimitiveValue = true; Initialization... > Source/WebCore/css/CSSPrimitiveValue.h:220 > + m_isPrimitiveValue = true; Initialization... > Source/WebCore/css/CSSPrimitiveValue.h:229 > + m_isPrimitiveValue = true; Initialization... > Source/WebCore/css/CSSPrimitiveValue.h:243 > + m_isPrimitiveValue = true; Initialization... > Source/WebCore/css/CSSPrimitiveValue.h:251 > + m_isPrimitiveValue = true; Initialization... > Source/WebCore/css/CSSQuirkPrimitiveValue.h:43 > + m_isQuirkValue = true; Constructor initialization: m_isQuirkValue(true);

> > Source/WebCore/css/CSSCanvasValue.h:52 > > + m_isFixedSize = true; > > This should use the constructor initialiazation: m_isFixedSize(true); You cannot initialize any member of the base class from the delivered class like this. ;) There are two popular ways to initialize the base members from delivered class: (1) using an assignment in the delivered constructor (as Tamas did) or (2) calling the base constructor with an extra parameter from the delivered constructor Probably you wanted to suggest the second approach. For example: class CSSValue : public RefCounted<CSSValue> { public: CSSValue(isPrimitiveValue = false) : m_isPrimitiveValue(isPrimitiveValue) ... protected: bool m_isPrimitiveValue; ); ... CSSPrimitiveValue(int ident) : CSSValue(true) , m_isQuirkValue(false) , m_type(CSS_IDENT) , m_hasCachedCSSText(false) { m_value.ident = ident; } ... etc ...

(In reply to comment #14) > > This should use the constructor initialiazation: m_isFixedSize(true); My bad, I was not thinking straight, that code is obviously in the subclass. > CSSPrimitiveValue(int ident) > : CSSValue(true) > , m_isQuirkValue(false) > , m_type(CSS_IDENT) > , m_hasCachedCSSText(false) > { > m_value.ident = ident; > } > ... etc ... Not necessarily. This make the internal variables m_isPrimitiveValue visible through the constructor which is not necessarily a good thing. My comments were just wrong in this case. My point was that I would like this code to be split. I had a quick skim over the patch afterwards but I overlooked which variables come from which class. Just ignore me there :)

Comment on attachment 101148 [details] Small speed up, which switches some virtual functions to inline ones. I just don't believe these performance claims. Can you explain your methodology here? I'm not interested in taking a memory footprint hit unless the performance claims are authentic. I'm extremely skeptical that virtualization is costing us as much as you claim. I think something is flawed with your testing.

This patch, i used gprof values. example: WebCore::CSSStyleApplyProperty::applyValue(CSSStyleSelector* selector, CSSValue* value) const cumulative seconds 158.50 self seconds 0.26 If we remove virtual function we will makes small speed up. example: Virtual function: #include <climits> using namespace std; class BaseClass { public: int k; BaseClass() { k = 6; } virtual int myFunction(int i) { return k+i; } }; int main() { BaseClass p; int a; for(int j=0;j<300;j++) for(int i=0; i<INT_MAX;i++) a+=i+p.myFunction(i); return a; } czene@czene-CELSIUS-W ~/Asztal $ g++ -O3 virtual.cpp -o virtual czene@czene-CELSIUS-W ~/Asztal $ time ./virtual real 6m6.756s user 2m30.241s sys 3m17.356s Inline: #include <climits> using namespace std; class BaseClass { public: int k; BaseClass() { k = 6; } inline int myFunction(int i) { return k+i; } }; int main() { BaseClass p; int a; for(int j=0;j<300;j++) for(int i=0; i<INT_MAX;i++) a+=i+p.myFunction(i); return a; } czene@czene-CELSIUS-W ~/Asztal $ g++ -O3 inline.cpp -o inline czene@czene-CELSIUS-W ~/Asztal $ time ./inline real 6m1.851s user 2m29.433s sys 3m17.628s The msn.com value not valid, i copied wrong text in the comment.

Created attachment 102248 [details] Small speed up, which switches some virtual functions to inline ones. We measured the following performance progressions on some very popular websites: Qt-Linuxon: twitter.com: 5.5% youtube.com: 7.4% Mac: twitter.com: 6.9% youtube.com: 7%

Comment on attachment 102248 [details] Small speed up, which switches some virtual functions to inline ones. Attachment 102248 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9265203 New failing tests: css2.1/t1202-counter-16-f.html css2.1/t1202-counters-03-b.html css2.1/t1202-counter-12-b.html css2.1/t1202-counter-08-b.html css2.1/t1202-counter-02-b.html css2.1/t1202-counter-14-b.html css2.1/t1202-counter-00-b.html css2.1/t1202-counter-09-b.html css2.1/t1202-counter-15-b.html css2.1/t1202-counter-05-b.html css2.1/t1202-counter-07-b.html css2.1/t1202-counter-06-b.html css2.1/t1202-counter-01-b.html css2.1/t1202-counter-03-b.html css2.1/t1202-counters-02-b.html css2.1/t1202-counter-04-b.html css2.1/t1202-counters-01-b.html css2.1/t1202-counter-13-b.html css2.1/t1202-counter-11-b.html css2.1/t1202-counters-00-b.html

Created attachment 102252 [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

> Qt-Linuxon: > twitter.com: 5.5% > youtube.com: 7.4% > Mac: It was measured on Mac Pro 2x2.93GHz 6 Core Intel Xeon with native WK. > twitter.com: 6.9% > youtube.com: 7% The benchmark engine was the Methanol (https://gitorious.org/methanol) which can measure the page loading time of mirrored sites. Tamas, did you see any performance regression?

Created attachment 102363 [details] Idea for comments I have been thinking about this issue. On one side, I understand how this can have a dramatic impact on performance. On the other side, I really do not like the patch because it is creating instance variable for something that should really be a class property. Ideally, we would attach the booleans/enums to the vtable to the class but C++ does not provide anything to do that as far as I know. So a solution to have both speed and tight memory usage is to make a vtable-like structure ourself :-D That should reduce the generated code size in this case, keep the same memory consumption as currently, and recuce the number of CPU cycles to access those properties. The cost is: -very C-like code, quite harder to read and edit than C++. -less type checking at compile time, like instanciating a class with pure virtual functions That is just an idea for comments. I attached a quick mockup of what CSSValue would look like. I am willing to make such changes if we agree this kind of patch can land (and of course iff that change is indeed a significant speed improvement).

I, like Dave, don't understand the testing methodology here. Is this a speed up on page load time?

I think so :) : > The benchmark engine was the Methanol (https://gitorious.org/methanol) > which can measure the page loading time of mirrored sites.

> We measured the following performance progressions on some very popular websites: > Qt-Linuxon: > twitter.com: 5.5% > youtube.com: 7.4% > Mac: > twitter.com: 6.9% > youtube.com: 7% Is that absolute time or valgrind CPU's cycles count? That seems like a lot if this is actual time.

> I, like Dave, don't understand the testing methodology here. Is this a speed up on page load time? Methanol measures only the page loads, nothing else. Lets see how you can reproduce this numbers. ;) 1.- Make a local mirror of a webpage (for example: http://gitorious.org/methanol/methanol/trees/master/sample/openbsd.org ) 2.- Add an event listener - which can call back the Methanol's engine - to that local webpage (see here: http://gitorious.org/methanol/methanol/blobs/master/sample/openbsd.org/index.html#line1 ) 3.- Register the relative path of that website in the test container (lets say: sample.js). 4.- Open Methanol's URL from the browser which loads the local website several times. 5.- Each page loads are saved separately and the final page (results.html) shows the average of page loads and the standard deviation as well. So, at the end we have the average of page loads (in millisecond). No gprof, valgrind or such thing is needed. Tamas's percentages are computed with the following formula: ((old value) / (new value) -1). For example: (100 ms)/(89 ms)-1 ~= 12.35% I hope that I have been a help.

(In reply to comment #26) > > I, like Dave, don't understand the testing methodology here. Is this a speed up on page load time? > > Methanol measures only the page loads, nothing else. > Lets see how you can reproduce this numbers. ;) > > 1.- Make a local mirror of a webpage (for example: http://gitorious.org/methanol/methanol/trees/master/sample/openbsd.org ) > 2.- Add an event listener - which can call back the Methanol's engine - to that local webpage (see here: http://gitorious.org/methanol/methanol/blobs/master/sample/openbsd.org/index.html#line1 ) > 3.- Register the relative path of that website in the test container (lets say: sample.js). > 4.- Open Methanol's URL from the browser which loads the local website several times. > 5.- Each page loads are saved separately and the final page (results.html) shows the average of page loads and the standard deviation as well. Do you have the complete set of data including the standard deviation? The issue is that these results are very surprising; these functions have not traditionally shown us as this hot.

I guess Tamas will be able to provide the raw page load data.

Created attachment 102510 [details] Methanol results

I figured we might as weel get more data so I ran our "cycler" benchmark on my phone :) Without patch: benchmark: cycler_one_qnetworkaccessmanager::load http://www.facebook.com/ mean: 309 msecs +/- 32 msecs, +/- 10.356 % avg: 318 msecs 407 311 309 310 308 309 305 311 307 309 benchmark: cycler_one_qnetworkaccessmanager::load http://www.twitter.com/ mean: 514 msecs +/- 6 msecs, +/- 1.16732 % avg: 512 msecs 517 514 507 504 509 506 514 511 519 521 benchmark: cycler_one_qnetworkaccessmanager::load http://www.youtube.com/ mean: 1930 msecs +/- 3 msecs, +/- 0.15544 % avg: 1930 msecs 1924 1931 1929 1928 1929 1927 1934 1937 1930 1931 benchmark: cycler_one_qnetworkaccessmanager::load https://twitter.com/?lang=en&logged_out=1#!/search/webkit mean: 30 msecs +/- 11 msecs, +/- 36.6667 % avg: 23 msecs 14 31 15 33 16 30 15 31 15 32 With patch: benchmark: cycler_one_qnetworkaccessmanager::load http://www.facebook.com/ mean: 310 msecs +/- 31 msecs, +/- 10 % avg: 321 msecs 401 309 305 310 307 309 305 320 329 316 benchmark: cycler_one_qnetworkaccessmanager::load http://www.twitter.com/ mean: 511 msecs +/- 4 msecs, +/- 0.782779 % avg: 510 msecs 514 511 504 505 506 509 513 511 515 513 benchmark: cycler_one_qnetworkaccessmanager::load http://www.youtube.com/ mean: 1899 msecs +/- 4 msecs, +/- 0.210637 % avg: 1898 msecs 1889 1899 1900 1901 1896 1898 1898 1903 1895 1905 benchmark: cycler_one_qnetworkaccessmanager::load https://twitter.com/?lang=en&logged_out=1#!/search/webkit mean: 31 msecs +/- 11 msecs, +/- 35.4839 % avg: 23 msecs 15 31 15 31 15 31 16 31 15 31 So basically no difference. I really doubt this patch improves loading performance. Maybe you should try to write a test page that keep changing the CSS values of the Elements to demonstrate how much time can be taken by CSSValue?

Comment on attachment 102248 [details] Small speed up, which switches some virtual functions to inline ones. Memory.

This patch uses too much memory and it doesnt worth the effort.

Marking WONTFIX per above comments.