Attachment 341323 Details for Bug 185933 – Patch

[patch] Patch

bug-185933-20180525135802.patch (text/plain), 12.12 KB, created by Myles C. Maxfield on 2018-05-25 13:58:03 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Myles C. Maxfield

Created: 2018-05-25 13:58:03 PDT

Size: 12.12 KB

patch

obsolete

>Subversion Revision: 232134
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 894ae700af9d610da747ec7560936e244b7e4c75..619298e2a7e7d5f55df233b624b0e40445c0c257 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,24 @@
>+2018-05-23  Myles C. Maxfield  <mmaxfield@apple.com>
>+
>+        Improve the performance of Font::canRenderCombiningCharacterSequence()
>+        https://bugs.webkit.org/show_bug.cgi?id=185933
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        We don't need to create a whole CTLine just to determine whether or not a font supports rendering a grapheme cluster.
>+        Instead, the right way to do it is just see if the font's cmap table supports every code point in the cluster.
>+
>+        This patch reports a 2% progression on the attached PerformanceTest.
>+
>+        Test: Layout/ComplexLongUnique.html
>+
>+        * platform/graphics/Font.cpp:
>+        (WebCore::Font::canRenderCombiningCharacterSequence const):
>+        * platform/graphics/Font.h:
>+        * platform/graphics/cocoa/FontCocoa.mm:
>+        (WebCore::provideStringAndAttributes): Deleted.
>+        (WebCore::Font::canRenderCombiningCharacterSequence const): Deleted.
>+
> 2018-05-23  Chris Dumez  <cdumez@apple.com>
> 
>         RenderLayer::scrollRectToVisible() should not propagate a subframe's scroll to its cross-origin parent
>diff --git a/Source/WebCore/platform/graphics/ComplexTextController.cpp b/Source/WebCore/platform/graphics/ComplexTextController.cpp
>index f9f18a80fbd97ed33d2d609c3094ea5e15762649..ed09dc33c84f385a49241a0ea0855e230e0c53a1 100644
>--- a/Source/WebCore/platform/graphics/ComplexTextController.cpp
>+++ b/Source/WebCore/platform/graphics/ComplexTextController.cpp
>@@ -262,51 +262,16 @@ 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, UChar32& baseCharacter, unsigned& markCount)
> {
>-    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;
>+    CFStringRef string = CFStringCreateWithCharactersNoCopy(kCFAllocatorDefault, iterator, end - iterator, kCFAllocatorNull);
>+    auto range = CFStringGetRangeOfCharacterClusterAtIndex(string, 0, kCFStringGraphemeCluster);
>+    CFRelease(string);
>+    assert(!range.location);
>+    CFIndex index = 0;
>+    U16_NEXT(iterator, index, range.length, baseCharacter);
>+    markCount = range.length - index;
>+    return iterator + range.length;
> }
> 
> // FIXME: Capitalization is language-dependent and context-dependent and should operate on grapheme clusters instead of codepoints.
>@@ -369,7 +334,8 @@ void ComplexTextController::collectComplexTextRuns()
> 
>     unsigned markCount;
>     UChar32 baseCharacter;
>-    if (!advanceByCombiningCharacterSequence(curr, end, baseCharacter, markCount))
>+    curr = advanceByCombiningCharacterSequence(curr, end, baseCharacter, markCount);
>+    if (U_IS_SURROGATE(baseCharacter))
>         return;
> 
>     nextFont = m_font.fontForCombiningCharacterSequence(cp, curr - cp);
>@@ -394,7 +360,8 @@ void ComplexTextController::collectComplexTextRuns()
>         isSmallCaps = nextIsSmallCaps;
>         unsigned index = curr - cp;
> 
>-        if (!advanceByCombiningCharacterSequence(curr, end, baseCharacter, markCount))
>+        curr = advanceByCombiningCharacterSequence(curr, end, baseCharacter, markCount);
>+        if (U_IS_SURROGATE(baseCharacter))
>             return;
> 
>         if (synthesizedFont) {
>diff --git a/Source/WebCore/platform/graphics/Font.cpp b/Source/WebCore/platform/graphics/Font.cpp
>index 2e2ad390dcc9c014b41427fe0561eb2aa1730da4..19a57caaebc6edc23d9d699e69d39e0280bcb483 100644
>--- a/Source/WebCore/platform/graphics/Font.cpp
>+++ b/Source/WebCore/platform/graphics/Font.cpp
>@@ -201,8 +201,6 @@ static RefPtr<GlyphPage> createAndFillGlyphPage(unsigned pageNumber, const Font&
>         overwriteCodePoint(rightToLeftOverride, zeroWidthSpace);
>         overwriteCodePoint(leftToRightIsolate, zeroWidthSpace);
>         overwriteCodePoint(rightToLeftIsolate, zeroWidthSpace);
>-        overwriteCodePoint(zeroWidthNonJoiner, zeroWidthSpace);
>-        overwriteCodePoint(zeroWidthJoiner, zeroWidthSpace);
>         overwriteCodePoint(popDirectionalFormatting, zeroWidthSpace);
>         overwriteCodePoint(popDirectionalIsolate, zeroWidthSpace);
>         overwriteCodePoint(firstStrongIsolate, zeroWidthSpace);
>@@ -513,4 +511,14 @@ bool Font::variantCapsSupportsCharacterForSynthesis(FontVariantCaps fontVariantC
> }
> #endif
> 
>+bool Font::canRenderCombiningCharacterSequence(const UChar* characters, size_t length) const
>+{
>+    ASSERT(isMainThread());
>+
>+    for (UChar32 codePoint : StringView(characters, length).codePoints()) {
>+        if (!glyphForCharacter(codePoint))
>+            return false;
>+    }
>+    return true;
>+}
> } // namespace WebCore
>diff --git a/Source/WebCore/platform/graphics/Font.h b/Source/WebCore/platform/graphics/Font.h
>index ed432ed46d0be75c4627982b77e4539f24bf41d0..73cdf2a852b7ad8382a70d47c6575df1be9994b0 100644
>--- a/Source/WebCore/platform/graphics/Font.h
>+++ b/Source/WebCore/platform/graphics/Font.h
>@@ -199,10 +199,7 @@ public:
>     const BitVector& glyphsSupportedByAllPetiteCaps() const;
> #endif
> 
>-#if PLATFORM(COCOA) || USE(HARFBUZZ)
>     bool canRenderCombiningCharacterSequence(const UChar*, size_t) const;
>-#endif
>-
>     bool applyTransforms(GlyphBufferGlyph*, GlyphBufferAdvance*, size_t glyphCount, bool enableKerning, bool requiresShaping) const;
> 
> #if PLATFORM(WIN)
>@@ -297,10 +294,6 @@ private:
>     mutable std::optional<BitVector> m_glyphsSupportedByAllPetiteCaps;
> #endif
> 
>-#if PLATFORM(COCOA) || USE(HARFBUZZ)
>-    mutable std::unique_ptr<HashMap<String, bool>> m_combiningCharacterSequenceSupport;
>-#endif
>-
> #if PLATFORM(WIN)
>     mutable SCRIPT_CACHE m_scriptCache;
>     mutable SCRIPT_FONTPROPERTIES* m_scriptFontProperties;
>diff --git a/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm b/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm
>index 13518a051075d02ce07af0cfcfdf1cc7efe0bee0..2239a8358c53273d7ffb98f32b1a2c41d89923c0 100644
>--- a/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm
>+++ b/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm
>@@ -597,53 +597,4 @@ float Font::platformWidthForGlyph(Glyph glyph) const
>     return advance.width + m_syntheticBoldOffset;
> }
> 
>-struct ProviderInfo {
>-    const UChar* characters;
>-    size_t length;
>-    CFDictionaryRef attributes;
>-};
>-
>-static const UniChar* provideStringAndAttributes(CFIndex stringIndex, CFIndex* count, CFDictionaryRef* attributes, void* context)
>-{
>-    ProviderInfo* info = static_cast<struct ProviderInfo*>(context);
>-    if (stringIndex < 0 || static_cast<size_t>(stringIndex) >= info->length)
>-        return 0;
>-
>-    *count = info->length - stringIndex;
>-    *attributes = info->attributes;
>-    return info->characters + stringIndex;
>-}
>-
>-bool Font::canRenderCombiningCharacterSequence(const UChar* characters, size_t length) const
>-{
>-    ASSERT(isMainThread());
>-
>-    if (!m_combiningCharacterSequenceSupport)
>-        m_combiningCharacterSequenceSupport = std::make_unique<HashMap<String, bool>>();
>-
>-    WTF::HashMap<String, bool>::AddResult addResult = m_combiningCharacterSequenceSupport->add(String(characters, length), false);
>-    if (!addResult.isNewEntry)
>-        return addResult.iterator->value;
>-
>-    RetainPtr<CFTypeRef> fontEqualityObject = platformData().objectForEqualityCheck();
>-
>-    ProviderInfo info = { characters, length, getCFStringAttributes(false, platformData().orientation()) };
>-    RetainPtr<CTLineRef> line = adoptCF(CTLineCreateWithUniCharProvider(&provideStringAndAttributes, 0, &info));
>-
>-    CFArrayRef runArray = CTLineGetGlyphRuns(line.get());
>-    CFIndex runCount = CFArrayGetCount(runArray);
>-
>-    for (CFIndex r = 0; r < runCount; r++) {
>-        CTRunRef ctRun = static_cast<CTRunRef>(CFArrayGetValueAtIndex(runArray, r));
>-        ASSERT(CFGetTypeID(ctRun) == CTRunGetTypeID());
>-        CFDictionaryRef runAttributes = CTRunGetAttributes(ctRun);
>-        CTFontRef runFont = static_cast<CTFontRef>(CFDictionaryGetValue(runAttributes, kCTFontAttributeName));
>-        if (!CFEqual(fontEqualityObject.get(), FontPlatformData::objectForEqualityCheck(runFont).get()))
>-            return false;
>-    }
>-
>-    addResult.iterator->value = true;
>-    return true;
>-}
>-
> } // namespace WebCore
>diff --git a/Source/WebCore/workers/service/ServiceWorkerContainer.cpp b/Source/WebCore/workers/service/ServiceWorkerContainer.cpp
>index 115f3371b5be2d554a7b3b5b646e88ec3c04a034..97db453b033facd7eec3889542744fb7175bc75a 100644
>--- a/Source/WebCore/workers/service/ServiceWorkerContainer.cpp
>+++ b/Source/WebCore/workers/service/ServiceWorkerContainer.cpp
>@@ -67,7 +67,8 @@ ServiceWorkerContainer::ServiceWorkerContainer(ScriptExecutionContext& context,
> ServiceWorkerContainer::~ServiceWorkerContainer()
> {
> #ifndef NDEBUG
>-    ASSERT(m_creationThread.ptr() == &Thread::current());
>+    if (m_creationThread.ptr() != &Thread::current())
>+        WTFLogAlways("ASSERT FAILED: (m_creationThread.ptr() == &Thread::current());");
> #endif
> }
> 
>diff --git a/PerformanceTests/ChangeLog b/PerformanceTests/ChangeLog
>index 2a85872d879d87c4a1b0727d9e1f29aad13ca200..db2c7244fa80f59316c06f0ac1c01dc259d1faa0 100644
>--- a/PerformanceTests/ChangeLog
>+++ b/PerformanceTests/ChangeLog
>@@ -1,3 +1,12 @@
>+2018-05-23  Myles C. Maxfield  <mmaxfield@apple.com>
>+
>+        Improve the performance of Font::canRenderCombiningCharacterSequence()
>+        https://bugs.webkit.org/show_bug.cgi?id=185933
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * Layout/ComplexLongUnique.html: Added.
>+
> 2018-05-22  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, rolling out r232052.
>diff --git a/PerformanceTests/Layout/ComplexLongUnique.html b/PerformanceTests/Layout/ComplexLongUnique.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..31d4592bce8bfff5ed7994950565ad4eeb78bd95
>--- /dev/null
>+++ b/PerformanceTests/Layout/ComplexLongUnique.html
>@@ -0,0 +1,35 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<meta charset="utf-8">
>+<script src="../resources/runner.js"></script>
>+</head>
>+<body>
>+<div id="target" style="width: 1000000px; display: none;"></div>
>+<script>
>+var target = document.getElementById("target");
>+var style = target.style;
>+
>+var s = "e\u0300";
>+var length = 10000;
>+var startCode = 0x4E00;
>+for (var i = 0; i < length; ++i) {
>+    s = s + String.fromCharCode(i + startCode) + "\u0300";
>+}
>+
>+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 185933: 341162 | 341167 | 341177 | 341323 | 341335 | 341337 | 341353 | 341358 | 341360 | 341364 | 341372