Offset-only GlyphBuffer

Source/platform/fonts/GlyphBuffer.h

Index: Source/platform/fonts/GlyphBuffer.h
diff --git a/Source/platform/fonts/GlyphBuffer.h b/Source/platform/fonts/GlyphBuffer.h
index 9ccd92ab1c360e70a8bd603f02ea2b9397178215..24f99a30bbcc4a094129e3a0dd3687877b9e558a 100644
--- a/Source/platform/fonts/GlyphBuffer.h
+++ b/Source/platform/fonts/GlyphBuffer.h
@@ -31,7 +31,7 @@
 #define GlyphBuffer_h
 
 #include "platform/fonts/Glyph.h"
-#include "platform/geometry/FloatSize.h"
+#include "platform/geometry/FloatPoint.h"
 #include "platform/heap/Heap.h"
 #include "wtf/Vector.h"
 
@@ -43,68 +43,106 @@ class GlyphBuffer {
     STACK_ALLOCATED();
 public:
     bool isEmpty() const { return m_fontData.isEmpty(); }
-    bool hasOffsets() const { return !m_offsets.isEmpty(); }
-    unsigned size() const { return m_fontData.size(); }
+    unsigned size() const
+    {
+        ASSERT(m_fontData.size() == m_glyphs.size());
+        ASSERT(m_fontData.size() == m_offsets.size() || 2 * m_fontData.size() == m_offsets.size());
+        return m_fontData.size();
+    }
 
-    const Glyph* glyphs(unsigned from) const { return m_glyphs.data() + from; }
-    const float* advances(unsigned from) const { return m_advances.data() + from; }
-    const FloatSize* offsets(unsigned from) const { return m_offsets.data() + from; }
+    bool hasVerticalOffsets() const
+    {
+        return size() != m_offsets.size();

[jbroman, 2014/10/29 15:06:07]

Any reason not to just use a bool to explicitly track this? It's not immediately
obvious why this works.

[f(malita), 2014/10/29 16:48:29]

add() is pretty hot if I'm not mistaken, and optimizing for it (avoiding any
unnecessary tracking work) seems desirable (I'm assuming hasVerticalOffsets is
not nearly as hot).

I haven't really measured the impact so maybe this is a case of premature
optimization, but if we can avoid it for now... added some comments, let me know
what you think.

[jbroman, 2014/10/29 19:05:36]

Hmm, okay. I suspect the expense is relatively minimal (e.g. there's also
branches for each vector we append to in case we need to increase capacity), but
I don't feel strongly.

+    }
+
+    const Glyph* glyphs(unsigned from) const
+    {
+        ASSERT(from < size());
+        return m_glyphs.data() + from;
+    }
+
+    const float* offsets(unsigned from) const
+    {
+        ASSERT(from < size());
+        return m_offsets.data() + (hasVerticalOffsets() ? from * 2 : from);
+    }
 
     const SimpleFontData* fontDataAt(unsigned index) const
     {
+        ASSERT(index < size());

[jbroman, 2014/10/29 15:06:07]

Do these assertions slow down debug builds much?

[f(malita), 2014/10/29 16:48:29]

Nothing noticeable.

A couple of local fast/text layout test runs, user time:

[0m2.657s, 0m2.655s] (ToT) vs. [0m2.642s, 0m2.672s] (patched)

[jbroman, 2014/10/29 19:05:36]

Acknowledged.

         return m_fontData[index];
     }
 
     Glyph glyphAt(unsigned index) const
     {
+        ASSERT(index < size());
         return m_glyphs[index];
     }
 
-    float advanceAt(unsigned index) const
+    float xOffsetAt(unsigned index) const
+    {
+        ASSERT(index < size());
+        return hasVerticalOffsets() ? m_offsets[index * 2] : m_offsets[index];
+
+    }
+
+    float yOffsetAt(unsigned index) const
     {
-        return m_advances[index];
+        ASSERT(index < size());
+        ASSERT(hasVerticalOffsets());
+        return m_offsets[index * 2 + 1];
     }
 
-    void add(Glyph glyph, const SimpleFontData* font, float width)
+    void add(Glyph glyph, const SimpleFontData* font, float x)
     {
-        // should not mix offset/advance-only glyphs
-        ASSERT(!hasOffsets());
+        // cannot mix x-only/xy offsets
+        ASSERT(!hasVerticalOffsets());
 
         m_fontData.append(font);
         m_glyphs.append(glyph);
-        m_advances.append(width);
+        m_offsets.append(x);
     }
 
-    void add(Glyph glyph, const SimpleFontData* font, const FloatSize& offset, float advance)
+    void add(Glyph glyph, const SimpleFontData* font, const FloatPoint& offset)
     {
-        // should not mix offset/advance-only glyphs
-        ASSERT(size() == m_offsets.size());
+        // cannot mix x-only/xy offsets
+        ASSERT(isEmpty() || hasVerticalOffsets());
 
         m_fontData.append(font);
         m_glyphs.append(glyph);
-        m_offsets.append(offset);
-        m_advances.append(advance);
+        m_offsets.append(offset.x());
+        m_offsets.append(offset.y());
     }
 
-    void reverse()
+    void reverse(float afterOffset, float totalWidth)
     {
+        ASSERT(!hasVerticalOffsets());
+
         m_fontData.reverse();
         m_glyphs.reverse();
-        m_advances.reverse();
-    }
 
-    void expandLastAdvance(float width)
-    {
-        ASSERT(!isEmpty());
-        float& lastAdvance = m_advances.last();
-        lastAdvance += width;
+        // | .. [X0 X1 ..   Xn]     ..   |
+        // ^                   ^         ^
+        // 0             afterOffset totalWidth

[jbroman, 2014/10/29 15:06:07]

This diagram is helpful, but I could use some words explaining what these
parameters are, too. Especially since this method is rather subtle.

[f(malita), 2014/10/29 16:48:29]

Done.

+        ASSERT_WITH_SECURITY_IMPLICATION(!m_offsets.isEmpty());

[jbroman, 2014/10/29 15:06:07]

Why is it illegal to reverse an empty glyph buffer?

[f(malita), 2014/10/29 16:48:29]

Ah, good catch: I assumed we check for an empty buffer at the call site, but
apparently not. Handling it here is the better choice - done.

+        float x = totalWidth - m_offsets[0];
+        for (unsigned i = 0; i + 1 < m_offsets.size(); ++i) {
+            float advance = m_offsets[i + 1] - m_offsets[i];
+            ASSERT(advance >= 0);
+            x -= advance;
+            m_offsets[i] = x;
+        }
+        float lastAdvance = afterOffset - m_offsets.last();
+        ASSERT(lastAdvance >= 0);
+        m_offsets.last() = x - lastAdvance;
+
+        m_offsets.reverse();
     }
 
 protected:
     Vector<const SimpleFontData*, 2048> m_fontData;
     Vector<Glyph, 2048> m_glyphs;
-    Vector<float, 2048> m_advances;
-    Vector<FloatSize, 1024> m_offsets;
+    Vector<float, 2048> m_offsets;
 };
 
 } // namespace blink