Attachment 288223 Details for Bug 161119 – GlyphPage size

[patch] GlyphPage size

bug-161119-20160907190354.patch (text/plain), 9.81 KB, created by Myles C. Maxfield on 2016-09-07 19:06:19 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Myles C. Maxfield

Created: 2016-09-07 19:06:19 PDT

Size: 9.81 KB

patch

obsolete

>Subversion Revision: 205533
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 16911c1e0a09d5e4f4db7cb17576c89ff7399942..3c9e55ab39ff5fa5e7bfd62a22a12557d8c198ae 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,30 @@
>+2016-09-07  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!).
>+
>+        No new tests (OOPS!).
>+
>+        * platform/graphics/Font.cpp:
>+        (WebCore::Font::platformGlyphInit):
>+        * platform/graphics/Font.h:
>+        (WebCore::Font::widthForGlyph):
>+        * platform/graphics/GlyphPage.h:
>+        (WebCore::GlyphData::GlyphData):
>+        (WebCore::GlyphPage::advanceForCharacter):
>+        (WebCore::GlyphPage::glyphDataForIndex):
>+        (WebCore::GlyphPage::advanceForIndex):
>+        (WebCore::GlyphPage::setGlyphForIndex):
>+        * platform/graphics/WidthIterator.cpp:
>+        (WebCore::WidthIterator::advanceInternal):
>+        * platform/graphics/cocoa/FontCocoa.mm:
>+        (WebCore::Font::platformWidthForGlyph):
>+        * platform/graphics/mac/GlyphPageMac.cpp:
>+        (WebCore::GlyphPage::fill):
>+        * platform/spi/cocoa/CoreTextSPI.h:
>+
> 2016-09-06  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r205521, r205526, and r205532.
>diff --git a/Source/WebCore/platform/graphics/Font.cpp b/Source/WebCore/platform/graphics/Font.cpp
>index f9914e50fb7cad2c709d2acc98b772b4f27a4c75..44aaa1ffeb207a70d5cf9bdf88d7feda3fa077cd 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('0' / GlyphPage::size);
> 
>     // 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,7 +101,9 @@ void Font::initCharWidths()
> void Font::platformGlyphInit()
> {
>     auto* glyphPageZero = glyphPage(0);
>-    if (!glyphPageZero) {
>+    auto* glyphPageCharacterZero = glyphPage('0' / GlyphPage::size);
>+    auto* glyphPageSpace = glyphPage(space / GlyphPage::size);
>+    if (!glyphPageZero || !glyphPageCharacterZero || !glyphPageSpace) {
>         determinePitch();
>         return;
>     }
>@@ -114,10 +115,10 @@ void Font::platformGlyphInit()
>     // 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;
>+    m_spaceGlyph = glyphPageSpace->glyphDataForCharacter(space).glyph;
>     float width = widthForGlyph(m_spaceGlyph);
>     m_spaceWidth = width;
>-    m_zeroGlyph = glyphPageZero->glyphDataForCharacter('0').glyph;
>+    m_zeroGlyph = glyphPageCharacterZero->glyphDataForCharacter('0').glyph;
>     m_fontMetrics.setZeroWidth(widthForGlyph(m_zeroGlyph));
>     determinePitch();
>     m_adjustedSpaceWidth = m_treatAsFixedPitch ? ceilf(width) : roundf(width);
>@@ -152,7 +153,7 @@ 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 (pageNumber >= 0x60 && pageNumber < 0x70 && && font.shouldNotBeUsedForArabic())
>         return nullptr;
> #endif
> 
>@@ -165,33 +166,44 @@ static RefPtr<GlyphPage> createAndFillGlyphPage(unsigned pageNumber, const Font&
>         for (unsigned i = 0; i < GlyphPage::size; i++)
>             buffer[i] = start + i;
> 
>+        static_assert(GlyphPage::size == 16 && !(GlyphPage::size & (GlyphPage::size - 1)), "createAndFillGlyphPage() assumes GlyphPage::size is 16.");
>         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++)
>+            for (unsigned i = 0; i < 0x10; ++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] = ' ';
>+            buffer[static_cast<int>('\t')] = space;
>+            buffer[static_cast<int>('\n')] = space;
>+        } else if (start == 1) {
>+            // Control characters must not render at all.
>+            for (unsigned i = 0; i < 0x10; ++i)
>+                buffer[i] = zeroWidthSpace;
>+        } else if (start == (0x7F & ~(GlyphPage::size - 1))) {
>+            buffer[0x7F - start] = zeroWidthSpace;
>+        } else if (pageNumber >= 8 && pageNumber < 10) {
>+            for (unsigned i = 0; i < GlyphPage::size; ++i)
>+                buffer[i] = zeroWidthSpace;
>+        } else if (pageNumber == 10) {
>+            buffer[softHyphen - start] = zeroWidthSpace;
>+            buffer[noBreakSpace - start] = space;
>         } 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;
>+        } else if (start == (leftToRightEmbed & ~(GlyphPage::size - 1))) {
>             buffer[leftToRightEmbed - start] = zeroWidthSpace;
>             buffer[rightToLeftEmbed - start] = zeroWidthSpace;
>             buffer[leftToRightOverride - start] = zeroWidthSpace;
>             buffer[rightToLeftOverride - start] = zeroWidthSpace;
>+            buffer[popDirectionalFormatting - start] = zeroWidthSpace;
>+        } else if (start == (leftToRightIsolate & ~(GlyphPage::size - 1))) {
>             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 == (zeroWidthNonJoiner & ~(GlyphPage::size - 1))) {
>+            buffer[zeroWidthNonJoiner - start] = zeroWidthSpace;
>+            buffer[zeroWidthJoiner - start] = zeroWidthSpace;
>         } else if (start == (objectReplacementCharacter & ~(GlyphPage::size - 1))) {
>             // Object replacement character must not render at all.
>             buffer[objectReplacementCharacter - start] = zeroWidthSpace;
>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/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..59f45e24775ab61ae7c9a786d1673ea32eb7a4e6 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,7 +72,7 @@ public:
> 
>     static unsigned count() { return s_count; }
> 
>-    static const size_t size = 256; // Covers Latin-1 in a single page.
>+    static const size_t size = 16;
>     static unsigned indexForCharacter(UChar32 c) { return c % GlyphPage::size; }
> 
>     GlyphData glyphDataForCharacter(UChar32 c) const

Flags:

buildbot: commit-queue-

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