Cleaner external buffer handling in SkWriter32

include/core/SkWriter32.h

Index: include/core/SkWriter32.h
diff --git a/include/core/SkWriter32.h b/include/core/SkWriter32.h
index b193cbe7c407663f7a82a613b8a65b5e1e637a47..55810ade22bf4fdc64d2287515a4d5ae3064fb54 100644
--- a/include/core/SkWriter32.h
+++ b/include/core/SkWriter32.h
@@ -30,12 +30,18 @@ public:
      *  first time an allocation doesn't fit.  From then it will use dynamically allocated storage.
      *  This used to be optional behavior, but pipe now relies on it.
      */
-    SkWriter32(void* external = NULL, size_t externalBytes = 0) {
-        this->reset(external, externalBytes);
+    SkWriter32(void* external = NULL, size_t externalBytes = 0)
+        : fData((uint8_t*)external)
+        , fCapacity(externalBytes)
+        , fUsed(0)
+        , fExternal(external)
+    {
+        SkASSERT(SkIsAlign4((uintptr_t)external));
+        SkASSERT(SkIsAlign4(externalBytes));

[reed1, 2014/02/06 20:14:40]

or we could just trim the low bits off of externalBytes and ignore them.

[iancottrell, 2014/02/06 21:33:06]

I was staying compatible with the contract it used to have, happy to change it,
but maybe as a separate cl?

[reed1, 2014/02/06 21:41:34]

Sure, separate CL or not at all.

     }
 
     // return the current offset (will always be a multiple of 4)
-    size_t bytesWritten() const { return fCount * 4; }
+    size_t bytesWritten() const { return fUsed; }
 
     SK_ATTR_DEPRECATED("use bytesWritten")
     size_t size() const { return this->bytesWritten(); }
@@ -43,45 +49,41 @@ public:
     void reset(void* external = NULL, size_t externalBytes = 0) {
         SkASSERT(SkIsAlign4((uintptr_t)external));
         SkASSERT(SkIsAlign4(externalBytes));
-        fExternal = (uint32_t*)external;
-        fExternalLimit = SkToInt(externalBytes/4);
-        fCount = 0;
-        fInternal.rewind();
+
+        fData = (uint8_t*)external;
+        fCapacity = externalBytes;
+        fUsed = 0;
+        fExternal = external;
     }
 
-    // If all data written is contiguous, then this returns a pointer to it, otherwise NULL.
-    // This will work if we've only written to the externally supplied block of storage, or if we've
-    // only written to our internal dynamic storage, but will fail if we have written into both.
+    // Returns the current buffer.
+    // The pointer may be invalidated by any future write calls.

[reed1, 2014/02/06 20:14:40]

Does this guy always return all of the space written to by the writer? If that
is the contract, then perhaps the name is over-qualified. If that isn't
necessarily the contract we want to commit to, then perhaps this guy needs to
return the length of those contiguous bytes...

[iancottrell, 2014/02/06 21:33:06]

I am trying hard to not change the public interface of skia in this cl, as I
have no idea how many users depend on it not changing or what the process is for
doing so.
The difference really is that it used to return null if you have written to both
internal and external storage, now it will always return a useful pointer, even
in cases where it would not have done before.
It only has 2 uses in skia, one of which should be rewritten to use flatten, as
it only gets it to memcpy to another buffer. The other is in
SkPictureFlat::resetScratch, where it really does want a pointer to the
underlying buffer, and assumes it will be a pointer to the data written as a
single contiguous buffer of size bytesWritten().

[reed1, 2014/02/06 21:41:34]

I'm mostly asking that we well-document the API, and specifically for this guy,
since its behavior is somewhat changing (improving), I want us to be sure we
want to commit to this new contract: we always return a ptr to the entire
buffer...

[mtklein, 2014/02/06 22:01:24]

Right, SkPictureFlat wants read-only contiguous access to all the bytes ever
written, starting from the front.  If this has become a problem, I can rewrite
SkPictureFlat to work around it again with flatten(), at the expense of a couple
hundred extra bytes of overhead and a some occasional reallocations: not a big
deal.  I don't want SkPictureFlat to constrain the implementation choices we can
make in SkWriter32.  This is nice to take advantage of if possible, but I
wouldn't call it important enough to be a requirement.

If it unties things, I'll happily follow this up by removing contiguousArray(). 
The more interesting method for us to support long-term is some flavor of
release().

[iancottrell, 2014/02/07 12:20:40]

Yes, let's just deprecate this method, the two use cases in skia are both easy
to replace, one with writeToStream and one with release.

     const uint32_t* contiguousArray() const {
-        if (this->externalCount()  == 0) {
-            return fInternal.begin();
-        } else if (fInternal.isEmpty()) {
-            return fExternal;
-        }
-        return NULL;
+        return (uint32_t*)fData;
     }
 
     // size MUST be multiple of 4
     uint32_t* reserve(size_t size) {
         SkASSERT(SkAlign4(size) == size);
-        const int count = SkToInt(size/4);
-
-        uint32_t* p;
-        // Once we start writing to fInternal, we never write to fExternal again.
-        // This simplifies tracking what data is where.
-        if (fInternal.isEmpty() && this->externalCount() + count <= fExternalLimit) {
-            p = fExternal + fCount;
-        } else {
-            p = fInternal.append(count);
-        }
-
-        fCount += count;
-        return p;
+        size_t offset = fUsed;
+        size_t used = fUsed + size;
+        if (used > fCapacity) { grow(used); }

[reed1, 2014/02/06 20:14:40]

style nit: skia always wants the body on the next line of an "if"

[reed1, 2014/02/06 20:14:40]

Perhaps a comment that we are passing "used" as the amount we want to grow by,
and that that is just the current growth policy, and not a strict requirement of
the API.

e.g.

if (used > fCapacity) {
    size_t amountToGrowBy = used; // current policy is to double each time we
grow
    this->grow(amountToGrowBy);
}

[iancottrell, 2014/02/06 21:33:06]

Done.

[iancottrell, 2014/02/06 21:33:06]

used is the space used in total after the current reserve is done, it is the
minimum size the array has to be after the grow not the delta. If the array
fails to grow to at least that large, bad things will immediately happen as the
caller overwrites the end of the array!
The growth function is only extracted because it reduces this function to the
minimum needed for the common non growth case to improve the inlining.
Would you like me to add comments to this effect?

The growth policy is dictated by SkTDArray, and unknown to this interface. At
the moment it is to increase by (n+4)*0.25 which is quite a slow growth
strategy.

On 2014/02/06 20:14:40, reed1 wrote:

[reed1, 2014/02/06 21:41:34]

I think here the value is just knowing what the meaning of the parameter is.

+        fUsed = used;
+        return (uint32_t*)(fData+offset);

[reed1, 2014/02/06 20:14:40]

style nit: skia likes spaces around binary operators

[iancottrell, 2014/02/06 21:33:06]

Done.

     }
 
     // Read or write 4 bytes at offset, which must be a multiple of 4 <= size().
-    uint32_t read32At(size_t offset) { return this->atOffset(offset); }
-    void write32At(size_t offset, uint32_t val) { this->atOffset(offset) = val; }
+    uint32_t read32At(size_t offset) {
+        SkASSERT(SkAlign4(offset) == offset);
+        SkASSERT(offset < fUsed);
+        return *(uint32_t*)(fData+offset);
+    }
+
+    void write32At(size_t offset, uint32_t val) {
+        SkASSERT(SkAlign4(offset) == offset);
+        SkASSERT(offset < fUsed);
+        *(uint32_t*)(fData+offset) = val;
+    }
 
     bool writeBool(bool value) {
         this->write32(value);
@@ -157,8 +159,6 @@ public:
      */
     void write(const void* values, size_t size) {
         SkASSERT(SkAlign4(size) == size);
-        // TODO: If we're going to spill from fExternal to fInternal, we might want to fill
-        // fExternal as much as possible before writing to fInternal.
         memcpy(this->reserve(size), values, size);
     }
 
@@ -209,26 +209,17 @@ public:
      */
     void rewindToOffset(size_t offset) {
         SkASSERT(SkAlign4(offset) == offset);
-        const int count = SkToInt(offset/4);
-        if (count < this->externalCount()) {
-            fInternal.setCount(0);
-        } else {
-            fInternal.setCount(count - this->externalCount());
-        }
-        fCount = count;
+        SkASSERT(offset <= bytesWritten());
+        fUsed = offset;
     }
 
     // copy into a single buffer (allocated by caller). Must be at least size()
     void flatten(void* dst) const {
-        const size_t externalBytes = this->externalCount()*4;
-        memcpy(dst, fExternal, externalBytes);
-        dst = (uint8_t*)dst + externalBytes;
-        memcpy(dst, fInternal.begin(), fInternal.bytes());
+        memcpy(dst, fData, fUsed);
     }
 
     bool writeToStream(SkWStream* stream) const {
-        return stream->write(fExternal, this->externalCount()*4)
-            && stream->write(fInternal.begin(), fInternal.bytes());
+        return stream->write(fData, fUsed);
     }
 
     // read from the stream, and write up to length bytes. Return the actual
@@ -238,25 +229,13 @@ public:
     }
 
 private:
-    uint32_t& atOffset(size_t offset) {
-        SkASSERT(SkAlign4(offset) == offset);
-        const int count = SkToInt(offset/4);
-        SkASSERT(count < fCount);
-
-        if (count < this->externalCount()) {
-            return fExternal[count];
-        }
-        return fInternal[count - this->externalCount()];
-    }
-
-
-    // Number of uint32_t written into fExternal.  <= fExternalLimit.
-    int externalCount() const { return fCount - fInternal.count(); }
+    void grow(size_t size);

[reed1, 2014/02/06 20:14:40]

// size is the number of bytes we want to grow by, not the new total size.

Perhaps naming the method growBy(size_t delta) would be clearer?

[iancottrell, 2014/02/06 21:33:06]

size is the new minimum size. Maybe calling the function growToAtLeast(size_t
size) would be clearer?

[reed1, 2014/02/06 21:41:34]

That seems much clearer.

 
-    int fCount;                     // Total number of uint32_t written.
-    int fExternalLimit;             // Number of uint32_t we can write to fExternal.
-    uint32_t* fExternal;            // Unmanaged memory block.
-    SkTDArray<uint32_t> fInternal;  // Managed memory block.
+    uint8_t* fData;                // Points to either fInternal or fExternal.
+    size_t fCapacity;              // Number of bytes we can write to fData.
+    size_t fUsed;                  // Number of bytes written.
+    void* fExternal;               // Unmanaged memory block.
+    SkTDArray<uint8_t> fInternal;  // Managed memory block.
 };
 
 /**