Issue 6777063: In bench_pictures --multi, maintain thread local caches.

Issue 6777063: In bench_pictures --multi, maintain thread local caches. (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
11 years, 8 months ago by Leon

Modified:
11 years, 8 months ago

Reviewers:
mtklein

CC:
skia-review_googlegroups.com

Base URL:
https://skia.googlecode.com/svn/trunk

Visibility:
Public.

More Reviews

Description

In bench_pictures --multi, maintain thread local caches. Builds on https://codereview.appspot.com/6718046/ by mtklein. Previously, each iteration of drawing a picture started new threads to draw the picture. Since each thread is using thread local storage for the font cache, this means that each iteration had to start with an empty font cache. The newly added MultiCorePictureRenderer, separated from TiledPictureRenderer, now starts the drawing threads at the beginning of the test using an SkThreadPool, and keeps them alive through all iterations, so the font cache can be reused. For now, I have removed the pipe version of the threaded renderer. Updated bench_pictures_main and render_pictures_main to use the new renderer, and to unref a renderer before early exit. Committed: https://code.google.com/p/skia/source/detail?r=6285

Patch Set 1 #

Patch Set 2 : Changes to accommodate changes in SkThreadPool #

Patch Set 3 : #

Patch Set 4 : fix a warning #

Patch Set 5 : attempt to fix a bad upload #

Patch Set 6 : #

Total comments: 20

Patch Set 7 : #

Patch Set 8 : rebase #

Patch Set 9 : another rebase #

Patch Set 10 : last rebase i hope... #

Patch Set 11 : ... #

Created: 11 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+264 lines, -296 lines)			Patch
M	tools/PictureRenderer.h	View	1 2 3 4 5 6 7	6 chunks	+46 lines, -28 lines	0 comments	Download
M	tools/PictureRenderer.cpp	View	1 2 3 4 5 6 7	4 chunks	+130 lines, -199 lines	0 comments	Download
M	tools/bench_pictures_main.cpp	View	1 2 3 4 5 6 7	15 chunks	+58 lines, -62 lines	0 comments	Download
M	tools/render_pictures_main.cpp	View		8 chunks	+30 lines, -7 lines	0 comments	Download

Messages

Total messages: 4

Expand All Messages | Collapse All Messages

mtklein

https://codereview.appspot.com/6777063/diff/15001/tools/PictureRenderer.cpp File tools/PictureRenderer.cpp (right): https://codereview.appspot.com/6777063/diff/15001/tools/PictureRenderer.cpp#newcode369 tools/PictureRenderer.cpp:369: class ThreadData : public SkRunnable { ThreadData is a ...

11 years, 8 months ago (2012-11-02 00:12:53 UTC) #2

Leon

11 years, 8 months ago (2012-11-02 19:38:03 UTC) #3

https://codereview.appspot.com/6777063/diff/15001/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6777063/diff/15001/tools/PictureRenderer.cpp#n...
tools/PictureRenderer.cpp:369: class ThreadData : public SkRunnable {
On 2012/11/02 00:12:53, mtklein wrote:
> ThreadData is a sort of vague name for this guy.  TileDrawer?  

Agreed. I stuck with the name that I used when I originally wrote this to be the
data passed to the thread's constructor. I'm not sure I like TileDrawer any
better. I ended up merging this with CloneData.

> It'd be nice if
> we could pass CloneData and Countdown into the constructor instead of filling
in
> later.  Something like this?
> 
> class TileDrawer : public SkRunnable {
> public:
>     TileDrawer(CloneData* cloneData, SkRunnable* done)
>     : fCloneData(cloneData)
>     , fDone(done) {}
>     
>     virtual void run() SK_OVERRIDE {
>       DrawTiles(fCloneData);
>       if (fDone != NULL) {
>          fDone->run();
>       }
>       SK_DELETE(this);

For this test, I don't think it makes sense to delete this on each run. If we
don't delete, we can reuse for the next render().

>     }
> 
> private:
>     CloneData* fCloneData;
>     SkRunnable* fDone;
> };

https://codereview.appspot.com/6777063/diff/15001/tools/PictureRenderer.cpp#n...
tools/PictureRenderer.cpp:382: SkCountdown*  fCountdown;
On 2012/11/02 00:12:53, mtklein wrote:
> Only needs to be an SkRunnable :)

Done.

https://codereview.appspot.com/6777063/diff/15001/tools/PictureRenderer.cpp#n...
tools/PictureRenderer.cpp:431: fCountdown.reset(fNumThreads);
On 2012/11/02 00:12:53, mtklein wrote:
> Might be simpler to create the ThreadData here as you need them?

I don't see the advantage. The current way means that we can create it once in
init(), then keep it around until we finish 'repeat' calls to render(), and then
delete when we are actually done.

https://codereview.appspot.com/6777063/diff/15001/tools/PictureRenderer.cpp#n...
tools/PictureRenderer.cpp:441: void MultiCorePictureRenderer::end() {
On 2012/11/02 00:12:53, mtklein wrote:
> What's the deal here?  Why do we need both end() and a destructor?

end() does cleanup that needs to be done between different SkPictures. The
destructor cleans up once we're done with this PictureRenderer.

https://codereview.appspot.com/6777063/diff/15001/tools/PictureRenderer.h
File tools/PictureRenderer.h (right):

https://codereview.appspot.com/6777063/diff/15001/tools/PictureRenderer.h#new...
tools/PictureRenderer.h:237: MultiCorePictureRenderer(int threadCount);
On 2012/11/02 00:12:53, mtklein wrote:
> Do you guys use explicit?  As written you can autoconvert an int to a
> MultiCorePictureRenderer, which can be quite surprising.  explicit makes that
> autoconversion impossible.

Done.

https://codereview.appspot.com/6777063/diff/15001/tools/bench_pictures_main.cpp
File tools/bench_pictures_main.cpp (left):

https://codereview.appspot.com/6777063/diff/15001/tools/bench_pictures_main.c...
tools/bench_pictures_main.cpp:75: SkDebugf(
On 2012/11/02 00:12:53, mtklein wrote:
> Have you guys ever considered pulling in gflags for this sort of thing?  It's
> open source and once you try it you'll never go back to manually parsing
flags.

We have discussed using some kind of utility, and we are definitely in favor. No
one has experience with any and we just haven't prioritized it. If you would
like to switch us over to gflags I will approve!

https://codereview.appspot.com/6777063/diff/15001/tools/bench_pictures_main.cpp
File tools/bench_pictures_main.cpp (right):

https://codereview.appspot.com/6777063/diff/15001/tools/bench_pictures_main.c...
tools/bench_pictures_main.cpp:165: SkSafeUnref(renderer);
On 2012/11/02 00:12:53, mtklein wrote:
> Might want a small function or macro to SkSafeUnref(renderer), usage(argv0),
and
> exit(-1)?

Good idea. This seems to be the one place where we do not call usage(), but it
is probably not intentional.

I switched to using an SkAutoTUnref for the renderer, so I can remove the
SkSafeUnref anyway.

https://codereview.appspot.com/6777063/diff/15001/tools/bench_pictures_main.c...
tools/bench_pictures_main.cpp:188: //SkSafeUnref(renderer);
On 2012/11/02 00:12:53, mtklein wrote:
> Doesn't this need to not be in // ?

We only need to unref if we exit. Since exit is currently commented out, I put
this in commented out. That said, I switched to SkAutoTUnref, so there is no
need to have this line.

mtklein

11 years, 8 months ago (2012-11-02 20:06:36 UTC) #4

LGTM

https://codereview.appspot.com/6777063/diff/15001/tools/PictureRenderer.cpp
File tools/PictureRenderer.cpp (right):

https://codereview.appspot.com/6777063/diff/15001/tools/PictureRenderer.cpp#n...
tools/PictureRenderer.cpp:369: class ThreadData : public SkRunnable {
On 2012/11/02 19:38:03, scroggo-work wrote:
> On 2012/11/02 00:12:53, mtklein wrote:
> > ThreadData is a sort of vague name for this guy.  TileDrawer?  
> 
> Agreed. I stuck with the name that I used when I originally wrote this to be
the
> data passed to the thread's constructor. I'm not sure I like TileDrawer any
> better. I ended up merging this with CloneData.
> 
> > It'd be nice if
> > we could pass CloneData and Countdown into the constructor instead of
filling
> in
> > later.  Something like this?
> > 
> > class TileDrawer : public SkRunnable {
> > public:
> >     TileDrawer(CloneData* cloneData, SkRunnable* done)
> >     : fCloneData(cloneData)
> >     , fDone(done) {}
> >     
> >     virtual void run() SK_OVERRIDE {
> >       DrawTiles(fCloneData);
> >       if (fDone != NULL) {
> >          fDone->run();
> >       }
> >       SK_DELETE(this);
> 
> For this test, I don't think it makes sense to delete this on each run. If we
> don't delete, we can reuse for the next render().
> 
> >     }
> > 
> > private:
> >     CloneData* fCloneData;
> >     SkRunnable* fDone;
> > };
> 

OK.  I suppose I'm less concerned about object reuse.  This thing is only two
pointers.  They probably get stuffed into registers. :)

https://codereview.appspot.com/6777063/diff/15001/tools/PictureRenderer.cpp#n...
tools/PictureRenderer.cpp:431: fCountdown.reset(fNumThreads);
On 2012/11/02 19:38:03, scroggo-work wrote:
> On 2012/11/02 00:12:53, mtklein wrote:
> > Might be simpler to create the ThreadData here as you need them?
> 
> I don't see the advantage. The current way means that we can create it once in
> init(), then keep it around until we finish 'repeat' calls to render(), and
then
> delete when we are actually done.

OK.

https://codereview.appspot.com/6777063/diff/15001/tools/PictureRenderer.cpp#n...
tools/PictureRenderer.cpp:441: void MultiCorePictureRenderer::end() {
On 2012/11/02 19:38:03, scroggo-work wrote:
> On 2012/11/02 00:12:53, mtklein wrote:
> > What's the deal here?  Why do we need both end() and a destructor?
> 
> end() does cleanup that needs to be done between different SkPictures. The
> destructor cleans up once we're done with this PictureRenderer.

Gotcha.

https://codereview.appspot.com/6777063/diff/15001/tools/bench_pictures_main.cpp
File tools/bench_pictures_main.cpp (right):

https://codereview.appspot.com/6777063/diff/15001/tools/bench_pictures_main.c...
tools/bench_pictures_main.cpp:188: //SkSafeUnref(renderer);
On 2012/11/02 19:38:03, scroggo-work wrote:
> On 2012/11/02 00:12:53, mtklein wrote:
> > Doesn't this need to not be in // ?
> 
> We only need to unref if we exit. Since exit is currently commented out, I put
> this in commented out. That said, I switched to SkAutoTUnref, so there is no
> need to have this line.

Oh, duh.  Thanks for explaining.

Expand All Messages | Collapse All Messages