Make nanobench setup configs outside of loop over benchmarks

Make nanobench setup configs outside of loop over benchmarks

Committed: https://skia.googlesource.com/skia/+/c2553373ee982b6c7c753e7e5035523bc01a7291

[mtklein, 2014-07-22 17:44:19]

just nits

https://codereview.chromium.org/410683005/diff/1/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/410683005/diff/1/bench/nanobench.cpp#newcode216
bench/nanobench.cpp:216: const Config* config;
// unowned

or perhaps this?

struct Target {
  explicit Target(const Config& config) : config(config) {}

  const Config& config;
  SkAutoTDelete<SkSurface> surface;
...
}

https://codereview.chromium.org/410683005/diff/1/bench/nanobench.cpp#newcode234
bench/nanobench.cpp:234: int sampleCnt) {
funky alignment here

https://codereview.chromium.org/410683005/diff/1/bench/nanobench.cpp#newcode284
bench/nanobench.cpp:284: // If bench is enabled for backend/config, returns a
Target* for them, otherwise NULL.
... is enabled for config, returns a Target* for it, otherwise NULL.

https://codereview.chromium.org/410683005/diff/1/bench/nanobench.cpp#newcode285
bench/nanobench.cpp:285: static Target* is_enabled(Benchmark* bench, const
Config* config) {
const Config& ?

https://codereview.chromium.org/410683005/diff/1/bench/nanobench.cpp#newcode296
bench/nanobench.cpp:296: info.fWidth = w;
just inline bench->getSize().fX and bench->getSize().fY here?

https://codereview.chromium.org/410683005/diff/1/bench/nanobench.cpp#newcode302
bench/nanobench.cpp:302: SkAutoTUnref<SkSurface> surface;
Why go through the extra pointer?

if (Benchmark::kRaster_Backend == config->backend) {
  target->surface.reset(SkSurface::NewRaster(info));
}
...

if (!target->surface && Benchmark::kNonRenderingBackend != config->backend) {
  ...
}

[bsalomon, 2014-07-22 18:10:15]

https://codereview.chromium.org/410683005/diff/1/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/410683005/diff/1/bench/nanobench.cpp#newcode216
bench/nanobench.cpp:216: const Config* config;
On 2014/07/22 17:44:19, mtklein wrote:
> // unowned
> 
> or perhaps this?
> 
> struct Target {
>   explicit Target(const Config& config) : config(config) {}
> 
>   const Config& config;
>   SkAutoTDelete<SkSurface> surface;
> ...
> }

Done, but makes a copy of config (seems harmless).

https://codereview.chromium.org/410683005/diff/1/bench/nanobench.cpp#newcode234
bench/nanobench.cpp:234: int sampleCnt) {
On 2014/07/22 17:44:19, mtklein wrote:
> funky alignment here

Done.

https://codereview.chromium.org/410683005/diff/1/bench/nanobench.cpp#newcode284
bench/nanobench.cpp:284: // If bench is enabled for backend/config, returns a
Target* for them, otherwise NULL.
On 2014/07/22 17:44:19, mtklein wrote:
> ... is enabled for config, returns a Target* for it, otherwise NULL.

Done.

https://codereview.chromium.org/410683005/diff/1/bench/nanobench.cpp#newcode285
bench/nanobench.cpp:285: static Target* is_enabled(Benchmark* bench, const
Config* config) {
On 2014/07/22 17:44:19, mtklein wrote:
> const Config& ?

Done.

https://codereview.chromium.org/410683005/diff/1/bench/nanobench.cpp#newcode296
bench/nanobench.cpp:296: info.fWidth = w;
On 2014/07/22 17:44:19, mtklein wrote:
> just inline bench->getSize().fX and bench->getSize().fY here?

Done.

https://codereview.chromium.org/410683005/diff/1/bench/nanobench.cpp#newcode302
bench/nanobench.cpp:302: SkAutoTUnref<SkSurface> surface;
On 2014/07/22 17:44:19, mtklein wrote:
> Why go through the extra pointer?
> 
> if (Benchmark::kRaster_Backend == config->backend) {
>   target->surface.reset(SkSurface::NewRaster(info));
> }
> ...
> 
> if (!target->surface && Benchmark::kNonRenderingBackend != config->backend) {
>   ...
> }

Done.

[mtklein, 2014-07-22 18:16:18]

lgtm

https://codereview.chromium.org/410683005/diff/40001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/410683005/diff/40001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:217: const Config config;
No copy if you hold this as const Config& config.

[bsalomon, 2014-07-22 18:39:55]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2014-07-22 18:40:01]

https://codereview.chromium.org/410683005/diff/40001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/410683005/diff/40001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:217: const Config config;
On 2014/07/22 18:16:17, mtklein wrote:
> No copy if you hold this as const Config& config.

I know but the copy seems harmless and less prone to any future lifetime errors.

[commit-bot: I haz the power, 2014-07-22 18:40:24]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/bsalomon@google.com/410683005/40001

[mtklein, 2014-07-22 18:44:02]

On 2014/07/22 18:40:01, bsalomon wrote:
> https://codereview.chromium.org/410683005/diff/40001/bench/nanobench.cpp
> File bench/nanobench.cpp (right):
> 
>
https://codereview.chromium.org/410683005/diff/40001/bench/nanobench.cpp#newc...
> bench/nanobench.cpp:217: const Config config;
> On 2014/07/22 18:16:17, mtklein wrote:
> > No copy if you hold this as const Config& config.
> 
> I know but the copy seems harmless and less prone to any future lifetime
errors.

Oh, agreed.  Just wanted to point this out, as a const& both identical cost and
identical lifetime issues of the const* you had at first.

[mtklein, 2014-07-22 18:44:25]

On 2014/07/22 18:44:02, mtklein wrote:
> On 2014/07/22 18:40:01, bsalomon wrote:
> > https://codereview.chromium.org/410683005/diff/40001/bench/nanobench.cpp
> > File bench/nanobench.cpp (right):
> > 
> >
>
https://codereview.chromium.org/410683005/diff/40001/bench/nanobench.cpp#newc...
> > bench/nanobench.cpp:217: const Config config;
> > On 2014/07/22 18:16:17, mtklein wrote:
> > > No copy if you hold this as const Config& config.
> > 
> > I know but the copy seems harmless and less prone to any future lifetime
> errors.
> 
> Oh, agreed.  Just wanted to point this out, as a const& both identical cost
and
> identical lifetime issues of the const* you had at first.

...const& *has* both identical...

[commit-bot: I haz the power, 2014-07-22 18:52:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Mac10.8-Clang-x86-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Mac10.8-Clang-x86-Release-Trybot/b...)
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-Clang-x86_64-Debug-Try...)
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Andr...)
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-...)

[commit-bot: I haz the power, 2014-07-22 18:54:39]

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

[commit-bot: I haz the power, 2014-07-22 18:54:40]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Andr...)

[bsalomon, 2014-07-22 19:52:54]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-07-22 19:54:02]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/bsalomon@google.com/410683005/60001

[commit-bot: I haz the power, 2014-07-22 20:09:11]

Change committed as c2553373ee982b6c7c753e7e5035523bc01a7291