Cleaner external buffer handling in SkWriter32

include/core/SkWriter32.h

 
 /*
  * Copyright 2008 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 
 #ifndef SkWriter32_DEFINED
@@ skipping 12 matching lines @@
 
 class SkWriter32 : SkNoncopyable {
 public:
     /**
      *  The caller can specify an initial block of storage, which the caller manages.
      *
      *  SkWriter32 will try to back reserve and write calls with this external storage until the
      *  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(); }
 
     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);
         return value;
     }
 
     void writeInt(int32_t value) {
         this->write32(value);
     }
 
@@ skipping 55 matching lines @@
     void writeMul4(const void* values, size_t size) {
         this->write(values, size);
     }
 
     /**
      *  Write size bytes from values. size must be a multiple of 4, though
      *  values need not be 4-byte aligned.
      */
     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);
     }
 
     /**
      *  Reserve size bytes. Does not need to be 4 byte aligned. The remaining space (if any) will be
      *  filled in with zeroes.
      */
     uint32_t* reservePad(size_t size) {
         uint32_t* p = this->reserve(SkAlign4(size));
         uint8_t* tail = (uint8_t*)p + size;
@@ skipping 30 matching lines @@
      *  computed by calling strlen().
      */
     static size_t WriteStringSize(const char* str, size_t len = (size_t)-1);
 
     /**
      *  Move the cursor back to offset bytes from the beginning.
      *  offset must be a multiple of 4 no greater than size().
      */
     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
     // number of bytes written.
     size_t readFromStream(SkStream* stream, size_t length) {
         return stream->read(this->reservePad(length), length);
     }
 
 private:
-    uint32_t& atOffset(size_t offset) {
-        SkASSERT(SkAlign4(offset) == offset);
-        const int count = SkToInt(offset/4);
-        SkASSERT(count < fCount);
+    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.

 
-        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(); }
-
-    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.
 };
 
 /**
  *  Helper class to allocated SIZE bytes as part of the writer, and to provide
  *  that storage to the constructor as its initial storage buffer.
  *
  *  This wrapper ensures proper alignment rules are met for the storage.
  */
 template <size_t SIZE> class SkSWriter32 : public SkWriter32 {
 public:
     SkSWriter32() : SkWriter32(fData.fStorage, SIZE) {}
 
 private:
     union {
         void*   fPtrAlignment;
         double  fDoubleAlignment;
         char    fStorage[SIZE];
     } fData;
 };
 
 #endif