Changeset 143137 in webkit

Timestamp:

Feb 17, 2013 3:26:10 PM (11 years ago)

Author:

akling@apple.com

Message:

REGRESSION(r143125): ~5% performance hit on Chromium's intl2 page cycler.
<​http://webkit.org/b/108835>

Reviewed by Ojan Vafai.

Streamline the case where GlyphPage has a per-glyph SimpleFontData* lookup table to allow
taking earlier branches on pages with lots of mixed-font text.
We accomplish this by explicitly storing a null SimpleFontData* for glyph #0 in the per-glyph
lookup table instead of relying on "if (!glyph)" checks in getters.

This is a speculative optimization, I can't get stable enough numbers locally to tell if this
will resolve the issue on the bots.

• platform/graphics/GlyphPage.h:

(WebCore::GlyphPage::glyphDataForIndex):
(WebCore::GlyphPage::fontDataForCharacter):
(WebCore::GlyphPage::setGlyphDataForIndex):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/graphics/GlyphPage.h (modified) (5 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-02-17  Andreas Kling  <akling@apple.com>
+
+        REGRESSION(r143125): ~5% performance hit on Chromium's intl2 page cycler.
+        <http://webkit.org/b/108835>
+
+        Reviewed by Ojan Vafai.
+
+        Streamline the case where GlyphPage has a per-glyph SimpleFontData* lookup table to allow
+        taking earlier branches on pages with lots of mixed-font text.
+        We accomplish this by explicitly storing a null SimpleFontData* for glyph #0 in the per-glyph
+        lookup table instead of relying on "if (!glyph)" checks in getters.
+
+        This is a speculative optimization, I can't get stable enough numbers locally to tell if this
+        will resolve the issue on the bots.
+
+        * platform/graphics/GlyphPage.h:
+        (WebCore::GlyphPage::glyphDataForIndex):
+        (WebCore::GlyphPage::fontDataForCharacter):
+        (WebCore::GlyphPage::setGlyphDataForIndex):
+
 2013-02-17  Chris Fleizach  <cfleizach@apple.com>
 

trunk/Source/WebCore/platform/graphics/GlyphPage.h

@@ -104 +104 @@
         ASSERT_WITH_SECURITY_IMPLICATION(index < size);
         Glyph glyph = m_glyphs[index];
+        if (m_perGlyphFontData)
+            return GlyphData(glyph, m_perGlyphFontData[index]);
         if (!glyph)
             return GlyphData(0, 0);
-        if (m_perGlyphFontData)
-            return GlyphData(glyph, m_perGlyphFontData[index]);
         return GlyphData(glyph, m_fontDataForAllGlyphs);
     }
@@ -119 +119 @@
     const SimpleFontData* fontDataForCharacter(UChar32 c) const
     {
-        return glyphDataForIndex(indexForCharacter(c)).fontData;
+        unsigned index = indexForCharacter(c);
+        if (m_perGlyphFontData)
+            return m_perGlyphFontData[index];
+        Glyph glyph = m_glyphs[index];
+        if (!glyph)
+            return 0;
+        return m_fontDataForAllGlyphs;
     }
 
@@ -132 +138 @@
         m_glyphs[index] = glyph;
 
-        // GlyphPage getters will always return a null SimpleFontData* for glyph #0, so don't worry about the pointer for them.
+        // GlyphPage getters will always return a null SimpleFontData* for glyph #0 if there's no per-glyph font array.
+        if (m_perGlyphFontData) {
+            m_perGlyphFontData[index] = glyph ? fontData : 0;
+            return;
+        }
+
         if (!glyph)
             return;
@@ -138 +149 @@
         // A glyph index without a font data pointer makes no sense.
         ASSERT(fontData);
-
-        if (m_perGlyphFontData) {
-            m_perGlyphFontData[index] = fontData;
-            return;
-        }
 
         if (!m_fontDataForAllGlyphs)
@@ -154 +160 @@
         m_perGlyphFontData = static_cast<const SimpleFontData**>(fastMalloc(size * sizeof(SimpleFontData*)));
         for (unsigned i = 0; i < size; ++i)
-            m_perGlyphFontData[i] = oldFontData;
+            m_perGlyphFontData[i] = m_glyphs[i] ? oldFontData : 0;
         m_perGlyphFontData[index] = fontData;
     }