Changeset 127974 in webkit

Timestamp:

Sep 8, 2012 5:35:47 PM (12 years ago)

Author:

benjamin@webkit.org

Message:

Specialize nextBreakablePosition depending on breakNBSP
​https://bugs.webkit.org/show_bug.cgi?id=96042

Patch by Benjamin Poulain <​bpoulain@apple.com> on 2012-09-08
Reviewed by Eric Seidel.

The speed of isBreakableSpace() is limited by the speed of the inner loop of nextBreakablePosition().
The branches done to handle noBreakSpace can be simplified outside the loop
to reduce the number of tests inside the loop.

This patch split the code of nextBreakablePosition() in two function, depending if breakNBSP is true
or false.

If breakNBSP is true, isBreakableSpace() would return true on noBreakSpace.
->There is no need to test that value again for needsLineBreakIterator().
->There is no need to special case the switch() of isBreakableSpace() for noBreakSpace.

If breakNBSP is false:
->isBreakableSpace() does not need to test for noBreakSpace.

On x86_64, this improves PerformanceTests/Layout/line-layout.html by 2.8%.

• rendering/break_lines.cpp:

(WebCore::isBreakableSpace):
(WebCore::nextBreakablePositionIgnoringNBSP):
(WebCore::nextBreakablePosition):

• rendering/break_lines.h:

(WebCore::isBreakable): Remove the default value for breakNBSP, no caller is using it.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• rendering/break_lines.cpp (modified) (5 diffs)
• rendering/break_lines.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-09-08  Benjamin Poulain  <bpoulain@apple.com>
+
+        Specialize nextBreakablePosition depending on breakNBSP
+        https://bugs.webkit.org/show_bug.cgi?id=96042
+
+        Reviewed by Eric Seidel.
+
+        The speed of isBreakableSpace() is limited by the speed of the inner loop of nextBreakablePosition().
+        The branches done to handle noBreakSpace can be simplified outside the loop
+        to reduce the number of tests inside the loop.
+
+        This patch split the code of nextBreakablePosition() in two function, depending if breakNBSP is true
+        or false.
+
+        If breakNBSP is true, isBreakableSpace() would return true on noBreakSpace.
+        ->There is no need to test that value again for needsLineBreakIterator().
+        ->There is no need to special case the switch() of isBreakableSpace() for noBreakSpace.
+
+        If breakNBSP is false:
+        ->isBreakableSpace() does not need to test for noBreakSpace.
+
+        On x86_64, this improves PerformanceTests/Layout/line-layout.html by 2.8%.
+
+        * rendering/break_lines.cpp:
+        (WebCore::isBreakableSpace):
+        (WebCore::nextBreakablePositionIgnoringNBSP):
+        (WebCore::nextBreakablePosition):
+        * rendering/break_lines.h:
+        (WebCore::isBreakable): Remove the default value for breakNBSP, no caller is using it.
+
 2012-09-08  Adam Barth  <abarth@chromium.org>
 

trunk/Source/WebCore/rendering/break_lines.cpp

@@ -39 +39 @@
 namespace WebCore {
 
-static inline bool isBreakableSpace(UChar ch, bool treatNoBreakSpaceAsBreak)
+template<bool treatNoBreakSpaceAsBreak>
+static inline bool isBreakableSpace(UChar ch)
 {
     switch (ch) {
@@ -139 +140 @@
 }
 
-static inline bool needsLineBreakIterator(UChar ch)
+template<bool treatNoBreakSpaceAsBreak>
+inline bool needsLineBreakIterator(UChar ch)
 {
+    if (treatNoBreakSpaceAsBreak)
+        return ch > asciiLineBreakTableLastChar;
     return ch > asciiLineBreakTableLastChar && ch != noBreakSpace;
 }
 
-int nextBreakablePosition(LazyLineBreakIterator& lazyBreakIterator, int pos, bool treatNoBreakSpaceAsBreak)
+template<bool treatNoBreakSpaceAsBreak>
+static inline int nextBreakablePosition(LazyLineBreakIterator& lazyBreakIterator, int pos)
 {
     const UChar* str = lazyBreakIterator.string();
@@ -155 +160 @@
         UChar ch = str[i];
 
-        if (isBreakableSpace(ch, treatNoBreakSpaceAsBreak) || shouldBreakAfter(lastLastCh, lastCh, ch))
+        if (isBreakableSpace<treatNoBreakSpaceAsBreak>(ch) || shouldBreakAfter(lastLastCh, lastCh, ch))
             return i;
 
-        if (needsLineBreakIterator(ch) || needsLineBreakIterator(lastCh)) {
+        if (needsLineBreakIterator<treatNoBreakSpaceAsBreak>(ch) || needsLineBreakIterator<treatNoBreakSpaceAsBreak>(lastCh)) {
             if (nextBreak < i && i) {
                 TextBreakIterator* breakIterator = lazyBreakIterator.get();
@@ -164 +169 @@
                     nextBreak = textBreakFollowing(breakIterator, i - 1);
             }
-            if (i == nextBreak && !isBreakableSpace(lastCh, treatNoBreakSpaceAsBreak))
+            if (i == nextBreak && !isBreakableSpace<treatNoBreakSpaceAsBreak>(lastCh))
                 return i;
         }
@@ -175 +180 @@
 }
 
+int nextBreakablePositionIgnoringNBSP(LazyLineBreakIterator& lazyBreakIterator, int pos)
+{
+    return nextBreakablePosition<false>(lazyBreakIterator, pos);
+}
+
+int nextBreakablePosition(LazyLineBreakIterator& lazyBreakIterator, int pos)
+{
+    return nextBreakablePosition<true>(lazyBreakIterator, pos);
+}
+
 } // namespace WebCore

trunk/Source/WebCore/rendering/break_lines.h

@@ -28 +28 @@
 class LazyLineBreakIterator;
 
-int nextBreakablePosition(LazyLineBreakIterator&, int pos, bool breakNBSP = false);
+int nextBreakablePositionIgnoringNBSP(LazyLineBreakIterator&, int pos);
+int nextBreakablePosition(LazyLineBreakIterator&, int pos);
 
-inline bool isBreakable(LazyLineBreakIterator& lazyBreakIterator, int pos, int& nextBreakable, bool breakNBSP = false)
+inline bool isBreakable(LazyLineBreakIterator& lazyBreakIterator, int pos, int& nextBreakable, bool breakNBSP)
 {
-    if (pos > nextBreakable)
-        nextBreakable = nextBreakablePosition(lazyBreakIterator, pos, breakNBSP);
+    if (pos > nextBreakable) {
+        if (breakNBSP)
+            nextBreakable = nextBreakablePosition(lazyBreakIterator, pos);
+        else
+            nextBreakable = nextBreakablePositionIgnoringNBSP(lazyBreakIterator, pos);
+    }
     return pos == nextBreakable;
 }