Attachment 341259 Details for Bug 185976 – Patch

[patch] Patch

bug-185976-20180525005329.patch (text/plain), 11.79 KB, created by Myles C. Maxfield on 2018-05-25 00:53:30 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Myles C. Maxfield

Created: 2018-05-25 00:53:30 PDT

Size: 11.79 KB

patch

obsolete

>Subversion Revision: 232184
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index 7c8ddfc7df9c04798cffaa0dc336374d15bb6418..16cf9876952c25b0935c1cbb0e526260bb79fbe9 100644
>--- a/Source/WTF/ChangeLog
>+++ b/Source/WTF/ChangeLog
>@@ -1,3 +1,19 @@
>+2018-05-25  Myles C. Maxfield  <mmaxfield@apple.com>
>+
>+        Improve the performance of determining character boundaries in ComplexTextController
>+        https://bugs.webkit.org/show_bug.cgi?id=185976
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * wtf/spi/cf/CFStringSPI.h:
>+        * wtf/text/TextBreakIterator.cpp:
>+        (WTF::mapModeToBackingIterator):
>+        * wtf/text/TextBreakIterator.h:
>+        * wtf/text/cf/TextBreakIteratorCF.h:
>+        (WTF::TextBreakIteratorCF::TextBreakIteratorCF):
>+        * wtf/text/mac/TextBreakIteratorInternalICUMac.mm:
>+        (WTF::mapModeToBackingIterator):
>+
> 2018-05-24  Carlos Alberto Lopez Perez  <clopez@igalia.com>
> 
>         [GTK][WPE] Memory pressure monitor doesn't reliable notify all the subprocesses
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 8df21124380e2d1e3a4c4c379501dd49b2c21cb7..026a169232ca16b2d80d0be72ebb96da98057822 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,22 @@
>+2018-05-25  Myles C. Maxfield  <mmaxfield@apple.com>
>+
>+        Improve the performance of determining character boundaries in ComplexTextController
>+        https://bugs.webkit.org/show_bug.cgi?id=185976
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Our previous code iterated over code points ourselves, interrogating each one to try to determine
>+        grapheme cluster boundaries. Because it wasn't very smart, it was reporting too many character
>+        boundaries, which led to us performing too many font lookups and creating too many CTLines.
>+
>+        This patch leads to a 4x performance improvement on the attached PerformanceTest.
>+
>+        Test: Layout/ComplexCharacterFallback.html
>+
>+        * platform/graphics/ComplexTextController.cpp:
>+        (WebCore::advanceByCombiningCharacterSequence):
>+        (WebCore::ComplexTextController::collectComplexTextRuns):
>+
> 2018-05-24  Chris Dumez  <cdumez@apple.com>
> 
>         Avoid doing unnecessary work in Document::shouldEnforceContentDispositionAttachmentSandbox() when setting is disabled
>diff --git a/Source/WTF/wtf/spi/cf/CFStringSPI.h b/Source/WTF/wtf/spi/cf/CFStringSPI.h
>index aae98dba4288f8a5c547152b2a7f7fdab05c7a2a..bdd8bf50e70f38b44d576290d173497dcffb73e2 100644
>--- a/Source/WTF/wtf/spi/cf/CFStringSPI.h
>+++ b/Source/WTF/wtf/spi/cf/CFStringSPI.h
>@@ -38,6 +38,7 @@ extern "C" {
> 
> typedef CF_ENUM(CFIndex, CFStringCharacterClusterType)
> {
>+    kCFStringGraphemeCluster = 1,
>     kCFStringComposedCharacterCluster = 2,
>     kCFStringCursorMovementCluster = 3,
>     kCFStringBackwardDeletionCluster = 4
>diff --git a/Source/WTF/wtf/text/TextBreakIterator.cpp b/Source/WTF/wtf/text/TextBreakIterator.cpp
>index 5b88b44578e47d823fb5888235d10103d0d5f46f..bacce11f0bd256c45755a12642bbd25c14164122 100644
>--- a/Source/WTF/wtf/text/TextBreakIterator.cpp
>+++ b/Source/WTF/wtf/text/TextBreakIterator.cpp
>@@ -44,6 +44,8 @@ static Variant<TextBreakIteratorICU, TextBreakIteratorPlatform> mapModeToBacking
>         return TextBreakIteratorICU(string, TextBreakIteratorICU::Mode::Character, locale.string().utf8().data());
>     case TextBreakIterator::Mode::Delete:
>         return TextBreakIteratorICU(string, TextBreakIteratorICU::Mode::Character, locale.string().utf8().data());
>+    case TextBreakIterator::Mode::GraphemeCluster:
>+        return TextBreakIteratorICU(string, TextBreakIteratorICU::Mode::Character, locale.string().utf8().data());
>     default:
>         ASSERT_NOT_REACHED();
>         return TextBreakIteratorICU(string, TextBreakIteratorICU::Mode::Character, locale.string().utf8().data());
>diff --git a/Source/WTF/wtf/text/TextBreakIterator.h b/Source/WTF/wtf/text/TextBreakIterator.h
>index 392329de231ee83bb3ff380bcc83ef24d14290c1..7231fbcea917563e81155af81443326b460d707a 100644
>--- a/Source/WTF/wtf/text/TextBreakIterator.h
>+++ b/Source/WTF/wtf/text/TextBreakIterator.h
>@@ -47,7 +47,8 @@ public:
>     enum class Mode {
>         Line,
>         Caret,
>-        Delete
>+        Delete,
>+        GraphemeCluster,
>     };
> 
>     TextBreakIterator() = delete;
>diff --git a/Source/WTF/wtf/text/cf/TextBreakIteratorCF.h b/Source/WTF/wtf/text/cf/TextBreakIteratorCF.h
>index 27e50bce5075f9ddadc4fdb9b192f6e834065e66..a17666908791e98322fbce7259b87fa98e691963 100644
>--- a/Source/WTF/wtf/text/cf/TextBreakIteratorCF.h
>+++ b/Source/WTF/wtf/text/cf/TextBreakIteratorCF.h
>@@ -29,7 +29,8 @@ class TextBreakIteratorCF {
> public:
>     enum class Mode {
>         Caret,
>-        Delete
>+        Delete,
>+        GraphemeCluster,
>     };
> 
>     TextBreakIteratorCF(StringView string, Mode mode)
>@@ -42,6 +43,9 @@ public:
>         case Mode::Delete:
>             m_type = kCFStringBackwardDeletionCluster;
>             break;
>+        case Mode::GraphemeCluster:
>+            m_type = kCFStringGraphemeCluster;
>+            break;
>         }
>     }
> 
>diff --git a/Source/WTF/wtf/text/mac/TextBreakIteratorInternalICUMac.mm b/Source/WTF/wtf/text/mac/TextBreakIteratorInternalICUMac.mm
>index 0cac4c9c56f364f885e77aac1528abb58a11cac0..e3d208d196d44b3f711986936d648a5f9d6fa773 100644
>--- a/Source/WTF/wtf/text/mac/TextBreakIteratorInternalICUMac.mm
>+++ b/Source/WTF/wtf/text/mac/TextBreakIteratorInternalICUMac.mm
>@@ -35,6 +35,8 @@ static Variant<TextBreakIteratorICU, TextBreakIteratorPlatform> mapModeToBacking
>         return TextBreakIteratorCF(string, TextBreakIteratorCF::Mode::Caret);
>     case TextBreakIterator::Mode::Delete:
>         return TextBreakIteratorCF(string, TextBreakIteratorCF::Mode::Delete);
>+    case TextBreakIterator::Mode::GraphemeCluster:
>+        return TextBreakIteratorCF(string, TextBreakIteratorCF::Mode::GraphemeCluster);
>     }
> }
> 
>diff --git a/Source/WebCore/platform/graphics/ComplexTextController.cpp b/Source/WebCore/platform/graphics/ComplexTextController.cpp
>index f9f18a80fbd97ed33d2d609c3094ea5e15762649..26e8a8c593285ccf081b0e11707eb46fe765c0a2 100644
>--- a/Source/WebCore/platform/graphics/ComplexTextController.cpp
>+++ b/Source/WebCore/platform/graphics/ComplexTextController.cpp
>@@ -262,51 +262,14 @@ unsigned ComplexTextController::offsetForPosition(float h, bool includePartialGl
>     return 0;
> }
> 
>-// FIXME: We should consider reimplementing this function using ICU to advance by grapheme.
>-// The current implementation only considers explicitly emoji sequences and emoji variations.
>-static bool advanceByCombiningCharacterSequence(const UChar*& iterator, const UChar* end, UChar32& baseCharacter, unsigned& markCount)
>+static const UChar* advanceByCombiningCharacterSequence(const UChar* iterator, const UChar* end, const AtomicString& locale, UChar32& baseCharacter, unsigned& numberOfCodeUnitsAfterBaseCharacter)
> {
>-    ASSERT(iterator < end);
>-
>-    markCount = 0;
>-
>-    unsigned i = 0;
>-    unsigned remainingCharacters = end - iterator;
>-    U16_NEXT(iterator, i, remainingCharacters, baseCharacter);
>-    iterator = iterator + i;
>-    if (U_IS_SURROGATE(baseCharacter))
>-        return false;
>-
>-    // Consume marks.
>-    bool sawEmojiGroupCandidate = isEmojiGroupCandidate(baseCharacter);
>-    bool sawJoiner = false;
>-    while (iterator < end) {
>-        UChar32 nextCharacter;
>-        unsigned markLength = 0;
>-        bool shouldContinue = false;
>-        ASSERT(end >= iterator);
>-        U16_NEXT(iterator, markLength, static_cast<unsigned>(end - iterator), nextCharacter);
>-
>-        if (isVariationSelector(nextCharacter) || isEmojiFitzpatrickModifier(nextCharacter))
>-            shouldContinue = true;
>-
>-        if (sawJoiner && isEmojiGroupCandidate(nextCharacter))
>-            shouldContinue = true;
>-
>-        sawJoiner = false;
>-        if (sawEmojiGroupCandidate && nextCharacter == zeroWidthJoiner) {
>-            sawJoiner = true;
>-            shouldContinue = true;
>-        }
>-        
>-        if (!shouldContinue && !(U_GET_GC_MASK(nextCharacter) & U_GC_M_MASK))
>-            break;
>-
>-        markCount += markLength;
>-        iterator += markLength;
>-    }
>-
>-    return true;
>+    CachedTextBreakIterator textBreakIterator(StringView(iterator, end - iterator), TextBreakIterator::Mode::GraphemeCluster, locale);
>+    auto clusterLength = textBreakIterator.following(0).value_or(end - iterator);
>+    unsigned offset = 0;
>+    U16_NEXT(iterator, offset, clusterLength, baseCharacter);
>+    numberOfCodeUnitsAfterBaseCharacter = (end - iterator) - offset;
>+    return iterator + clusterLength;
> }
> 
> // FIXME: Capitalization is language-dependent and context-dependent and should operate on grapheme clusters instead of codepoints.
>@@ -367,9 +330,10 @@ void ComplexTextController::collectComplexTextRuns()
>     const Font* synthesizedFont = nullptr;
>     const Font* smallSynthesizedFont = nullptr;
> 
>-    unsigned markCount;
>+    unsigned numberOfCodeUnitsAfterBaseCharacter;
>     UChar32 baseCharacter;
>-    if (!advanceByCombiningCharacterSequence(curr, end, baseCharacter, markCount))
>+    curr = advanceByCombiningCharacterSequence(curr, end, m_font.fontDescription().locale(), baseCharacter, numberOfCodeUnitsAfterBaseCharacter);
>+    if (U_IS_SURROGATE(baseCharacter))
>         return;
> 
>     nextFont = m_font.fontForCombiningCharacterSequence(cp, curr - cp);
>@@ -394,14 +358,15 @@ void ComplexTextController::collectComplexTextRuns()
>         isSmallCaps = nextIsSmallCaps;
>         unsigned index = curr - cp;
> 
>-        if (!advanceByCombiningCharacterSequence(curr, end, baseCharacter, markCount))
>+        curr = advanceByCombiningCharacterSequence(curr, end, m_font.fontDescription().locale(), baseCharacter, numberOfCodeUnitsAfterBaseCharacter);
>+        if (U_IS_SURROGATE(baseCharacter))
>             return;
> 
>         if (synthesizedFont) {
>             if (auto capitalizedBase = capitalized(baseCharacter)) {
>                 unsigned characterIndex = index;
>                 U16_APPEND_UNSAFE(m_smallCapsBuffer, characterIndex, capitalizedBase.value());
>-                for (unsigned i = 0; i < markCount; ++i)
>+                for (unsigned i = 0; i < numberOfCodeUnitsAfterBaseCharacter; ++i)
>                     m_smallCapsBuffer[i + characterIndex] = cp[i + characterIndex];
>                 nextIsSmallCaps = true;
>             } else {
>diff --git a/PerformanceTests/ChangeLog b/PerformanceTests/ChangeLog
>index 2a85872d879d87c4a1b0727d9e1f29aad13ca200..2576e04989f9b97633d524a4a481abd90a445478 100644
>--- a/PerformanceTests/ChangeLog
>+++ b/PerformanceTests/ChangeLog
>@@ -1,3 +1,12 @@
>+2018-05-25  Myles C. Maxfield  <mmaxfield@apple.com>
>+
>+        Improve the performance of determining character boundaries in ComplexTextController
>+        https://bugs.webkit.org/show_bug.cgi?id=185976
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * Layout/ComplexCharacterFallback.html: Added.
>+
> 2018-05-22  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, rolling out r232052.
>diff --git a/PerformanceTests/Layout/ComplexCharacterFallback.html b/PerformanceTests/Layout/ComplexCharacterFallback.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..e87fe877c3a8077a25b3267e008853721782372d
>--- /dev/null
>+++ b/PerformanceTests/Layout/ComplexCharacterFallback.html
>@@ -0,0 +1,34 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<meta charset="utf-8">
>+<script src="../resources/runner.js"></script>
>+</head>
>+<body>
>+<div id="target" style="width: 300px; display: none;"></div>
>+<script>
>+var target = document.getElementById("target");
>+var style = target.style;
>+
>+var s = "";
>+for (var i = 0; i < 100; ++i) {
>+    s = s + "\u0C1C\u0C4D\u0C1E\u200C\u0C3E";
>+}
>+
>+
>+function test() {
>+    if (window.internals)
>+        window.internals.invalidateFontCache();
>+
>+    style.display = "block";
>+    target.offsetLeft;
>+    target.textContent = s;
>+    target.offsetLeft;
>+    target.textContent = "";
>+    style.display = "none";
>+}
>+
>+PerfTestRunner.measureRunsPerSecond({ run: test });
>+</script>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 185976: 341258 | 341259 | 341261 | 341263 | 341265 | 341370 | 341375 | 341379 | 341380 | 341387 | 341622 | 341630 | 341632 | 341634 | 341635 | 341667 | 341670 | 341671 | 341675 | 341718