SKPs-as-benches in nanobench

SKPs-as-benches in nanobench

This is meant to replace bench_pictures.

CQ_EXTRA_TRYBOTS=tryserver.skia:Build-Mac10.7-Clang-Arm7-Release-iOS-Trybot

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/92007583e43115998412ac8b0a06cc2780eb025c

[mtklein, 2014-07-31 21:28:13]

JSON looks like this:

        "desk_gmailthread.skp_1.000000_1000_1000": {
            "565": {
                "max_ms": 85.610029,
                "mean_ms": 85.610029,
                "median_ms": 85.610029,
                "min_ms": 85.610029,
                "options": {
                    "clip": "0 0 1000 1000",
                    "scale": "1.000000",
                    "source_type": "skp"
                }
            },
            "8888": {
                "max_ms": 36.570436,
                "mean_ms": 36.570436,
                "median_ms": 36.570436,
                "min_ms": 36.570436,
                "options": {
                    "clip": "0 0 1000 1000",
                    "scale": "1.000000",
                    "source_type": "skp"
                }
            },
            "gpu": {
                "max_ms": 59.030865,
                "mean_ms": 59.030865,
                "median_ms": 59.030865,
                "min_ms": 59.030865,
                "options": {
                    "GL_RENDERER": "Quadro 600/PCIe/SSE2",
                    "GL_SHADING_LANGUAGE_VERSION": "4.40 NVIDIA via Cg
compiler",
                    "GL_VENDOR": "NVIDIA Corporation",
                    "GL_VERSION": "4.4.0 NVIDIA 331.67",
                    "clip": "0 0 1000 1000",
                    "scale": "1.000000",
                    "source_type": "skp"
                }
            }
        },
        "desk_gmailthread.skp_1.100000_1000_1000": {
            "565": {
                "max_ms": 93.968093,
                "mean_ms": 93.968093,
                "median_ms": 93.968093,
                "min_ms": 93.968093,
                "options": {
                    "clip": "0 0 1000 1000",
                    "scale": "1.100000",
                    "source_type": "skp"
                }
            },
            "8888": {
                "max_ms": 40.119341,
                "mean_ms": 40.119341,
                "median_ms": 40.119341,
                "min_ms": 40.119341,
                "options": {
                    "clip": "0 0 1000 1000",
                    "scale": "1.100000",
                    "source_type": "skp"
                }
            },
            "gpu": {
                "max_ms": 69.502035,
                "mean_ms": 69.502035,
                "median_ms": 69.502035,
                "min_ms": 69.502035,
                "options": {
                    "GL_RENDERER": "Quadro 600/PCIe/SSE2",
                    "GL_SHADING_LANGUAGE_VERSION": "4.40 NVIDIA via Cg
compiler",
                    "GL_VENDOR": "NVIDIA Corporation",
                    "GL_VERSION": "4.4.0 NVIDIA 331.67",
                    "clip": "0 0 1000 1000",
                    "scale": "1.100000",
                    "source_type": "skp"
                }
            }
        }

[jcgregorio, 2014-08-01 11:37:23]

On 2014/07/31 21:28:13, mtklein wrote:
> JSON looks like this:
> 
>         "desk_gmailthread.skp_1.000000_1000_1000": {
>             "565": {
>                 "max_ms": 85.610029,
>                 "mean_ms": 85.610029,
>                 "median_ms": 85.610029,
>                 "min_ms": 85.610029,
>                 "options": {
>                     "clip": "0 0 1000 1000",
>                     "scale": "1.000000",

Do we need that much precision on scale?

>                     "source_type": "skp"
>                 }
>             },
>             "8888": {
>                 "max_ms": 36.570436,
>                 "mean_ms": 36.570436,
>                 "median_ms": 36.570436,
>                 "min_ms": 36.570436,
>                 "options": {
>                     "clip": "0 0 1000 1000",
>                     "scale": "1.000000",
>                     "source_type": "skp"
>                 }
>             },
>             "gpu": {
>                 "max_ms": 59.030865,
>                 "mean_ms": 59.030865,
>                 "median_ms": 59.030865,
>                 "min_ms": 59.030865,
>                 "options": {
>                     "GL_RENDERER": "Quadro 600/PCIe/SSE2",
>                     "GL_SHADING_LANGUAGE_VERSION": "4.40 NVIDIA via Cg
> compiler",
>                     "GL_VENDOR": "NVIDIA Corporation",
>                     "GL_VERSION": "4.4.0 NVIDIA 331.67",
>                     "clip": "0 0 1000 1000",
>                     "scale": "1.000000",
>                     "source_type": "skp"
>                 }
>             }
>         },
>         "desk_gmailthread.skp_1.100000_1000_1000": {
>             "565": {
>                 "max_ms": 93.968093,
>                 "mean_ms": 93.968093,
>                 "median_ms": 93.968093,
>                 "min_ms": 93.968093,
>                 "options": {
>                     "clip": "0 0 1000 1000",
>                     "scale": "1.100000",
>                     "source_type": "skp"
>                 }
>             },
>             "8888": {
>                 "max_ms": 40.119341,
>                 "mean_ms": 40.119341,
>                 "median_ms": 40.119341,
>                 "min_ms": 40.119341,
>                 "options": {
>                     "clip": "0 0 1000 1000",
>                     "scale": "1.100000",
>                     "source_type": "skp"
>                 }
>             },
>             "gpu": {
>                 "max_ms": 69.502035,
>                 "mean_ms": 69.502035,
>                 "median_ms": 69.502035,
>                 "min_ms": 69.502035,
>                 "options": {
>                     "GL_RENDERER": "Quadro 600/PCIe/SSE2",
>                     "GL_SHADING_LANGUAGE_VERSION": "4.40 NVIDIA via Cg
> compiler",
>                     "GL_VENDOR": "NVIDIA Corporation",
>                     "GL_VERSION": "4.4.0 NVIDIA 331.67",
>                     "clip": "0 0 1000 1000",
>                     "scale": "1.100000",
>                     "source_type": "skp"
>                 }
>             }
>         }

[mtklein, 2014-08-01 11:49:46]

> Do we need that much precision on scale?

Probably not.  Switched it to %.2g to print "1" and "1.1".

[jcgregorio, 2014-08-01 11:53:49]

On 2014/08/01 11:49:46, mtklein wrote:
> > Do we need that much precision on scale?
> 
> Probably not.  Switched it to %.2g to print "1" and "1.1".

LGTM for JSON format.

[bsalomon, 2014-08-01 13:23:24]

https://codereview.chromium.org/425393004/diff/80001/bench/SKPBench.cpp
File bench/SKPBench.cpp (right):

https://codereview.chromium.org/425393004/diff/80001/bench/SKPBench.cpp#newco...
bench/SKPBench.cpp:26: return SkIPoint::Make(fClip.fRight - fClip.fLeft,
fClip.fBottom - fClip.fTop);
fClip.width(), fClip.height()?

https://codereview.chromium.org/425393004/diff/80001/bench/SKPBench.cpp#newco...
bench/SKPBench.cpp:31: SkRegion deviceClip;
No regions, please! The GPU doesn't like them and they don't scale well. It
seems this is also the size of the canvas, so probably it isn't necessary at
all.

One thing that bench supports that'd be nice for nanobench is a mode where it
writes a file so a dev can verify that the right thing is being rendered (while
writing a new benchmark or working with a skp and trying to find the right
viewport, etc). I might add something similar to nanobench in a separate CL. I
was thinking about something like -w <dir> [<loopcnt_default_1>].

https://codereview.chromium.org/425393004/diff/80001/bench/SKPBench.cpp#newco...
bench/SKPBench.cpp:38: fPic->draw(canvas);
I'm not sure whether we should have a canvas flush or not in here...

https://codereview.chromium.org/425393004/diff/80001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/425393004/diff/80001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:55: DEFINE_string(scales, "1.0,1.1", "Scales for SKPs.");
Maybe the default should just be 1.0?

https://codereview.chromium.org/425393004/diff/80001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:532: // FIXME: Some SKPs SkASSERT in debug mode.  Skip them
for now until we can fix things.
Should prolly file a bug and stick a link to it here.

https://codereview.chromium.org/425393004/diff/80001/tools/flags/SkCommonFlag...
File tools/flags/SkCommonFlags.cpp (right):

https://codereview.chromium.org/425393004/diff/80001/tools/flags/SkCommonFlag...
tools/flags/SkCommonFlags.cpp:47: DEFINE_string(skps, "", "Directory to read
skps from.");
Does specifying a skp file work too? b_p supports that and it's handy to not
have to create a dir with one file when you're working with a particular skp.

[mtklein, 2014-08-01 13:55:34]

https://codereview.chromium.org/425393004/diff/80001/bench/SKPBench.cpp
File bench/SKPBench.cpp (right):

https://codereview.chromium.org/425393004/diff/80001/bench/SKPBench.cpp#newco...
bench/SKPBench.cpp:26: return SkIPoint::Make(fClip.fRight - fClip.fLeft,
fClip.fBottom - fClip.fTop);
On 2014/08/01 13:23:24, bsalomon wrote:
> fClip.width(), fClip.height()?

Done.

https://codereview.chromium.org/425393004/diff/80001/bench/SKPBench.cpp#newco...
bench/SKPBench.cpp:31: SkRegion deviceClip;
On 2014/08/01 13:23:24, bsalomon wrote:
> No regions, please! The GPU doesn't like them and they don't scale well. It
> seems this is also the size of the canvas, so probably it isn't necessary at
> all.
> 
> One thing that bench supports that'd be nice for nanobench is a mode where it
> writes a file so a dev can verify that the right thing is being rendered
(while
> writing a new benchmark or working with a skp and trying to find the right
> viewport, etc). I might add something similar to nanobench in a separate CL. I
> was thinking about something like -w <dir> [<loopcnt_default_1>].

Done.  Writing output SGTM.  Let's follow up.

https://codereview.chromium.org/425393004/diff/80001/bench/SKPBench.cpp#newco...
bench/SKPBench.cpp:38: fPic->draw(canvas);
On 2014/08/01 13:23:24, bsalomon wrote:
> I'm not sure whether we should have a canvas flush or not in here...

Good question.  Looks like bench_pictures does, so added here.

https://codereview.chromium.org/425393004/diff/80001/bench/nanobench.cpp
File bench/nanobench.cpp (right):

https://codereview.chromium.org/425393004/diff/80001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:55: DEFINE_string(scales, "1.0,1.1", "Scales for SKPs.");
On 2014/08/01 13:23:24, bsalomon wrote:
> Maybe the default should just be 1.0?

Done.

Oddly, having a single value by default makes the parsing code simpler, because
we can just space separate them and lean more on SkCommandlineFlags.

https://codereview.chromium.org/425393004/diff/80001/bench/nanobench.cpp#newc...
bench/nanobench.cpp:532: // FIXME: Some SKPs SkASSERT in debug mode.  Skip them
for now until we can fix things.
On 2014/08/01 13:23:24, bsalomon wrote:
> Should prolly file a bug and stick a link to it here.

Done.

https://codereview.chromium.org/425393004/diff/80001/tools/flags/SkCommonFlag...
File tools/flags/SkCommonFlags.cpp (right):

https://codereview.chromium.org/425393004/diff/80001/tools/flags/SkCommonFlag...
tools/flags/SkCommonFlags.cpp:47: DEFINE_string(skps, "", "Directory to read
skps from.");
On 2014/08/01 13:23:24, bsalomon wrote:
> Does specifying a skp file work too? b_p supports that and it's handy to not
> have to create a dir with one file when you're working with a particular skp.

It does now.

[bsalomon, 2014-08-01 14:03:29]

lgtm

https://codereview.chromium.org/425393004/diff/100001/bench/SKPBench.h
File bench/SKPBench.h (right):

https://codereview.chromium.org/425393004/diff/100001/bench/SKPBench.h#newcode8
bench/SKPBench.h:8: #ifndef SKPBench_DEFINED
I guess this was missing from SkGMBench.h? Prolly my fault.

[mtklein, 2014-08-01 14:05:31]

https://codereview.chromium.org/425393004/diff/100001/bench/SKPBench.h
File bench/SKPBench.h (right):

https://codereview.chromium.org/425393004/diff/100001/bench/SKPBench.h#newcode8
bench/SKPBench.h:8: #ifndef SKPBench_DEFINED
On 2014/08/01 14:03:29, bsalomon wrote:
> I guess this was missing from SkGMBench.h? Prolly my fault.

Guess so.  Added it there for you now.  Think of it as karmic balance for my
chronic lack of copyright notices.

[mtklein, 2014-08-01 14:06:37]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-08-01 14:07:28]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/425393004/140001

[commit-bot: I haz the power, 2014-08-01 14:46:57]

Change committed as 92007583e43115998412ac8b0a06cc2780eb025c