Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
220963002:
DevTools: Add energy value under Timeline pie chart (Closed)
|
Created:
6 years, 8 months ago by Pan Modified:
6 years, 8 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, alph+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionDevTools: Add Energy Value under Timeline pie chart
BUG=337138
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171350
Patch Set 1 : Add to PowerGraph #
Total comments: 2
Patch Set 2 : #Patch Set 3 : #
Total comments: 10
Patch Set 4 : Add to Pie Chart #
Total comments: 14
Patch Set 5 : #
Total comments: 5
Patch Set 6 : #
Total comments: 5
Patch Set 7 : #
Messages
Total messages: 27 (0 generated)
Hi @pfeldman, This change add power consumption of a "selected window" in TimelinePowerOverview. Could you please review it? thanks Pan
https://codereview.chromium.org/220963002/diff/1/Source/devtools/front_end/Co... File Source/devtools/front_end/CountersGraph.js (right): https://codereview.chromium.org/220963002/diff/1/Source/devtools/front_end/Co... Source/devtools/front_end/CountersGraph.js:336: get minimumIndex() We try to not use getters in the new code. Also, requires annotations. https://codereview.chromium.org/220963002/diff/1/Source/devtools/front_end/Ti... File Source/devtools/front_end/TimelinePowerGraph.js (right): https://codereview.chromium.org/220963002/diff/1/Source/devtools/front_end/Ti... Source/devtools/front_end/TimelinePowerGraph.js:50: _drawGraph: function(counterUI) You can't override _drawGraph from another file - it is file-private. You need to make CountersGraph do what you want.
On 2014/04/01 14:21:10, pfeldman wrote: > https://codereview.chromium.org/220963002/diff/1/Source/devtools/front_end/Co... > File Source/devtools/front_end/CountersGraph.js (right): > > https://codereview.chromium.org/220963002/diff/1/Source/devtools/front_end/Co... > Source/devtools/front_end/CountersGraph.js:336: get minimumIndex() > We try to not use getters in the new code. Also, requires annotations. > > https://codereview.chromium.org/220963002/diff/1/Source/devtools/front_end/Ti... > File Source/devtools/front_end/TimelinePowerGraph.js (right): > > https://codereview.chromium.org/220963002/diff/1/Source/devtools/front_end/Ti... > Source/devtools/front_end/TimelinePowerGraph.js:50: _drawGraph: > function(counterUI) > You can't override _drawGraph from another file - it is file-private. You need > to make CountersGraph do what you want. thanks, how about PS2? Pan
I'm wondering can you just add another counter to the graph and populate it with accumulated values in regular way, i.e. on record arrival. wdyt?
On 2014/04/01 15:26:18, alph wrote: > I'm wondering can you just add another counter to the graph and populate it with > accumulated values in regular way, i.e. on record arrival. wdyt? do you mean record energy values into an independent "PowerCounter", then calculate everything just in PowerGraph? thanks Pan
On 2014/04/01 15:46:40, Pan wrote: > On 2014/04/01 15:26:18, alph wrote: > > I'm wondering can you just add another counter to the graph and populate it > with > > accumulated values in regular way, i.e. on record arrival. wdyt? > > do you mean record energy values into an independent "PowerCounter", then > calculate everything just in PowerGraph? > > thanks > Pan Yes. You can calculate the current value for energy right in _onRecordAdded
On 2014/04/01 17:02:50, alph wrote: > On 2014/04/01 15:46:40, Pan wrote: > > On 2014/04/01 15:26:18, alph wrote: > > > I'm wondering can you just add another counter to the graph and populate it > > with > > > accumulated values in regular way, i.e. on record arrival. wdyt? > > > > do you mean record energy values into an independent "PowerCounter", then > > calculate everything just in PowerGraph? > > > > thanks > > Pan > > Yes. You can calculate the current value for energy right in _onRecordAdded done, PS3 introduce two arrays, the benefit is clean code :) please review, thanks for your help. Pan
I probably didn't make myself clear. I meant don't you want to add a separate full-citizen graph (i.e. a separate Counter) for the integral energy? If there's no need for it then it's ok to just show its value without the graph. https://codereview.chromium.org/220963002/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/CountersGraph.js (right): https://codereview.chromium.org/220963002/diff/40001/Source/devtools/front_en... Source/devtools/front_end/CountersGraph.js:322: this.graphDrawn(); You don't need this, just override refresh in PowerGraph https://codereview.chromium.org/220963002/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/TimelinePowerGraph.js (right): https://codereview.chromium.org/220963002/diff/40001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerGraph.js:17: this._energy = this._currentValuesBar.createChild("span", "memory-counter-value"); You probably want to hide this guy when the power graph is turned off. https://codereview.chromium.org/220963002/diff/40001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerGraph.js:19: this._counter = this.createCounter(WebInspector.UIString("Power"), WebInspector.UIString("Power: %.2f\u2009watts"), color); Will it be clear to the user that this value represent the energy for the currently selected window? There's a power value next to it that changes as user moves the mouse. So it seems to be a bit inconsistent to me. https://codereview.chromium.org/220963002/diff/40001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerGraph.js:22: this._times = []; there's no real need for this array. https://codereview.chromium.org/220963002/diff/40001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerGraph.js:43: this._energies.push((record.timestamp - previousTime) * record.value); If you store integral values of energy here, you don't need to loop over it in graphDrawn. https://codereview.chromium.org/220963002/diff/40001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerGraph.js:75: for (var i = start + 1; i < end - 1; i++) { style: drop {} https://codereview.chromium.org/220963002/diff/40001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerGraph.js:81: } else { There are not handled corner cases, e.g.: minTime < times[0] maxTime > times[len-1] times.length === 1 times.length === 0 https://codereview.chromium.org/220963002/diff/40001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerGraph.js:86: this._energy.textContent = WebInspector.UIString("Energy: %.2f\u2009joules", totalEnergy / 1000); How about changing it to "Energy [J]: %.2f" ?
https://codereview.chromium.org/220963002/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/TimelinePowerGraph.js (right): https://codereview.chromium.org/220963002/diff/40001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerGraph.js:19: this._counter = this.createCounter(WebInspector.UIString("Power"), WebInspector.UIString("Power: %.2f\u2009watts"), color); On 2014/04/02 08:19:57, alph wrote: > Will it be clear to the user that this value represent the energy for the > currently selected window? > There's a power value next to it that changes as user moves the mouse. So it > seems to be a bit inconsistent to me. I think Energy:xxJoules, Power:xxWatts, is ok. or can we draw a a little bigger xxjoules just on the canvas of power graph?
On 2014/04/02 08:19:56, alph wrote: > I probably didn't make myself clear. > I meant don't you want to add a separate full-citizen graph (i.e. a separate > Counter) for the integral energy? If there's no need for it then it's ok to just > show its value without the graph. > I think draw the energy value is enough :) thanks Pan
https://codereview.chromium.org/220963002/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/TimelinePowerGraph.js (right): https://codereview.chromium.org/220963002/diff/40001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerGraph.js:19: this._counter = this.createCounter(WebInspector.UIString("Power"), WebInspector.UIString("Power: %.2f\u2009watts"), color); On 2014/04/03 13:58:39, Pan wrote: > On 2014/04/02 08:19:57, alph wrote: > > Will it be clear to the user that this value represent the energy for the > > currently selected window? > > There's a power value next to it that changes as user moves the mouse. So it > > seems to be a bit inconsistent to me. > > I think Energy:xxJoules, Power:xxWatts, is ok. > or can we draw a a little bigger xxjoules just on the canvas of power graph? Showing energy on the values bar seems inconsistent as I wrote above. Moreover the values bar disappears when the user moves mouse off the chart. The energy value should not be affected by the mouse movements, as it is defined by the current window and not the mouse position. Btw, wouldn't a separate chart for energy solve the problem?
> Showing energy on the values bar seems inconsistent as I wrote above. Moreover > the values bar disappears when the user moves mouse off the chart. The energy > value should not be affected by the mouse movements, as it is defined by the > current window and not the mouse position. > > Btw, wouldn't a separate chart for energy solve the problem? That was my idea. We have too many canvas-based charts in house and need some consistency.
On 2014/04/03 14:01:00, Pan wrote: > On 2014/04/02 08:19:56, alph wrote: > > I probably didn't make myself clear. > > I meant don't you want to add a separate full-citizen graph (i.e. a separate > > Counter) for the integral energy? If there's no need for it then it's ok to > just > > show its value without the graph. > > > I think draw the energy value is enough :) > > thanks > Pan Pavel, has suggested to show energy for the selected window in the details view. It seems to be the right place for that kind of information, as currently it shows a pie chart with event type for the selected window. Wdyt?
On 2014/04/03 17:11:40, alph wrote: > On 2014/04/03 14:01:00, Pan wrote: > > On 2014/04/02 08:19:56, alph wrote: > > > I probably didn't make myself clear. > > > I meant don't you want to add a separate full-citizen graph (i.e. a separate > > > Counter) for the integral energy? If there's no need for it then it's ok to > > just > > > show its value without the graph. > > > > > I think draw the energy value is enough :) > > > > thanks > > Pan > > Pavel, has suggested to show energy for the selected window in the details view. > It seems to be the right place for that kind of information, as currently it > shows a pie chart with event type for the selected window. Wdyt? Wdyt stands for "what do you think"? :) instant power values bar lives with mouse move, it will disappear when mouse move out of the chart. While energy value bar live at the front of power value bar, it will be always there(even mouse out of chart), it changes according to overview windows selected, rather than mouse move. it only disappears when other timeline modes is selected. is it ok? thanks Pan
On 2014/04/08 14:21:01, Pan wrote: > On 2014/04/03 17:11:40, alph wrote: > > On 2014/04/03 14:01:00, Pan wrote: > > > On 2014/04/02 08:19:56, alph wrote: > > > > I probably didn't make myself clear. > > > > I meant don't you want to add a separate full-citizen graph (i.e. a > separate > > > > Counter) for the integral energy? If there's no need for it then it's ok > to > > > just > > > > show its value without the graph. > > > > > > > I think draw the energy value is enough :) > > > > > > thanks > > > Pan > > > > Pavel, has suggested to show energy for the selected window in the details > view. > > It seems to be the right place for that kind of information, as currently it > > shows a pie chart with event type for the selected window. Wdyt? > > Wdyt stands for "what do you think"? :) yes. > instant power values bar lives with mouse move, it will disappear when mouse > move out of the chart. > While energy value bar live at the front of power value bar, it will be always > there(even mouse out of chart), it changes according to overview windows > selected, rather than mouse move. it only disappears when other timeline modes > is selected. > is it ok? yes, but Pavel suggested to put Energy value into details view (the one with pie chart), not to the values bar. Do you think it's convenient? > thanks > Pan
> > yes, but Pavel suggested to put Energy value into details view (the one with pie > chart), not to the values bar. Do you think it's convenient? > Did he talk this to you offline? I didn't aware that idea. Actually, I think it will be convenient if we show watt and joules together, while it is not strong preference. thanks Pan
Hi @pfeldman @alph PS4 add energy under timeline pie chart. please reivew. thanks Pan
https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/TimelinePanel.js (right): https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/TimelinePanel.js:895: var energy = this._viewsForMode(WebInspector.TimelinePanel.Mode.Power).overviewView.calculateEnergy(startTime, endTime); drop {} and 'var' https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/TimelinePowerOverview.js (right): https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerOverview.js:41: var start = minTime >= times[0] ? Number.constrain(times.upperBound(minTime) - 1, 0, times.length - 1) : 0; no need for "minTime >= times[0]" check as constrain return 0 anyway for that case. https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerOverview.js:44: var end = maxTime <= times[times.length - 1] ? Number.constrain(times.lowerBound(maxTime), 0, times.length - 1) : times.length - 1; ditto for maxTime <= times[...] check. https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerOverview.js:46: if (start + 1 === end) There seems to be another case when start === end, e.g. when maxTime < times[0] https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerOverview.js:47: return (maxTime - minTime) / (times[end] - times[start]) * (energies[end] - energies[start]) / 1000; seems to be incorrect if minTime < times[0]. same for maxTime > times[last] https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerOverview.js:51: totalEnergy += (times[start + 1] - minTime) / (times[start + 1] - times[start]) * (energies[start + 1] - energies[start]); A case of minTime < times[0] and the same at the other end are not handled. https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/TimelineUIUtils.js (right): https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/TimelineUIUtils.js:353: rowElement.createChild("div", "timeline-aggregated-category timeline-" + "power"); what does this div do? Is it empty? https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/TimelineUIUtils.js:354: rowElement.createTextChild(WebInspector.UIString("%0.2f joules %s", energy, "Energy")); %.2f Why not just put "Energy" inside the UIString, e.g. "Energy: %.2f Joules"?
thanks for your help, please review :) Pan https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/TimelinePanel.js (right): https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/TimelinePanel.js:895: var energy = this._viewsForMode(WebInspector.TimelinePanel.Mode.Power).overviewView.calculateEnergy(startTime, endTime); On 2014/04/09 16:02:27, alph wrote: > drop {} and 'var' Done. https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/TimelinePowerOverview.js (right): https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerOverview.js:41: var start = minTime >= times[0] ? Number.constrain(times.upperBound(minTime) - 1, 0, times.length - 1) : 0; On 2014/04/09 16:02:27, alph wrote: > no need for "minTime >= times[0]" check as constrain return 0 anyway for that > case. Done. https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerOverview.js:44: var end = maxTime <= times[times.length - 1] ? Number.constrain(times.lowerBound(maxTime), 0, times.length - 1) : times.length - 1; On 2014/04/09 16:02:27, alph wrote: > ditto for maxTime <= times[...] check. Done. https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerOverview.js:46: if (start + 1 === end) On 2014/04/09 16:02:27, alph wrote: > There seems to be another case when start === end, e.g. when maxTime < times[0] thanks, I'd like to return 0 when window is out of times[] :) https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerOverview.js:47: return (maxTime - minTime) / (times[end] - times[start]) * (energies[end] - energies[start]) / 1000; On 2014/04/09 16:02:27, alph wrote: > seems to be incorrect if minTime < times[0]. > same for maxTime > times[last] thanks, if window's range is bigger than times[], I think we have to just return power consumption during times[]. so I'd like to adjust the window time to times[] boundary. https://codereview.chromium.org/220963002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/TimelinePowerOverview.js:51: totalEnergy += (times[start + 1] - minTime) / (times[start + 1] - times[start]) * (energies[start + 1] - energies[start]); On 2014/04/09 16:02:27, alph wrote: > A case of minTime < times[0] and the same at the other end are not handled. Done.
Great! lgtm from me. Thought you need to get one from an owner to land the patch. https://codereview.chromium.org/220963002/diff/100001/Source/devtools/front_e... File Source/devtools/front_end/TimelinePowerOverview.js (right): https://codereview.chromium.org/220963002/diff/100001/Source/devtools/front_e... Source/devtools/front_end/TimelinePowerOverview.js:30: * @param {number} maxTime nit: @return https://codereview.chromium.org/220963002/diff/100001/Source/devtools/front_e... Source/devtools/front_end/TimelinePowerOverview.js:36: var recordNumber = times.length; nit: replace recordNumber with: var last = times.length - 1; https://codereview.chromium.org/220963002/diff/100001/Source/devtools/front_e... Source/devtools/front_end/TimelinePowerOverview.js:196: * @param {number} maxTime nit: @return https://codereview.chromium.org/220963002/diff/100001/Source/devtools/front_e... File Source/devtools/front_end/TimelineUIUtils.js (right): https://codereview.chromium.org/220963002/diff/100001/Source/devtools/front_e... Source/devtools/front_end/TimelineUIUtils.js:354: rowElement.createChild("div", "timeline-aggregated-category timeline-" + "power"); nit: concatenate "timeline-" and "power" I still don't understand why do you need an empty div?
thanks a lot @alph, Hi @pfeldman, could you please have a look at this? thanks Pan https://codereview.chromium.org/220963002/diff/100001/Source/devtools/front_e... File Source/devtools/front_end/TimelineUIUtils.js (right): https://codereview.chromium.org/220963002/diff/100001/Source/devtools/front_e... Source/devtools/front_end/TimelineUIUtils.js:354: rowElement.createChild("div", "timeline-aggregated-category timeline-" + "power"); On 2014/04/10 15:38:55, alph wrote: > nit: concatenate "timeline-" and "power" > I still don't understand why do you need an empty div? Thanks a lot! this empty div is a colored rectangle, I intend to keep consistent to other categories(Scripting, Idle, Paint etc).
A couple of nits and it is good to go. https://codereview.chromium.org/220963002/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/TimelinePanel.js (right): https://codereview.chromium.org/220963002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/TimelinePanel.js:894: if (Capabilities.canProfilePower) This particular bit does not look modular. Eventually, views will register in the panel, so that it would not know their concrete interfaces. Lets leave this for now, but please JSDoc-cast the overviewView to your particular type below. https://codereview.chromium.org/220963002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/TimelinePanel.js:898: fragment.appendChild(WebInspector.TimelineUIUtils.generatePieChart(aggregatedStats, undefined, undefined, energy)); Lets append energy to the fragment instead - pie chart should not know about it. https://codereview.chromium.org/220963002/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/TimelineUIUtils.js (right): https://codereview.chromium.org/220963002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/TimelineUIUtils.js:302: WebInspector.TimelineUIUtils.generatePieChart = function(aggregatedStats, selfCategory, selfTime, energy) I don't think energy fits here well - it used to be abstract from what it renders.
https://codereview.chromium.org/220963002/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/TimelinePanel.js (right): https://codereview.chromium.org/220963002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/TimelinePanel.js:894: if (Capabilities.canProfilePower) On 2014/04/11 12:17:49, pfeldman wrote: > This particular bit does not look modular. Eventually, views will register in > the panel, so that it would not know their concrete interfaces. Lets leave this > for now, but please JSDoc-cast the overviewView to your particular type below. thanks, done https://codereview.chromium.org/220963002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/TimelinePanel.js:898: fragment.appendChild(WebInspector.TimelineUIUtils.generatePieChart(aggregatedStats, undefined, undefined, energy)); On 2014/04/11 12:17:49, pfeldman wrote: > Lets append energy to the fragment instead - pie chart should not know about it. How about append it after title? people will know energy while they know the time range
lgtm
The CQ bit was checked by pan.deng@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pan.deng@intel.com/220963002/160001
Message was sent while issue was closed.
Change committed as 171350 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
