Optimize GlyphPage for case where all glyphs are available in the same font.
authorakling@apple.com <akling@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 17 Feb 2013 08:17:42 +0000 (08:17 +0000)
committerakling@apple.com <akling@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 17 Feb 2013 08:17:42 +0000 (08:17 +0000)
<http://webkit.org/b/108835>
<rdar://problem/13157042>

Reviewed by Antti Koivisto.

Let GlyphPage begin optimistically assuming that all its glyphs will be represented in
the same SimpleFontData*. In this (very common) case, only keep a single SimpleFontData*.

If glyphs from multiple fonts are mixed in one page, an array of per-glyph SimpleFontData*
is allocated transparently.

This was landed before with some bogus branch prediction hints and didn't fare well on
page cyclers (intl2 specifically.) These have been removed this time around, and will
hopefully be regression-free.

4.98 MB progression on Membuster3.

* platform/graphics/GlyphPageTreeNode.cpp:
(WebCore::GlyphPageTreeNode::initializePage):
* platform/graphics/GlyphPage.h:
(WebCore::GlyphPage::createUninitialized):
(WebCore::GlyphPage::createZeroedSystemFallbackPage):
(WebCore::GlyphPage::createCopiedSystemFallbackPage):

    There are now three ways of constructing a GlyphPage, two of them are only used for
    creating system fallback pages.

(WebCore::GlyphPage::setGlyphDataForIndex):

    Hold off creating a SimpleFontData* array until we're sure there are two different
    SimpleFontData* backing the glyphs in this page.
    We don't store font data for glyph #0, instead we let the getters always return null for it.

(WebCore::GlyphPage::~GlyphPage):

    Free the SimpleFontData* array if needed.

(WebCore::GlyphPage::glyphDataForCharacter):
(WebCore::GlyphPage::glyphDataForIndex):
(WebCore::GlyphPage::fontDataForCharacter):

    The font data for glyph #0 is always a null pointer now.

(WebCore::GlyphPage::clearForFontData):

    Updated for new storage format.

* rendering/svg/SVGTextRunRenderingContext.cpp:
(WebCore::SVGTextRunRenderingContext::glyphDataForCharacter):

    Fix bug where non-zero glyph was temporarily associated with null font data,
    which triggered the new assertion in setGlyphDataForIndex().

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@143125 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/GlyphPage.h
Source/WebCore/platform/graphics/GlyphPageTreeNode.cpp
Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp

index d9f0fb11d9cc2d9dacdc1f9624e3bc171e849389..828d7077491b70da3500dbf7d8f79b92224f07d9 100644 (file)
@@ -1,3 +1,59 @@
+2013-02-17  Andreas Kling  <akling@apple.com>
+
+        Optimize GlyphPage for case where all glyphs are available in the same font.
+        <http://webkit.org/b/108835>
+        <rdar://problem/13157042>
+
+        Reviewed by Antti Koivisto.
+
+        Let GlyphPage begin optimistically assuming that all its glyphs will be represented in
+        the same SimpleFontData*. In this (very common) case, only keep a single SimpleFontData*.
+
+        If glyphs from multiple fonts are mixed in one page, an array of per-glyph SimpleFontData*
+        is allocated transparently.
+
+        This was landed before with some bogus branch prediction hints and didn't fare well on
+        page cyclers (intl2 specifically.) These have been removed this time around, and will
+        hopefully be regression-free.
+
+        4.98 MB progression on Membuster3.
+
+        * platform/graphics/GlyphPageTreeNode.cpp:
+        (WebCore::GlyphPageTreeNode::initializePage):
+        * platform/graphics/GlyphPage.h:
+        (WebCore::GlyphPage::createUninitialized):
+        (WebCore::GlyphPage::createZeroedSystemFallbackPage):
+        (WebCore::GlyphPage::createCopiedSystemFallbackPage):
+
+            There are now three ways of constructing a GlyphPage, two of them are only used for
+            creating system fallback pages.
+
+        (WebCore::GlyphPage::setGlyphDataForIndex):
+
+            Hold off creating a SimpleFontData* array until we're sure there are two different
+            SimpleFontData* backing the glyphs in this page.
+            We don't store font data for glyph #0, instead we let the getters always return null for it.
+
+        (WebCore::GlyphPage::~GlyphPage):
+
+            Free the SimpleFontData* array if needed.
+
+        (WebCore::GlyphPage::glyphDataForCharacter):
+        (WebCore::GlyphPage::glyphDataForIndex):
+        (WebCore::GlyphPage::fontDataForCharacter):
+
+            The font data for glyph #0 is always a null pointer now.
+
+        (WebCore::GlyphPage::clearForFontData):
+
+            Updated for new storage format.
+
+        * rendering/svg/SVGTextRunRenderingContext.cpp:
+        (WebCore::SVGTextRunRenderingContext::glyphDataForCharacter):
+
+            Fix bug where non-zero glyph was temporarily associated with null font data,
+            which triggered the new assertion in setGlyphDataForIndex().
+
 2013-02-16  Andreas Kling  <akling@apple.com>
 
         Remove multi-threading gunk from WebKit2's PluginInfoStore.
index 97d935fe8141e0c27e8efb6b3334b708da441475..967585d1e21db579d25254ed22d4f12726c8a7e5 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2006, 2007, 2008 Apple Inc. All rights reserved.
+ * Copyright (C) 2006, 2007, 2008, 2013 Apple Inc. All rights reserved.
  * Copyright (C) Research In Motion Limited 2011. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -33,6 +33,7 @@
 #include "Glyph.h"
 #include <wtf/PassRefPtr.h>
 #include <wtf/RefCounted.h>
+#include <wtf/RefPtr.h>
 #include <wtf/unicode/Unicode.h>
 
 namespace WebCore {
@@ -62,9 +63,32 @@ struct GlyphData {
 // to be overriding the parent's node, but provide no additional information.
 class GlyphPage : public RefCounted<GlyphPage> {
 public:
-    static PassRefPtr<GlyphPage> create(GlyphPageTreeNode* owner)
+    static PassRefPtr<GlyphPage> createUninitialized(GlyphPageTreeNode* owner)
     {
-        return adoptRef(new GlyphPage(owner));
+        return adoptRef(new GlyphPage(owner, false));
+    }
+
+    static PassRefPtr<GlyphPage> createZeroedSystemFallbackPage(GlyphPageTreeNode* owner)
+    {
+        return adoptRef(new GlyphPage(owner, true));
+    }
+
+    PassRefPtr<GlyphPage> createCopiedSystemFallbackPage(GlyphPageTreeNode* owner) const
+    {
+        RefPtr<GlyphPage> page = GlyphPage::createUninitialized(owner);
+        memcpy(page->m_glyphs, m_glyphs, sizeof(m_glyphs));
+        page->m_fontDataForAllGlyphs = m_fontDataForAllGlyphs;
+        if (m_perGlyphFontData) {
+            page->m_perGlyphFontData = static_cast<const SimpleFontData**>(fastMalloc(size * sizeof(SimpleFontData*)));
+            memcpy(page->m_perGlyphFontData, m_perGlyphFontData, size * sizeof(SimpleFontData*));
+        }
+        return page.release();
+    }
+
+    ~GlyphPage()
+    {
+        if (m_perGlyphFontData)
+            fastFree(m_perGlyphFontData);
     }
 
     static const size_t size = 256; // Covers Latin-1 in a single page.
@@ -72,14 +96,18 @@ public:
     unsigned indexForCharacter(UChar32 c) const { return c % size; }
     GlyphData glyphDataForCharacter(UChar32 c) const
     {
-        unsigned index = indexForCharacter(c);
-        return GlyphData(m_glyphs[index], m_glyphFontData[index]);
+        return glyphDataForIndex(indexForCharacter(c));
     }
 
     GlyphData glyphDataForIndex(unsigned index) const
     {
         ASSERT_WITH_SECURITY_IMPLICATION(index < size);
-        return GlyphData(m_glyphs[index], m_glyphFontData[index]);
+        Glyph glyph = m_glyphs[index];
+        if (!glyph)
+            return GlyphData(0, 0);
+        if (m_perGlyphFontData)
+            return GlyphData(glyph, m_perGlyphFontData[index]);
+        return GlyphData(glyph, m_fontDataForAllGlyphs);
     }
 
     Glyph glyphAt(unsigned index) const
@@ -90,7 +118,7 @@ public:
 
     const SimpleFontData* fontDataForCharacter(UChar32 c) const
     {
-        return m_glyphFontData[indexForCharacter(c)];
+        return glyphDataForIndex(indexForCharacter(c)).fontData;
     }
 
     void setGlyphDataForCharacter(UChar32 c, Glyph g, const SimpleFontData* f)
@@ -98,36 +126,55 @@ public:
         setGlyphDataForIndex(indexForCharacter(c), g, f);
     }
 
-    void setGlyphDataForIndex(unsigned index, Glyph g, const SimpleFontData* f)
+    void setGlyphDataForIndex(unsigned index, Glyph glyph, const SimpleFontData* fontData)
     {
         ASSERT_WITH_SECURITY_IMPLICATION(index < size);
-        m_glyphs[index] = g;
-        m_glyphFontData[index] = f;
+        m_glyphs[index] = glyph;
+
+        // GlyphPage getters will always return a null SimpleFontData* for glyph #0, so don't worry about the pointer for them.
+        if (!glyph)
+            return;
+
+        // A glyph index without a font data pointer makes no sense.
+        ASSERT(fontData);
+
+        if (m_perGlyphFontData) {
+            m_perGlyphFontData[index] = fontData;
+            return;
+        }
+
+        if (!m_fontDataForAllGlyphs)
+            m_fontDataForAllGlyphs = fontData;
+
+        if (m_fontDataForAllGlyphs == fontData)
+            return;
+
+        // This GlyphPage houses glyphs from multiple fonts, transition to an array of SimpleFontData pointers.
+        const SimpleFontData* oldFontData = m_fontDataForAllGlyphs;
+        m_perGlyphFontData = static_cast<const SimpleFontData**>(fastMalloc(size * sizeof(SimpleFontData*)));
+        for (unsigned i = 0; i < size; ++i)
+            m_perGlyphFontData[i] = oldFontData;
+        m_perGlyphFontData[index] = fontData;
     }
 
     void setGlyphDataForIndex(unsigned index, const GlyphData& glyphData)
     {
         setGlyphDataForIndex(index, glyphData.glyph, glyphData.fontData);
     }
-    
-    void copyFrom(const GlyphPage& other)
-    {
-        memcpy(m_glyphs, other.m_glyphs, sizeof(m_glyphs));
-        memcpy(m_glyphFontData, other.m_glyphFontData, sizeof(m_glyphFontData));
-    }
-
-    void clear()
-    {
-        memset(m_glyphs, 0, sizeof(m_glyphs));
-        memset(m_glyphFontData, 0, sizeof(m_glyphFontData));
-    }
 
     void clearForFontData(const SimpleFontData* fontData)
     {
+        if (!m_perGlyphFontData) {
+            if (m_fontDataForAllGlyphs == fontData) {
+                memset(m_glyphs, 0, sizeof(m_glyphs));
+                m_fontDataForAllGlyphs = 0;
+            }
+            return;
+        }
         for (size_t i = 0; i < size; ++i) {
-            if (m_glyphFontData[i] == fontData) {
+            if (m_perGlyphFontData[i] == fontData) {
                 m_glyphs[i] = 0;
-                m_glyphFontData[i] = 0;
+                m_perGlyphFontData[i] = 0;
             }
         }
     }
@@ -138,18 +185,22 @@ public:
     bool fill(unsigned offset, unsigned length, UChar* characterBuffer, unsigned bufferLength, const SimpleFontData*);
 
 private:
-    GlyphPage(GlyphPageTreeNode* owner)
-        : m_owner(owner)
+    GlyphPage(GlyphPageTreeNode* owner, bool clearGlyphs)
+        : m_fontDataForAllGlyphs(0)
+        , m_perGlyphFontData(0)
+        , m_owner(owner)
     {
+        if (clearGlyphs)
+            memset(m_glyphs, 0, sizeof(m_glyphs));
     }
 
-    // Separate arrays, rather than array of GlyphData, to save space.
-    Glyph m_glyphs[size];
-    const SimpleFontData* m_glyphFontData[size];
+    const SimpleFontData* m_fontDataForAllGlyphs;
+    const SimpleFontData** m_perGlyphFontData;
 
     GlyphPageTreeNode* m_owner;
+    Glyph m_glyphs[size];
 };
 
 } // namespace WebCore
 
-#endif // GlyphPageTreeNode_h
+#endif // GlyphPage_h
index d9b0de288d1eaa133a1065e9a91eff459779a1a4..7e7ce7ee7218088545457fc3c0b7991073143182 100644 (file)
@@ -200,8 +200,8 @@ void GlyphPageTreeNode::initializePage(const FontData* fontData, unsigned pageNu
                     buffer[i * 2 + 1] = U16_TRAIL(c);
                 }
             }
-            
-            m_page = GlyphPage::create(this);
+
+            m_page = GlyphPage::createUninitialized(this);
 
             // Now that we have a buffer full of characters, we want to get back an array
             // of glyph indices.  This part involves calling into the platform-specific 
@@ -225,7 +225,7 @@ void GlyphPageTreeNode::initializePage(const FontData* fontData, unsigned pageNu
                     int to = 1 + min(static_cast<int>(range.to()) - static_cast<int>(start), static_cast<int>(GlyphPage::size) - 1);
                     if (from < static_cast<int>(GlyphPage::size) && to > 0) {
                         if (haveGlyphs && !scratchPage) {
-                            scratchPage = GlyphPage::create(this);
+                            scratchPage = GlyphPage::createUninitialized(this);
                             pageToFill = scratchPage.get();
                         }
 
@@ -279,7 +279,7 @@ void GlyphPageTreeNode::initializePage(const FontData* fontData, unsigned pageNu
                 m_page = parentPage;
             } else {
                 // Combine the parent's glyphs and ours to form a new more complete page.
-                m_page = GlyphPage::create(this);
+                m_page = GlyphPage::createUninitialized(this);
 
                 // Overlay the parent page on the fallback page. Check if the fallback font
                 // has added anything.
@@ -300,15 +300,14 @@ void GlyphPageTreeNode::initializePage(const FontData* fontData, unsigned pageNu
             }
         }
     } else {
-        m_page = GlyphPage::create(this);
         // System fallback. Initialized with the parent's page here, as individual
         // entries may use different fonts depending on character. If the Font
         // ever finds it needs a glyph out of the system fallback page, it will
         // ask the system for the best font to use and fill that glyph in for us.
         if (parentPage)
-            m_page->copyFrom(*parentPage);
+            m_page = parentPage->createCopiedSystemFallbackPage(this);
         else
-            m_page->clear();
+            m_page = GlyphPage::createZeroedSystemFallbackPage(this);
     }
 }
 
index 7dd60d49eece6015117d301cd1ef98043af2c019..b41086be44c287226e83a028c22f57586a1d104f 100644 (file)
@@ -230,7 +230,7 @@ GlyphData SVGTextRunRenderingContext::glyphDataForCharacter(const Font& font, co
     // No suitable glyph found that is compatible with the requirments (same language, arabic-form, orientation etc.)
     // Even though our GlyphPage contains an entry for eg. glyph "a", it's not compatible. So we have to temporarily
     // remove the glyph data information from the GlyphPage, and retry the lookup, which handles font fallbacks correctly.
-    page->setGlyphDataForCharacter(character, glyphData.glyph, 0);
+    page->setGlyphDataForCharacter(character, 0, 0);
 
     // Assure that the font fallback glyph selection worked, aka. the fallbackGlyphData font data is not the same as before.
     GlyphData fallbackGlyphData = font.glyphDataForCharacter(character, mirror);