Perform multi core rendering in bench_pictures.

Perform multi core rendering in bench_pictures.

Add a flag in SkGPipeWriter for threadsafe drawing.

Add a deferred pipe controller to SamplePipeControllers, which can
be called to play back in multiple threads.

Depends on http://codereview.appspot.com/6459105/

Committed: https://code.google.com/p/skia/source/detail?r=5371

[Leon, 2012-08-29 21:25:32]

New patch, now that Derek's change has landed. Ready to go in if you guys
approve.

[reed1, 2012-08-30 12:02:18]

https://codereview.appspot.com/6482068/diff/25001/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.appspot.com/6482068/diff/25001/src/pipe/SkGPipeRead.cpp#ne...
src/pipe/SkGPipeRead.cpp:441: // Make a shallow copy for thread safety in
SkBitmap::lockPixels.
thread safety in lockpixels? Just there? I presume we make a copy of the
"struct" because none of its methods are thread-safe.

https://codereview.appspot.com/6482068/diff/25001/src/pipe/SkGPipeRead.cpp#ne...
src/pipe/SkGPipeRead.cpp:447: } else {
what is this case? no flags are set at all? can we make an assert on the state
of flags here (or is that not meaningful)?

[DerekS, 2012-08-30 15:14:21]

https://codereview.appspot.com/6482068/diff/25001/include/pipe/SkGPipe.h
File include/pipe/SkGPipe.h (right):

https://codereview.appspot.com/6482068/diff/25001/include/pipe/SkGPipe.h#newc...
include/pipe/SkGPipe.h:107: *  Tells the writer that there will be multiple
threads reading the stream.
should we comment that this is only meaningful if the kCrossProcess_Flag is not
set?

https://codereview.appspot.com/6482068/diff/25001/include/pipe/SkGPipe.h#newc...
include/pipe/SkGPipe.h:109: kCrossThread_Flag               = 1 << 2,
no comma here so the linter doesn't complain

https://codereview.appspot.com/6482068/diff/25001/src/pipe/utils/SamplePipeCo...
File src/pipe/utils/SamplePipeControllers.h (right):

https://codereview.appspot.com/6482068/diff/25001/src/pipe/utils/SamplePipeCo...
src/pipe/utils/SamplePipeControllers.h:75: size_t fSize;
size is a little ambiguous to me since it is not the allocated size of the
fBlock but the bytes of the block that were used.

https://codereview.appspot.com/6482068/diff/25001/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6482068/diff/25001/tools/PictureRenderer.cpp#n...
tools/PictureRenderer.cpp:322: , fClone(original->clone())
if we batch create the clones we will get better perf than creating them one at
a time.

[Leon, 2012-08-30 20:03:58]

New patch uploaded.

https://codereview.appspot.com/6482068/diff/25001/include/pipe/SkGPipe.h
File include/pipe/SkGPipe.h (right):

https://codereview.appspot.com/6482068/diff/25001/include/pipe/SkGPipe.h#newc...
include/pipe/SkGPipe.h:107: *  Tells the writer that there will be multiple
threads reading the stream.
On 2012/08/30 15:14:22, DerekS wrote:
> should we comment that this is only meaningful if the kCrossProcess_Flag is
not
> set?

Actually, I think the two flags are orthogonal. kCrossProcess_Flag means that
the reader is in a different process, so we need to either flatten bitmaps or
use kSharedAddressSpace_Flag (it also means that we cannot use function pointers
for factories). The kCrossThread_Flag means that multiple threads will be
potentially reading the same bitmap simultaneously. This does make me think the
names should be improved, though. What if I set changed them such that:

kCrossProcess_Flag -> kReaderInASeparateProcess_Flag
kCrossThread_Flag  -> kMultipleSimultaneousReaders_Flag

? I am open to suggestion; those names are a mouthful (though I think they
better represent what is meant).

https://codereview.appspot.com/6482068/diff/25001/include/pipe/SkGPipe.h#newc...
include/pipe/SkGPipe.h:109: kCrossThread_Flag               = 1 << 2,
On 2012/08/30 15:14:22, DerekS wrote:
> no comma here so the linter doesn't complain

We had a thread a while back about commas on skia-discuss:
https://groups.google.com/forum/?fromgroups=#!searchin/skia-discuss/comma$20e...

We had some resistance when someone encouraged eliminating them on the last enum
values. Brian likes trailing commas, and Ben likes them except in the case where
the last enum is a count. I am in favor of being consistent. Keeping the linter
happy seems like a good goal, but it sounds like we have strong voices in favor
of the commas. I think we should modify the style guide and enforce it (either
way).

https://codereview.appspot.com/6482068/diff/25001/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.appspot.com/6482068/diff/25001/src/pipe/SkGPipeRead.cpp#ne...
src/pipe/SkGPipeRead.cpp:441: // Make a shallow copy for thread safety in
SkBitmap::lockPixels.
On 2012/08/30 12:02:18, reed1 wrote:
> thread safety in lockpixels? Just there? I presume we make a copy of the
> "struct" because none of its methods are thread-safe.

When I was doing multi core drawing in the past, I recall there being two places
that bit me: lockPixels and the color table lock, which Derek fixed. I am not
aware of other places that are not thread safe. That said, I am happy to make
the comment more general if we think there are other gotchas.

https://codereview.appspot.com/6482068/diff/25001/src/pipe/SkGPipeRead.cpp#ne...
src/pipe/SkGPipeRead.cpp:447: } else {
On 2012/08/30 12:02:18, reed1 wrote:
> what is this case? no flags are set at all? can we make an assert on the state
> of flags here (or is that not meaningful)?

Assert added. This is the case where we are not cross thread, and we are not
flattening bitmaps.

https://codereview.appspot.com/6482068/diff/25001/src/pipe/utils/SamplePipeCo...
File src/pipe/utils/SamplePipeControllers.h (right):

https://codereview.appspot.com/6482068/diff/25001/src/pipe/utils/SamplePipeCo...
src/pipe/utils/SamplePipeControllers.h:75: size_t fSize;
On 2012/08/30 15:14:22, DerekS wrote:
> size is a little ambiguous to me since it is not the allocated size of the
> fBlock but the bytes of the block that were used.

Changed to fBytes, and added a comment.

https://codereview.appspot.com/6482068/diff/25001/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6482068/diff/25001/tools/PictureRenderer.cpp#n...
tools/PictureRenderer.cpp:322: , fClone(original->clone())
On 2012/08/30 15:14:22, DerekS wrote:
> if we batch create the clones we will get better perf than creating them one
at
> a time.

Oops. I wrote this before your final patch. Fixed in next patch.

[DerekS, 2012-08-30 20:33:24]

https://codereview.appspot.com/6482068/diff/25001/include/pipe/SkGPipe.h
File include/pipe/SkGPipe.h (right):

https://codereview.appspot.com/6482068/diff/25001/include/pipe/SkGPipe.h#newc...
include/pipe/SkGPipe.h:107: *  Tells the writer that there will be multiple
threads reading the stream.
On 2012/08/30 20:03:58, scroggo-work wrote:
> On 2012/08/30 15:14:22, DerekS wrote:
> > should we comment that this is only meaningful if the kCrossProcess_Flag is
> not
> > set?
> 
> Actually, I think the two flags are orthogonal. kCrossProcess_Flag means that
> the reader is in a different process, so we need to either flatten bitmaps or
> use kSharedAddressSpace_Flag (it also means that we cannot use function
pointers
> for factories). The kCrossThread_Flag means that multiple threads will be
> potentially reading the same bitmap simultaneously. This does make me think
the
> names should be improved, though. What if I set changed them such that:
> 
> kCrossProcess_Flag -> kReaderInASeparateProcess_Flag
> kCrossThread_Flag  -> kMultipleSimultaneousReaders_Flag
> 
> ? I am open to suggestion; those names are a mouthful (though I think they
> better represent what is meant).

I think that kCrossProcess is pretty clear at what it does and is nice and
concise.  You could call the other one kParallelReaders or kSimultaneousReaders.

https://codereview.appspot.com/6482068/diff/25001/include/pipe/SkGPipe.h#newc...
include/pipe/SkGPipe.h:109: kCrossThread_Flag               = 1 << 2,
On 2012/08/30 20:03:58, scroggo-work wrote:
> On 2012/08/30 15:14:22, DerekS wrote:
> > no comma here so the linter doesn't complain
> 
> We had a thread a while back about commas on skia-discuss:
>
https://groups.google.com/forum/?fromgroups=#%21searchin/skia-discuss/comma%2...
> 
> We had some resistance when someone encouraged eliminating them on the last
enum
> values. Brian likes trailing commas, and Ben likes them except in the case
where
> the last enum is a count. I am in favor of being consistent. Keeping the
linter
> happy seems like a good goal, but it sounds like we have strong voices in
favor
> of the commas. I think we should modify the style guide and enforce it (either
> way).

I forgot that the thread had no resolution, so I'm fine with either way as it
doesn't look like we will turn --pedantic on.

https://codereview.appspot.com/6482068/diff/24014/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.appspot.com/6482068/diff/24014/src/pipe/SkGPipeRead.cpp#ne...
src/pipe/SkGPipeRead.cpp:436: if (shouldFlattenBitmaps(flags)) {
I assume that shouldFlattenBitmaps really means cross process? If so can we
update the name? I'm fine if that goes into another CL if needed.

https://codereview.appspot.com/6482068/diff/24014/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6482068/diff/24014/tools/PictureRenderer.cpp#n...
tools/PictureRenderer.cpp:350: SkPicture* clones = SkNEW_ARRAY(SkPicture,
fTiles.count());
could we could put this on the stack...

SkPicture clones[fTiles.count()];

[Leon, 2012-08-30 21:31:15]

https://codereview.appspot.com/6482068/diff/25001/include/pipe/SkGPipe.h
File include/pipe/SkGPipe.h (right):

https://codereview.appspot.com/6482068/diff/25001/include/pipe/SkGPipe.h#newc...
include/pipe/SkGPipe.h:107: *  Tells the writer that there will be multiple
threads reading the stream.
On 2012/08/30 20:33:24, DerekS wrote:
> On 2012/08/30 20:03:58, scroggo-work wrote:
> > On 2012/08/30 15:14:22, DerekS wrote:
> > > should we comment that this is only meaningful if the kCrossProcess_Flag
is
> > not
> > > set?
> > 
> > Actually, I think the two flags are orthogonal. kCrossProcess_Flag means
that
> > the reader is in a different process, so we need to either flatten bitmaps
or
> > use kSharedAddressSpace_Flag (it also means that we cannot use function
> pointers
> > for factories). The kCrossThread_Flag means that multiple threads will be
> > potentially reading the same bitmap simultaneously. This does make me think
> the
> > names should be improved, though. What if I set changed them such that:
> > 
> > kCrossProcess_Flag -> kReaderInASeparateProcess_Flag
> > kCrossThread_Flag  -> kMultipleSimultaneousReaders_Flag
> > 
> > ? I am open to suggestion; those names are a mouthful (though I think they
> > better represent what is meant).
> 
> I think that kCrossProcess is pretty clear at what it does and is nice and
> concise.  You could call the other one kParallelReaders or
kSimultaneousReaders.

Done.

https://codereview.appspot.com/6482068/diff/24014/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.appspot.com/6482068/diff/24014/src/pipe/SkGPipeRead.cpp#ne...
src/pipe/SkGPipeRead.cpp:436: if (shouldFlattenBitmaps(flags)) {
On 2012/08/30 20:33:24, DerekS wrote:
> I assume that shouldFlattenBitmaps really means cross process? If so can we
> update the name? I'm fine if that goes into another CL if needed.

It does not. It means cross process && !shared address space.

Theoretically we had a plan to have the reader in a different process but
sharing some address space, in which case we do not flatten bitmaps.

https://codereview.appspot.com/6482068/diff/24014/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6482068/diff/24014/tools/PictureRenderer.cpp#n...
tools/PictureRenderer.cpp:350: SkPicture* clones = SkNEW_ARRAY(SkPicture,
fTiles.count());
On 2012/08/30 20:33:24, DerekS wrote:
> could we could put this on the stack...
> 
> SkPicture clones[fTiles.count()];

No. Since SkPicture is not plain old data, I cannot use a variable length array.

[DerekS, 2012-08-31 13:24:17]

lgtm with a few nits.

https://codereview.appspot.com/6482068/diff/24014/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.appspot.com/6482068/diff/24014/src/pipe/SkGPipeRead.cpp#ne...
src/pipe/SkGPipeRead.cpp:436: if (shouldFlattenBitmaps(flags)) {
On 2012/08/30 21:31:16, scroggo-work wrote:
> On 2012/08/30 20:33:24, DerekS wrote:
> > I assume that shouldFlattenBitmaps really means cross process? If so can we
> > update the name? I'm fine if that goes into another CL if needed.
> 
> It does not. It means cross process && !shared address space.
> 
> Theoretically we had a plan to have the reader in a different process but
> sharing some address space, in which case we do not flatten bitmaps.

Does that just mean !shared_address_space? In other words can you have
!shared_address_space and not be cross process?

https://codereview.appspot.com/6482068/diff/20005/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.appspot.com/6482068/diff/20005/src/pipe/SkGPipeRead.cpp#ne...
src/pipe/SkGPipeRead.cpp:441: if (SkToBool(flags &
SkGPipeWriter::kSimultaneousReaders_Flag)) {
would it be worth adding a inlined header function for
SkToBool(flags & SkGPipeWriter::kSimultaneousReaders_Flag)

So you could just say something like
isFlagSet(SkGPipeWriter::kSimultaneousReaders_Flag)

[Leon, 2012-08-31 14:33:56]

https://codereview.appspot.com/6482068/diff/24014/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.appspot.com/6482068/diff/24014/src/pipe/SkGPipeRead.cpp#ne...
src/pipe/SkGPipeRead.cpp:436: if (shouldFlattenBitmaps(flags)) {
> Does that just mean !shared_address_space? In other words can you have
> !shared_address_space and not be cross process?

The way I have written it (and commented it in SkGPipe.h), shared_address_space
is meaningless without cross_process; i.e. we assume that address space is
shared in single process mode (and there's no way to change that).

Arguably, I could have written it this way:

CrossProcess_Flag: Cannot share function pointers for factories
SeparateAddressSpace_Flag: Requires flattening bitmaps

This would have the advantage of meaning that we only look at one flag to
determine whether to flatten bitmaps. On the other hand, it seems like an
unintuitive interface to me. Cross process normally means no shared address
space, so the natural case would require two flags.

Another option would be the following:

CrossProcess_Flag: Cannot share function pointers, and requires flattening
bitmaps
CrossProcessWithSharedAddressSpace_Flag: Cannot share function pointers

Internally CrossProcess_Flag could resolve to two bits, one meaning no sharing
function pointers (shared with the other flag), the other meaning we have to
flatten bitmaps.

Another approach is to not provide the shared address space flag at all right
now, since there is no actual client at the moment.

https://codereview.appspot.com/6482068/diff/20005/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.appspot.com/6482068/diff/20005/src/pipe/SkGPipeRead.cpp#ne...
src/pipe/SkGPipeRead.cpp:441: if (SkToBool(flags &
SkGPipeWriter::kSimultaneousReaders_Flag)) {
On 2012/08/31 13:24:18, DerekS wrote:
> would it be worth adding a inlined header function for
> SkToBool(flags & SkGPipeWriter::kSimultaneousReaders_Flag)
> 
> So you could just say something like
> isFlagSet(SkGPipeWriter::kSimultaneousReaders_Flag)

Right now this is the only place where we check for a single flag. Would it help
readability if I add the following function?

static bool allowSimultaneousReaders(const unsigned flags) {
    return SkToBool(flags & SkGPipeWriter::kSimultaneousReaders_Flag);
}