Currently length calculation functions in LengthFunctions.h are inline. A decent rule of thumb is to not inline a function if it is more than 10 lines long. Also it's typically not cost effective to inline functions with loops or switch statements (Reference: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Inline_Functions). Each of these functions are expected to grow by 10 more line as part of bug 27160. Also for bug 27160, in order to calculate viewport size using RenderView class, the inclusion of RenderView.h in LengthFunctions.h cause circular dependency between headers and it fails to compile, so it's better to separate the implementation of these functions.
Created attachment 132961 [details] ProposedPatch
Created attachment 132971 [details] Patch-updated Resubmitting the patch as the previous patch failed to apply.
Comment on attachment 132971 [details] Patch-updated Attachment 132971 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12035257 New failing tests: fast/dom/error-to-string-stack-overflow.html
(In reply to comment #3) > (From update of attachment 132971 [details]) > Attachment 132971 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/12035257 > > New failing tests: > fast/dom/error-to-string-stack-overflow.html Looks like this test case is flaky. Bug 81740 also failed this test. I will re-submit the patch.
Created attachment 133040 [details] Patch Patch resubmitting as the chromium test fails.
Comment on attachment 133040 [details] Patch Attachment 133040 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12035532 New failing tests: fast/eventsource/eventsource-url-attribute.html
(In reply to comment #6) > (From update of attachment 133040 [details]) > Attachment 133040 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/12035532 > > New failing tests: > fast/eventsource/eventsource-url-attribute.html Not related to my change. I think revision 111544 should fix this.
Did you check svn history, why are these functions inline in the first place? Applying rules of thumb can be destructive if someone did actual research that showed the benefit of inlining these.
It is hard to say if inlining is helping or hurting here. It would be good to run some performance tests with and without the change. PerformanceTests/Layout/html5-full-render.html for example might be interesting.
I checked the svn history (http://trac.webkit.org/browser/trunk/WebCore/khtml/misc/khtmllayout.h?rev=1326), and these Length calculation functions are copied from KDE project. These functions were in the same format then also (inline with swtich statement). I will do the profiling today evening as Antti suggested and update the results.
I ran PerformanceTests/Parser/html5-full-render.html on Mac Snow-Leopard with and without the patch with webkit version 111625 and did not see much performance difference. r111625 - {16156.0 ms, 16106.0 ms, 16130.5 ms, 16138.5 ms, 16142.0 ms, 16099.5 ms} -- Average: 16129 ms. r111625 with patch - {16192.0 ms, 16148.0 ms, 16115.0 ms, 16120.5 ms, 16145.0 ms 16146.0 ms} -- Average: 16144 ms. Since these functions are expected to grow again with the introduction of new units in bug 27160, it might be good idea to make these functions regular.
Comment on attachment 133040 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133040&action=review r=me, we can always roll out if the perf bots hate it. I think it should be ok though, these functions are pretty big for inlining. > Source/WebCore/ChangeLog:3 > + Make Length Calculation functions regular. Please make the title match the bug. > Source/WebCore/ChangeLog:6 > + Making the Length calculation functions regular and moving them to LengthFunctions.cpp file. Some motivation and perf testing information here would be good.
Created attachment 133286 [details] Patch-BasedOn-ReviewComments
Comment on attachment 133286 [details] Patch-BasedOn-ReviewComments Clearing flags on attachment: 133286 Committed r111733: <http://trac.webkit.org/changeset/111733>
All reviewed patches have been landed. Closing bug.