Adds a per-frame power usage chart to the power sample analysis view.

Adds a per-frame power usage chart to the power sample analysis view.

Screenshot: http://goo.gl/xWVp7a

BUG=#1108

[nduca(google), 2015-08-03 19:42:58]

looks really nice, lgtm on my part but broad question

https://codereview.appspot.com/254510043/diff/80001/tracing/tracing/ui/analys...
File tracing/tracing/ui/analysis/frame_power_usage_chart.html (right):

https://codereview.appspot.com/254510043/diff/80001/tracing/tracing/ui/analys...
tracing/tracing/ui/analysis/frame_power_usage_chart.html:103: getData_:
function(samples, vSyncTimestamps) {
what do you think some of this vsync-power correlation being more of a model
level function?

VSyncEvent.associatedPowerSamples? Internally it could do a gather on
this.device.powerSamples ... though that'd require vsync events to have a start
& duration. And they'd probably more be VSYncIntervals than an event.

Nice thing here is that device object could ensure that the power samples are
sorted in timestamp order, and vsync events could cache their samples.

Seems like even with VSyncEvent.associatedPowerSamples, you'd end up doing some
subsetting so only the brushed samples get shown, but maybe it'd be more like
(1) get all the vsync events touched by the given samples, (2) something...

[beaudoin, 2015-08-03 21:23:46]

https://codereview.appspot.com/254510043/diff/80001/tracing/tracing/ui/analys...
File tracing/tracing/ui/analysis/frame_power_usage_chart.html (right):

https://codereview.appspot.com/254510043/diff/80001/tracing/tracing/ui/analys...
tracing/tracing/ui/analysis/frame_power_usage_chart.html:61:
this.updateContents_();
Should you have a way to set both the samples and vSyncTimestamps at once? As it
stands, you'll refresh the chart twice when you set its data. Not cool...

https://codereview.appspot.com/254510043/diff/80001/tracing/tracing/ui/analys...
tracing/tracing/ui/analysis/frame_power_usage_chart.html:67: var data =
this.getData_(this.samples, this.vSyncTimestamps.slice());
Is there a reason why you pass samples and vSyncTimestamps into getData_ rather
than accessing the members within that other method? The latter would look
cleaner to me.

https://codereview.appspot.com/254510043/diff/80001/tracing/tracing/ui/analys...
tracing/tracing/ui/analysis/frame_power_usage_chart.html:114: lastVSyncTimestamp
= vSyncTimestamps.shift();
You assume the vSyncTimstamps to be sorted in ascending order, this should be
mentioned as a comment on the setter at a minimum.

https://codereview.appspot.com/254510043/diff/80001/tracing/tracing/ui/analys...
File tracing/tracing/ui/analysis/frame_power_usage_chart_test.html (right):

https://codereview.appspot.com/254510043/diff/80001/tracing/tracing/ui/analys...
tracing/tracing/ui/analysis/frame_power_usage_chart_test.html:236:
series.addPowerSample(7, 3000);
You should probably have a test where the power samples are not added in
increasing order of time just to make sure it works in this case.

[charliea, 2015-08-04 14:35:54]

https://codereview.appspot.com/254510043/diff/80001/tracing/tracing/ui/analys...
File tracing/tracing/ui/analysis/frame_power_usage_chart.html (right):

https://codereview.appspot.com/254510043/diff/80001/tracing/tracing/ui/analys...
tracing/tracing/ui/analysis/frame_power_usage_chart.html:61:
this.updateContents_();
On 2015/08/03 21:23:46, beaudoin wrote:
> Should you have a way to set both the samples and vSyncTimestamps at once? As
it
> stands, you'll refresh the chart twice when you set its data. Not cool...

Agreed. Added a setData() function, removed vSyncTimestamps and samples setters.

https://codereview.appspot.com/254510043/diff/80001/tracing/tracing/ui/analys...
tracing/tracing/ui/analysis/frame_power_usage_chart.html:67: var data =
this.getData_(this.samples, this.vSyncTimestamps.slice());
On 2015/08/03 21:23:46, beaudoin wrote:
> Is there a reason why you pass samples and vSyncTimestamps into getData_
rather
> than accessing the members within that other method? The latter would look
> cleaner to me.

I liked the idea of making it clear what members the chart data was actually
being constructed from. That being said, I don't think it's worth the
redundancy, so it's now removed.

https://codereview.appspot.com/254510043/diff/80001/tracing/tracing/ui/analys...
tracing/tracing/ui/analysis/frame_power_usage_chart.html:103: getData_:
function(samples, vSyncTimestamps) {
On 2015/08/03 19:42:58, nduca(google) wrote:
> what do you think some of this vsync-power correlation being more of a model
> level function?
> 
> VSyncEvent.associatedPowerSamples? Internally it could do a gather on
> this.device.powerSamples ... though that'd require vsync events to have a
start
> & duration. And they'd probably more be VSYncIntervals than an event.
> 
> Nice thing here is that device object could ensure that the power samples are
> sorted in timestamp order, and vsync events could cache their samples.
> 
> Seems like even with VSyncEvent.associatedPowerSamples, you'd end up doing
some
> subsetting so only the brushed samples get shown, but maybe it'd be more like
> (1) get all the vsync events touched by the given samples, (2) something...

I like this idea a lot - it seems like it would make this function a lot more
readable if it were just a matter of

var points = [];
var frameNumber = 0;
vSyncEvents.forEach(function(vSyncEvent) {
  vSyncEvent.getPowerSamplesForFrame().forEach(function(powerSample) {
    var point = { x: powerSample.timestamp - vSyncEvent.timestamp };
    point['f' + frameNumber] = sample.power;
    points.push(point);
  });

  frameNumber++;
});

Given the fact that this is a relatively straightforward refactor and doesn't
change much in this CL (besides this function), I'm inclined to get this code
submitted first and do the model rejiggering afterwards, if that's alright with
you.

https://codereview.appspot.com/254510043/diff/80001/tracing/tracing/ui/analys...
tracing/tracing/ui/analysis/frame_power_usage_chart.html:114: lastVSyncTimestamp
= vSyncTimestamps.shift();
On 2015/08/03 21:23:46, beaudoin wrote:
> You assume the vSyncTimstamps to be sorted in ascending order, this should be
> mentioned as a comment on the setter at a minimum.

Added a note on the setter regarding this.

https://codereview.appspot.com/254510043/diff/80001/tracing/tracing/ui/analys...
File tracing/tracing/ui/analysis/frame_power_usage_chart_test.html (right):

https://codereview.appspot.com/254510043/diff/80001/tracing/tracing/ui/analys...
tracing/tracing/ui/analysis/frame_power_usage_chart_test.html:236:
series.addPowerSample(7, 3000);
On 2015/08/03 21:23:46, beaudoin wrote:
> You should probably have a test where the power samples are not added in
> increasing order of time just to make sure it works in this case.

Done.

[beaudoin, 2015-08-04 14:54:57]

LGTM

[nduca(google), 2015-08-04 16:03:00]

> Given the fact that this is a relatively straightforward refactor and doesn't
> change much in this CL (besides this function), I'm inclined to get this code
> submitted first and do the model rejiggering afterwards, if that's alright
with
> you.
Thats fine. I find that a great technique for avoiding a review roundtrip here
and/or asking for permission to land-the-clean is to file and self assign a bug,
and put todos in the code saying TODO(charliea): fix, link. ymmv of course, but
I've found it to be effective in the past.