Expand VisualBench to a real benching tool

Expand VisualBench to a real benching tool

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/7fe8ee4cb7396754bfdb8c3bc1bf7c09af7459dd

[joshualitt, 2015-05-29 16:15:28]

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

[joshualitt, 2015-05-29 16:15:29]

With this CL visual bench becomes actually useful.

[bsalomon, 2015-06-01 13:49:54]

https://codereview.chromium.org/1159213002/diff/20001/include/views/SkOSWindo...
File include/views/SkOSWindow_Unix.h (right):

https://codereview.chromium.org/1159213002/diff/20001/include/views/SkOSWindo...
include/views/SkOSWindow_Unix.h:68: void privateCloseWindow();
I think it is slightly more consistent with other code to call this internal
rather than private. Also, this seems pretty different than the new
closeWindow(). It might be helpful to have some comments.

Is it a problem that this is diverging from the other OS variants for this
class? I don't remember whether they are or need to be consistent.

https://codereview.chromium.org/1159213002/diff/20001/tools/VisualBench.cpp
File tools/VisualBench.cpp (right):

https://codereview.chromium.org/1159213002/diff/20001/tools/VisualBench.cpp#n...
tools/VisualBench.cpp:129: WallTimer timer;
I think you should time outside the loop

[robertphillips, 2015-06-01 14:26:00]

https://codereview.chromium.org/1159213002/diff/20001/tools/VisualBench.cpp
File tools/VisualBench.cpp (right):

https://codereview.chromium.org/1159213002/diff/20001/tools/VisualBench.cpp#n...
tools/VisualBench.cpp:29: DEFINE_int32(maxFrames, 20, "Number of times to render
each skp.");
Should maxFrames be repeat or samples ?

https://codereview.chromium.org/1159213002/diff/20001/tools/VisualBench.cpp#n...
tools/VisualBench.cpp:32: static SkString humanize(double ms) {
add brackets & newlines ?

https://codereview.chromium.org/1159213002/diff/20001/tools/VisualBench.cpp#n...
tools/VisualBench.cpp:152: SkDebugf("%4d/%-4dMB\t%s\t%s\t%s\t%s\t%.0f%%\t%s\n"
I think we only do the "lead-with-," thing for ctor lists.

[robertphillips, 2015-06-01 14:31:07]

https://codereview.chromium.org/1159213002/diff/20001/tools/VisualBench.cpp
File tools/VisualBench.cpp (right):

https://codereview.chromium.org/1159213002/diff/20001/tools/VisualBench.cpp#n...
tools/VisualBench.cpp:150: Stats stats(measurements.begin(),
measurements.count());
stdDevPercent ?

https://codereview.chromium.org/1159213002/diff/20001/tools/VisualBench.cpp#n...
tools/VisualBench.cpp:154: , sk_tools::getMaxResidentSetSizeMB()
This probably isn't yours but why isn't this fMin ?

[joshualitt, 2015-06-01 16:16:09]

I'll do the timing of the empty skp in another CL.  I saw a number of single
digit std deviations in this change, though I didn't directly compare to the
earlier version.  We can always switch back to the old timing methodology if
desired.

https://codereview.chromium.org/1159213002/diff/20001/include/views/SkOSWindo...
File include/views/SkOSWindow_Unix.h (right):

https://codereview.chromium.org/1159213002/diff/20001/include/views/SkOSWindo...
include/views/SkOSWindow_Unix.h:68: void privateCloseWindow();
On 2015/06/01 13:49:54, bsalomon wrote:
> I think it is slightly more consistent with other code to call this internal
> rather than private. Also, this seems pretty different than the new
> closeWindow(). It might be helpful to have some comments.
ack
> 
> Is it a problem that this is diverging from the other OS variants for this
> class? I don't remember whether they are or need to be consistent.
Not exactly sure what you mean.  All three of the calls I have added are general
methods which any backend SkOSWindow can implement.  In the longterm, I will try
and write implementations for atleast android and probably windows / mac.  Also,
there are other virtual methods on SkWindow with noop implementations.

https://codereview.chromium.org/1159213002/diff/20001/tools/VisualBench.cpp
File tools/VisualBench.cpp (right):

https://codereview.chromium.org/1159213002/diff/20001/tools/VisualBench.cpp#n...
tools/VisualBench.cpp:29: DEFINE_int32(maxFrames, 20, "Number of times to render
each skp.");
On 2015/06/01 14:26:00, robertphillips wrote:
> Should maxFrames be repeat or samples ?

Acknowledged.

https://codereview.chromium.org/1159213002/diff/20001/tools/VisualBench.cpp#n...
tools/VisualBench.cpp:32: static SkString humanize(double ms) {
On 2015/06/01 14:26:00, robertphillips wrote:
> add brackets & newlines ?

Acknowledged.

https://codereview.chromium.org/1159213002/diff/20001/tools/VisualBench.cpp#n...
tools/VisualBench.cpp:129: WallTimer timer;
On 2015/06/01 13:49:54, bsalomon wrote:
> I think you should time outside the loop

Acknowledged.

https://codereview.chromium.org/1159213002/diff/20001/tools/VisualBench.cpp#n...
tools/VisualBench.cpp:150: Stats stats(measurements.begin(),
measurements.count());
On 2015/06/01 14:31:07, robertphillips wrote:
> stdDevPercent ?

Acknowledged.

https://codereview.chromium.org/1159213002/diff/20001/tools/VisualBench.cpp#n...
tools/VisualBench.cpp:152: SkDebugf("%4d/%-4dMB\t%s\t%s\t%s\t%s\t%.0f%%\t%s\n"
On 2015/06/01 14:26:00, robertphillips wrote:
> I think we only do the "lead-with-," thing for ctor lists.

Acknowledged.

https://codereview.chromium.org/1159213002/diff/20001/tools/VisualBench.cpp#n...
tools/VisualBench.cpp:154: , sk_tools::getMaxResidentSetSizeMB()
On 2015/06/01 14:31:07, robertphillips wrote:
> This probably isn't yours but why isn't this fMin ?

no idea, might be Mike Klein's code.

[bsalomon, 2015-06-01 16:21:24]

https://codereview.chromium.org/1159213002/diff/20001/include/views/SkOSWindo...
File include/views/SkOSWindow_Unix.h (right):

https://codereview.chromium.org/1159213002/diff/20001/include/views/SkOSWindo...
include/views/SkOSWindow_Unix.h:68: void privateCloseWindow();
On 2015/06/01 16:16:09, joshualitt wrote:
> On 2015/06/01 13:49:54, bsalomon wrote:
> > I think it is slightly more consistent with other code to call this internal
> > rather than private. Also, this seems pretty different than the new
> > closeWindow(). It might be helpful to have some comments.
> ack
> > 
> > Is it a problem that this is diverging from the other OS variants for this
> > class? I don't remember whether they are or need to be consistent.
> Not exactly sure what you mean.  All three of the calls I have added are
general
> methods which any backend SkOSWindow can implement.  In the longterm, I will
try
> and write implementations for atleast android and probably windows / mac. 
Also,
> there are other virtual methods on SkWindow with noop implementations.

ok, lgtm.

[bsalomon, 2015-06-01 16:21:25]

https://codereview.chromium.org/1159213002/diff/20001/include/views/SkOSWindo...
File include/views/SkOSWindow_Unix.h (right):

https://codereview.chromium.org/1159213002/diff/20001/include/views/SkOSWindo...
include/views/SkOSWindow_Unix.h:68: void privateCloseWindow();
On 2015/06/01 16:16:09, joshualitt wrote:
> On 2015/06/01 13:49:54, bsalomon wrote:
> > I think it is slightly more consistent with other code to call this internal
> > rather than private. Also, this seems pretty different than the new
> > closeWindow(). It might be helpful to have some comments.
> ack
> > 
> > Is it a problem that this is diverging from the other OS variants for this
> > class? I don't remember whether they are or need to be consistent.
> Not exactly sure what you mean.  All three of the calls I have added are
general
> methods which any backend SkOSWindow can implement.  In the longterm, I will
try
> and write implementations for atleast android and probably windows / mac. 
Also,
> there are other virtual methods on SkWindow with noop implementations.

ok, lgtm.

[robertphillips, 2015-06-01 16:23:56]

https://codereview.chromium.org/1159213002/diff/60001/tools/VisualBench.cpp
File tools/VisualBench.cpp (right):

https://codereview.chromium.org/1159213002/diff/60001/tools/VisualBench.cpp#n...
tools/VisualBench.cpp:137: 
Do we still need timeFrame ?

https://codereview.chromium.org/1159213002/diff/60001/tools/VisualBench.cpp#n...
tools/VisualBench.cpp:177: }
break; ?

https://codereview.chromium.org/1159213002/diff/60001/tools/VisualBench.cpp#n...
tools/VisualBench.cpp:194: }
break; ?

[joshualitt, 2015-06-01 16:58:05]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-06-01 16:58:05]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1159213002/#ps80001
(title: "tweaks")

[commit-bot: I haz the power, 2015-06-01 16:58:18]

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

[commit-bot: I haz the power, 2015-06-01 17:03:59]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/7fe8ee4cb7396754bfdb8c3bc1bf7c09af7459dd