Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
130913018:
Templetized SkWriter32 readTAt() & overwriteTAt() (Closed)
|
Created:
6 years, 10 months ago by f(malita) Modified:
6 years, 10 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionTempletized SkWriter32 readTAt() & overwriteTAt()
Convert SkWriter32::{read,write}32At() to ::{read,overwrite}TAt<>() to allow
peeking/updating arbitrary records.
BUG=skia:
R=reed@google.com,robertphillips@google.com,mtklein@google.com
Committed: http://code.google.com/p/skia/source/detail?r=13416
Patch Set 1 #Patch Set 2 : Win build fix #
Total comments: 12
Patch Set 3 : readTAt()/overwriteTAt() only #
Total comments: 4
Patch Set 4 : Refactored writeString, reservePad. #
Total comments: 4
Patch Set 5 : Updated, rebased. #Patch Set 6 : Rebased #
Messages
Total messages: 22 (0 generated)
This might be useful even considering mtklein's upcoming SkWriter32 refactor.
I think it's a reasonable thing to guarantee that SkWriter32 will keep the bytes it was given contiguously available to be read contiguously, and I don't think it constrains our implementation any more than it's already constrained. Now, caution: Let's note that this is a stronger guarantee than we currently make, which is only that you can read one uint32_t at a time from a SkWriter32. Conceivably we could be scattering 4 byte chunks all over RAM. Today we get around this by doing a copy once to a fully contiguous block. Now, it's also a less strong guarantee than we're soon going to be able to provide in practice, which will be that all bytes ever written are contiguous. We'd like to never make this particular guarantee, to leave flexibility for different implementations. There's a weird tug of war going on between three things we'd like SkWriter32 to do: - allow fast, variable sized appends - use a dynamic allocation strategy that doesn't waste a lot of RAM - make a method like atOffset O(1) If we pick any two of these things are easy and we can choose between a bunch of simple data structures. All three are tricky. Upshot is, I think making this change and guaranteeing contiguous writes are contiguously readable does not eliminate any implementation choices that have not already been practically eliminated by the existence of a public reserve(). But we need to be careful. +Ian, who's doing the real work of making writer better. https://codereview.chromium.org/130913018/diff/70001/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/130913018/diff/70001/include/core/SkWriter32.... include/core/SkWriter32.h:84: // atomically at the given offset. Now that we're looking at it again, it might make sense to rename writeTAt to overwriteTAt. It's different enough from the other write methods (it doesn't reserve space) that it could use a very different name. https://codereview.chromium.org/130913018/diff/70001/include/core/SkWriter32.... include/core/SkWriter32.h:84: // atomically at the given offset. This atomically isn't necessarily congruent to anything in the API. Some writeFoo calls are atomic, some are not. Its hard for me to know whether say, writeString is going to be contiguous. Maybe we should guarantee that every byte written within any single write call is contiguous. Then we replace "atomically" with "by a single call to SkWriter32" and call it a day. That's a nice guarantee to reason about, and really, now that I'm looking, we'll only need to change writeString. https://codereview.chromium.org/130913018/diff/70001/include/core/SkWriter32.... include/core/SkWriter32.h:96: inline void writeT(const T& value) { My only reservation about doing this is that it allows us to write more types publically to SkWriter32 than we used to. That's more power I'd feel better leaving inaccessible until we need it. I'd feel a little cozier with a public API that looks more or less as it was, maybe backed by private template code to make the implementation nicer. I don't mind overloading, so we can keep a public write() that handles everything we support. Here's one idea: public: bool write(bool val) { return this->writeT<uint32_t>(val ? 1 : 0); } void write(uint8_t val) { this->writeT<uint32_t>(val); } void write(uint16_t val) { this->writeT<uint32_t>(val); } void write(uint32_t val) { this->writeT(val); } void write(SkScalar val) { this->writeT(val); } void write(const SkRect& val) { this->writeT(val); } template <typename T> void write(const T* val) { this->writeT(val); } ... // Special cases we may want to keep. void write8(uint32_t val) { this->writeT(val & 0xFF); } void write16(uint32_t val) { this->writeT(val & 0xFFFF); } private: template <typename T> const T& writeT(const T& val) { SkASSERT(sizeof(T) % 4 == 0); *(T)this->reserve(sizeof(T)) = val; return val; } or maybe public: bool write(bool val) { return this->writeT(val); } void write(uint8_t val) { this->writeT(val); } void write(SkScalar val) { this->writeT(val); } ... private: template <typename T> const T& writeT(const T& val) { T* out = sizeof(T) % 4 == 0 ? this->reserve(sizeof(T)) : this->reservePad(sizeof(T)); *out = val; return val; } https://codereview.chromium.org/130913018/diff/70001/include/core/SkWriter32.... include/core/SkWriter32.h:239: SkASSERT(offset + sizeof(T) <= (unsigned)fCount << 2); * 4 would be a bit more consistent with the rest of the file than << 2. https://codereview.chromium.org/130913018/diff/70001/src/core/SkPictureRecord.h File src/core/SkPictureRecord.h (right): https://codereview.chromium.org/130913018/diff/70001/src/core/SkPictureRecord... src/core/SkPictureRecord.h:159: fWriter.writeT<SkScalar>(scalar); This may just be me, but when I see a call like this I assume we're giving the type explicitly to force a type onto the argument. I'd generally prefer to see this as fWriter.writeT(scalar) for simple call sites, so that if we do see an fWriter.writeT<SomeType>(x) we can know we've got some tricky reason to mention the type. https://codereview.chromium.org/130913018/diff/70001/src/pipe/SkGPipeWrite.cpp File src/pipe/SkGPipeWrite.cpp (right): https://codereview.chromium.org/130913018/diff/70001/src/pipe/SkGPipeWrite.cp... src/pipe/SkGPipeWrite.cpp:460: fWriter.writeT<void*>(fBitmapHeap); This guy strikes me as weird now. If we're going to have a writeT, we shouldn't have to say void* any more. void* was our poor man's T*.
https://codereview.chromium.org/130913018/diff/70001/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/130913018/diff/70001/include/core/SkWriter32.... include/core/SkWriter32.h:84: // atomically at the given offset. On 2014/02/06 21:38:16, mtklein wrote: > Now that we're looking at it again, it might make sense to rename writeTAt to > overwriteTAt. It's different enough from the other write methods (it doesn't > reserve space) that it could use a very different name. Sounds good. https://codereview.chromium.org/130913018/diff/70001/include/core/SkWriter32.... include/core/SkWriter32.h:84: // atomically at the given offset. On 2014/02/06 21:38:16, mtklein wrote: > This atomically isn't necessarily congruent to anything in the API. Some > writeFoo calls are atomic, some are not. Its hard for me to know whether say, > writeString is going to be contiguous. > > Maybe we should guarantee that every byte written within any single write call > is contiguous. Then we replace "atomically" with "by a single call to > SkWriter32" and call it a day. That's a nice guarantee to reason about, and > really, now that I'm looking, we'll only need to change writeString. Good catch - I hadn't realized some of the writeXYZ() calls are not atomic. Sounds easy enough to fix, I'll take a look at writeString and update the verbiage. https://codereview.chromium.org/130913018/diff/70001/include/core/SkWriter32.... include/core/SkWriter32.h:96: inline void writeT(const T& value) { On 2014/02/06 21:38:16, mtklein wrote: > My only reservation about doing this is that it allows us to write more types > publically to SkWriter32 than we used to. That's more power I'd feel better > leaving inaccessible until we need it. > > I'd feel a little cozier with a public API that looks more or less as it was, > maybe backed by private template code to make the implementation nicer. I don't > mind overloading, so we can keep a public write() that handles everything we > support. Ah, nice - overloading is not shun upon in Skia. I feel liberated! https://codereview.chromium.org/130913018/diff/70001/include/core/SkWriter32.... include/core/SkWriter32.h:239: SkASSERT(offset + sizeof(T) <= (unsigned)fCount << 2); On 2014/02/06 21:38:16, mtklein wrote: > * 4 would be a bit more consistent with the rest of the file than << 2. Will do. https://codereview.chromium.org/130913018/diff/70001/src/core/SkPictureRecord.h File src/core/SkPictureRecord.h (right): https://codereview.chromium.org/130913018/diff/70001/src/core/SkPictureRecord... src/core/SkPictureRecord.h:159: fWriter.writeT<SkScalar>(scalar); On 2014/02/06 21:38:16, mtklein wrote: > This may just be me, but when I see a call like this I assume we're giving the > type explicitly to force a type onto the argument. I'd generally prefer to see > this as fWriter.writeT(scalar) for simple call sites, so that if we do see an > fWriter.writeT<SomeType>(x) we can know we've got some tricky reason to mention > the type. Will do. https://codereview.chromium.org/130913018/diff/70001/src/pipe/SkGPipeWrite.cpp File src/pipe/SkGPipeWrite.cpp (right): https://codereview.chromium.org/130913018/diff/70001/src/pipe/SkGPipeWrite.cp... src/pipe/SkGPipeWrite.cpp:460: fWriter.writeT<void*>(fBitmapHeap); On 2014/02/06 21:38:16, mtklein wrote: > This guy strikes me as weird now. If we're going to have a writeT, we shouldn't > have to say void* any more. void* was our poor man's T*. Good point (mechanical refactor). Since we're not exposing writeT(), we'll have to revert to using writePtr.
On 2014/02/10 16:10:34, Florin Malita wrote: > https://codereview.chromium.org/130913018/diff/70001/include/core/SkWriter32.... > include/core/SkWriter32.h:96: inline void writeT(const T& value) { > On 2014/02/06 21:38:16, mtklein wrote: > > My only reservation about doing this is that it allows us to write more types > > publically to SkWriter32 than we used to. That's more power I'd feel better > > leaving inaccessible until we need it. > > > > I'd feel a little cozier with a public API that looks more or less as it was, > > maybe backed by private template code to make the implementation nicer. I > don't > > mind overloading, so we can keep a public write() that handles everything we > > support. > > Ah, nice - overloading is not shun upon in Skia. I feel liberated! And, sure enough, overload resolution ambiguities are quick to bring me back to Earth :) Since templetizing readAt/writeAt is what this CL is really aimed at, I'll leave the other methods for another time.
So when I commit https://codereview.chromium.org/156683004/ the entire buffer will always be contiguous in practice, but I agree it would not be nice to make this a promise of the api. I also agree it is reasonable to assume that anything written as a single call can be read with a single call, with the possible exception of void write(const void* values, size_t size) which is writing multiple independant values, and it would not be unreasonable to have them spread across buffers if the implementation went that way. I was also thinking about how to clean up the api in a similar way, but I actually think we ought to go further, and standardise the "writer" interfaces in skia. There seem to be quite a few, and they have fractionally different apis. For instance, I know of at least 4 writeBool methods, 1 of which returns the bool written, one returns whether the writing was a success, and the others return void. I think this is an unnecessary conceptual load, and we should try to clean them all up at once...
On 2014/02/10 20:57:07, ian_cottrell wrote: > So when I commit https://codereview.chromium.org/156683004/ the entire buffer > will always be contiguous in practice, but I agree it would not be nice to make > this a promise of the api. > I also agree it is reasonable to assume that anything written as a single call > can be read with a single call, with the possible exception of > void write(const void* values, size_t size) > which is writing multiple independant values, and it would not be unreasonable > to have them spread across buffers if the implementation went that way. > > I was also thinking about how to clean up the api in a similar way, but I > actually think we ought to go further, and standardise the "writer" interfaces > in skia. There seem to be quite a few, and they have fractionally different > apis. > For instance, I know of at least 4 writeBool methods, 1 of which returns the > bool written, one returns whether the writing was a success, and the others > return void. I think this is an unnecessary conceptual load, and we should try > to clean them all up at once... Agreed, the SkWriter32 API could certainly use a cleanup. This CL is not it, for sure: (I should have made it more explicit to begin with) the intent is to (re)introduce the capability of peeking at atomically written structs (I'm working on a different CL that requires this feature). So if it doesn't directly interfere with ongoing efforts, there is still value in adding readTAt()/overwriteTAt() independently of other cleanup/refactoring.
https://codereview.chromium.org/130913018/diff/200001/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/130913018/diff/200001/include/core/SkWriter32... include/core/SkWriter32.h:90: void overwriteTAt(const T& value, size_t offset) { I have a slight preference for it to go back the other way, to overwriteTAt(offset, value), just so the "At" makes a little more sense in there. Any reason to swap it as you have? https://codereview.chromium.org/130913018/diff/200001/src/core/SkWriter32.cpp File src/core/SkWriter32.cpp (right): https://codereview.chromium.org/130913018/diff/200001/src/core/SkWriter32.cpp... src/core/SkWriter32.cpp:48: // add 1 since we also write a terminating 0 Hmm, maybe we can make this clearer? Does this work? // [ 4 byte len ] [ str ... ] [1-4 \0s] uint32_t* ptr = this->reservePad(sizeof(uint32_t) + len + 1); ptr[0] = len; memcpy(ptr+1, str, len);
On 2014/02/10 22:37:34, mtklein wrote: > https://codereview.chromium.org/130913018/diff/200001/include/core/SkWriter32.h > File include/core/SkWriter32.h (right): > > https://codereview.chromium.org/130913018/diff/200001/include/core/SkWriter32... > include/core/SkWriter32.h:90: void overwriteTAt(const T& value, size_t offset) { > I have a slight preference for it to go back the other way, to > overwriteTAt(offset, value), just so the "At" makes a little more sense in > there. Any reason to swap it as you have? > > https://codereview.chromium.org/130913018/diff/200001/src/core/SkWriter32.cpp > File src/core/SkWriter32.cpp (right): > > https://codereview.chromium.org/130913018/diff/200001/src/core/SkWriter32.cpp... > src/core/SkWriter32.cpp:48: // add 1 since we also write a terminating 0 > Hmm, maybe we can make this clearer? Does this work? > > // [ 4 byte len ] [ str ... ] [1-4 \0s] > uint32_t* ptr = this->reservePad(sizeof(uint32_t) + len + 1); > ptr[0] = len; > memcpy(ptr+1, str, len); Ooof, I clearly forgot to write the \0 at the end... uint32_t* ptr = this->reservePad(sizeof(uint32_t) + len + 1); ptr[0] = len; char* chars = (char*)(ptr+1); memcpy(chars, str, len); chars[len] = '\0'; ?
https://codereview.chromium.org/130913018/diff/200001/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/130913018/diff/200001/include/core/SkWriter32... include/core/SkWriter32.h:90: void overwriteTAt(const T& value, size_t offset) { On 2014/02/10 22:37:34, mtklein wrote: > I have a slight preference for it to go back the other way, to > overwriteTAt(offset, value), just so the "At" makes a little more sense in > there. Any reason to swap it as you have? Seemed more natural to follow the name hints ("overwrite <T> <at>"), but I can see why changing the order may be confusing. I'll switch back. https://codereview.chromium.org/130913018/diff/200001/src/core/SkWriter32.cpp File src/core/SkWriter32.cpp (right): https://codereview.chromium.org/130913018/diff/200001/src/core/SkWriter32.cpp... src/core/SkWriter32.cpp:48: // add 1 since we also write a terminating 0 On 2014/02/10 22:37:34, mtklein wrote: > Hmm, maybe we can make this clearer? Does this work? > > // [ 4 byte len ] [ str ... ] [1-4 \0s] > uint32_t* ptr = this->reservePad(sizeof(uint32_t) + len + 1); > ptr[0] = len; > memcpy(ptr+1, str, len); Ah, reservePad takes care of padding, doesn't it. Except its padding doesn't look quite as efficient as the one-write trick below, but we can fix that.
lgtm https://codereview.chromium.org/130913018/diff/330001/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/130913018/diff/330001/include/core/SkWriter32... include/core/SkWriter32.h:188: p[alignedSize / 4 - 1] = 0; Duh. I don't know why I wrote the old one to be so complicated. We could even satisfy the requirements with just this: SkASSERT(size > 0); size_t alignedSize = SkAlign4(size); uint32_t* p = this->reserve(alignedSize); p[alignedSize / 4 - 1] = 0; Anyway, silly me. LGTM. https://codereview.chromium.org/130913018/diff/330001/tests/Writer32Test.cpp File tests/Writer32Test.cpp (right): https://codereview.chromium.org/130913018/diff/330001/tests/Writer32Test.cpp#... tests/Writer32Test.cpp:197: for (size_t i = 0; i < (padding >> 2); ++i) { padding / 4 ?
https://codereview.chromium.org/130913018/diff/330001/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/130913018/diff/330001/include/core/SkWriter32... include/core/SkWriter32.h:188: p[alignedSize / 4 - 1] = 0; On 2014/02/11 13:18:10, mtklein wrote: > Duh. I don't know why I wrote the old one to be so complicated. We could even > satisfy the requirements with just this: > > SkASSERT(size > 0); > size_t alignedSize = SkAlign4(size); > uint32_t* p = this->reserve(alignedSize); > p[alignedSize / 4 - 1] = 0; Yeah, I kept the conditional mainly for handling reservePad(0) gracefully. https://codereview.chromium.org/130913018/diff/330001/tests/Writer32Test.cpp File tests/Writer32Test.cpp (right): https://codereview.chromium.org/130913018/diff/330001/tests/Writer32Test.cpp#... tests/Writer32Test.cpp:197: for (size_t i = 0; i < (padding >> 2); ++i) { On 2014/02/11 13:18:10, mtklein wrote: > padding / 4 ? Will do.
The CQ bit was checked by fmalita@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/fmalita@chromium.org/130913018/430001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 130913018-430001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Since the CL is editing public API, you must have an LGTM from one of: ('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org', 'bsalomon@google.com') Presubmit checks took 1.4s to calculate.
Wow, are we really promising to support arbitrary-sized contiguous blocks for peeking/reading? I thought we were limiting to 32bits, which (to me) made sense since we explicitly (even in the name of the class) ensure 32bit alignment for all boundaries. If so, lgtm (and good luck to you folks supporting this api in the future :)
The CQ bit was checked by fmalita@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/fmalita@chromium.org/130913018/430001
The CQ bit was unchecked by fmalita@chromium.org
The CQ bit was checked by fmalita@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/fmalita@chromium.org/130913018/600001
Message was sent while issue was closed.
Change committed as 13416 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
