Bug 81733 – Make Length Calculation functions non-inline

• Component: CSS
• Status: RESOLVED FIXED

[Joe Thomas]

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.

[Joe Thomas]

Created attachment 132961 [details]
ProposedPatch

[Joe Thomas]

Created attachment 132971 [details]
Patch-updated

Resubmitting the patch as the previous patch failed to apply.

[WebKit Review Bot]

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

[Joe Thomas]

(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.

[Joe Thomas]

Created attachment 133040 [details]
Patch

Patch resubmitting as the chromium test fails.

[WebKit Review Bot]

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

[Joe Thomas]

(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.

[Alexey Proskuryakov]

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.

[Antti Koivisto]

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.

[Joe Thomas]

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.

[Joe Thomas]

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.

[Antti Koivisto]

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.

[Joe Thomas]

Created attachment 133286 [details]
Patch-BasedOn-ReviewComments

[WebKit Review Bot]

Comment on attachment 133286 [details]
Patch-BasedOn-ReviewComments

Clearing flags on attachment: 133286

Committed r111733: <http://trac.webkit.org/changeset/111733>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.