Add MultiPictureDraw to nanobench

Add MultiPictureDraw to nanobench

I would like some guard against performance regressions on our side before turning layer hoisting on in Chromium.

TBR=bsalomon@google.com

Committed: https://skia.googlesource.com/skia/+/0ddad31012dabfc1267effc8071d37f7d606efbe

Committed: https://skia.googlesource.com/skia/+/5b69377507478623dcf5b11f3ecb010f87c4794f

[robertphillips, 2014-11-19 18:30:03]

robertphillips@google.com changed reviewers:
+ mtklein@google.com

[mtklein, 2014-11-19 19:12:37]

https://codereview.chromium.org/731973005/diff/20001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/731973005/diff/20001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:72: DEFINE_bool(bbh, true, "Build a BBH for SKPs?");
Seems like bbh and mpd don't really need to be flags (they should just always be
true).  I'm okay to land --mpd, but I'm inclined to follow up and remove both. 
Do you care or just following the pattern here?

https://codereview.chromium.org/731973005/diff/20001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:530: while (fCurrentUseMPD < fUseMPDs.count()) {
I seem to be falling into an infinite loop when I run something like
  out/Release/nanobench --match carsvg
It's probably somewhere in here.  Sorry this BenchStream class is so weird...
been meaning to make it less error prone to extend.

https://codereview.chromium.org/731973005/diff/20001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:595: 
Let's add "SkTaskGroup::Enabler enabled;" here  (and #include "SkTaskGroup.h" up
top)?  That'll parallelize the 565 and 888 _mpds.

[robertphillips, 2014-11-19 19:58:58]

https://codereview.chromium.org/731973005/diff/20001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/731973005/diff/20001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:72: DEFINE_bool(bbh, true, "Build a BBH for SKPs?");
On 2014/11/19 19:12:37, mtklein wrote:
> Seems like bbh and mpd don't really need to be flags (they should just always
be
> true).  I'm okay to land --mpd, but I'm inclined to follow up and remove both.

> Do you care or just following the pattern here?

I was mainly following the pattern. It does seem like the bbh flag would enable
checking the recording overhead of the BBH while the mpd flag would allow faster
execution if someone didn't care about mpd.

https://codereview.chromium.org/731973005/diff/20001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:530: while (fCurrentUseMPD < fUseMPDs.count()) {
On 2014/11/19 19:12:37, mtklein wrote:
> I seem to be falling into an infinite loop when I run something like
>   out/Release/nanobench --match carsvg
> It's probably somewhere in here.  Sorry this BenchStream class is so weird...
> been meaning to make it less error prone to extend.

Done.

https://codereview.chromium.org/731973005/diff/20001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:595: 
On 2014/11/19 19:12:36, mtklein wrote:
> Let's add "SkTaskGroup::Enabler enabled;" here  (and #include "SkTaskGroup.h"
up
> top)?  That'll parallelize the 565 and 888 _mpds.

Done.

[mtklein, 2014-11-19 20:03:53]

The CQ bit was checked by mtklein@google.com

[mtklein, 2014-11-19 20:03:53]

lgtm

[commit-bot: I haz the power, 2014-11-19 20:04:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/731973005/40001

[commit-bot: I haz the power, 2014-11-19 20:17:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-19 20:17:49]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on client.skia
(http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)

[robertphillips, 2014-11-19 20:26:17]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-11-19 20:27:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/731973005/40001

[commit-bot: I haz the power, 2014-11-19 20:38:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-19 20:38:38]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on client.skia
(http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)

[robertphillips, 2014-11-19 20:52:49]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-11-19 20:53:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/731973005/60001

[robertphillips, 2014-11-19 20:53:53]

The CQ bit was unchecked by robertphillips@google.com

[robertphillips, 2014-11-19 21:05:42]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-11-19 21:06:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/731973005/60001

[robertphillips, 2014-11-19 21:06:52]

The CQ bit was unchecked by robertphillips@google.com

[robertphillips, 2014-11-20 14:17:06]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-11-20 14:17:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/731973005/80001

[commit-bot: I haz the power, 2014-11-20 14:17:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-20 14:17:21]

Presubmit check for 731973005-80001 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', 'djsollen@chromium.org', 'djsollen@google.com')

Presubmit checks took 1.3s to calculate.

[robertphillips, 2014-11-20 14:18:25]

robertphillips@google.com changed reviewers:
+ bsalomon@google.com

[robertphillips, 2014-11-20 14:18:42]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-11-20 14:19:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/731973005/80001

[robertphillips, 2014-11-20 14:19:52]

The CQ bit was unchecked by robertphillips@google.com

[mtklein, 2014-11-20 14:21:47]

https://codereview.chromium.org/731973005/diff/80001/include/core/SkColorTable.h
File include/core/SkColorTable.h (right):

https://codereview.chromium.org/731973005/diff/80001/include/core/SkColorTabl...
include/core/SkColorTable.h:49: SkDEBUGCODE(sk_atomic_inc(&fColorLockCount);)
It's funny we inline lockColors() and unlock16BitCache(), but outline
unlockColors() and lock16BitCache().

:)

Sort of seems we should drop the lock() and unlock() methods here if we're only
doing anything in debug mode.  What's the point of the extra burden to call them
if we don't really require it?

[robertphillips, 2014-11-21 13:18:35]

https://codereview.chromium.org/731973005/diff/120001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (left):

https://codereview.chromium.org/731973005/diff/120001/src/core/SkBitmap.cpp#o...
src/core/SkBitmap.cpp:1325: SkASSERT(fPixelRef);
In a multithreaded world this bitmap's unlockPixels method can be called on this
bitmap between the 'fPixels' check and here so, 'fPixelLockCount' can be zero.

[robertphillips, 2014-11-21 13:18:40]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-11-21 13:19:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/731973005/120001

[commit-bot: I haz the power, 2014-11-21 13:36:01]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/0ddad31012dabfc1267effc8071d37f7d606efbe

[mtklein, 2014-11-21 13:40:44]

https://codereview.chromium.org/731973005/diff/120001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (left):

https://codereview.chromium.org/731973005/diff/120001/src/core/SkBitmap.cpp#o...
src/core/SkBitmap.cpp:1325: SkASSERT(fPixelRef);
On 2014/11/21 13:18:35, robertphillips wrote:
> In a multithreaded world this bitmap's unlockPixels method can be called on
this
> bitmap between the 'fPixels' check and here so, 'fPixelLockCount' can be zero.

Not lgtm

This seems important for correctness.  Isn't the issue SkBitmaps are just not
threadafe?  We've always copied them before use... Did we miss a spot?

If you like, let's revert this, reland the original cl without threading, and
I'll follow up on turning it on.  Don't want to block getting mpd running.

[robertphillips, 2014-11-21 13:49:16]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/750583002/ by robertphillips@google.com.

The reason for reverting is: Needs more work.

[robertphillips, 2014-11-21 14:01:11]

PTAL.

[mtklein, 2014-11-21 14:05:02]

lgtm

[robertphillips, 2014-11-21 14:07:39]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-11-21 14:08:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/731973005/140001

[commit-bot: I haz the power, 2014-11-21 14:19:40]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/5b69377507478623dcf5b11f3ecb010f87c4794f

[jcgregorio, 2014-11-21 15:02:30]

On 2014/11/21 14:19:40, I haz the power (commit-bot) wrote:
> Committed patchset #8 (id:140001) as
> https://skia.googlesource.com/skia/+/5b69377507478623dcf5b11f3ecb010f87c4794f

https://skiaperf.com/#1501

Tests with mpd are now flowing into skiaperf, but it doesn't look like this CL
added any new parameters to query those tests results by?

[jcgregorio, 2014-11-21 15:03:10]

jcgregorio@google.com changed reviewers:
+ jcgregorio@google.com

[jcgregorio, 2014-11-21 15:03:11]

Adding mtklein for the mpd parameter question.

[mtklein, 2014-11-21 15:13:21]

https://codereview.chromium.org/731973005/diff/140001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/731973005/diff/140001/bench/nanobench.cpp#new...
bench/nanobench.cpp:578: log->configOption("multi_picture_draw",
fUseMPDs[fCurrentUseMPD-1] ? "true" : "false");
This doesn't do it?