Attachment 288365 Details for Bug 161119 – Patch

[patch] Patch

bug-161119-20160908171927.patch (text/plain), 18.98 KB, created by Myles C. Maxfield on 2016-09-08 17:21:54 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Myles C. Maxfield

Created: 2016-09-08 17:21:54 PDT

Size: 18.98 KB

patch

obsolete

>Subversion Revision: 205668
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 05a3197527bc0efef902bfd1f7e3338239971cc8..adcaf3db9d68663e3607fec0c730c7f82f728db4 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,30 @@
>+2016-09-08  Myles C. Maxfield  <mmaxfield@apple.com>
>+
>+        [Cocoa] Improve performance of glyph advance metrics gathering
>+        https://bugs.webkit.org/show_bug.cgi?id=161119
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Most of the glyphs in a GlyphPage are never read from. Therefore, we can get a performance boost
>+        by not populating as many items in the GlyphPage. Because of the performance characteristics of
>+        CTFontGetGlyphsForCharacters(), a better size for a GlyphPage is 16 items. This, coupled with
>+        using CTFontGetUnsummedAdvancesForGlyphsAndStyle(), causes between a 0.01%-0.5% speedup on PLT.
>+
>+        No new tests because there is no behavior change.
>+
>+        * platform/graphics/Font.cpp:
>+        (WebCore::Font::initCharWidths):
>+        (WebCore::Font::platformGlyphInit):
>+        (WebCore::createAndFillGlyphPage):
>+        * platform/graphics/Font.h:
>+        (WebCore::Font::widthForGlyph):
>+        * platform/graphics/GlyphMetricsMap.h:
>+        * platform/graphics/GlyphPage.h:
>+        * platform/graphics/cocoa/FontCocoa.mm:
>+        * platform/spi/cocoa/CoreTextSPI.h:
>+        (WebCore::Font::platformWidthForGlyph):
>+        (WebCore::canUseFastGlyphAdvanceGetter): Deleted.
>+
> 2016-09-08  Alex Christensen  <achristensen@webkit.org>
> 
>         URLParser should correctly handle \ in path
>diff --git a/Source/WebCore/platform/graphics/Font.cpp b/Source/WebCore/platform/graphics/Font.cpp
>index f9914e50fb7cad2c709d2acc98b772b4f27a4c75..f59aa9e61a963947a6ea32d8c76c0c3ae880ea25 100644
>--- a/Source/WebCore/platform/graphics/Font.cpp
>+++ b/Source/WebCore/platform/graphics/Font.cpp
>@@ -81,12 +81,11 @@ Font::Font(const FontPlatformData& platformData, bool isCustomFont, bool isLoadi
> // Estimates of avgCharWidth and maxCharWidth for platforms that don't support accessing these values from the font.
> void Font::initCharWidths()
> {
>-    auto* glyphPageZero = glyphPage(0);
>+    auto* glyphPageZero = glyphPage(GlyphPage::pageNumberForCodePoint('0'));
> 
>     // Treat the width of a '0' as the avgCharWidth.
>     if (m_avgCharWidth <= 0.f && glyphPageZero) {
>-        static const UChar32 digitZeroChar = '0';
>-        Glyph digitZeroGlyph = glyphPageZero->glyphDataForCharacter(digitZeroChar).glyph;
>+        Glyph digitZeroGlyph = glyphPageZero->glyphDataForCharacter('0').glyph;
>         if (digitZeroGlyph)
>             m_avgCharWidth = widthForGlyph(digitZeroGlyph);
>     }
>@@ -102,22 +101,23 @@ void Font::initCharWidths()
> void Font::platformGlyphInit()
> {
>     auto* glyphPageZero = glyphPage(0);
>-    if (!glyphPageZero) {
>-        determinePitch();
>-        return;
>-    }
>+    auto* glyphPageCharacterZero = glyphPage(GlyphPage::pageNumberForCodePoint('0'));
>+    auto* glyphPageSpace = glyphPage(GlyphPage::pageNumberForCodePoint(space));
> 
>     // Ask for the glyph for 0 to avoid paging in ZERO WIDTH SPACE. Control characters, including 0,
>     // are mapped to the ZERO WIDTH SPACE glyph.
>-    m_zeroWidthSpaceGlyph = glyphPageZero->glyphDataForCharacter(0).glyph;
>+    if (glyphPageZero)
>+        m_zeroWidthSpaceGlyph = glyphPageZero->glyphDataForCharacter(0).glyph;
> 
>     // Nasty hack to determine if we should round or ceil space widths.
>     // If the font is monospace or fake monospace we ceil to ensure that 
>     // every character and the space are the same width. Otherwise we round.
>-    m_spaceGlyph = glyphPageZero->glyphDataForCharacter(' ').glyph;
>+    if (glyphPageSpace)
>+        m_spaceGlyph = glyphPageSpace->glyphDataForCharacter(space).glyph;
>     float width = widthForGlyph(m_spaceGlyph);
>     m_spaceWidth = width;
>-    m_zeroGlyph = glyphPageZero->glyphDataForCharacter('0').glyph;
>+    if (glyphPageCharacterZero)
>+        m_zeroGlyph = glyphPageCharacterZero->glyphDataForCharacter('0').glyph;
>     m_fontMetrics.setZeroWidth(widthForGlyph(m_zeroGlyph));
>     determinePitch();
>     m_adjustedSpaceWidth = m_treatAsFixedPitch ? ceilf(width) : roundf(width);
>@@ -152,56 +152,59 @@ static RefPtr<GlyphPage> createAndFillGlyphPage(unsigned pageNumber, const Font&
>     // FIXME: Times New Roman contains Arabic glyphs, but Core Text doesn't know how to shape them. See <rdar://problem/9823975>.
>     // Once we have the fix for <rdar://problem/9823975> then remove this code together with Font::shouldNotBeUsedForArabic()
>     // in <rdar://problem/12096835>.
>-    if (pageNumber == 6 && font.shouldNotBeUsedForArabic())
>+    if (GlyphPage::pageNumberIsUsedForArabic(pageNumber) && font.shouldNotBeUsedForArabic())
>         return nullptr;
> #endif
> 
>-    unsigned start = pageNumber * GlyphPage::size;
>-    UChar buffer[GlyphPage::size * 2 + 2];
>+    unsigned glyphPageSize = GlyphPage::sizeForPageNumber(pageNumber);
>+
>+    unsigned start = GlyphPage::startingCodePointInPageNumber(pageNumber);
>+    unsigned end = start + glyphPageSize;
>+    UChar buffer[glyphPageSize * 2 + 2];
>     unsigned bufferLength;
>     // Fill in a buffer with the entire "page" of characters that we want to look up glyphs for.
>     if (U_IS_BMP(start)) {
>-        bufferLength = GlyphPage::size;
>-        for (unsigned i = 0; i < GlyphPage::size; i++)
>+        bufferLength = glyphPageSize;
>+        for (unsigned i = 0; i < bufferLength; i++)
>             buffer[i] = start + i;
> 
>-        if (!start) {
>-            // Control characters must not render at all.
>-            for (unsigned i = 0; i < 0x20; ++i)
>-                buffer[i] = zeroWidthSpace;
>-            for (unsigned i = 0x7F; i < 0xA0; i++)
>-                buffer[i] = zeroWidthSpace;
>-            buffer[softHyphen] = zeroWidthSpace;
>-
>-            // \n, \t, and nonbreaking space must render as a space.
>-            buffer[(int)'\n'] = ' ';
>-            buffer[(int)'\t'] = ' ';
>-            buffer[noBreakSpace] = ' ';
>-        } else if (start == (leftToRightMark & ~(GlyphPage::size - 1))) {
>-            // LRM, RLM, LRE, RLE, ZWNJ, ZWJ, and PDF must not render at all.
>-            buffer[leftToRightMark - start] = zeroWidthSpace;
>-            buffer[rightToLeftMark - start] = zeroWidthSpace;
>-            buffer[leftToRightEmbed - start] = zeroWidthSpace;
>-            buffer[rightToLeftEmbed - start] = zeroWidthSpace;
>-            buffer[leftToRightOverride - start] = zeroWidthSpace;
>-            buffer[rightToLeftOverride - start] = zeroWidthSpace;
>-            buffer[leftToRightIsolate - start] = zeroWidthSpace;
>-            buffer[rightToLeftIsolate - start] = zeroWidthSpace;
>-            buffer[zeroWidthNonJoiner - start] = zeroWidthSpace;
>-            buffer[zeroWidthJoiner - start] = zeroWidthSpace;
>-            buffer[popDirectionalFormatting - start] = zeroWidthSpace;
>-            buffer[popDirectionalIsolate - start] = zeroWidthSpace;
>-            buffer[firstStrongIsolate - start] = zeroWidthSpace;
>-        } else if (start == (objectReplacementCharacter & ~(GlyphPage::size - 1))) {
>-            // Object replacement character must not render at all.
>-            buffer[objectReplacementCharacter - start] = zeroWidthSpace;
>-        } else if (start == (zeroWidthNoBreakSpace & ~(GlyphPage::size - 1))) {
>-            // ZWNBS/BOM must not render at all.
>-            buffer[zeroWidthNoBreakSpace - start] = zeroWidthSpace;
>-        }
>+        // Code points 0x0 - 0x20 and 0x7F - 0xA0 are control character and shouldn't render. Map them to ZERO WIDTH SPACE.
>+        auto overwriteCodePoints = [&](unsigned minimum, unsigned maximum, UChar newCodePoint) {
>+            unsigned begin = std::max(start, minimum);
>+            unsigned complete = std::min(end, maximum);
>+            for (unsigned i = begin; i < complete; ++i)
>+                buffer[i - start] = newCodePoint;
>+        };
>+
>+        auto overwriteCodePoint = [&](UChar codePoint, UChar newCodePoint) {
>+            if (codePoint >= start && codePoint < end)
>+                buffer[codePoint - start] = newCodePoint;
>+        };
>+
>+        overwriteCodePoints(0x0, 0x20, zeroWidthSpace);
>+        overwriteCodePoints(0x7F, 0xA0, zeroWidthSpace);
>+        overwriteCodePoint(softHyphen, zeroWidthSpace);
>+        overwriteCodePoint('\n', space);
>+        overwriteCodePoint('\t', space);
>+        overwriteCodePoint(noBreakSpace, space);
>+        overwriteCodePoint(leftToRightMark, zeroWidthSpace);
>+        overwriteCodePoint(rightToLeftMark, zeroWidthSpace);
>+        overwriteCodePoint(leftToRightEmbed, zeroWidthSpace);
>+        overwriteCodePoint(rightToLeftEmbed, zeroWidthSpace);
>+        overwriteCodePoint(leftToRightOverride, zeroWidthSpace);
>+        overwriteCodePoint(rightToLeftOverride, zeroWidthSpace);
>+        overwriteCodePoint(leftToRightIsolate, zeroWidthSpace);
>+        overwriteCodePoint(rightToLeftIsolate, zeroWidthSpace);
>+        overwriteCodePoint(zeroWidthNonJoiner, zeroWidthSpace);
>+        overwriteCodePoint(zeroWidthJoiner, zeroWidthSpace);
>+        overwriteCodePoint(popDirectionalFormatting, zeroWidthSpace);
>+        overwriteCodePoint(popDirectionalIsolate, zeroWidthSpace);
>+        overwriteCodePoint(firstStrongIsolate, zeroWidthSpace);
>+        overwriteCodePoint(objectReplacementCharacter, zeroWidthSpace);
>+        overwriteCodePoint(zeroWidthNoBreakSpace, zeroWidthSpace);
>     } else {
>-        bufferLength = GlyphPage::size * 2;
>-        for (unsigned i = 0; i < GlyphPage::size; i++) {
>+        bufferLength = glyphPageSize * 2;
>+        for (unsigned i = 0; i < glyphPageSize; i++) {
>             int c = i + start;
>             buffer[i * 2] = U16_LEAD(c);
>             buffer[i * 2 + 1] = U16_TRAIL(c);
>@@ -238,7 +241,7 @@ const GlyphPage* Font::glyphPage(unsigned pageNumber) const
> 
> Glyph Font::glyphForCharacter(UChar32 character) const
> {
>-    auto* page = glyphPage(character / GlyphPage::size);
>+    auto* page = glyphPage(GlyphPage::pageNumberForCodePoint(character));
>     if (!page)
>         return 0;
>     return page->glyphForCharacter(character);
>@@ -246,7 +249,7 @@ Glyph Font::glyphForCharacter(UChar32 character) const
> 
> GlyphData Font::glyphDataForCharacter(UChar32 character) const
> {
>-    auto* page = glyphPage(character / GlyphPage::size);
>+    auto* page = glyphPage(GlyphPage::pageNumberForCodePoint(character));
>     if (!page)
>         return GlyphData();
>     return page->glyphDataForCharacter(character);
>diff --git a/Source/WebCore/platform/graphics/Font.h b/Source/WebCore/platform/graphics/Font.h
>index b49c4d7029d7ba0a688b9cd46947790be3cfd040..918dc99087f8b350af2d1b8d779a40634f954253 100644
>--- a/Source/WebCore/platform/graphics/Font.h
>+++ b/Source/WebCore/platform/graphics/Font.h
>@@ -337,13 +337,13 @@ ALWAYS_INLINE float Font::widthForGlyph(Glyph glyph) const
>         return width;
> 
> #if ENABLE(OPENTYPE_VERTICAL)
>-    if (m_verticalData)
>+    if (m_verticalData) {
> #if USE(CG) || USE(CAIRO)
>         width = m_verticalData->advanceHeight(this, glyph) + m_syntheticBoldOffset;
> #else
>         width = m_verticalData->advanceHeight(this, glyph);
> #endif
>-    else
>+    } else
> #endif
>         width = platformWidthForGlyph(glyph);
> 
>diff --git a/Source/WebCore/platform/graphics/FontCascadeFonts.cpp b/Source/WebCore/platform/graphics/FontCascadeFonts.cpp
>index d36ed09274e3b7da852bea2caf9482bd4e1c8ef2..3d5455b2f76e40583cc962551e538cf005cdf9cb 100644
>--- a/Source/WebCore/platform/graphics/FontCascadeFonts.cpp
>+++ b/Source/WebCore/platform/graphics/FontCascadeFonts.cpp
>@@ -48,14 +48,14 @@ public:
> 
>     GlyphData glyphDataForCharacter(UChar32 c) const
>     {
>-        unsigned index = GlyphPage::indexForCharacter(c);
>+        unsigned index = GlyphPage::indexForCodePoint(c);
>         ASSERT_WITH_SECURITY_IMPLICATION(index < GlyphPage::size);
>         return { m_glyphs[index], m_fonts[index] };
>     }
> 
>     void setGlyphDataForCharacter(UChar32 c, GlyphData glyphData)
>     {
>-        setGlyphDataForIndex(GlyphPage::indexForCharacter(c), glyphData);
>+        setGlyphDataForIndex(GlyphPage::indexForCodePoint(c), glyphData);
>     }
> 
> private:
>@@ -428,7 +428,7 @@ GlyphData FontCascadeFonts::glyphDataForCharacter(UChar32 c, const FontCascadeDe
>     if (variant != NormalVariant)
>         return glyphDataForVariant(c, description, variant, 0);
> 
>-    const unsigned pageNumber = c / GlyphPage::size;
>+    const unsigned pageNumber = GlyphPage::pageNumberForCodePoint(c);
> 
>     auto& cacheEntry = pageNumber ? m_cachedPages.add(pageNumber, GlyphPageCacheEntry()).iterator->value : m_cachedPageZero;
> 
>diff --git a/Source/WebCore/platform/graphics/GlyphMetricsMap.h b/Source/WebCore/platform/graphics/GlyphMetricsMap.h
>index 730c1d3a6124a920eecd6fa2226cf2d752daa1c4..4f0f58a587216068771c4a47bcf8d92ba5f5899b 100644
>--- a/Source/WebCore/platform/graphics/GlyphMetricsMap.h
>+++ b/Source/WebCore/platform/graphics/GlyphMetricsMap.h
>@@ -54,7 +54,7 @@ private:
>     class GlyphMetricsPage {
>         WTF_MAKE_FAST_ALLOCATED;
>     public:
>-        static const size_t size = 256; // Usually covers Latin-1 in a single page.
>+        static const size_t size = 16;
> 
>         GlyphMetricsPage() = default;
>         explicit GlyphMetricsPage(const T& initialValue)
>diff --git a/Source/WebCore/platform/graphics/GlyphPage.h b/Source/WebCore/platform/graphics/GlyphPage.h
>index 99e618be93b3879067c2d67574b926ce61f46ed8..1a3c10cc5489f6d501bf1e432fe9782b7f1950bc 100644
>--- a/Source/WebCore/platform/graphics/GlyphPage.h
>+++ b/Source/WebCore/platform/graphics/GlyphPage.h
>@@ -57,7 +57,7 @@ struct GlyphData {
> 
> // A GlyphPage contains a fixed-size set of GlyphData mappings for a contiguous
> // range of characters in the Unicode code space. GlyphPages are indexed
>-// starting from 0 and incrementing for each 256 glyphs.
>+// starting from 0 and incrementing for each "size" number of glyphs.
> class GlyphPage : public RefCounted<GlyphPage> {
> public:
>     static Ref<GlyphPage> create(const Font& font)
>@@ -72,17 +72,22 @@ public:
> 
>     static unsigned count() { return s_count; }
> 
>-    static const size_t size = 256; // Covers Latin-1 in a single page.
>-    static unsigned indexForCharacter(UChar32 c) { return c % GlyphPage::size; }
>+    static const unsigned size = 16;
>+
>+    static unsigned sizeForPageNumber(unsigned) { return 16; }
>+    static unsigned indexForCodePoint(UChar32 c) { return c % size; }
>+    static unsigned pageNumberForCodePoint(UChar32 c) { return c / size; }
>+    static UChar32 startingCodePointInPageNumber(unsigned pageNumber) { return pageNumber * size; }
>+    static bool pageNumberIsUsedForArabic(unsigned pageNumber) { return pageNumber >= 0x60 && pageNumber < 0x70; }
> 
>     GlyphData glyphDataForCharacter(UChar32 c) const
>     {
>-        return glyphDataForIndex(indexForCharacter(c));
>+        return glyphDataForIndex(indexForCodePoint(c));
>     }
> 
>     Glyph glyphForCharacter(UChar32 c) const
>     {
>-        return glyphForIndex(indexForCharacter(c));
>+        return glyphForIndex(indexForCodePoint(c));
>     }
> 
>     GlyphData glyphDataForIndex(unsigned index) const
>diff --git a/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm b/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm
>index f3001f5061ca1d87870bb690ecdf5563c59785a1..c0db4cc9df5249c9931e121206415c3f56b735f1 100644
>--- a/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm
>+++ b/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm
>@@ -596,6 +596,7 @@ FloatRect Font::platformBoundsForGlyph(Glyph glyph) const
>     return boundingBox;
> }
> 
>+#if !((PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000))
> static inline Optional<CGSize> advanceForColorBitmapFont(const FontPlatformData& platformData, Glyph glyph)
> {
>     CTFontRef font = platformData.font();
>@@ -617,16 +618,30 @@ static inline bool canUseFastGlyphAdvanceGetter(const FontPlatformData& platform
>     }
>     return true;
> }
>+#endif
> 
> float Font::platformWidthForGlyph(Glyph glyph) const
> {
>     CGSize advance = CGSizeZero;
>     bool horizontal = platformData().orientation() == Horizontal;
>+    CGFontRenderingStyle style = kCGFontRenderingStyleAntialiasing | kCGFontRenderingStyleSubpixelPositioning | kCGFontRenderingStyleSubpixelQuantization | kCGFontAntialiasingStyleUnfiltered;
>+
>+#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000)
>+    if (platformData().size()) {
>+        CTFontOrientation orientation = horizontal || m_isBrokenIdeographFallback ? kCTFontOrientationHorizontal : kCTFontOrientationVertical;
>+        // FIXME: Remove this special-casing when <rdar://problem/28197291> is fixed.
>+        if (CTFontIsAppleColorEmoji(m_platformData.ctFont()))
>+            CTFontGetAdvancesForGlyphs(m_platformData.ctFont(), orientation, &glyph, &advance, 1);
>+        else
>+            CTFontGetUnsummedAdvancesForGlyphsAndStyle(m_platformData.ctFont(), orientation, style, &glyph, &advance, 1);
>+    }
>+
>+#else
>+
>     bool populatedAdvance = false;
>     if ((horizontal || m_isBrokenIdeographFallback) && canUseFastGlyphAdvanceGetter(this->platformData(), glyph, advance, populatedAdvance)) {
>         float pointSize = platformData().size();
>         CGAffineTransform m = CGAffineTransformMakeScale(pointSize, pointSize);
>-        CGFontRenderingStyle style = kCGFontRenderingStyleAntialiasing | kCGFontRenderingStyleSubpixelPositioning | kCGFontRenderingStyleSubpixelQuantization | kCGFontAntialiasingStyleUnfiltered;
>         if (!CGFontGetGlyphAdvancesForStyle(platformData().cgFont(), &m, style, &glyph, 1, &advance)) {
>             RetainPtr<CFStringRef> fullName = adoptCF(CGFontCopyFullName(platformData().cgFont()));
>             LOG_ERROR("Unable to cache glyph widths for %@ %f", fullName.get(), pointSize);
>@@ -634,6 +649,7 @@ float Font::platformWidthForGlyph(Glyph glyph) const
>         }
>     } else if (!populatedAdvance && platformData().size())
>         CTFontGetAdvancesForGlyphs(m_platformData.ctFont(), horizontal ? kCTFontOrientationHorizontal : kCTFontOrientationVertical, &glyph, &advance, 1);
>+#endif
> 
>     return advance.width + m_syntheticBoldOffset;
> }
>diff --git a/Source/WebCore/platform/spi/cocoa/CoreTextSPI.h b/Source/WebCore/platform/spi/cocoa/CoreTextSPI.h
>index 951a7e085da6b947f7634e7254c506c82fefcad3..afd09ee08ab53c4b619cca1d7d2664145aacf3ad 100644
>--- a/Source/WebCore/platform/spi/cocoa/CoreTextSPI.h
>+++ b/Source/WebCore/platform/spi/cocoa/CoreTextSPI.h
>@@ -71,6 +71,7 @@ CTLineRef CTLineCreateWithUniCharProvider(CTUniCharProviderCallback provide, CTU
> void CTRunGetBaseAdvancesAndOrigins(CTRunRef, CFRange, CGSize baseAdvances[], CGPoint origins[]);
> CTTypesetterRef CTTypesetterCreateWithUniCharProviderAndOptions(CTUniCharProviderCallback provide, CTUniCharDisposeCallback dispose, void* refCon, CFDictionaryRef options);
> bool CTFontGetVerticalGlyphsForCharacters(CTFontRef, const UniChar characters[], CGGlyph glyphs[], CFIndex count);
>+void CTFontGetUnsummedAdvancesForGlyphsAndStyle(CTFontRef, CTFontOrientation, CGFontRenderingStyle, const CGGlyph[], CGSize advances[], CFIndex count);
> 
> CTFontDescriptorRef CTFontDescriptorCreateForUIType(CTFontUIFontType, CGFloat size, CFStringRef language);
> CTFontDescriptorRef CTFontDescriptorCreateWithTextStyle(CFStringRef style, CFStringRef size, CFStringRef language);

Flags:

simon.fraser: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 161119: 286808 | 286823 | 286828 | 287002 | 287003 | 287008 | 287012 | 287013 | 287047 | 287050 | 287086 | 287117 | 287122 | 287214 | 287922 | 288169 | 288183 | 288210 | 288223 | 288228 | 288230 | 288231 | 288282 | 288293 | 288295 | 288297 | 288299 | 288319 | 288330 | 288343 | 288365 | 288368 | 288392 | 288393