Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(584)

Issue 6482068: Perform multi core rendering in bench_pictures. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 7 months ago by Leon
Modified:
11 years, 7 months ago
Reviewers:
DerekS, keyar, reed1
CC:
skia-review_googlegroups.com
Base URL:
https://skia.googlecode.com/svn/trunk
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Total comments: 15

Patch Set 6 : #

Total comments: 6

Patch Set 7 : #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+336 lines, -96 lines) Patch
M gyp/tools.gyp View 1 1 chunk +3 lines, -1 line 0 comments Download
M include/core/SkTemplates.h View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M include/pipe/SkGPipe.h View 1 2 3 4 5 6 1 chunk +9 lines, -2 lines 0 comments Download
M src/pipe/SkGPipeRead.cpp View 1 2 3 4 5 6 1 chunk +19 lines, -4 lines 2 comments Download
M src/pipe/utils/SamplePipeControllers.h View 1 2 3 4 5 6 2 chunks +40 lines, -0 lines 0 comments Download
M src/pipe/utils/SamplePipeControllers.cpp View 1 2 3 4 5 1 chunk +37 lines, -0 lines 0 comments Download
M tools/PictureBenchmark.h View 1 2 1 chunk +8 lines, -0 lines 0 comments Download
M tools/PictureBenchmark.cpp View 1 2 1 chunk +8 lines, -5 lines 0 comments Download
M tools/PictureRenderer.h View 1 2 1 chunk +18 lines, -0 lines 0 comments Download
M tools/PictureRenderer.cpp View 1 2 3 4 5 6 4 chunks +98 lines, -10 lines 0 comments Download
M tools/bench_pictures_main.cpp View 1 2 3 4 6 chunks +92 lines, -70 lines 0 comments Download
M tools/picture_utils.h View 1 chunk +1 line, -1 line 0 comments Download
M tools/picture_utils.cpp View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 9
Leon
11 years, 7 months ago (2012-08-24 18:39:18 UTC) #1
Leon
New patch, now that Derek's change has landed. Ready to go in if you guys ...
11 years, 7 months ago (2012-08-29 21:25:32 UTC) #2
reed1
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#newcode441 src/pipe/SkGPipeRead.cpp:441: // Make a shallow copy for thread safety in ...
11 years, 7 months ago (2012-08-30 12:02:18 UTC) #3
DerekS
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#newcode107 include/pipe/SkGPipe.h:107: * Tells the writer that there will be multiple ...
11 years, 7 months ago (2012-08-30 15:14:21 UTC) #4
Leon
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#newcode107 include/pipe/SkGPipe.h:107: * Tells the writer that there ...
11 years, 7 months ago (2012-08-30 20:03:58 UTC) #5
DerekS
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#newcode107 include/pipe/SkGPipe.h:107: * Tells the writer that there will be multiple ...
11 years, 7 months ago (2012-08-30 20:33:24 UTC) #6
Leon
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#newcode107 include/pipe/SkGPipe.h:107: * Tells the writer that there will be multiple ...
11 years, 7 months ago (2012-08-30 21:31:15 UTC) #7
DerekS
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#newcode436 src/pipe/SkGPipeRead.cpp:436: if (shouldFlattenBitmaps(flags)) { On ...
11 years, 7 months ago (2012-08-31 13:24:17 UTC) #8
Leon
11 years, 7 months ago (2012-08-31 14:33:56 UTC) #9
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);
}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b