Revert of Souped-up SkTextBlob. (patchset #3 id:40001 of https://codereview.chromium.org/581173003/)

Reason for revert:
Broke the new blobshader gm.

Original issue's description:
> Souped-up SkTextBlob.
>
> Refactored text blob backend for improved performance: instead of using
> separate buffers for runs/positions/glyphs, everything is now packed in
> a consolidated slab (including the SkTextBlob object itself!).
>
> Benefits:
>
>  * number of allocations per blob construction reduced from ~4 to 1
>    (also minimizes internal fragmentation)
>  * run record size reduced by 8 bytes
>
> This takes the blob construction overhead down to negligible levels
> (for the current Blink uncached textblob implementation).
>
> Unfortunately, the code is much more finicky (run merging in
> particular) -- hence the assert spree.
>
> Multi-run blobs are vulnerable to realloc storms but this is not a
> problem at the moment because Blink is using one-run blobs 99% of the
> time. Will be addressed in the future.
>
>
> R=reed@google.com,mtklein@google.com,robertphillips@google.com
>
> Committed: https://skia.googlesource.com/skia/+/13645ea0ea87038ebd71be3bd6d53b313069a9e4

R=mtklein@google.com, reed@google.com, robertphillips@google.com
TBR=mtklein@google.com, reed@google.com, robertphillips@google.com
NOTREECHECKS=true
NOTRY=true

Author: fmalita@chromium.org

Review URL: https://codereview.chromium.org/588853002

include/core/SkTextBlob.h

@@ -53,8 +53,6 @@ private:
         kFull_Positioning         = 2  // Point positioning -- two scalars per glyph.
     };
 
-    class RunRecord;
-
     class RunIterator {
     public:
         RunIterator(const SkTextBlob* blob);
@@ -70,16 +68,21 @@ private:
         GlyphPositioning positioning() const;
 
     private:
-        const RunRecord* fCurrentRun;
+        const SkTextBlob* fBlob;
-        int              fRemainingRuns;
+        int               fIndex;
-
-        SkDEBUGCODE(uint8_t* fStorageTop;)
     };
 
-    SkTextBlob(int runCount, const SkRect& bounds);
+    // A run is a sequence of glyphs sharing the same font metrics and positioning mode.
+    struct Run {
+        uint32_t         count;
+        uint32_t         glyphStart; // index into fGlyphBuffer
+        uint32_t         posStart;   // index into fPosBuffer
+        SkPoint          offset;     // run offset (unsued for fully positioned glyphs)
+        SkPaint          font;
+        GlyphPositioning positioning;
+    };
 
-    virtual ~SkTextBlob();
+    SkTextBlob(uint16_t* glyphs, SkScalar* pos, const SkTArray<Run>* runs, const SkRect& bounds);
-    virtual void internal_dispose() const SK_OVERRIDE;
 
     static unsigned ScalarsPerGlyph(GlyphPositioning pos);
 
@@ -87,15 +90,15 @@ private:
     friend class SkTextBlobBuilder;
     friend class TextBlobTester;
 
-    const int        fRunCount;
+    const SkAutoTMalloc<uint16_t>       fGlyphBuffer;
+    const SkAutoTMalloc<SkScalar>       fPosBuffer;
+
+    // SkTArray required here for run font destruction.
+    SkAutoTDelete<const SkTArray<Run> > fRuns;
     const SkRect                        fBounds;
+
     mutable uint32_t                    fUniqueID;
 
-    SkDEBUGCODE(size_t fStorageSize;)
-
-    // The actual payload resides in externally-managed storage, following the object.
-    // (see the .cpp for more details)
-
     typedef SkRefCnt INHERITED;
 };
 
@@ -105,7 +108,11 @@ private:
  */
 class SK_API SkTextBlobBuilder {
 public:
-    SkTextBlobBuilder();
+    /**
+     *  @param runs The number of runs to be added, if known. This is a storage hint and
+     *              not a limit.
+     */
+    SkTextBlobBuilder(unsigned runs = 0);
 
     ~SkTextBlobBuilder();
 
@@ -173,21 +180,18 @@ public:
     const RunBuffer& allocRunPos(const SkPaint& font, int count, const SkRect* bounds = NULL);
 
 private:
-    void reserve(size_t size);
     void allocInternal(const SkPaint& font, SkTextBlob::GlyphPositioning positioning,
                        int count, SkPoint offset, const SkRect* bounds);
-    bool mergeRun(const SkPaint& font, SkTextBlob::GlyphPositioning positioning,
+    void ensureRun(const SkPaint& font, SkTextBlob::GlyphPositioning positioning,
-                  int count, SkPoint offset);
+                   const SkPoint& offset);
     void updateDeferredBounds();
 
-    SkAutoTMalloc<uint8_t> fStorage;
+    SkTDArray<uint16_t>        fGlyphBuffer;
-    size_t                 fStorageSize;
+    SkTDArray<SkScalar>        fPosBuffer;
-    size_t                 fStorageUsed;
+    SkTArray<SkTextBlob::Run>* fRuns;
 
     SkRect                     fBounds;
-    int                    fRunCount;
     bool                       fDeferredBounds;
-    size_t                 fLastRun; // index into fStorage
 
     RunBuffer                  fCurrentRunBuffer;
 };

src/core/SkTextBlob.cpp

@@ -10,125 +10,14 @@
 #include "SkReadBuffer.h"
 #include "SkWriteBuffer.h"
 
-//
+SkTextBlob::SkTextBlob(uint16_t *glyphs, SkScalar *pos, const SkTArray<Run> *runs,
-// Textblob data is laid out into externally-managed storage as follows:
+                       const SkRect& bounds)
-//
+    : fGlyphBuffer(glyphs)
-//    -----------------------------------------------------------------------------
+    , fPosBuffer(pos)
-//   | SkTextBlob | RunRecord | Glyphs[] | Pos[] | RunRecord | Glyphs[] | Pos[] | ...
+    , fRuns(runs)
-//    -----------------------------------------------------------------------------
-//
-//  Each run record describes a text blob run, and can be used to determine the (implicit)
-//  location of the following record.
-
-SkDEBUGCODE(static const unsigned kRunRecordMagic = 0xb10bcafe;)
-
-class SkTextBlob::RunRecord {
-public:
-    RunRecord(uint32_t count, const SkPoint& offset, const SkPaint& font, GlyphPositioning pos)
-        : fCount(count)
-        , fOffset(offset)
-        , fFont(font)
-        , fPositioning(pos) {
-        SkDEBUGCODE(fMagic = kRunRecordMagic);
-    }
-
-    uint32_t glyphCount() const {
-        return fCount;
-    }
-
-    const SkPoint& offset() const {
-        return fOffset;
-    }
-
-    const SkPaint& font() const {
-        return fFont;
-    }
-
-    GlyphPositioning positioning() const {
-        return fPositioning;
-    }
-
-    uint16_t* glyphBuffer() const {
-        // Glyph are stored immediately following the record.
-        return reinterpret_cast<uint16_t*>(const_cast<RunRecord*>(this) + 1);
-    }
-
-    SkScalar* posBuffer() const {
-        // Position scalars follow the (aligned) glyph buffer.
-        return reinterpret_cast<SkScalar*>(reinterpret_cast<uint8_t*>(this->glyphBuffer()) +
-                                           SkAlign4(fCount * sizeof(uint16_t)));
-    }
-
-    static size_t StorageSize(int glyphCount, SkTextBlob::GlyphPositioning positioning) {
-        // RunRecord object + (aligned) glyph buffer + position buffer
-        return SkAlignPtr(sizeof(SkTextBlob::RunRecord)
-                        + SkAlign4(glyphCount* sizeof(uint16_t))
-                        + glyphCount * sizeof(SkScalar) * ScalarsPerGlyph(positioning));
-    }
-
-    static const RunRecord* First(const SkTextBlob* blob) {
-        // The first record (if present) is stored following the blob object.
-        return reinterpret_cast<const RunRecord*>(blob + 1);
-    }
-
-    static const RunRecord* Next(const RunRecord* run) {
-        return reinterpret_cast<const RunRecord*>(reinterpret_cast<const uint8_t*>(run)
-            + StorageSize(run->glyphCount(), run->positioning()));
-    }
-
-    void validate(uint8_t* storageTop) const {
-        SkASSERT(kRunRecordMagic == fMagic);
-        SkASSERT((uint8_t*)Next(this) <= storageTop);
-        SkASSERT(glyphBuffer() + fCount <= (uint16_t*)posBuffer());
-        SkASSERT(posBuffer() + fCount * ScalarsPerGlyph(fPositioning) == (SkScalar*)Next(this));
-    }
-
-private:
-    friend class SkTextBlobBuilder;
-
-    void grow(uint32_t count) {
-        SkScalar* initialPosBuffer = posBuffer();
-        uint32_t initialCount = fCount;
-        fCount += count;
-
-        // Move the initial pos scalars to their new location.
-        size_t copySize = initialCount * sizeof(SkScalar) * ScalarsPerGlyph(fPositioning);
-        SkASSERT((uint8_t*)posBuffer() + copySize <= (uint8_t*)Next(this));
-
-        // memmove, as the buffers may overlap
-        memmove(posBuffer(), initialPosBuffer, copySize);
-    }
-
-    uint32_t         fCount;
-    SkPoint          fOffset;
-    SkPaint          fFont;
-    GlyphPositioning fPositioning;
-
-    SkDEBUGCODE(unsigned fMagic;)
-};
-
-SkTextBlob::SkTextBlob(int runCount, const SkRect& bounds)
-    : fRunCount(runCount)
     , fBounds(bounds) {
 }
 
-SkTextBlob::~SkTextBlob() {
-    const RunRecord* run = RunRecord::First(this);
-    for (int i = 0; i < fRunCount; ++i) {
-        const RunRecord* nextRun = RunRecord::Next(run);
-        SkDEBUGCODE(run->validate((uint8_t*)this + fStorageSize);)
-        run->~RunRecord();
-        run = nextRun;
-    }
-}
-
-void SkTextBlob::internal_dispose() const {
-    // SkTextBlobs use externally-managed storage.
-    this->internal_dispose_restore_refcnt_to_1();
-    this->~SkTextBlob();
-    sk_free(const_cast<SkTextBlob*>(this));
-}
-
 uint32_t SkTextBlob::uniqueID() const {
     static int32_t  gTextBlobGenerationID; // = 0;
 
@@ -140,8 +29,14 @@ uint32_t SkTextBlob::uniqueID() const {
     return fUniqueID;
 }
 
+unsigned SkTextBlob::ScalarsPerGlyph(GlyphPositioning pos) {
+    // GlyphPositioning values are directly mapped to scalars-per-glyph.
+    SkASSERT(pos <= 2);
+    return pos;
+}
+
 void SkTextBlob::flatten(SkWriteBuffer& buffer) const {
-    int runCount = fRunCount;
+    int runCount = (NULL == fRuns.get()) ? 0 : fRuns->count();
 
     buffer.write32(runCount);
     buffer.writeRect(fBounds);
@@ -206,8 +101,7 @@ const SkTextBlob* SkTextBlob::CreateFromBuffer(SkReadBuffer& reader) {
         }
 
         if (!reader.readByteArray(buf->glyphs, glyphCount * sizeof(uint16_t)) ||
-            !reader.readByteArray(buf->pos,
+            !reader.readByteArray(buf->pos, glyphCount * sizeof(SkScalar) * ScalarsPerGlyph(pos))) {
-                                  glyphCount * sizeof(SkScalar) * ScalarsPerGlyph(pos))) {
             return NULL;
         }
     }
@@ -215,61 +109,50 @@ const SkTextBlob* SkTextBlob::CreateFromBuffer(SkReadBuffer& reader) {
     return blobBuilder.build();
 }
 
-unsigned SkTextBlob::ScalarsPerGlyph(GlyphPositioning pos) {
-    // GlyphPositioning values are directly mapped to scalars-per-glyph.
-    SkASSERT(pos <= 2);
-    return pos;
-}
-
 SkTextBlob::RunIterator::RunIterator(const SkTextBlob* blob)
-    : fCurrentRun(RunRecord::First(blob))
+    : fBlob(blob)
-    , fRemainingRuns(blob->fRunCount) {
+    , fIndex(0) {
-    SkDEBUGCODE(fStorageTop = (uint8_t*)blob + blob->fStorageSize;)
+    SkASSERT(blob);
 }
 
 bool SkTextBlob::RunIterator::done() const {
-    return fRemainingRuns <= 0;
+    return NULL == fBlob->fRuns.get() || fIndex >= fBlob->fRuns->count();
 }
 
 void SkTextBlob::RunIterator::next() {
     SkASSERT(!this->done());
-
+    fIndex++;
-    if (!this->done()) {
-        SkDEBUGCODE(fCurrentRun->validate(fStorageTop);)
-        fCurrentRun = RunRecord::Next(fCurrentRun);
-        fRemainingRuns--;
-    }
 }
 
 uint32_t SkTextBlob::RunIterator::glyphCount() const {
     SkASSERT(!this->done());
-    return fCurrentRun->glyphCount();
+    return (*fBlob->fRuns)[fIndex].count;
 }
 
 const uint16_t* SkTextBlob::RunIterator::glyphs() const {
     SkASSERT(!this->done());
-    return fCurrentRun->glyphBuffer();
+    return fBlob->fGlyphBuffer.get() + (*fBlob->fRuns)[fIndex].glyphStart;
 }
 
 const SkScalar* SkTextBlob::RunIterator::pos() const {
     SkASSERT(!this->done());
-    return fCurrentRun->posBuffer();
+    return fBlob->fPosBuffer.get() + (*fBlob->fRuns)[fIndex].posStart;
 }
 
 const SkPoint& SkTextBlob::RunIterator::offset() const {
     SkASSERT(!this->done());
-    return fCurrentRun->offset();
+    return (*fBlob->fRuns)[fIndex].offset;
 }
 
 SkTextBlob::GlyphPositioning SkTextBlob::RunIterator::positioning() const {
     SkASSERT(!this->done());
-    return fCurrentRun->positioning();
+    return (*fBlob->fRuns)[fIndex].positioning;
 }
 
 void SkTextBlob::RunIterator::applyFontToPaint(SkPaint* paint) const {
     SkASSERT(!this->done());
 
-    const SkPaint& font = fCurrentRun->font();
+    const SkPaint& font = (*fBlob->fRuns)[fIndex].font;
 
     paint->setTypeface(font.getTypeface());
     paint->setTextEncoding(font.getTextEncoding());
@@ -294,25 +177,24 @@ void SkTextBlob::RunIterator::applyFontToPaint(SkPaint* paint) const {
     paint->setFlags((paint->getFlags() & ~flagsMask) | (font.getFlags() & flagsMask));
 }
 
-SkTextBlobBuilder::SkTextBlobBuilder()
+SkTextBlobBuilder::SkTextBlobBuilder(unsigned runs)
-    : fStorageSize(0)
+    : fRuns(NULL)
-    , fStorageUsed(0)
+    , fDeferredBounds(false) {
-    , fRunCount(0)
+
-    , fDeferredBounds(false)
+    if (runs > 0) {
-    , fLastRun(0) {
+        // if the number of runs is known, size our run storage accordingly.
+        fRuns = SkNEW(SkTArray<SkTextBlob::Run>(runs));
+    }
     fBounds.setEmpty();
 }
 
 SkTextBlobBuilder::~SkTextBlobBuilder() {
-    if (NULL != fStorage.get()) {
+    // unused runs
-        // We are abandoning runs and must destruct the associated font data.
+    SkDELETE(fRuns);
-        // The easiest way to accomplish that is to use the blob destructor.
-        build()->unref();
-    }
 }
 
 void SkTextBlobBuilder::updateDeferredBounds() {
-    SkASSERT(!fDeferredBounds || fRunCount > 0);
+    SkASSERT(!fDeferredBounds || (fRuns && !fRuns->empty()));
 
     if (!fDeferredBounds) {
         return;
@@ -322,104 +204,64 @@ void SkTextBlobBuilder::updateDeferredBounds() {
     fDeferredBounds = false;
 }
 
-void SkTextBlobBuilder::reserve(size_t size) {
+void SkTextBlobBuilder::ensureRun(const SkPaint& font, SkTextBlob::GlyphPositioning pos,
-    // We don't currently pre-allocate, but maybe someday...
+                                  const SkPoint& offset) {
-    if (fStorageUsed + size <= fStorageSize) {
+    SkASSERT(SkPaint::kGlyphID_TextEncoding == font.getTextEncoding());
-        return;
-    }
 
-    if (0 == fRunCount) {
+    if (NULL == fRuns) {
-        SkASSERT(NULL == fStorage.get());
+        fRuns = SkNEW(SkTArray<SkTextBlob::Run>());
-        SkASSERT(0 == fStorageSize);
-        SkASSERT(0 == fStorageUsed);
-
-        // the first allocation also includes blob storage
-        fStorageUsed += sizeof(SkTextBlob);
-    }
-
-    fStorageSize = fStorageUsed + size;
-    // FYI: This relies on everything we store being relocatable, particularly SkPaint.
-    fStorage.realloc(fStorageSize);
-}
-
-bool SkTextBlobBuilder::mergeRun(const SkPaint &font, SkTextBlob::GlyphPositioning positioning,
-                                 int count, SkPoint offset) {
-    if (0 == fLastRun) {
-        SkASSERT(0 == fRunCount);
-        return false;
-    }
-
-    SkASSERT(fLastRun >= sizeof(SkTextBlob));
-    SkTextBlob::RunRecord* run = reinterpret_cast<SkTextBlob::RunRecord*>(fStorage.get() +
-                                                                          fLastRun);
-    SkASSERT(run->glyphCount() > 0);
-
-    if (run->positioning() != positioning
-        || run->font() != font
-        || (run->glyphCount() + count < run->glyphCount())) {
-        return false;
     }
 
     // we can merge same-font/same-positioning runs in the following cases:
     //   * fully positioned run following another fully positioned run
     //   * horizontally postioned run following another horizontally positioned run with the same
     //     y-offset
-    if (SkTextBlob::kFull_Positioning != positioning
+    if (!fRuns->empty()
-        && (SkTextBlob::kHorizontal_Positioning != positioning
+        && fRuns->back().positioning == pos
-            || run->offset().y() != offset.y())) {
+        && fRuns->back().font == font
-        return false;
+        && (SkTextBlob::kFull_Positioning == fRuns->back().positioning
+            || (SkTextBlob::kHorizontal_Positioning == fRuns->back().positioning
+                && fRuns->back().offset.y() == offset.y()))){
+        return;
     }
 
-    size_t sizeDelta = SkTextBlob::RunRecord::StorageSize(run->glyphCount() + count, positioning) -
+    this->updateDeferredBounds();
-                       SkTextBlob::RunRecord::StorageSize(run->glyphCount(), positioning);
-    this->reserve(sizeDelta);
 
-    // reserve may have realloced
+    // start a new run
-    run = reinterpret_cast<SkTextBlob::RunRecord*>(fStorage.get() + fLastRun);
+    SkTextBlob::Run& newRun = fRuns->push_back();
-    uint32_t preMergeCount = run->glyphCount();
+    newRun.count = 0;
-    run->grow(count);
+    newRun.glyphStart = fGlyphBuffer.count();
-
+    newRun.posStart = fPosBuffer.count();
-    // Callers expect the buffers to point at the newly added slice, ant not at the beginning.
+    newRun.offset = offset;
-    fCurrentRunBuffer.glyphs = run->glyphBuffer() + preMergeCount;
+    newRun.font = font;
-    fCurrentRunBuffer.pos = run->posBuffer()
+    newRun.positioning = pos;
-                          + preMergeCount * SkTextBlob::ScalarsPerGlyph(positioning);
-
-    fStorageUsed += sizeDelta;
-
-    SkASSERT(fStorageUsed <= fStorageSize);
-    run->validate(fStorage.get() + fStorageUsed);
-
-    return true;
 }
 
 void SkTextBlobBuilder::allocInternal(const SkPaint &font,
                                       SkTextBlob::GlyphPositioning positioning,
                                       int count, SkPoint offset, const SkRect* bounds) {
     SkASSERT(count > 0);
-    SkASSERT(SkPaint::kGlyphID_TextEncoding == font.getTextEncoding());
 
-    if (!this->mergeRun(font, positioning, count, offset)) {
+    this->ensureRun(font, positioning, offset);
-        updateDeferredBounds();
 
-        size_t runSize = SkTextBlob::RunRecord::StorageSize(count, positioning);
+    unsigned posScalarsPerGlyph = SkTextBlob::ScalarsPerGlyph(positioning);
-        this->reserve(runSize);
 
-        SkASSERT(fStorageUsed >= sizeof(SkTextBlob));
+    fGlyphBuffer.append(count);
-        SkASSERT(fStorageUsed + runSize <= fStorageSize);
+    fPosBuffer.append(count * posScalarsPerGlyph);
 
-        SkTextBlob::RunRecord* run = new (fStorage.get() + fStorageUsed)
+    SkASSERT(fRuns && !fRuns->empty());
-                                         SkTextBlob::RunRecord(count, offset, font, positioning);
+    SkTextBlob::Run& run = fRuns->back();
 
-        fCurrentRunBuffer.glyphs = run->glyphBuffer();
+    run.count += count;
-        fCurrentRunBuffer.pos = run->posBuffer();
 
-        fLastRun = fStorageUsed;
+    // The current run might have been merged, so the start offset may point to prev run data.
-        fStorageUsed += runSize;
+    // Start from the back (which always points to the end of the current run buffers) instead.
-        fRunCount++;
+    fCurrentRunBuffer.glyphs = fGlyphBuffer.isEmpty()
-
+            ? NULL : fGlyphBuffer.end() - count;
-        SkASSERT(fStorageUsed <= fStorageSize);
+    SkASSERT(NULL == fCurrentRunBuffer.glyphs || fCurrentRunBuffer.glyphs >= fGlyphBuffer.begin());
-        run->validate(fStorage.get() + fStorageUsed);
+    fCurrentRunBuffer.pos = fPosBuffer.isEmpty()
-    }
+            ? NULL : fPosBuffer.end() - count * posScalarsPerGlyph;
+    SkASSERT(NULL == fCurrentRunBuffer.pos || fCurrentRunBuffer.pos >= fPosBuffer.begin());
 
     if (!fDeferredBounds) {
         if (bounds) {
@@ -455,23 +297,28 @@ const SkTextBlobBuilder::RunBuffer& SkTextBlobBuilder::allocRunPos(const SkPaint
 }
 
 const SkTextBlob* SkTextBlobBuilder::build() {
-    SkASSERT((fRunCount > 0) == (NULL != fStorage.get()));
+    const SkTextBlob* blob;
+
+    if (fGlyphBuffer.count() > 0) {
+        // we have some glyphs, construct a real blob
+        SkASSERT(fRuns && !fRuns->empty());
 
         this->updateDeferredBounds();
 
-    if (0 == fRunCount) {
+        // ownership of all buffers is transferred to the blob
-        SkASSERT(NULL == fStorage.get());
+        blob = SkNEW_ARGS(SkTextBlob, (fGlyphBuffer.detach(),
-        fStorage.realloc(sizeof(SkTextBlob));
+                                       fPosBuffer.detach(),
-    }
+                                       fRuns,
-
+                                       fBounds));
-    const SkTextBlob* blob = new (fStorage.detach()) SkTextBlob(fRunCount, fBounds);
+        fRuns = NULL;
-    SkDEBUGCODE(const_cast<SkTextBlob*>(blob)->fStorageSize = fStorageSize;)
-
-    fStorageUsed = 0;
-    fStorageSize = 0;
-    fRunCount = 0;
-    fLastRun = 0;
         fBounds.setEmpty();
+    } else {
+        // empty blob
+        SkASSERT(NULL == fRuns || fRuns->empty());
+        SkASSERT(fBounds.isEmpty());
+
+        blob = SkNEW_ARGS(SkTextBlob, (NULL, NULL, NULL, SkRect::MakeEmpty()));
+    }
 
     return blob;
 }

tests/TextBlobTest.cpp

@@ -175,6 +175,9 @@ private:
         }
 
         SkAutoTUnref<const SkTextBlob> blob(builder.build());
+        REPORTER_ASSERT(reporter, SkToBool(blob->fGlyphBuffer) == (glyphCount > 0));
+        REPORTER_ASSERT(reporter, SkToBool(blob->fPosBuffer) == (posCount > 0));
+        REPORTER_ASSERT(reporter, SkToBool(blob->fRuns.get()) == (inCount > 0));
 
         SkTextBlob::RunIterator it(blob);
         for (unsigned i = 0; i < outCount; ++i) {