Title: [111733] trunk/Source/WebCore
Revision
111733
Author
commit-qu...@webkit.org
Date
2012-03-22 11:10:55 -0700 (Thu, 22 Mar 2012)

Log Message

Make Length Calculation functions non-inline
https://bugs.webkit.org/show_bug.cgi?id=81733

Currently length calculation functions in LengthFunctions.h are inline. These functions are pretty big to be inline.
And these functions are expected to grow again when new length units will be introduced in bug 27160.

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

Ran PerformanceTests/Parser/html5-full-render.html on Mac Snow-Leopard with and without the patch and did not see much performance difference.

Patch by Joe Thomas <joetho...@motorola.com> on 2012-03-22
Reviewed by Antti Koivisto.

* CMakeLists.txt:
* GNUmakefile.list.am:
* Target.pri:
* WebCore.gypi:
* WebCore.vcproj/WebCore.vcproj:
* WebCore.xcodeproj/project.pbxproj:
* css/LengthFunctions.cpp: Added.
(WebCore):
(WebCore::miminumValueForLength):
(WebCore::valueForLength):
(WebCore::floatValueForLength):
* css/LengthFunctions.h:
(WebCore):

Modified Paths

Added Paths

Diff

Modified: trunk/Source/WebCore/CMakeLists.txt (111732 => 111733)


--- trunk/Source/WebCore/CMakeLists.txt	2012-03-22 18:02:00 UTC (rev 111732)
+++ trunk/Source/WebCore/CMakeLists.txt	2012-03-22 18:10:55 UTC (rev 111733)
@@ -499,6 +499,7 @@
     css/CSSWrapShapes.cpp
     css/FontFeatureValue.cpp
     css/FontValue.cpp
+    css/LengthFunctions.cpp
     css/MediaFeatureNames.cpp
     css/MediaList.cpp
     css/MediaQuery.cpp

Modified: trunk/Source/WebCore/ChangeLog (111732 => 111733)


--- trunk/Source/WebCore/ChangeLog	2012-03-22 18:02:00 UTC (rev 111732)
+++ trunk/Source/WebCore/ChangeLog	2012-03-22 18:10:55 UTC (rev 111733)
@@ -1,3 +1,32 @@
+2012-03-22  Joe Thomas  <joetho...@motorola.com>
+
+        Make Length Calculation functions non-inline
+        https://bugs.webkit.org/show_bug.cgi?id=81733
+
+        Currently length calculation functions in LengthFunctions.h are inline. These functions are pretty big to be inline.
+        And these functions are expected to grow again when new length units will be introduced in bug 27160.
+
+        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).
+
+        Ran PerformanceTests/Parser/html5-full-render.html on Mac Snow-Leopard with and without the patch and did not see much performance difference.
+
+        Reviewed by Antti Koivisto.
+
+        * CMakeLists.txt:
+        * GNUmakefile.list.am:
+        * Target.pri:
+        * WebCore.gypi:
+        * WebCore.vcproj/WebCore.vcproj:
+        * WebCore.xcodeproj/project.pbxproj:
+        * css/LengthFunctions.cpp: Added.
+        (WebCore):
+        (WebCore::miminumValueForLength):
+        (WebCore::valueForLength):
+        (WebCore::floatValueForLength):
+        * css/LengthFunctions.h:
+        (WebCore):
+
 2012-03-22  Alexis Menard  <alexis.men...@openbossa.org>
 
         Increase code sharing between CSSParser and CSSPropertyLonghand.

Modified: trunk/Source/WebCore/GNUmakefile.list.am (111732 => 111733)


--- trunk/Source/WebCore/GNUmakefile.list.am	2012-03-22 18:02:00 UTC (rev 111732)
+++ trunk/Source/WebCore/GNUmakefile.list.am	2012-03-22 18:10:55 UTC (rev 111733)
@@ -1647,6 +1647,7 @@
 	Source/WebCore/css/FontFeatureValue.h \
 	Source/WebCore/css/FontValue.cpp \
 	Source/WebCore/css/FontValue.h \
+	Source/WebCore/css/LengthFunctions.cpp \
 	Source/WebCore/css/LengthFunctions.h \
 	Source/WebCore/css/MediaFeatureNames.cpp \
 	Source/WebCore/css/MediaFeatureNames.h \

Modified: trunk/Source/WebCore/Target.pri (111732 => 111733)


--- trunk/Source/WebCore/Target.pri	2012-03-22 18:02:00 UTC (rev 111732)
+++ trunk/Source/WebCore/Target.pri	2012-03-22 18:10:55 UTC (rev 111733)
@@ -475,6 +475,7 @@
     css/CSSWrapShapes.cpp \
     css/FontFeatureValue.cpp \
     css/FontValue.cpp \
+    css/LengthFunctions.cpp \
     css/MediaFeatureNames.cpp \
     css/MediaList.cpp \
     css/MediaQuery.cpp \

Modified: trunk/Source/WebCore/WebCore.gypi (111732 => 111733)


--- trunk/Source/WebCore/WebCore.gypi	2012-03-22 18:02:00 UTC (rev 111732)
+++ trunk/Source/WebCore/WebCore.gypi	2012-03-22 18:10:55 UTC (rev 111733)
@@ -2430,6 +2430,7 @@
             'css/FontFeatureValue.h',
             'css/FontValue.cpp',
             'css/FontValue.h',
+            'css/LengthFunctions.cpp',
             'css/MediaFeatureNames.cpp',
             'css/MediaFeatureNames.h',
             'css/MediaList.cpp',

Modified: trunk/Source/WebCore/WebCore.vcproj/WebCore.vcproj (111732 => 111733)


--- trunk/Source/WebCore/WebCore.vcproj/WebCore.vcproj	2012-03-22 18:02:00 UTC (rev 111732)
+++ trunk/Source/WebCore/WebCore.vcproj/WebCore.vcproj	2012-03-22 18:10:55 UTC (rev 111733)
@@ -36474,6 +36474,10 @@
 				>
 			</File>
 			<File
+				RelativePath="..\css\LengthFunctions.cpp"
+				>
+			</File>
+			<File
 				RelativePath="..\css\LengthFunctions.h"
 				>
 			</File>

Modified: trunk/Source/WebCore/WebCore.xcodeproj/project.pbxproj (111732 => 111733)


--- trunk/Source/WebCore/WebCore.xcodeproj/project.pbxproj	2012-03-22 18:02:00 UTC (rev 111732)
+++ trunk/Source/WebCore/WebCore.xcodeproj/project.pbxproj	2012-03-22 18:10:55 UTC (rev 111733)
@@ -5965,6 +5965,7 @@
 		E4C279590CF9741900E97B98 /* RenderMedia.h in Headers */ = {isa = PBXBuildFile; fileRef = E4C279570CF9741900E97B98 /* RenderMedia.h */; };
 		E4D687770ED7AE3D006EA978 /* PurgeableBufferMac.cpp in Sources */ = {isa = PBXBuildFile; fileRef = E4D687760ED7AE3D006EA978 /* PurgeableBufferMac.cpp */; };
 		E4D687790ED7AE4F006EA978 /* PurgeableBuffer.h in Headers */ = {isa = PBXBuildFile; fileRef = E4D687780ED7AE4F006EA978 /* PurgeableBuffer.h */; };
+		E55F497A151B888000BB67DB /* LengthFunctions.cpp in Sources */ = {isa = PBXBuildFile; fileRef = E55F4979151B888000BB67DB /* LengthFunctions.cpp */; };
 		E5BA7D63151437CA00FE1E3F /* LengthFunctions.h in Headers */ = {isa = PBXBuildFile; fileRef = E5BA7D62151437CA00FE1E3F /* LengthFunctions.h */; settings = {ATTRIBUTES = (Private, ); }; };
 		ED2BA83C09A24B91006C0AC4 /* DocumentMarker.h in Headers */ = {isa = PBXBuildFile; fileRef = ED2BA83B09A24B91006C0AC4 /* DocumentMarker.h */; settings = {ATTRIBUTES = (Private, ); }; };
 		ED501DC60B249F2900AE18D9 /* EditorMac.mm in Sources */ = {isa = PBXBuildFile; fileRef = ED501DC50B249F2900AE18D9 /* EditorMac.mm */; };
@@ -13086,6 +13087,7 @@
 		E4C279570CF9741900E97B98 /* RenderMedia.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = RenderMedia.h; sourceTree = "<group>"; };
 		E4D687760ED7AE3D006EA978 /* PurgeableBufferMac.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = PurgeableBufferMac.cpp; sourceTree = "<group>"; };
 		E4D687780ED7AE4F006EA978 /* PurgeableBuffer.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = PurgeableBuffer.h; sourceTree = "<group>"; };
+		E55F4979151B888000BB67DB /* LengthFunctions.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = LengthFunctions.cpp; sourceTree = "<group>"; };
 		E5BA7D62151437CA00FE1E3F /* LengthFunctions.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = LengthFunctions.h; sourceTree = "<group>"; };
 		ED2BA83B09A24B91006C0AC4 /* DocumentMarker.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = DocumentMarker.h; sourceTree = "<group>"; };
 		ED501DC50B249F2900AE18D9 /* EditorMac.mm */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.objcpp; name = EditorMac.mm; path = mac/EditorMac.mm; sourceTree = "<group>"; };
@@ -20490,6 +20492,7 @@
 				CD4E0AFA11F7BC27009D3811 /* fullscreen.css */,
 				CDBD93BA1333BD4B002570E3 /* fullscreenQuickTime.css */,
 				93CA4C9909DF93FA00DF8677 /* html.css */,
+				E55F4979151B888000BB67DB /* LengthFunctions.cpp */,
 				E5BA7D62151437CA00FE1E3F /* LengthFunctions.h */,
 				93CA4C9A09DF93FA00DF8677 /* make-css-file-arrays.pl */,
 				93CA4C9B09DF93FA00DF8677 /* makeprop.pl */,
@@ -27610,6 +27613,7 @@
 				BCE93F471517C6D5008CCF74 /* RenderRegionSet.cpp in Sources */,
 				9393E5FF151A99F200066F06 /* CSSImageSetValue.cpp in Sources */,
 				9393E604151A9A1800066F06 /* StyleCachedImageSet.cpp in Sources */,
+				E55F497A151B888000BB67DB /* LengthFunctions.cpp in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};

Copied: trunk/Source/WebCore/css/LengthFunctions.cpp (from rev 111732, trunk/Source/WebCore/css/LengthFunctions.h) (0 => 111733)


--- trunk/Source/WebCore/css/LengthFunctions.cpp	                        (rev 0)
+++ trunk/Source/WebCore/css/LengthFunctions.cpp	2012-03-22 18:10:55 UTC (rev 111733)
@@ -0,0 +1,121 @@
+/*
+    Copyright (C) 1999 Lars Knoll (kn...@kde.org)
+    Copyright (C) 2006, 2008 Apple Inc. All rights reserved.
+    Copyright (C) 2011 Rik Cabanier (caban...@adobe.com)
+    Copyright (C) 2011 Adobe Systems Incorporated. All rights reserved.
+    Copyright (C) 2012 Motorola Mobility, Inc. All rights reserved.
+
+    This library is free software; you can redistribute it and/or
+    modify it under the terms of the GNU Library General Public
+    License as published by the Free Software Foundation; either
+    version 2 of the License, or (at your option) any later version.
+
+    This library is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+    Library General Public License for more details.
+
+    You should have received a copy of the GNU Library General Public License
+    along with this library; see the file COPYING.LIB.  If not, write to
+    the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+    Boston, MA 02110-1301, USA.
+*/
+
+#include "config.h"
+#include "LengthFunctions.h"
+
+#include "Length.h"
+
+namespace WebCore {
+
+int miminumValueForLength(Length length, int maximumValue, bool roundPercentages)
+{
+    switch (length.type()) {
+    case Fixed:
+        return length.value();
+    case Percent:
+        if (roundPercentages)
+            return static_cast<int>(round(maximumValue * length.percent() / 100.0f));
+        // Don't remove the extra cast to float. It is needed for rounding on 32-bit Intel machines that use the FPU stack.
+        return static_cast<int>(static_cast<float>(maximumValue * length.percent() / 100.0f));
+    case Calculated:
+        return length.nonNanCalculatedValue(maximumValue);
+    case Auto:
+        return 0;
+    case Relative:
+    case Intrinsic:
+    case MinIntrinsic:
+    case Undefined:
+        ASSERT_NOT_REACHED();
+        return 0;
+    }
+    ASSERT_NOT_REACHED();
+    return 0;
+}
+
+int valueForLength(Length length, int maximumValue, bool roundPercentages)
+{
+    switch (length.type()) {
+    case Fixed:
+    case Percent:
+    case Calculated:
+        return miminumValueForLength(length, maximumValue, roundPercentages);
+    case Auto:
+        return maximumValue;
+    case Relative:
+    case Intrinsic:
+    case MinIntrinsic:
+    case Undefined:
+        ASSERT_NOT_REACHED();
+        return 0;
+    }
+    ASSERT_NOT_REACHED();
+    return 0;
+}
+
+// FIXME: when subpixel layout is supported this copy of floatValueForLength() can be removed. See bug 71143.
+float floatValueForLength(Length length, int maximumValue)
+{
+    switch (length.type()) {
+    case Fixed:
+        return length.getFloatValue();
+    case Percent:
+        return static_cast<float>(maximumValue * length.percent() / 100.0f);
+    case Auto:
+        return static_cast<float>(maximumValue);
+    case Calculated:
+        return length.nonNanCalculatedValue(maximumValue);                
+    case Relative:
+    case Intrinsic:
+    case MinIntrinsic:
+    case Undefined:
+        ASSERT_NOT_REACHED();
+        return 0;
+    }
+    ASSERT_NOT_REACHED();
+    return 0;
+}
+
+float floatValueForLength(Length length, float maximumValue)
+{
+    switch (length.type()) {
+    case Fixed:
+        return length.getFloatValue();
+    case Percent:
+        return static_cast<float>(maximumValue * length.percent() / 100.0f);
+    case Auto:
+        return static_cast<float>(maximumValue);
+    case Calculated:
+        return length.nonNanCalculatedValue(maximumValue);
+    case Relative:
+    case Intrinsic:
+    case MinIntrinsic:
+    case Undefined:
+        ASSERT_NOT_REACHED();
+        return 0;
+    }
+    ASSERT_NOT_REACHED();
+    return 0;
+}
+
+} // namespace WebCore

Modified: trunk/Source/WebCore/css/LengthFunctions.h (111732 => 111733)


--- trunk/Source/WebCore/css/LengthFunctions.h	2012-03-22 18:02:00 UTC (rev 111732)
+++ trunk/Source/WebCore/css/LengthFunctions.h	2012-03-22 18:10:55 UTC (rev 111733)
@@ -24,100 +24,15 @@
 #ifndef LengthFunctions_h
 #define LengthFunctions_h
 
-#include "Length.h"
-
 namespace WebCore {
 
-inline int miminumValueForLength(Length length, int maximumValue, bool roundPercentages = false)
-{
-    switch (length.type()) {
-    case Fixed:
-        return length.value();
-    case Percent:
-        if (roundPercentages)
-            return static_cast<int>(round(maximumValue * length.percent() / 100.0f));
-        // Don't remove the extra cast to float. It is needed for rounding on 32-bit Intel machines that use the FPU stack.
-        return static_cast<int>(static_cast<float>(maximumValue * length.percent() / 100.0f));
-    case Calculated:
-        return length.nonNanCalculatedValue(maximumValue);
-    case Auto:
-        return 0;
-    case Relative:
-    case Intrinsic:
-    case MinIntrinsic:
-    case Undefined:
-        ASSERT_NOT_REACHED();
-        return 0;
-    }
-    ASSERT_NOT_REACHED();
-    return 0;
-}
+struct Length;
 
-inline int valueForLength(Length length, int maximumValue, bool roundPercentages = false)
-{
-    switch (length.type()) {
-    case Fixed:
-    case Percent:
-    case Calculated:
-        return miminumValueForLength(length, maximumValue, roundPercentages);
-    case Auto:
-        return maximumValue;
-    case Relative:
-    case Intrinsic:
-    case MinIntrinsic:
-    case Undefined:
-        ASSERT_NOT_REACHED();
-        return 0;
-    }
-    ASSERT_NOT_REACHED();
-    return 0;
-}
+int miminumValueForLength(Length, int maximumValue, bool roundPercentages = false);
+int valueForLength(Length, int maximumValue, bool roundPercentages = false);
+float floatValueForLength(Length, int maximumValue);
+float floatValueForLength(Length, float maximumValue);
 
-// FIXME: when subpixel layout is supported this copy of floatValueForLength() can be removed. See bug 71143.
-inline float floatValueForLength(Length length, int maximumValue)
-{
-    switch (length.type()) {
-    case Fixed:
-        return length.getFloatValue();
-    case Percent:
-        return static_cast<float>(maximumValue * length.percent() / 100.0f);
-    case Auto:
-        return static_cast<float>(maximumValue);
-    case Calculated:
-        return length.nonNanCalculatedValue(maximumValue);                
-    case Relative:
-    case Intrinsic:
-    case MinIntrinsic:
-    case Undefined:
-        ASSERT_NOT_REACHED();
-        return 0;
-    }
-    ASSERT_NOT_REACHED();
-    return 0;
-}
-
-inline float floatValueForLength(Length length, float maximumValue)
-{
-    switch (length.type()) {
-    case Fixed:
-        return length.getFloatValue();
-    case Percent:
-        return static_cast<float>(maximumValue * length.percent() / 100.0f);
-    case Auto:
-        return static_cast<float>(maximumValue);
-    case Calculated:
-        return length.nonNanCalculatedValue(maximumValue);
-    case Relative:
-    case Intrinsic:
-    case MinIntrinsic:
-    case Undefined:
-        ASSERT_NOT_REACHED();
-        return 0;
-    }
-    ASSERT_NOT_REACHED();
-    return 0;
-}
-
 } // namespace WebCore
 
 #endif // LengthFunctions_h
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes

Reply via email to