cc: Add invalidation micro benchmark.

cc: Add invalidation micro benchmark.

This micro benchmark will used in a follow-up patch to generate various
kinds of invalidations (viewport, random, full layer) from the telemetry
repaint benchmark.

R=vmpstr@chromium.org
BUG=376379
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274065

[ernstm, 2014-05-22 22:49:18]

PTAL

[vmpstr, 2014-05-27 18:29:22]

From my understanding this is just an action right? Like it doesn't do any
measurements, and it does this thing only once.

How would this be used? (I looked at the other patch, but I'm way out of date on
telemetry stuff so I didn't quite get where the measurement was happening)

https://codereview.chromium.org/298723013/diff/1/cc/debug/invalidation_benchm...
File cc/debug/invalidation_benchmark.cc (right):

https://codereview.chromium.org/298723013/diff/1/cc/debug/invalidation_benchm...
cc/debug/invalidation_benchmark.cc:73: NOTREACHED();
can you
NOTREACHED() << "Invalid mode: " << mode_ << ". One of {layer, random, viewport}
expected";

or something similar instead? Since this is a user specified value, it might be
worth it to have a more verbose message.

I would also do the verification in the constructor and convert it to an enum so
that the scope of relying on user specified values is limited, but that's up to
you if you wanna do that.

[ernstm, 2014-05-27 18:35:47]

On 2014/05/27 18:29:22, vmpstr wrote:
> From my understanding this is just an action right? Like it doesn't do any
> measurements, and it does this thing only once.
> 
> How would this be used? (I looked at the other patch, but I'm way out of date
on
> telemetry stuff so I didn't quite get where the measurement was happening)

The micro 'benchmark' is basically installed as a repetitive action that creates
an invalidation every frame. The actual benchmarking happens entirely on the
telemetry side. The telemetry benchmark fires RAFs for several seconds, collects
the trace, and computes the smoothness metrics. This way, we get end-to-end
performance of the whole system.

[vmpstr, 2014-05-27 18:45:39]

On 2014/05/27 18:35:47, ernstm wrote:
> On 2014/05/27 18:29:22, vmpstr wrote:
> > From my understanding this is just an action right? Like it doesn't do any
> > measurements, and it does this thing only once.
> > 
> > How would this be used? (I looked at the other patch, but I'm way out of
date
> on
> > telemetry stuff so I didn't quite get where the measurement was happening)
> 
> The micro 'benchmark' is basically installed as a repetitive action that
creates
> an invalidation every frame. The actual benchmarking happens entirely on the
> telemetry side. The telemetry benchmark fires RAFs for several seconds,
collects
> the trace, and computes the smoothness metrics. This way, we get end-to-end
> performance of the whole system.

Ah I see. That's cool. Could you add a nice big "NOTE" comment in
invalidation_benchmark.h right before class InvalidationBenchmark that explains
this to some degree? Something along the lines of "note this benchmark will not
measure or return any results, it will simply invalidate blah blah blah"

[ernstm, 2014-05-27 21:17:07]

Updated version with:
- enum instead of string
- comment explaining how this is used
- random number generator with local state

https://codereview.chromium.org/298723013/diff/1/cc/debug/invalidation_benchm...
File cc/debug/invalidation_benchmark.cc (right):

https://codereview.chromium.org/298723013/diff/1/cc/debug/invalidation_benchm...
cc/debug/invalidation_benchmark.cc:73: NOTREACHED();
On 2014/05/27 18:29:22, vmpstr wrote:
> can you
> NOTREACHED() << "Invalid mode: " << mode_ << ". One of {layer, random,
viewport}
> expected";
> 
> or something similar instead? Since this is a user specified value, it might
be
> worth it to have a more verbose message.
> 
> I would also do the verification in the constructor and convert it to an enum
so
> that the scope of relying on user specified values is limited, but that's up
to
> you if you wanna do that.

Done.

[vmpstr, 2014-05-27 21:24:20]

Thanks for the comments!

https://codereview.chromium.org/298723013/diff/20001/cc/debug/invalidation_be...
File cc/debug/invalidation_benchmark.cc (right):

https://codereview.chromium.org/298723013/diff/20001/cc/debug/invalidation_be...
cc/debug/invalidation_benchmark.cc:48: LOG(ERROR) << "Invalid mode: " <<
mode_string
You can just do CHECK(false) << "blah" (the macro ends up evaluating to a stream
that will be reported along with the stack trace)

https://codereview.chromium.org/298723013/diff/20001/cc/debug/invalidation_be...
cc/debug/invalidation_benchmark.cc:104: seed_ = a * seed_ + c;
I'm not familiar with rng algorithms, but I assume it's expected that this will
overflow and wrap around?

https://codereview.chromium.org/298723013/diff/20001/cc/debug/invalidation_be...
cc/debug/invalidation_benchmark.cc:105: return seed_ /
std::numeric_limits<uint32>::max();
Doesn't this just give you 0, because of integer division. You should cast one
of the terms.

[ernstm, 2014-05-27 21:34:46]

https://codereview.chromium.org/298723013/diff/20001/cc/debug/invalidation_be...
File cc/debug/invalidation_benchmark.cc (right):

https://codereview.chromium.org/298723013/diff/20001/cc/debug/invalidation_be...
cc/debug/invalidation_benchmark.cc:48: LOG(ERROR) << "Invalid mode: " <<
mode_string
On 2014/05/27 21:24:21, vmpstr wrote:
> You can just do CHECK(false) << "blah" (the macro ends up evaluating to a
stream
> that will be reported along with the stack trace)

Done.

https://codereview.chromium.org/298723013/diff/20001/cc/debug/invalidation_be...
cc/debug/invalidation_benchmark.cc:104: seed_ = a * seed_ + c;
On 2014/05/27 21:24:21, vmpstr wrote:
> I'm not familiar with rng algorithms, but I assume it's expected that this
will
> overflow and wrap around?

Yes, this is expected to overflow and wrap around. The algorithm for LCG is
x_(i+1) = ((a * x_i) + c) mod m
where m is the period of the random number generator. For this particular choice
of a and c, m = 2^32, so we can optimize away the mod instruction.

https://codereview.chromium.org/298723013/diff/20001/cc/debug/invalidation_be...
cc/debug/invalidation_benchmark.cc:105: return seed_ /
std::numeric_limits<uint32>::max();
On 2014/05/27 21:24:21, vmpstr wrote:
> Doesn't this just give you 0, because of integer division. You should cast one
> of the terms.

Yes, I accidentally removed the cast in #2 and added it back in #3.

[vmpstr, 2014-05-27 21:54:18]

lgtm

https://codereview.chromium.org/298723013/diff/50001/cc/debug/invalidation_be...
File cc/debug/invalidation_benchmark.cc (right):

https://codereview.chromium.org/298723013/diff/50001/cc/debug/invalidation_be...
cc/debug/invalidation_benchmark.cc:91: default:
nit: you can remove the default, so that if we add new modes compiler will
complain.

[ernstm, 2014-05-27 21:57:11]

https://codereview.chromium.org/298723013/diff/50001/cc/debug/invalidation_be...
File cc/debug/invalidation_benchmark.cc (right):

https://codereview.chromium.org/298723013/diff/50001/cc/debug/invalidation_be...
cc/debug/invalidation_benchmark.cc:91: default:
On 2014/05/27 21:54:18, vmpstr wrote:
> nit: you can remove the default, so that if we add new modes compiler will
> complain.

Done.

[ernstm, 2014-05-27 21:57:15]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-05-27 21:58:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ernstm@chromium.org/298723013/70001

[ernstm, 2014-05-27 22:23:21]

The CQ bit was unchecked by ernstm@chromium.org

[vmpstr, 2014-05-30 18:50:43]

https://codereview.chromium.org/298723013/diff/130001/cc/debug/invalidation_b...
File cc/debug/invalidation_benchmark.cc (right):

https://codereview.chromium.org/298723013/diff/130001/cc/debug/invalidation_b...
cc/debug/invalidation_benchmark.cc:57: << ". One of {layer, viewport, random}
expected.";
Update this message please.

https://codereview.chromium.org/298723013/diff/130001/cc/debug/invalidation_b...
cc/debug/invalidation_benchmark.cc:80: int center_x = LCGRandom() *
visible_content_rect.width();
I was thinking maybe something like

int min_x = LCGRandom() * (visible_content_rect.width() - width_);
max_x = min_x + width_;

with the center approach, sometimes we'd get a cut off invalidation, no?

[ernstm, 2014-05-30 19:57:16]

https://codereview.chromium.org/298723013/diff/130001/cc/debug/invalidation_b...
File cc/debug/invalidation_benchmark.cc (right):

https://codereview.chromium.org/298723013/diff/130001/cc/debug/invalidation_b...
cc/debug/invalidation_benchmark.cc:57: << ". One of {layer, viewport, random}
expected.";
On 2014/05/30 18:50:43, vmpstr wrote:
> Update this message please.

Done.

https://codereview.chromium.org/298723013/diff/130001/cc/debug/invalidation_b...
cc/debug/invalidation_benchmark.cc:80: int center_x = LCGRandom() *
visible_content_rect.width();
On 2014/05/30 18:50:43, vmpstr wrote:
> I was thinking maybe something like
> 
> int min_x = LCGRandom() * (visible_content_rect.width() - width_);
> max_x = min_x + width_;
> 
> with the center approach, sometimes we'd get a cut off invalidation, no?

Yes, I was concerned about the uniformity of coverage. But that is probably less
important than keeping the invalidation size constant. Done.

[vmpstr, 2014-05-30 20:27:25]

lgtm

https://codereview.chromium.org/298723013/diff/140001/cc/debug/invalidation_b...
File cc/debug/invalidation_benchmark.cc (right):

https://codereview.chromium.org/298723013/diff/140001/cc/debug/invalidation_b...
cc/debug/invalidation_benchmark.cc:83:
invalidation_rect.Intersect(visible_content_rect);
nit: this might be reduntant

[ernstm, 2014-05-30 21:44:45]

https://codereview.chromium.org/298723013/diff/140001/cc/debug/invalidation_b...
File cc/debug/invalidation_benchmark.cc (right):

https://codereview.chromium.org/298723013/diff/140001/cc/debug/invalidation_b...
cc/debug/invalidation_benchmark.cc:83:
invalidation_rect.Intersect(visible_content_rect);
On 2014/05/30 20:27:26, vmpstr wrote:
> nit: this might be reduntant

Done.

[ernstm, 2014-05-30 21:45:05]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-05-30 21:49:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ernstm@chromium.org/298723013/160001

[commit-bot: I haz the power, 2014-05-31 03:06:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-31 22:17:20]

Change committed as 274065