Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1106113002:
SkRWBuffer for thread-safe 'stream' sharing (Closed)
DescriptionSkRWBuffer for thread-safe 'stream' sharing
WIP
- Can accumulate (write) data in one thread, and share snapshots of it in other threads
... e.g. network accumulates image data, and periodically we want to decode/draw it
- If this sort of thing sticks, should we promote SkData to have the same generality as
SkRBuffer?
BUG=skia:
TBR=
Committed: https://skia.googlesource.com/skia/+/5b6db07fb5d3b67476db6df126eb8290d49e564d
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : starter set of unittests #Patch Set 4 : complete alphabet test #Patch Set 5 : impl stream (untested so far) #Patch Set 6 : add tests for stream #
Total comments: 4
Patch Set 7 : #Patch Set 8 : add chunkalloc append(size) method that returns a contiguous ptr #Patch Set 9 : try to fix warnings #
Total comments: 18
Patch Set 10 : make header private (for now at least) #
Messages
Total messages: 30 (9 generated)
reed@chromium.org changed reviewers: + bungeman@google.com, djsollen@google.com, mtklein@google.com, scroggo@google.com
reed@chromium.org changed reviewers: + reed@google.com
https://codereview.chromium.org/1106113002/diff/20001/src/core/SkRWBuffer.cpp File src/core/SkRWBuffer.cpp (right): https://codereview.chromium.org/1106113002/diff/20001/src/core/SkRWBuffer.cpp... src/core/SkRWBuffer.cpp:53: mutable int32_t fRefCnt; How come we're not using SkRefCnt / SkNVRefCnt? https://codereview.chromium.org/1106113002/diff/20001/src/core/SkRWBuffer.cpp... src/core/SkRWBuffer.cpp:182: fTail->fNext = block; // does this need to be atomic? Which parts of these APIs are meant to be thread safe? It'll be easier to make things thread safe with a mutex than trying to create a lockfree structure. They can be really subtle.
On 2015/04/25 18:49:53, mtklein wrote: > https://codereview.chromium.org/1106113002/diff/20001/src/core/SkRWBuffer.cpp > File src/core/SkRWBuffer.cpp (right): > > https://codereview.chromium.org/1106113002/diff/20001/src/core/SkRWBuffer.cpp... > src/core/SkRWBuffer.cpp:53: mutable int32_t fRefCnt; > How come we're not using SkRefCnt / SkNVRefCnt? Maybe I can, but I wanted to control the allocation directly, rather than calling new/delete. > > https://codereview.chromium.org/1106113002/diff/20001/src/core/SkRWBuffer.cpp... > src/core/SkRWBuffer.cpp:182: fTail->fNext = block; // does this need to be > atomic? > Which parts of these APIs are meant to be thread safe? > > It'll be easier to make things thread safe with a mutex than trying to create a > lockfree structure. They can be really subtle. I'd like to discuss this part on Monday. It seems to me that it should be straight-forward to do this w/o a mutex, but who knows.
Some initial observations. https://codereview.chromium.org/1106113002/diff/100001/include/core/SkRWBuffer.h File include/core/SkRWBuffer.h (right): https://codereview.chromium.org/1106113002/diff/100001/include/core/SkRWBuffe... include/core/SkRWBuffer.h:82: SkData* newDataSnapshot() const; I would be very, very careful about adding this, as it appears that the only way to implement this is to make a copy. Blink's SharedBuffer has a "data()" method which, though it has lots of warnings about what a bad idea it is to call it, is the most used method on SharedBuffer. If allowed, this *will* be over/misused, to the point where there is no point in having the complexity of this class. (Much as there is little point these days to SharredBuffer, since it requires *every user* to use it correctly or it becomes no better than just making copies all the time.) https://codereview.chromium.org/1106113002/diff/100001/src/core/SkRWBuffer.cpp File src/core/SkRWBuffer.cpp (right): https://codereview.chromium.org/1106113002/diff/100001/src/core/SkRWBuffer.cp... src/core/SkRWBuffer.cpp:224: class SkRBufferStreamAsset : public SkStreamAsset { You may want to take a look at SkBlockMemoryStream which is a similar tear-off of SkDynamicMemoryWStream which works very much like this. A few of the method implementations there are optimized for common use cases.
https://codereview.chromium.org/1106113002/diff/100001/include/core/SkRWBuffer.h File include/core/SkRWBuffer.h (right): https://codereview.chromium.org/1106113002/diff/100001/include/core/SkRWBuffe... include/core/SkRWBuffer.h:82: SkData* newDataSnapshot() const; On 2015/04/27 14:52:42, bungeman1 wrote: > I would be very, very careful about adding this, as it appears that the only way > to implement this is to make a copy. Blink's SharedBuffer has a "data()" method > which, though it has lots of warnings about what a bad idea it is to call it, is > the most used method on SharedBuffer. If allowed, this *will* be over/misused, > to the point where there is no point in having the complexity of this class. > (Much as there is little point these days to SharredBuffer, since it requires > *every user* to use it correctly or it becomes no better than just making copies > all the time.) will remove for now https://codereview.chromium.org/1106113002/diff/100001/src/core/SkRWBuffer.cpp File src/core/SkRWBuffer.cpp (right): https://codereview.chromium.org/1106113002/diff/100001/src/core/SkRWBuffer.cp... src/core/SkRWBuffer.cpp:224: class SkRBufferStreamAsset : public SkStreamAsset { On 2015/04/27 14:52:42, bungeman1 wrote: > You may want to take a look at SkBlockMemoryStream which is a similar tear-off > of SkDynamicMemoryWStream which works very much like this. A few of the method > implementations there are optimized for common use cases. Acknowledged.
The CQ bit was checked by reed@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1106113002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by reed@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1106113002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
What do you (reviewers) say to tweaking this (and moving the header into private) and then landing? I agree that we may want to consolidate/modify/share with existing chuck allocators and streams, but perhaps that is best done in parallel, after an initial (private) version lands. thoughts?
https://codereview.chromium.org/1106113002/diff/160001/include/core/SkRWBuffer.h File include/core/SkRWBuffer.h (right): https://codereview.chromium.org/1106113002/diff/160001/include/core/SkRWBuffe... include/core/SkRWBuffer.h:80: void* append(size_t length); How do we know when they're done? You've handed out a pointer to some memory into which the user is going to write, but we can't allow anyone to read it until they're done writing it.
sgtm
https://codereview.chromium.org/1106113002/diff/160001/include/core/SkRWBuffer.h File include/core/SkRWBuffer.h (right): https://codereview.chromium.org/1106113002/diff/160001/include/core/SkRWBuffe... include/core/SkRWBuffer.h:80: void* append(size_t length); On 2015/04/28 15:32:53, bungeman1 wrote: > How do we know when they're done? You've handed out a pointer to some memory > into which the user is going to write, but we can't allow anyone to read it > until they're done writing it. I will add a comment... But the answer is : no existing reader will see the new block (initialized or not). This append call has updated the "used" length of that chunk, but only new readers will pay attention to that value (since they always take the MIN of it and their private budget). Certainly a new reader that is spawned before the data was written will see uninitialized data, but that is just a caller's bug for saying void* ptr = rw.append(size); reader = rw.newRBufferSnapshot(); memcpy(ptr, ...); I will document that the memory returned by append() must be written to *before* a new reader is spawned.
btw -- I want to keep the semantics such that a spawned reader (buffer or stream) *can* share the same memory, but need not if the impl decided otherwise.
https://codereview.chromium.org/1106113002/diff/160001/src/core/SkRWBuffer.cpp File src/core/SkRWBuffer.cpp (right): https://codereview.chromium.org/1106113002/diff/160001/src/core/SkRWBuffer.cp... src/core/SkRWBuffer.cpp:61: static size_t LengthToCapacity(size_t length) { Could be private? https://codereview.chromium.org/1106113002/diff/160001/src/core/SkRWBuffer.cp... src/core/SkRWBuffer.cpp:85: if (1 == sk_atomic_fetch_add(&fRefCnt, -1, sk_memory_order_acq_rel)) { It would be nice to share this code with the other two places (so far) that we use it. https://codereview.chromium.org/1106113002/diff/160001/src/core/SkRWBuffer.cp... src/core/SkRWBuffer.cpp:94: SkDebugf("done\n"); Do you want to check this in? https://codereview.chromium.org/1106113002/diff/160001/src/core/SkRWBuffer.cp... src/core/SkRWBuffer.cpp:118: SkROBuffer::SkROBuffer(const SkBufferHead* head, size_t used) : fHead(SkRef(head)), fUsed(used) { This should be SkSafeRef(head), since head may be NULL. Alternatively, an SkROBuffer is useless with no data. Since this is created by a factory method, maybe that should return NULL if head is NULL? https://codereview.chromium.org/1106113002/diff/160001/src/core/SkRWBuffer.cp... src/core/SkRWBuffer.cpp:247: class AutoValidate { Only needed in SK_DEBUG? https://codereview.chromium.org/1106113002/diff/160001/src/core/SkRWBuffer.cp... src/core/SkRWBuffer.cpp:280: size_t size = fIter.size(); const? https://codereview.chromium.org/1106113002/diff/160001/src/core/SkRWBuffer.cp... src/core/SkRWBuffer.cpp:282: size_t avail = SkTMin(size - fLocalOffset, request - bytesRead); const? https://codereview.chromium.org/1106113002/diff/160001/tests/DataRefTest.cpp File tests/DataRefTest.cpp (right): https://codereview.chromium.org/1106113002/diff/160001/tests/DataRefTest.cpp#... tests/DataRefTest.cpp:288: // Knowning that the default capacity is 4096, choose N large enough so we force it to use Knowing* https://codereview.chromium.org/1106113002/diff/160001/tests/DataRefTest.cpp#... tests/DataRefTest.cpp:297: if (i & 1) { SkToBool?
https://codereview.chromium.org/1106113002/diff/160001/src/core/SkRWBuffer.cpp File src/core/SkRWBuffer.cpp (right): https://codereview.chromium.org/1106113002/diff/160001/src/core/SkRWBuffer.cp... src/core/SkRWBuffer.cpp:61: static size_t LengthToCapacity(size_t length) { On 2015/04/28 17:27:39, scroggo wrote: > Could be private? Possibly, tho this struct is private just to this impl file. https://codereview.chromium.org/1106113002/diff/160001/src/core/SkRWBuffer.cp... src/core/SkRWBuffer.cpp:85: if (1 == sk_atomic_fetch_add(&fRefCnt, -1, sk_memory_order_acq_rel)) { On 2015/04/28 17:27:39, scroggo wrote: > It would be nice to share this code with the other two places (so far) that we > use it. Agreed, but I haven't seen how to do that sharing yet. https://codereview.chromium.org/1106113002/diff/160001/src/core/SkRWBuffer.cp... src/core/SkRWBuffer.cpp:94: SkDebugf("done\n"); On 2015/04/28 17:27:39, scroggo wrote: > Do you want to check this in? Doh! Removed. https://codereview.chromium.org/1106113002/diff/160001/src/core/SkRWBuffer.cp... src/core/SkRWBuffer.cpp:118: SkROBuffer::SkROBuffer(const SkBufferHead* head, size_t used) : fHead(SkRef(head)), fUsed(used) { On 2015/04/28 17:27:39, scroggo wrote: > This should be SkSafeRef(head), since head may be NULL. > > Alternatively, an SkROBuffer is useless with no data. Since this is created by a > factory method, maybe that should return NULL if head is NULL? Done. https://codereview.chromium.org/1106113002/diff/160001/src/core/SkRWBuffer.cp... src/core/SkRWBuffer.cpp:247: class AutoValidate { On 2015/04/28 17:27:39, scroggo wrote: > Only needed in SK_DEBUG? Done. https://codereview.chromium.org/1106113002/diff/160001/tests/DataRefTest.cpp File tests/DataRefTest.cpp (right): https://codereview.chromium.org/1106113002/diff/160001/tests/DataRefTest.cpp#... tests/DataRefTest.cpp:288: // Knowning that the default capacity is 4096, choose N large enough so we force it to use On 2015/04/28 17:27:40, scroggo wrote: > Knowing* Done. https://codereview.chromium.org/1106113002/diff/160001/tests/DataRefTest.cpp#... tests/DataRefTest.cpp:297: if (i & 1) { On 2015/04/28 17:27:40, scroggo wrote: > SkToBool? Acknowledged.
The CQ bit was checked by reed@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1106113002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by reed@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1106113002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://skia.googlesource.com/skia/+/5b6db07fb5d3b67476db6df126eb8290d49e564d |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
