Change growth function for SkWriter32

include/core/SkTDArray.h

 
 /*
  * Copyright 2006 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 SkTDArray_DEFINED
@@ skipping 133 matching lines @@
         } else {
             SkASSERT(fReserve == 0 && fCount == 0);
         }
     }
 
     void rewind() {
         // same as setCount(0)
         fCount = 0;
     }
 
+    /**
+     *  Sets the number of elements in the array.
+     *  If the array does not have space for count elements, it will increase
+     *  the storage allocated to some amount greater than that required.
+     *  It will never shrink the shrink the storage.
+     */
     void setCount(int count) {

[mtklein, 2014/02/11 12:52:55]

I suspect that we'll ultimately find we can get away with the equivalents of
just setReserve, setCountExact, and the various append/insert methods in the
public API here.  Don't mind this for now given the focus of this CL is not
really SkTDArray.

Can you add a "TODO(mtklein): eliminate this method, setCountExact -> setCount"
here?

[iancottrell, 2014/02/11 13:25:04]

Done.

+        SkASSERT(count >= 0);
         if (count > fReserve) {
-            this->growBy(count - fCount);
-        } else {
-            fCount = count;
+            resizeStorageToAtLeast(count);
         }
+        fCount = count;
+    }
+
+    /**
+     *  Adjusts the number of elements in the array.
+     *  This is the same as calling setCount(count() + delta).
+     */
+    void adjustCount(int delta) {

[mtklein, 2014/02/11 12:52:55]

Can you move this down to private?

[iancottrell, 2014/02/11 13:25:04]

Done.

+        setCount(fCount + delta);
+    }
+
+    /**
+     *  Sets the number of elements in the array.
+     *  If the array does not have space for count elements, it will increase
+     *  the storage allocated to exactly the amount required, with no remaining
+     *  reserved space.
+     *  It will never shrink the shrink the storage.
+     */
+    void setCountExact(int count) {
+        if (count > fReserve) {
+            this->resizeStorageToExact(count);
+        }
+        fCount = count;
     }
 
     void setReserve(int reserve) {
         if (reserve > fReserve) {
-            SkASSERT(reserve > fCount);
-            int count = fCount;
-            this->growBy(reserve - fCount);
-            fCount = count;
+            resizeStorageToAtLeast(reserve);
         }
     }
 
     T* prepend() {
-        this->growBy(1);
+        this->adjustCount(1);
         memmove(fArray + 1, fArray, (fCount - 1) * sizeof(T));
         return fArray;
     }
 
     T* append() {
         return this->append(1, NULL);
     }
     T* append(int count, const T* src = NULL) {
         int oldCount = fCount;
         if (count)  {
             SkASSERT(src == NULL || fArray == NULL ||
                     src + count <= fArray || fArray + oldCount <= src);
 
-            this->growBy(count);
+            this->adjustCount(count);
             if (src) {
                 memcpy(fArray + oldCount, src, sizeof(T) * count);
             }
         }
         return fArray + oldCount;
     }
 
     T* appendClear() {
         T* result = this->append();
         *result = 0;
         return result;
     }
 
     T* insert(int index) {
         return this->insert(index, 1, NULL);
     }
     T* insert(int index, int count, const T* src = NULL) {
         SkASSERT(count);
         SkASSERT(index <= fCount);
         size_t oldCount = fCount;
-        this->growBy(count);
+        this->adjustCount(count);
         T* dst = fArray + index;
         memmove(dst + count, dst, sizeof(T) * (oldCount - index));
         if (src) {
             memcpy(dst, src, sizeof(T) * count);
         }
         return dst;
     }
 
     void remove(int index, int count = 1) {
         SkASSERT(index + count <= fCount);
@@ skipping 131 matching lines @@
     enum {
         kDebugArraySize = 16
     };
     typedef T ArrayT[kDebugArraySize];
     ArrayT* fData;
 #endif
     T*      fArray;
     int     fReserve;
     int     fCount;
 
-    void growBy(int extra) {
-        SkASSERT(extra);
+    /**
+     *  This resizes the storage to *exactly* count elements, growing or
+     *  shrinking the allocation as needed. It does not ASSERT anything about
+     *  the previous allocation size, or about fCount.
+     *
+     *  note: does NOT modify fCount
+     */
+    void resizeStorageToExact(int count) {
+        SkASSERT(count >= 0);
+        fArray = (T*)sk_realloc_throw(fArray, count * sizeof(T));
+#ifdef SK_DEBUG
+        fData = (ArrayT*)fArray;
+#endif
+        fReserve = count;
+    }
 
-        if (fCount + extra > fReserve) {
-            int size = fCount + extra + 4;
-            size += size >> 2;
-
-            fArray = (T*)sk_realloc_throw(fArray, size * sizeof(T));
-#ifdef SK_DEBUG
-            fData = (ArrayT*)fArray;
-#endif
-            fReserve = size;
-        }
-        fCount += extra;
+    /**
+     *  Increase the storage allocation such that it can hold (fCount + extra)
+     *  elements.
+     *  It never shrinks the allocation, and it may increase the allocation by
+     *  more than is strictly required, based on a private growth heuristic.
+     *
+     *  note: does NOT modify fCount
+     */
+    void resizeStorageToAtLeast(int count) {
+        SkASSERT(count > fReserve);
+        int space = count + 4;
+        space += space>>2;

[mtklein, 2014/02/11 12:52:55]

Is this CL blocking any others?  If not, I'd be interested in seeing what we
could make work with a pluggable growth policy.  We're effectively routing
around an undesirable resizeStorageToAtLeast in SkWriter32 now.  It'd be nicer
to inject the desirable one.  Can we try templatizing the growth policy as
f(current, delta), defaulting to (current+delta+4)*5/4?

[iancottrell, 2014/02/11 13:25:04]

It is blocking one more cl, yes.
I have already done some experiments with how to add this directly to SkTDArray,
but would rather do them as separate cl's if that is ok. I suspect they will be
long discussions :)

+        resizeStorageToExact(space);
     }
 };
 
 #endif