Major bench refactoring.

Major bench refactoring.
   - Use FLAGS_.
   - Remove outer repeat loop.
   - Tune inner loop automatically.

BUG=skia:1590
R=epoger@google.com, scroggo@google.com

Committed: https://code.google.com/p/skia/source/detail?r=11187

[mtklein, 2013-09-03 19:05:54]

Guys, you've won my lottery of being the two who seem best to review this code.

There's quite a lot changed.  I encourage you to defer or skip reviewing
everything but benchmain.cpp.  That's where the meat is, and I fear that
reviewing all the boring changes in *Bench.cpp will dull your sharp wit that I'd
like to have applied to benchmain.cpp.

You are of course welcome to review the whole lot.

Timing on my desktop, comparing this CL vs head (with --repeat 30 in Release
mode),
  Release: 10m -> 1.5m
  Debug:   30s -> 20s

[scroggo, 2013-09-03 19:36:43]

benchmain looks good for the most part. Should probably get a GPU guy to take a
look as well.

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp
File bench/benchmain.cpp (left):

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#oldco...
bench/benchmain.cpp:353: SkTDict<const char*> defineDict(1024);
It seems this functionality (supplying defines on the command line) was lost?

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#oldco...
bench/benchmain.cpp:724: Iter iter(&defineDict);
I suppose no benches ever use the dictionary?

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:31: enum BenchMode {
Officially I think we're supposed to make the enum name plural, but I don't know
that we're consistent about it.

enum BenchModes {
  kNormal_BenchMode,
  .
  .
  .
}

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:40: enum TimerType {
Would be nice to consolidate this with bench_pictures.

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:127: str.prepend("/");
SkOSFile now has a function for modifying path names. It supplies '/' or '\\'
depending on the operating system.

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:253: DEFINE_string(timers, "cg", "Timers to display. "
It would be nice to combine this with bench_pictures.

We can move the DEFINE_string to another file (shared by both tools) and use
DECLARE_string here.

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:268: DEFINE_string(match, "",  "[~][^]substring[$] [...] of
test name to run.\n"
Isn't this now built in to SkCommandLineFlags?

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:500: device.reset(make_device(config.config, dim,
config.backend, config.sampleCount, context));
100 chars

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:595: // Lets not mislead ourselves by looking at Debug build
bench times!
+1

Should this print out an error message as well?

[mtklein, 2013-09-03 20:04:43]

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp
File bench/benchmain.cpp (left):

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#oldco...
bench/benchmain.cpp:353: SkTDict<const char*> defineDict(1024);
On 2013/09/03 19:36:43, scroggo wrote:
> It seems this functionality (supplying defines on the command line) was lost?

Yes.  Only DecodeBench.cpp was using it for one potential define, so I added a
flag there.  We aren't actually setting the define anywhere I can see, so
removing this doesn't break anything.  In fact, I gave the new flag a default
value that lets DecodeBench actually run now.

Generally, SkCommandLineFlags makes this functionality obsolete.

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#oldco...
bench/benchmain.cpp:724: Iter iter(&defineDict);
On 2013/09/03 19:36:43, scroggo wrote:
> I suppose no benches ever use the dictionary?

Yup.  I've just been too lazy to un-plumb the void* that we pass through all the
constructors.  I will do this before we're done reviewing here.

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:31: enum BenchMode {
Updated the style guide so that BenchMode and Backend are correct, and renamed
TimerType to TimerTypes.

On 2013/09/03 19:36:43, scroggo wrote:
> Officially I think we're supposed to make the enum name plural, but I don't
know
> that we're consistent about it.
> 
> enum BenchModes {
>   kNormal_BenchMode,
>   .
>   .
>   .
> }

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:40: enum TimerType {
On 2013/09/03 19:36:43, scroggo wrote:
> Would be nice to consolidate this with bench_pictures.

I was thinking maybe I'd go whole-hog and follow up by merging bench_pictures
into bench.  How's that sound?

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:127: str.prepend("/");
On 2013/09/03 19:36:43, scroggo wrote:
> SkOSFile now has a function for modifying path names. It supplies '/' or '\\'
> depending on the operating system.

Done.

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:253: DEFINE_string(timers, "cg", "Timers to display. "
On 2013/09/03 19:36:43, scroggo wrote:
> It would be nice to combine this with bench_pictures.
> 
> We can move the DEFINE_string to another file (shared by both tools) and use
> DECLARE_string here.

+1

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:268: DEFINE_string(match, "",  "[~][^]substring[$] [...] of
test name to run.\n"
On 2013/09/03 19:36:43, scroggo wrote:
> Isn't this now built in to SkCommandLineFlags?

Sort of.  We can now parse FLAGS_match directly rather than going through a
SkTDArray<const char*> or something.  But the match flag still needs to be
defined somewhere.  SkCommandLineFlags doesn't, and I think shouldn't, define
FLAGS_match.

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:500: device.reset(make_device(config.config, dim,
config.backend, config.sampleCount, context));
On 2013/09/03 19:36:43, scroggo wrote:
> 100 chars

Done.

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:595: // Lets not mislead ourselves by looking at Debug build
bench times!
On 2013/09/03 19:36:43, scroggo wrote:
> +1
> 
> Should this print out an error message as well?

Nah, it's not really an error to run bench in Debug.  We do already run bench in
debug mode with one loop on each bench just to catch asserts.  That
functionality remains here.

[mtklein, 2013-09-03 20:09:17]

Brian, can you take a look at this as my designated "I care about GPUs"
reviewer?  Again, most of the meat is in benchmain.cpp.  I won't be heartbroken
if you ignore everything but that.

[scroggo, 2013-09-03 20:13:53]

lgtm, pending GPU expertise and making sure our benchmark output
parser/regressions system is ready for it.

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:40: enum TimerType {
On 2013/09/03 20:04:43, mtklein wrote:
> On 2013/09/03 19:36:43, scroggo wrote:
> > Would be nice to consolidate this with bench_pictures.
> 
> I was thinking maybe I'd go whole-hog and follow up by merging bench_pictures
> into bench.  How's that sound?

+1

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:268: DEFINE_string(match, "",  "[~][^]substring[$] [...] of
test name to run.\n"
On 2013/09/03 20:04:43, mtklein wrote:
> On 2013/09/03 19:36:43, scroggo wrote:
> > Isn't this now built in to SkCommandLineFlags?
> 
> Sort of.  We can now parse FLAGS_match directly rather than going through a
> SkTDArray<const char*> or something.  But the match flag still needs to be
> defined somewhere.  SkCommandLineFlags doesn't, and I think shouldn't, define
> FLAGS_match.

What if SkCommandLineFlags defined it optionally? It seems odd to have
SkCommandLineFlags have a function which depends on setting a DEFINE, and
unfortunate that we have to copy past this code in all our tools.

[epoger, 2013-09-04 13:32:06]

Looking at the CL now...

[epoger, 2013-09-04 14:36:40]

Reviewed at patchset 8.

Thanks for taking this on, Mike... it's a bear, and it should reduce developer
pain.

Feel free to grab me for in-person discussion of any of my comments, if you
like.

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp
File bench/benchmain.cpp (left):

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#oldco...
bench/benchmain.cpp:35: #include "TimerData.h"
Now that benchmain doesn't use TimerData.h anymore, does it mean that we can
delete TimerData.h and TimerData.cpp?

(For that matter, why not update the code in TimerData.h rather than replacing
TimerData::TimerFlags with TimerTypes within this file?  It would be easier to
see what is changing that way, I think.)

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:251: DEFINE_string(outDir, "", "If given, image of each
bench will be put in outDir.");
In future CLs (I'm not asking you to back out any changes here!) I would really
prefer for a massive set of modifications like this to be split into multiple
CLs.

For example, switching to SkCommandLineFlags would have been a great candidate
for a CL of its own.

Reasons for this:
1. Let's say there is some problem with this CL (unrelated to the switch to
SkCommandLineFlags) after we commit it, so we roll it out… that will cause us to
lose the switch to SkCommandLineFlags, along with the other change.
2. It's a lot easier to fully understand/review piecewise changes like that.

On the other hand, I know sometimes I start in on a CL and I find "one little
related thing" to fix along the way, and before you know it I have what would
ideally be 2 separate CLs in one giant CL.  My guess is that may be what
happened here.

Anyway, I wanted to make sure you knew my preference on that, and if anyone has
counterpoints to my above reasoning I would love to hear them.  I am prepared to
change my mind. :-)

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:310: for (const char* c = FLAGS_timers[0]; *c; ++c) {
I think code like this would be much easier to understand and reuse if it was
encapsulated within its own file, along the lines of
https://code.google.com/p/skia/source/browse/trunk/gm/gm_error.h .  Something
like this:

enum TimerTypes {
  kWall_TimerType,
  kTruncatedWall_TimerType,
  …
}

class TimerTypeSet {
  TimerTypeSet(const char *arrayOfShortNames);
  bool includes(TimerType t);
}

As the code stands now, the details of how to read/write the timerTypes bitfield
are entangled within the benchmain code at large.

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:456: // Run each bench in each configuration it supports and
we asked for.
Where is (was) the outer repeat loop that you deleted in this CL?  I don't see
it... obscured by all the other changes, I'm afraid.

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:552: if (loops >= (1<<30)) {
What's with the magic "30" here?  I take it you are trying to make sure we don't
hit MAX_INT?

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:598: // Lets not mislead ourselves by looking at Debug build
bench times!
I think it would be better to print a warning here, indicating why the bench
times are not reported.

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:602: // In same order as TimerTypes, multiplied to normalize
times to ms per 1000 iterations.
Does something bad happen if the TimerTypes enum is reordered?  If so, is there
some way we can make sure they stay in sync?  (Or, figure out a way that it
doesn't matter.)

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:614: filteredTimerTypes &= ~(1<<kGpu_TimerType);
In my fantasy world, this becomes the easier-to-follow

filteredTimerTypeSet.remove(kGpu_TimerType)

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:620: for (size_t i = 0; i < SK_ARRAY_COUNT(TimerType_Name);
i++) {
In my fantasy world, this becomes:

  for (int typeInt = 0; typeInt <= kLast_TimerType; typeInt++) {
    TimerType thisType = static_cast<TimerType>(typeInt);
    if (filteredTimerTypeSet.includes(thisType)) {
      ...

Which is not necessarily more readable than what you have now.  Typecasting
provides some protection, but at a cost.

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:621: if (filteredTimerTypes & (1<<i) && times[i] > 0) {
Why don't we report this timer if times[i] == 0?

https://codereview.chromium.org/23478013/diff/22001/src/core/SkBitmapProcShad...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/23478013/diff/22001/src/core/SkBitmapProcShad...
src/core/SkBitmapProcShader.cpp:370: /*
Why is this file part of this CL?

https://codereview.chromium.org/23478013/diff/22001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/23478013/diff/22001/src/gpu/SkGpuDevice.cpp#n...
src/gpu/SkGpuDevice.cpp:1190: /*
Why is this file part of this CL?

[epoger, 2013-09-04 14:45:51]

https://codereview.chromium.org/23478013/diff/22001/bench/AAClipBench.cpp
File bench/AAClipBench.cpp (right):

https://codereview.chromium.org/23478013/diff/22001/bench/AAClipBench.cpp#new...
bench/AAClipBench.cpp:1: /*
One more toplevel comment:

Presumably, this CL is intended to address some particular problem(s) with
bench.  (Primarily, I assume, that bench takes too long to run.)

Personally, I prefer for us to keep a "paper trail" (usually in the form of
bugs) indicating why we make substantial changes, and addressing the potential
alternatives (e.g., https://code.google.com/p/skia/issues/detail?id=1566
('GenerateGMs takes way too long on the bots')), for the following reasons:

1. It helps us make sure we are data-driven in our decisionmaking
2. It may help us 6 months from now, when we want to make further adjustments
and we have forgotten many of the considerations we thought were valuable way
back in 2013.

But I may be in the minority here.  I think many people prefer the higher
execution velocity made possible by NOT keeping the paper trail.  I suppose
there are pros and cons both ways.

If we worked at General Dynamics, I suppose somebody "in charge" would tell us
to do it one way or the other.  But we don't. :-)

[scroggo, 2013-09-04 14:51:31]

https://codereview.chromium.org/23478013/diff/22001/bench/AAClipBench.cpp
File bench/AAClipBench.cpp (right):

https://codereview.chromium.org/23478013/diff/22001/bench/AAClipBench.cpp#new...
bench/AAClipBench.cpp:1: /*
On 2013/09/04 14:45:51, epoger wrote:
> One more toplevel comment:
> 
> Presumably, this CL is intended to address some particular problem(s) with
> bench.  (Primarily, I assume, that bench takes too long to run.)
> 
> Personally, I prefer for us to keep a "paper trail" (usually in the form of
> bugs) indicating why we make substantial changes, and addressing the potential
> alternatives (e.g., https://code.google.com/p/skia/issues/detail?id=1566
> ('GenerateGMs takes way too long on the bots')), for the following reasons:
> 
> 1. It helps us make sure we are data-driven in our decisionmaking
> 2. It may help us 6 months from now, when we want to make further adjustments
> and we have forgotten many of the considerations we thought were valuable way
> back in 2013.
> 
> But I may be in the minority here.  I think many people prefer the higher
> execution velocity made possible by NOT keeping the paper trail.  I suppose
> there are pros and cons both ways.
> 
> If we worked at General Dynamics, I suppose somebody "in charge" would tell us
> to do it one way or the other.  But we don't. :-)
> 

I don't know whether you're in the minority, but I agree. I'd say it's more
important for changes like this, which are large in scope. On the other end of
the spectrum, if you need to add a check for NULL, I think execution velocity is
more important. Somewhere in between the paper trail becomes "worth it", which
may vary from person to person.

[mtklein, 2013-09-04 19:06:50]

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp
File bench/benchmain.cpp (left):

https://codereview.chromium.org/23478013/diff/17001/bench/benchmain.cpp#oldco...
bench/benchmain.cpp:724: Iter iter(&defineDict);
On 2013/09/03 20:04:43, mtklein wrote:
> On 2013/09/03 19:36:43, scroggo wrote:
> > I suppose no benches ever use the dictionary?
> 
> Yup.  I've just been too lazy to un-plumb the void* that we pass through all
the
> constructors.  I will do this before we're done reviewing here.

Turns out unplumbing the void* here is another CL about the size of this one. 
I'll follow up with it.

https://codereview.chromium.org/23478013/diff/22001/bench/AAClipBench.cpp
File bench/AAClipBench.cpp (right):

https://codereview.chromium.org/23478013/diff/22001/bench/AAClipBench.cpp#new...
bench/AAClipBench.cpp:1: /*
On 2013/09/04 14:51:31, scroggo wrote:
> On 2013/09/04 14:45:51, epoger wrote:
> > One more toplevel comment:
> > 
> > Presumably, this CL is intended to address some particular problem(s) with
> > bench.  (Primarily, I assume, that bench takes too long to run.)
> > 
> > Personally, I prefer for us to keep a "paper trail" (usually in the form of
> > bugs) indicating why we make substantial changes, and addressing the
potential
> > alternatives (e.g., https://code.google.com/p/skia/issues/detail?id=1566
> > ('GenerateGMs takes way too long on the bots')), for the following reasons:
> > 
> > 1. It helps us make sure we are data-driven in our decisionmaking
> > 2. It may help us 6 months from now, when we want to make further
adjustments
> > and we have forgotten many of the considerations we thought were valuable
way
> > back in 2013.
> > 
> > But I may be in the minority here.  I think many people prefer the higher
> > execution velocity made possible by NOT keeping the paper trail.  I suppose
> > there are pros and cons both ways.
> > 
> > If we worked at General Dynamics, I suppose somebody "in charge" would tell
us
> > to do it one way or the other.  But we don't. :-)
> > 
> 
> I don't know whether you're in the minority, but I agree. I'd say it's more
> important for changes like this, which are large in scope. On the other end of
> the spectrum, if you need to add a check for NULL, I think execution velocity
is
> more important. Somewhere in between the paper trail becomes "worth it", which
> may vary from person to person.

Sure.  Added 1590.  Guess I'm spoiled by cs/ where the CLs themselves make
easily searchable paper-trail.

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp
File bench/benchmain.cpp (left):

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#oldco...
bench/benchmain.cpp:35: #include "TimerData.h"
On 2013/09/04 14:36:40, epoger wrote:
> Now that benchmain doesn't use TimerData.h anymore, does it mean that we can
> delete TimerData.h and TimerData.cpp?
> 
> (For that matter, why not update the code in TimerData.h rather than replacing
> TimerData::TimerFlags with TimerTypes within this file?  It would be easier to
> see what is changing that way, I think.)

Yeah, deleting TimerData is coming.  I haven't done it yet because
bench_pictures is still using it, and this seemed massive enough as it is.  My
plan is to delete TimerData after folding bench_pictures into bench.

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:251: DEFINE_string(outDir, "", "If given, image of each
bench will be put in outDir.");
On 2013/09/04 14:36:40, epoger wrote:
> In future CLs (I'm not asking you to back out any changes here!) I would
really
> prefer for a massive set of modifications like this to be split into multiple
> CLs.
> 
> For example, switching to SkCommandLineFlags would have been a great candidate
> for a CL of its own.
> 
> Reasons for this:
> 1. Let's say there is some problem with this CL (unrelated to the switch to
> SkCommandLineFlags) after we commit it, so we roll it out… that will cause us
to
> lose the switch to SkCommandLineFlags, along with the other change.
> 2. It's a lot easier to fully understand/review piecewise changes like that.
> 
> On the other hand, I know sometimes I start in on a CL and I find "one little
> related thing" to fix along the way, and before you know it I have what would
> ideally be 2 separate CLs in one giant CL.  My guess is that may be what
> happened here.
> 
> Anyway, I wanted to make sure you knew my preference on that, and if anyone
has
> counterpoints to my above reasoning I would love to hear them.  I am prepared
to
> change my mind. :-)

Yeah, this was a case where the code was such that I didn't trust myself to make
a correct change without converting to FLAGS_ first.  I don't mind splitting out
the flags into another CL if you like.

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:310: for (const char* c = FLAGS_timers[0]; *c; ++c) {
On 2013/09/04 14:36:40, epoger wrote:
> I think code like this would be much easier to understand and reuse if it was
> encapsulated within its own file, along the lines of
> https://code.google.com/p/skia/source/browse/trunk/gm/gm_error.h .  Something
> like this:
> 
> enum TimerTypes {
>   kWall_TimerType,
>   kTruncatedWall_TimerType,
>   …
> }
> 
> class TimerTypeSet {
>   TimerTypeSet(const char *arrayOfShortNames);
>   bool includes(TimerType t);
> }
> 
> As the code stands now, the details of how to read/write the timerTypes
bitfield
> are entangled within the benchmain code at large.

Agreed.  I took a closer look at this and realized that timers were way
overcomplicated.  I redid all the timer code to be considerably simpler, which
should clean up all of these problems.

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:456: // Run each bench in each configuration it supports and
we asked for.
On 2013/09/04 14:36:40, epoger wrote:
> Where is (was) the outer repeat loop that you deleted in this CL?  I don't see
> it... obscured by all the other changes, I'm afraid.

Yeah, exactly.  Hard to read.  It was everything involving repeatDraw.

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:552: if (loops >= (1<<30)) {
On 2013/09/04 14:36:40, epoger wrote:
> What's with the magic "30" here?  I take it you are trying to make sure we
don't
> hit MAX_INT?

1<<30 is about a billion, which is a somewhat arbitrary cutoff for the number of
loops we allow to run.  We can make this a flag or a named constant if you want,
but I wouldn't bother until we find we need to change it.  1<<30 ought to be a
big enough upper limit to reach ~millisecond runtimes until we start getting
CPUs running at >=THz and the loop bodies get down into ~picoseconds.  In
practice, a couple million is fine for skia.

You're right, though, that like in most other cases involving overflow, overflow
would break things terribly.  This check does double duty.

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:598: // Lets not mislead ourselves by looking at Debug build
bench times!
On 2013/09/04 14:36:40, epoger wrote:
> I think it would be better to print a warning here, indicating why the bench
> times are not reported.

Added one up front.

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:602: // In same order as TimerTypes, multiplied to normalize
times to ms per 1000 iterations.
On 2013/09/04 14:36:40, epoger wrote:
> Does something bad happen if the TimerTypes enum is reordered?  If so, is
there
> some way we can make sure they stay in sync?  (Or, figure out a way that it
> doesn't matter.)

now moot

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:614: filteredTimerTypes &= ~(1<<kGpu_TimerType);
On 2013/09/04 14:36:40, epoger wrote:
> In my fantasy world, this becomes the easier-to-follow
> 
> filteredTimerTypeSet.remove(kGpu_TimerType)

this turned out to be a complete no-op.  removed it

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:620: for (size_t i = 0; i < SK_ARRAY_COUNT(TimerType_Name);
i++) {
On 2013/09/04 14:36:40, epoger wrote:
> In my fantasy world, this becomes:
> 
>   for (int typeInt = 0; typeInt <= kLast_TimerType; typeInt++) {
>     TimerType thisType = static_cast<TimerType>(typeInt);
>     if (filteredTimerTypeSet.includes(thisType)) {
>       ...
> 
> Which is not necessarily more readable than what you have now.  Typecasting
> provides some protection, but at a cost.

Should be moot now.  Just out of curiosity, what cost are you referring to?

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:621: if (filteredTimerTypes & (1<<i) && times[i] > 0) {
On 2013/09/04 14:36:40, epoger wrote:
> Why don't we report this timer if times[i] == 0?

I don't have a opinion on whether we want to print out 0 times.  They're mostly
uninsightful, like seeing 0 GPU time spent in the NULLGPU configuration... duh. 
bench at head does not print these zeroes.

The important part is to skip reporting timers which were never measured, which
come back from BenchTimer as negative values.

[epoger, 2013-09-04 19:18:25]

LGTM at patchset 9.

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:552: if (loops >= (1<<30)) {
On 2013/09/04 19:06:51, mtklein wrote:
> On 2013/09/04 14:36:40, epoger wrote:
> > What's with the magic "30" here?  I take it you are trying to make sure we
> don't
> > hit MAX_INT?
> 
> 1<<30 is about a billion, which is a somewhat arbitrary cutoff for the number
of
> loops we allow to run.  We can make this a flag or a named constant if you
want,
> but I wouldn't bother until we find we need to change it.  1<<30 ought to be a
> big enough upper limit to reach ~millisecond runtimes until we start getting
> CPUs running at >=THz and the loop bodies get down into ~picoseconds.  In
> practice, a couple million is fine for skia.
> 
> You're right, though, that like in most other cases involving overflow,
overflow
> would break things terribly.  This check does double duty.

That all sounds fine, please just paste your explanation into the code as a
comment.

https://codereview.chromium.org/23478013/diff/35001/bench/MatrixBench.cpp
File bench/MatrixBench.cpp (right):

https://codereview.chromium.org/23478013/diff/35001/bench/MatrixBench.cpp#new...
bench/MatrixBench.cpp:347: class DecomposeMatrixBench : public MatrixBench {
Why this new DecomposeMatrixBench in patchset 9?

https://codereview.chromium.org/23478013/diff/35001/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/23478013/diff/35001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:366: "  It's for your own good!\n");
Love it.  Finally, a benchmark tool with sass.

[mtklein, 2013-09-04 19:25:14]

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/23478013/diff/22001/bench/benchmain.cpp#newco...
bench/benchmain.cpp:552: if (loops >= (1<<30)) {
On 2013/09/04 19:18:26, epoger wrote:
> On 2013/09/04 19:06:51, mtklein wrote:
> > On 2013/09/04 14:36:40, epoger wrote:
> > > What's with the magic "30" here?  I take it you are trying to make sure we
> > don't
> > > hit MAX_INT?
> > 
> > 1<<30 is about a billion, which is a somewhat arbitrary cutoff for the
number
> of
> > loops we allow to run.  We can make this a flag or a named constant if you
> want,
> > but I wouldn't bother until we find we need to change it.  1<<30 ought to be
a
> > big enough upper limit to reach ~millisecond runtimes until we start getting
> > CPUs running at >=THz and the loop bodies get down into ~picoseconds.  In
> > practice, a couple million is fine for skia.
> > 
> > You're right, though, that like in most other cases involving overflow,
> overflow
> > would break things terribly.  This check does double duty.
> 
> That all sounds fine, please just paste your explanation into the code as a
> comment.

Done.

https://codereview.chromium.org/23478013/diff/35001/bench/MatrixBench.cpp
File bench/MatrixBench.cpp (right):

https://codereview.chromium.org/23478013/diff/35001/bench/MatrixBench.cpp#new...
bench/MatrixBench.cpp:347: class DecomposeMatrixBench : public MatrixBench {
On 2013/09/04 19:18:26, epoger wrote:
> Why this new DecomposeMatrixBench in patchset 9?

New at head after patchset 8.

[mtklein, 2013-09-10 19:25:21]

Committed patchset #14 manually as r11187.