Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
156683004:
Cleaner external buffer handling in 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@master Visibility:
Public. |
DescriptionCleaner external buffer handling in SkWriter32
This unifies the internal and external buffer handling so that the difference only has to be noticed when growing.
Removing the branches from the common read and write cases gives a significant speedup.
BUG=skia:2125
Committed: http://code.google.com/p/skia/source/detail?r=13396
Patch Set 1 #
Total comments: 21
Patch Set 2 : Review fixes (spaced and comments) #
Total comments: 15
Patch Set 3 : Review fixes: Renames, comments and asserts #Patch Set 4 : Make the SkWriter32 constructor call reset #
Messages
Total messages: 15 (0 generated)
This does not alter the growth pattern, or implement the buffer transfer optimizations, it just fixes external buffer handling.
https://codereview.chromium.org/156683004/diff/1/include/core/SkTDArray.h File include/core/SkTDArray.h (right): https://codereview.chromium.org/156683004/diff/1/include/core/SkTDArray.h#new... include/core/SkTDArray.h:104: * Return the number of elements there is space for without growing. // This includes however many elements may already have been added or // This is the number of remaining "slots" that have been allocated but not accounted for or // reserved() - count() is the number of additional slots that have been allocated but not "used" yet. or ? https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:40: SkASSERT(SkIsAlign4(externalBytes)); or we could just trim the low bits off of externalBytes and ignore them. https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:60: // The pointer may be invalidated by any future write calls. 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... https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:70: if (used > fCapacity) { grow(used); } style nit: skia always wants the body on the next line of an "if" https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:70: if (used > fCapacity) { grow(used); } 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); } https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:72: return (uint32_t*)(fData+offset); style nit: skia likes spaces around binary operators https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:232: void grow(size_t size); // 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?
https://codereview.chromium.org/156683004/diff/1/include/core/SkTDArray.h File include/core/SkTDArray.h (right): https://codereview.chromium.org/156683004/diff/1/include/core/SkTDArray.h#new... include/core/SkTDArray.h:104: * Return the number of elements there is space for without growing. On 2014/02/06 20:14:40, reed1 wrote: > // This includes however many elements may already have been added > > or > > // This is the number of remaining "slots" that have been allocated but not > accounted for > > or > > // reserved() - count() is the number of additional slots that have been > allocated but not "used" yet. > > or ? Done. Is this better? https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:40: SkASSERT(SkIsAlign4(externalBytes)); On 2014/02/06 20:14:40, reed1 wrote: > or we could just trim the low bits off of externalBytes and ignore them. I was staying compatible with the contract it used to have, happy to change it, but maybe as a separate cl? https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:60: // The pointer may be invalidated by any future write calls. On 2014/02/06 20:14:40, reed1 wrote: > 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... 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(). https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:70: if (used > fCapacity) { grow(used); } On 2014/02/06 20:14:40, reed1 wrote: > style nit: skia always wants the body on the next line of an "if" Done. https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:70: if (used > fCapacity) { grow(used); } 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: > 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); > } https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:72: return (uint32_t*)(fData+offset); On 2014/02/06 20:14:40, reed1 wrote: > style nit: skia likes spaces around binary operators Done. https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:232: void grow(size_t size); On 2014/02/06 20:14:40, reed1 wrote: > // 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? size is the new minimum size. Maybe calling the function growToAtLeast(size_t size) would be clearer?
lgtm https://codereview.chromium.org/156683004/diff/1/include/core/SkTDArray.h File include/core/SkTDArray.h (right): https://codereview.chromium.org/156683004/diff/1/include/core/SkTDArray.h#new... include/core/SkTDArray.h:104: * Return the number of elements there is space for without growing. On 2014/02/06 21:33:06, iancottrell wrote: > On 2014/02/06 20:14:40, reed1 wrote: > > // This includes however many elements may already have been added > > > > or > > > > // This is the number of remaining "slots" that have been allocated but not > > accounted for > > > > or > > > > // reserved() - count() is the number of additional slots that have been > > allocated but not "used" yet. > > > > or ? > > Done. Is this better? If I add 3 elements, and call reserve() and it returns 5, can I add 5 more or 2 more without growing? https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:40: SkASSERT(SkIsAlign4(externalBytes)); On 2014/02/06 21:33:06, iancottrell wrote: > On 2014/02/06 20:14:40, reed1 wrote: > > or we could just trim the low bits off of externalBytes and ignore them. > > I was staying compatible with the contract it used to have, happy to change it, > but maybe as a separate cl? Sure, separate CL or not at all. https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:60: // The pointer may be invalidated by any future write calls. On 2014/02/06 21:33:06, iancottrell wrote: > On 2014/02/06 20:14:40, reed1 wrote: > > 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... > > 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(). 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... https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:70: if (used > fCapacity) { grow(used); } On 2014/02/06 21:33:06, iancottrell wrote: > 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: > > 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); > > } > I think here the value is just knowing what the meaning of the parameter is. https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:232: void grow(size_t size); On 2014/02/06 21:33:06, iancottrell wrote: > On 2014/02/06 20:14:40, reed1 wrote: > > // 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? > > size is the new minimum size. Maybe calling the function growToAtLeast(size_t > size) would be clearer? That seems much clearer. https://codereview.chromium.org/156683004/diff/70001/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/156683004/diff/70001/include/core/SkWriter32.... include/core/SkWriter32.h:34: : fData((uint8_t*)external) Seems a tiny shame that the constructor and reset() don't share code. https://codereview.chromium.org/156683004/diff/70001/include/core/SkWriter32.... include/core/SkWriter32.h:219: void flatten(void* dst) const { A future iteration of the API might deprecate this signature, in favor of one where the caller also passes the size of their buffer, just for sanity checking.
https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:60: // The pointer may be invalidated by any future write calls. On 2014/02/06 21:33:06, iancottrell wrote: > On 2014/02/06 20:14:40, reed1 wrote: > > 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... > > 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(). 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(). https://codereview.chromium.org/156683004/diff/70001/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/156683004/diff/70001/include/core/SkWriter32.... include/core/SkWriter32.h:34: : fData((uint8_t*)external) On 2014/02/06 21:41:34, reed1 wrote: > Seems a tiny shame that the constructor and reset() don't share code. Agreed. It's equivalent to write this, right? SkWriter32(void* external = NULL, size_t externalBytes = 0) { this->reset(external, externalBytes); } https://codereview.chromium.org/156683004/diff/70001/include/core/SkWriter32.... include/core/SkWriter32.h:69: size_t used = fUsed + size; This might make more sense if you rename used to required. size_t offset = fUsed; size_t required = fUsed + size; if (required > fCapacity) { this->grow(required); } fUsed = required; return (uint32_t*)(fData + offset); https://codereview.chromium.org/156683004/diff/70001/include/core/SkWriter32.... include/core/SkWriter32.h:219: void flatten(void* dst) const { On 2014/02/06 21:41:34, reed1 wrote: > A future iteration of the API might deprecate this signature, in favor of one > where the caller also passes the size of their buffer, just for sanity checking. Ack. With a .release() and a .writeToStream(), we may be able to even eliminate this method altogether. https://codereview.chromium.org/156683004/diff/70001/src/core/SkWriter32.cpp File src/core/SkWriter32.cpp (right): https://codereview.chromium.org/156683004/diff/70001/src/core/SkWriter32.cpp#... src/core/SkWriter32.cpp:81: // expand the buffer to it's full capacity Can you just add another half sentence explaining why? Is it, // Expand the buffer to its full capacity; we're going to be writing the memory manually rather than calling its methods. https://codereview.chromium.org/156683004/diff/70001/src/core/SkWriter32.cpp#... src/core/SkWriter32.cpp:83: } Can you add an SkASSERT(fInternal.reserved() == fCapacity); after this line? Helps document what we're trying to accomplish.
https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... include/core/SkWriter32.h:60: // The pointer may be invalidated by any future write calls. On 2014/02/06 22:01:24, mtklein wrote: > On 2014/02/06 21:33:06, iancottrell wrote: > > On 2014/02/06 20:14:40, reed1 wrote: > > > 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... > > > > 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(). > > 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(). 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. https://codereview.chromium.org/156683004/diff/70001/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/156683004/diff/70001/include/core/SkWriter32.... include/core/SkWriter32.h:34: : fData((uint8_t*)external) On 2014/02/06 22:01:24, mtklein wrote: > On 2014/02/06 21:41:34, reed1 wrote: > > Seems a tiny shame that the constructor and reset() don't share code. > > Agreed. It's equivalent to write this, right? > > SkWriter32(void* external = NULL, size_t externalBytes = 0) { > this->reset(external, externalBytes); > } It's functionally equivalent, but I have always thought it was wrong to do that. Reset ought to be able to assume it is starting from a valid state, a constructor cannot. Also a compiler will do a much better job dealing with an initializer list than a method call. If there was any actual logic in here, I would extract it to something called by both. I will happily change this if you like, just explaining my reasoning. https://codereview.chromium.org/156683004/diff/70001/include/core/SkWriter32.... include/core/SkWriter32.h:69: size_t used = fUsed + size; On 2014/02/06 22:01:24, mtklein wrote: > This might make more sense if you rename used to required. > > size_t offset = fUsed; > size_t required = fUsed + size; > if (required > fCapacity) { > this->grow(required); > } > fUsed = required; > return (uint32_t*)(fData + offset); Done. https://codereview.chromium.org/156683004/diff/70001/include/core/SkWriter32.... include/core/SkWriter32.h:219: void flatten(void* dst) const { On 2014/02/06 22:01:24, mtklein wrote: > On 2014/02/06 21:41:34, reed1 wrote: > > A future iteration of the API might deprecate this signature, in favor of one > > where the caller also passes the size of their buffer, just for sanity > checking. > > Ack. With a .release() and a .writeToStream(), we may be able to even eliminate > this method altogether. Yes, the stuff to release the buffer is coming in cl number 3 (2 is the one that changes the array growth pattern). Once we have that, we should deprecate flatten and contiguousArray https://codereview.chromium.org/156683004/diff/70001/src/core/SkWriter32.cpp File src/core/SkWriter32.cpp (right): https://codereview.chromium.org/156683004/diff/70001/src/core/SkWriter32.cpp#... src/core/SkWriter32.cpp:81: // expand the buffer to it's full capacity On 2014/02/06 22:01:24, mtklein wrote: > Can you just add another half sentence explaining why? Is it, > > // Expand the buffer to its full capacity; we're going to be writing the memory > manually rather than calling its methods. Done. https://codereview.chromium.org/156683004/diff/70001/src/core/SkWriter32.cpp#... src/core/SkWriter32.cpp:81: // expand the buffer to it's full capacity On 2014/02/06 22:01:24, mtklein wrote: > Can you just add another half sentence explaining why? Is it, > > // Expand the buffer to its full capacity; we're going to be writing the memory > manually rather than calling its methods. Done. https://codereview.chromium.org/156683004/diff/70001/src/core/SkWriter32.cpp#... src/core/SkWriter32.cpp:81: // expand the buffer to it's full capacity On 2014/02/06 22:01:24, mtklein wrote: > Can you just add another half sentence explaining why? Is it, > > // Expand the buffer to its full capacity; we're going to be writing the memory > manually rather than calling its methods. Done. This is changing again in the next cl, where I take control of the growth directly. https://codereview.chromium.org/156683004/diff/70001/src/core/SkWriter32.cpp#... src/core/SkWriter32.cpp:83: } On 2014/02/06 22:01:24, mtklein wrote: > Can you add an > SkASSERT(fInternal.reserved() == fCapacity); > after this line? Helps document what we're trying to accomplish. Done. https://codereview.chromium.org/156683004/diff/70001/src/core/SkWriter32.cpp#... src/core/SkWriter32.cpp:83: } On 2014/02/06 22:01:24, mtklein wrote: > Can you add an > SkASSERT(fInternal.reserved() == fCapacity); > after this line? Helps document what we're trying to accomplish. Done.
On 2014/02/07 12:20:40, iancottrell wrote: > https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h > File include/core/SkWriter32.h (right): > > https://codereview.chromium.org/156683004/diff/1/include/core/SkWriter32.h#ne... > include/core/SkWriter32.h:60: // The pointer may be invalidated by any future > write calls. > On 2014/02/06 22:01:24, mtklein wrote: > > On 2014/02/06 21:33:06, iancottrell wrote: > > > On 2014/02/06 20:14:40, reed1 wrote: > > > > 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... > > > > > > 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(). > > > > 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(). > > 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. > > https://codereview.chromium.org/156683004/diff/70001/include/core/SkWriter32.h > File include/core/SkWriter32.h (right): > > https://codereview.chromium.org/156683004/diff/70001/include/core/SkWriter32.... > include/core/SkWriter32.h:34: : fData((uint8_t*)external) > On 2014/02/06 22:01:24, mtklein wrote: > > On 2014/02/06 21:41:34, reed1 wrote: > > > Seems a tiny shame that the constructor and reset() don't share code. > > > > Agreed. It's equivalent to write this, right? > > > > SkWriter32(void* external = NULL, size_t externalBytes = 0) { > > this->reset(external, externalBytes); > > } > > It's functionally equivalent, but I have always thought it was wrong to do that. > Reset ought to be able to assume it is starting from a valid state, a > constructor cannot. Also a compiler will do a much better job dealing with an > initializer list than a method call. > If there was any actual logic in here, I would extract it to something called by > both. > I will happily change this if you like, just explaining my reasoning. > > https://codereview.chromium.org/156683004/diff/70001/include/core/SkWriter32.... > include/core/SkWriter32.h:69: size_t used = fUsed + size; > On 2014/02/06 22:01:24, mtklein wrote: > > This might make more sense if you rename used to required. > > > > size_t offset = fUsed; > > size_t required = fUsed + size; > > if (required > fCapacity) { > > this->grow(required); > > } > > fUsed = required; > > return (uint32_t*)(fData + offset); > > Done. > > https://codereview.chromium.org/156683004/diff/70001/include/core/SkWriter32.... > include/core/SkWriter32.h:219: void flatten(void* dst) const { > On 2014/02/06 22:01:24, mtklein wrote: > > On 2014/02/06 21:41:34, reed1 wrote: > > > A future iteration of the API might deprecate this signature, in favor of > one > > > where the caller also passes the size of their buffer, just for sanity > > checking. > > > > Ack. With a .release() and a .writeToStream(), we may be able to even > eliminate > > this method altogether. > > Yes, the stuff to release the buffer is coming in cl number 3 (2 is the one that > changes the array growth pattern). Once we have that, we should deprecate > flatten and contiguousArray > > https://codereview.chromium.org/156683004/diff/70001/src/core/SkWriter32.cpp > File src/core/SkWriter32.cpp (right): > > https://codereview.chromium.org/156683004/diff/70001/src/core/SkWriter32.cpp#... > src/core/SkWriter32.cpp:81: // expand the buffer to it's full capacity > On 2014/02/06 22:01:24, mtklein wrote: > > Can you just add another half sentence explaining why? Is it, > > > > // Expand the buffer to its full capacity; we're going to be writing the > memory > > manually rather than calling its methods. > > Done. > > https://codereview.chromium.org/156683004/diff/70001/src/core/SkWriter32.cpp#... > src/core/SkWriter32.cpp:81: // expand the buffer to it's full capacity > On 2014/02/06 22:01:24, mtklein wrote: > > Can you just add another half sentence explaining why? Is it, > > > > // Expand the buffer to its full capacity; we're going to be writing the > memory > > manually rather than calling its methods. > > Done. > > https://codereview.chromium.org/156683004/diff/70001/src/core/SkWriter32.cpp#... > src/core/SkWriter32.cpp:81: // expand the buffer to it's full capacity > On 2014/02/06 22:01:24, mtklein wrote: > > Can you just add another half sentence explaining why? Is it, > > > > // Expand the buffer to its full capacity; we're going to be writing the > memory > > manually rather than calling its methods. > > Done. This is changing again in the next cl, where I take control of the growth > directly. > > https://codereview.chromium.org/156683004/diff/70001/src/core/SkWriter32.cpp#... > src/core/SkWriter32.cpp:83: } > On 2014/02/06 22:01:24, mtklein wrote: > > Can you add an > > SkASSERT(fInternal.reserved() == fCapacity); > > after this line? Helps document what we're trying to accomplish. > > Done. > > https://codereview.chromium.org/156683004/diff/70001/src/core/SkWriter32.cpp#... > src/core/SkWriter32.cpp:83: } > On 2014/02/06 22:01:24, mtklein wrote: > > Can you add an > > SkASSERT(fInternal.reserved() == fCapacity); > > after this line? Helps document what we're trying to accomplish. > > Done. lgtm
perhaps we could introduce a private init() method, that can be called by both the construct and reset(). That way we don't impose starting-from-uninitialized on reset() which is public, but we allow for a central place to perform this work.
On 2014/02/07 14:52:49, reed1 wrote: > perhaps we could introduce a private init() method, that can be called by both > the construct and reset(). That way we don't impose starting-from-uninitialized > on reset() which is public, but we allow for a central place to perform this > work. Adding init() strikes me as roundabout. The constructor and reset() are public, sure, but the fact that the constructor calls reset is not. Requiring reset() to work with an uninitialized writer is an implementation constraint that doesn't affect the public API. I don't see any reason not to do it directly if we do want them to call the same code. I have a hunch this will become moot as soon as Ian sends out the next CL. At that point, won't ctor != reset?
On 2014/02/07 15:02:08, mtklein wrote: > On 2014/02/07 14:52:49, reed1 wrote: > > perhaps we could introduce a private init() method, that can be called by both > > the construct and reset(). That way we don't impose > starting-from-uninitialized > > on reset() which is public, but we allow for a central place to perform this > > work. > > Adding init() strikes me as roundabout. The constructor and reset() are public, > sure, but the fact that the constructor calls reset is not. Requiring reset() > to work with an uninitialized writer is an implementation constraint that > doesn't affect the public API. I don't see any reason not to do it directly if > we do want them to call the same code. > > I have a hunch this will become moot as soon as Ian sends out the next CL. At > that point, won't ctor != reset? I don't disagree, just noting that *if* we want to make it more obvious to future implementers that the constructor is relying on a particular behavior of reset(), then calling that out with a separate method, with a less ambiguous name, might help. I'm perfectly fine to just call reset today and assume that if/when reset changes its assumptions, that our testing will catch that.
On 2014/02/07 15:06:34, reed1 wrote: > On 2014/02/07 15:02:08, mtklein wrote: > > On 2014/02/07 14:52:49, reed1 wrote: > > > perhaps we could introduce a private init() method, that can be called by > both > > > the construct and reset(). That way we don't impose > > starting-from-uninitialized > > > on reset() which is public, but we allow for a central place to perform this > > > work. > > > > Adding init() strikes me as roundabout. The constructor and reset() are > public, > > sure, but the fact that the constructor calls reset is not. Requiring reset() > > to work with an uninitialized writer is an implementation constraint that > > doesn't affect the public API. I don't see any reason not to do it directly > if > > we do want them to call the same code. > > > > I have a hunch this will become moot as soon as Ian sends out the next CL. At > > that point, won't ctor != reset? > > I don't disagree, just noting that *if* we want to make it more obvious to > future implementers that the constructor is relying on a particular behavior of > reset(), then calling that out with a separate method, with a less ambiguous > name, might help. I'm perfectly fine to just call reset today and assume that > if/when reset changes its assumptions, that our testing will catch that. Yeah, that's the state I'd like to strive for generally. In my experience, SkWriter32 specifically is tested thoroughly by the combination of unit tests and DM.
On 2014/02/07 15:19:11, mtklein wrote: > On 2014/02/07 15:06:34, reed1 wrote: > > On 2014/02/07 15:02:08, mtklein wrote: > > > On 2014/02/07 14:52:49, reed1 wrote: > > > > perhaps we could introduce a private init() method, that can be called by > > both > > > > the construct and reset(). That way we don't impose > > > starting-from-uninitialized > > > > on reset() which is public, but we allow for a central place to perform > this > > > > work. > > > > > > Adding init() strikes me as roundabout. The constructor and reset() are > > public, > > > sure, but the fact that the constructor calls reset is not. Requiring > reset() > > > to work with an uninitialized writer is an implementation constraint that > > > doesn't affect the public API. I don't see any reason not to do it directly > > if > > > we do want them to call the same code. > > > > > > I have a hunch this will become moot as soon as Ian sends out the next CL. > At > > > that point, won't ctor != reset? > > > > I don't disagree, just noting that *if* we want to make it more obvious to > > future implementers that the constructor is relying on a particular behavior > of > > reset(), then calling that out with a separate method, with a less ambiguous > > name, might help. I'm perfectly fine to just call reset today and assume that > > if/when reset changes its assumptions, that our testing will catch that. > > Yeah, that's the state I'd like to strive for generally. In my experience, > SkWriter32 specifically is tested thoroughly by the combination of unit tests > and DM. Done.
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/156683004/210001
Message was sent while issue was closed.
Change committed as 13396 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
