Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
150663014:
Change growth function for SkWriter32 (Closed)
|
Created:
6 years, 10 months ago by iancottrell Modified:
6 years, 10 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlesource.com/skia.git@no_external_test Visibility:
Public. |
DescriptionChange growth function for SkWriter32
Add setCountExact to SkTDArray to allow external control of array growth.
Use it to allow SkWriter to control the buffer growth pattern.
Change buffer growth pattern to 1.5n+4096
BUG=skia:2125
Committed: http://code.google.com/p/skia/source/detail?r=13401
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add documentation #
Total comments: 4
Patch Set 3 : Cleaning up the SkTDArray growth code to be more understandable #Patch Set 4 : Rename growTo to resizeStorageTo #
Total comments: 6
Patch Set 5 : Add comment and assert. #
Total comments: 6
Patch Set 6 : Make adjustCount private and add TODO for setCountExact #Patch Set 7 : Rebase patch #Patch Set 8 : Read back reserved space in case it is bigger #Messages
Total messages: 19 (0 generated)
This one might need some discussion. This cl takes the approach of adding explicit size controls outside of SkTDArray. We could go with making the SkTDArray more complicated, and adding strategy arguments to the template, but until there is another use case for the same strategy, I thought this was easier/cleaer. I also tried adjusting SkTDArray to take an allocation type strategy that would allow the "external" buffer handling to be inside the array rather than outside, but that seemed very heavy as well. Thoughts?
https://codereview.chromium.org/150663014/diff/1/include/core/SkTDArray.h File include/core/SkTDArray.h (right): https://codereview.chromium.org/150663014/diff/1/include/core/SkTDArray.h#new... include/core/SkTDArray.h:154: void setCount(int count) { yikes -- what can this API mean if it doesn't change fCount all the time? Who calls it? Do we really need both this and setCountExact? Can we add dox for both, explaining the difference? https://codereview.chromium.org/150663014/diff/1/include/core/SkTDArray.h#new... include/core/SkTDArray.h:366: void growTo(int size) { /** * This resizes the storage to *exactly* size (should be count) elements, growing or shrink the allocation as needed. It does not ASSERT anything about the previous allocation size, or about fCount. */ https://codereview.chromium.org/150663014/diff/1/include/core/SkTDArray.h#new... include/core/SkTDArray.h:374: void growBy(int extra) { /** * Increase the storage allocation such 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. */
https://codereview.chromium.org/150663014/diff/1/include/core/SkTDArray.h File include/core/SkTDArray.h (right): https://codereview.chromium.org/150663014/diff/1/include/core/SkTDArray.h#new... include/core/SkTDArray.h:154: void setCount(int count) { On 2014/02/07 15:37:49, reed1 wrote: > yikes -- what can this API mean if it doesn't change fCount all the time? Who > calls it? Do we really need both this and setCountExact? Can we add dox for > both, explaining the difference? Done. https://codereview.chromium.org/150663014/diff/1/include/core/SkTDArray.h#new... include/core/SkTDArray.h:366: void growTo(int size) { On 2014/02/07 15:37:49, reed1 wrote: > /** > * This resizes the storage to *exactly* size (should be count) elements, > growing or shrink the allocation as needed. It does not ASSERT anything about > the previous allocation size, or about fCount. > */ Done. https://codereview.chromium.org/150663014/diff/1/include/core/SkTDArray.h#new... include/core/SkTDArray.h:374: void growBy(int extra) { On 2014/02/07 15:37:49, reed1 wrote: > /** > * Increase the storage allocation such 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. > */ Done.
Thank you for iterating on this. https://codereview.chromium.org/150663014/diff/50001/include/core/SkTDArray.h File include/core/SkTDArray.h (right): https://codereview.chromium.org/150663014/diff/50001/include/core/SkTDArray.h... include/core/SkTDArray.h:163: } else { This is so hard to read (for me this time at least). It appears that sometimes we set fCount and sometimes we don't! I now know that growBy as the side-effect of updating fCount, but that is not clear at all (to me), given that we have this if/else, and given that setCountExact *always* sets fCount, even though it also checks reserve. https://codereview.chromium.org/150663014/diff/50001/include/core/SkTDArray.h... include/core/SkTDArray.h:173: * It will never shrink the shrink the storage. ... * this call allows the caller explicit control over the growth/allocation strategy. calling setCount() defers to the class as to how much extra should be allocated when the request requires a larger allocation. https://codereview.chromium.org/150663014/diff/50001/include/core/SkTDArray.h... include/core/SkTDArray.h:384: void growTo(int count) { rename to: resizeStorageTo() so we know what's going on at the call-sites. https://codereview.chromium.org/150663014/diff/50001/include/core/SkTDArray.h... include/core/SkTDArray.h:401: if (fCount + extra > fReserve) { Slightly unrelated to this CL, but I feel like we should take this opportunity to clean-up this class if possible: Some clients of growBy() check reserve first, and some don't. Can we unify that pattern one way or the other? I ask partly to help clarify (to implementors) what each help method is really doing, what it is checking/asserting on, etc.
On 2014/02/07 17:05:15, reed1 wrote: > Thank you for iterating on this. > > https://codereview.chromium.org/150663014/diff/50001/include/core/SkTDArray.h > File include/core/SkTDArray.h (right): > > https://codereview.chromium.org/150663014/diff/50001/include/core/SkTDArray.h... > include/core/SkTDArray.h:163: } else { > This is so hard to read (for me this time at least). It appears that sometimes > we set fCount and sometimes we don't! I now know that growBy as the side-effect > of updating fCount, but that is not clear at all (to me), given that we have > this if/else, and given that setCountExact *always* sets fCount, even though it > also checks reserve. > > https://codereview.chromium.org/150663014/diff/50001/include/core/SkTDArray.h... > include/core/SkTDArray.h:173: * It will never shrink the shrink the storage. > ... * this call allows the caller explicit control over the growth/allocation > strategy. calling setCount() defers to the class as to how much extra should be > allocated when the request requires a larger allocation. > > https://codereview.chromium.org/150663014/diff/50001/include/core/SkTDArray.h... > include/core/SkTDArray.h:384: void growTo(int count) { > rename to: resizeStorageTo() so we know what's going on at the call-sites. > > https://codereview.chromium.org/150663014/diff/50001/include/core/SkTDArray.h... > include/core/SkTDArray.h:401: if (fCount + extra > fReserve) { > Slightly unrelated to this CL, but I feel like we should take this opportunity > to clean-up this class if possible: > > Some clients of growBy() check reserve first, and some don't. Can we unify that > pattern one way or the other? I ask partly to help clarify (to implementors) > what each help method is really doing, what it is checking/asserting on, etc. How about this try. I have change grows to be called resizeStorage, and they *never* adjust the count. setCount now adjusts the count itself, and the previous calls to growBy in the append/prepend/insert functions now call adjustCount that calls setCount.
lgtm https://codereview.chromium.org/150663014/diff/160001/include/core/SkTDArray.h File include/core/SkTDArray.h (right): https://codereview.chromium.org/150663014/diff/160001/include/core/SkTDArray.... include/core/SkTDArray.h:387: * the previous allocation size, or about fCount. * * note: does NOT modify fCount https://codereview.chromium.org/150663014/diff/160001/include/core/SkTDArray.... include/core/SkTDArray.h:389: void resizeStorageToExact(int count) { SkASSERT(count >= 0); https://codereview.chromium.org/150663014/diff/160001/include/core/SkTDArray.... include/core/SkTDArray.h:401: * more than is strictly required, based on a private growth heuristic. * * note: does NOT modify fCount
https://codereview.chromium.org/150663014/diff/160001/include/core/SkTDArray.h File include/core/SkTDArray.h (right): https://codereview.chromium.org/150663014/diff/160001/include/core/SkTDArray.... include/core/SkTDArray.h:387: * the previous allocation size, or about fCount. On 2014/02/07 18:00:05, reed1 wrote: > * > * note: does NOT modify fCount Done. https://codereview.chromium.org/150663014/diff/160001/include/core/SkTDArray.... include/core/SkTDArray.h:389: void resizeStorageToExact(int count) { On 2014/02/07 18:00:05, reed1 wrote: > SkASSERT(count >= 0); Done. https://codereview.chromium.org/150663014/diff/160001/include/core/SkTDArray.... include/core/SkTDArray.h:401: * more than is strictly required, based on a private growth heuristic. On 2014/02/07 18:00:05, reed1 wrote: > * > * note: does NOT modify fCount Done.
https://codereview.chromium.org/150663014/diff/210001/include/core/SkTDArray.h File include/core/SkTDArray.h (right): https://codereview.chromium.org/150663014/diff/210001/include/core/SkTDArray.... include/core/SkTDArray.h:160: void setCount(int count) { 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? https://codereview.chromium.org/150663014/diff/210001/include/core/SkTDArray.... include/core/SkTDArray.h:172: void adjustCount(int delta) { Can you move this down to private? https://codereview.chromium.org/150663014/diff/210001/include/core/SkTDArray.... include/core/SkTDArray.h:411: space += space>>2; 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?
https://codereview.chromium.org/150663014/diff/210001/include/core/SkTDArray.h File include/core/SkTDArray.h (right): https://codereview.chromium.org/150663014/diff/210001/include/core/SkTDArray.... include/core/SkTDArray.h:160: void setCount(int count) { On 2014/02/11 12:52:55, mtklein wrote: > 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? Done. https://codereview.chromium.org/150663014/diff/210001/include/core/SkTDArray.... include/core/SkTDArray.h:172: void adjustCount(int delta) { On 2014/02/11 12:52:55, mtklein wrote: > Can you move this down to private? Done. https://codereview.chromium.org/150663014/diff/210001/include/core/SkTDArray.... include/core/SkTDArray.h:411: space += space>>2; On 2014/02/11 12:52:55, mtklein wrote: > 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? 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 :)
On 2014/02/11 13:25:04, iancottrell wrote: > https://codereview.chromium.org/150663014/diff/210001/include/core/SkTDArray.h > File include/core/SkTDArray.h (right): > > https://codereview.chromium.org/150663014/diff/210001/include/core/SkTDArray.... > include/core/SkTDArray.h:160: void setCount(int count) { > On 2014/02/11 12:52:55, mtklein wrote: > > 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? > > Done. > > https://codereview.chromium.org/150663014/diff/210001/include/core/SkTDArray.... > include/core/SkTDArray.h:172: void adjustCount(int delta) { > On 2014/02/11 12:52:55, mtklein wrote: > > Can you move this down to private? > > Done. > > https://codereview.chromium.org/150663014/diff/210001/include/core/SkTDArray.... > include/core/SkTDArray.h:411: space += space>>2; > On 2014/02/11 12:52:55, mtklein wrote: > > 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? > > 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 :) lgtm
The CQ bit was checked by iancottrell@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/iancottrell@google.com/150663014/220006
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for src/core/SkWriter32.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file src/core/SkWriter32.cpp
Hunk #1 FAILED at 67.
1 out of 1 hunk FAILED -- saving rejects to file src/core/SkWriter32.cpp.rej
Patch: src/core/SkWriter32.cpp
Index: src/core/SkWriter32.cpp
diff --git a/src/core/SkWriter32.cpp b/src/core/SkWriter32.cpp
index
7285459c3d1e5767fff762d0bb38a4a6f848a2a0..76664e72b385f8f35d26f8570187476f99f53552
100644
--- a/src/core/SkWriter32.cpp
+++ b/src/core/SkWriter32.cpp
@@ -67,20 +67,20 @@ size_t SkWriter32::WriteStringSize(const char* str, size_t
len) {
return SkAlign4(lenBytes + len + 1);
}
+const size_t kMinBufferBytes=4096;
+
void SkWriter32::growToAtLeast(size_t size) {
bool wasExternal = (fExternal != NULL) && (fData == fExternal);
+ fCapacity = kMinBufferBytes +
+ SkTMax(size, fCapacity + (fCapacity >> 1));
+
// cause the buffer to grow
- fInternal.setCount(size);
+ fInternal.setCountExact(fCapacity);
fData = fInternal.begin();
if (wasExternal) {
// we were external, so copy in the data
memcpy(fData, fExternal, fUsed);
}
- // Find out the size the buffer grew to, it may be more than we asked for.
- fCapacity = fInternal.reserved();
- // Expand the array so all reserved space is "used", we maintain the
- // amount we have written manually outside the array
- fInternal.setCount(fCapacity);
- SkASSERT(fInternal.count() == fCapacity);
- SkASSERT(fInternal.reserved() == fCapacity);
+ SkASSERT(fInternal.count() == (int)fCapacity);
+ SkASSERT(fInternal.reserved() == (int)fCapacity);
}
The CQ bit was checked by iancottrell@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/iancottrell@google.com/150663014/400001
Message was sent while issue was closed.
Change committed as 13401
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/132313013/ by mtklein@google.com. The reason for reverting is: See https://codereview.chromium.org/152703007/..
Adding stuff to deal with reset buffers. |
