Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2333053002:
Put all power metrics in one file. (Closed)
|
Created:
4 years, 3 months ago by alexandermont Modified:
4 years, 3 months ago Reviewers:
charliea (OOO until 10-5) CC:
catapult-reviews_chromium.org, tracing-review_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionPut all power metrics in one file.
Refactors the power metrics code to put all the power metrics in
one file.
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/ee7da6a366113b3ac7732ed78519589457f26911
Patch Set 1 #
Total comments: 12
Patch Set 2 : refactoring #
Total comments: 22
Patch Set 3 : refactor #Patch Set 4 : rebase #Patch Set 5 : update for rebase #
Total comments: 30
Patch Set 6 : changes from code review #
Total comments: 14
Patch Set 7 : code review changes #Patch Set 8 : code review changes #Patch Set 9 : revert loading metric test #Messages
Total messages: 35 (12 generated)
alexandermont@chromium.org changed reviewers: + charliea@chromium.org
https://codereview.chromium.org/2333053002/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2333053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/power_metric.html:21: // TODO(alexandermont): Per-frame power metric will be deprecated once supernit: here and elsewhere, please put TODO above the existing comment so that the code and the comment that documents it are still right next to each other. https://codereview.chromium.org/2333053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/power_metric.html:27: // Gets the power data (time(ms), energy(J), power(W) from an given nit (here and elsewhere): When documenting functions and classes, please use jsdoc (http://usejsdoc.org), which is used elsewhere in the trace viewer codebase and intended for this purpose. Also, it's probably worthwhile to document what the return object contains, given that it's a part of the interface of the function. As an example, this might look something like: /** * Returns an object of the form: * * { * duration: durationInMs, * energy: energyInJ, * power: powerInW * } * * for the specified interval. */ https://codereview.chromium.org/2333053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/power_metric.html:29: nit: delete empty line https://codereview.chromium.org/2333053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/power_metric.html:30: function getPowerData(model, start, end) { nit: because these are package private functions, they should end in "_" https://codereview.chromium.org/2333053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/power_metric.html:53: for (var bin of tr.b.getOnlyElement(ttiValues).numeric.allBins) Please add braces around the inner for loop https://codereview.chromium.org/2333053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/power_metric.html:63: if (!model.device.powerSeries) Rather than having one big long function, please break this into subfunctions like the memory metric does: https://cs.chromium.org/chromium/src/third_party/catapult/tracing/tracing/met... In general, any time you have a long function (I would consider 150 lines *really* long), you should ask yourself if it's absolutely necessary, or if there's any way to break it up. Most of the time, the latter is possible.
https://codereview.chromium.org/2333053002/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2333053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/power_metric.html:21: // TODO(alexandermont): Per-frame power metric will be deprecated once On 2016/09/13 at 19:33:07, charliea wrote: > supernit: here and elsewhere, please put TODO above the existing comment so that the code and the comment that documents it are still right next to each other. Done https://codereview.chromium.org/2333053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/power_metric.html:27: // Gets the power data (time(ms), energy(J), power(W) from an given On 2016/09/13 at 19:33:07, charliea wrote: > nit (here and elsewhere): When documenting functions and classes, please use jsdoc (http://usejsdoc.org), which is used elsewhere in the trace viewer codebase and intended for this purpose. > > Also, it's probably worthwhile to document what the return object contains, given that it's a part of the interface of the function. > > As an example, this might look something like: > > /** > * Returns an object of the form: > * > * { > * duration: durationInMs, > * energy: energyInJ, > * power: powerInW > * } > * > * for the specified interval. > */ Done https://codereview.chromium.org/2333053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/power_metric.html:29: On 2016/09/13 at 19:33:07, charliea wrote: > nit: delete empty line Done https://codereview.chromium.org/2333053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/power_metric.html:30: function getPowerData(model, start, end) { On 2016/09/13 at 19:33:07, charliea wrote: > nit: because these are package private functions, they should end in "_" Done https://codereview.chromium.org/2333053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/power_metric.html:53: for (var bin of tr.b.getOnlyElement(ttiValues).numeric.allBins) On 2016/09/13 at 19:33:07, charliea wrote: > Please add braces around the inner for loop Done https://codereview.chromium.org/2333053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/power_metric.html:63: if (!model.device.powerSeries) On 2016/09/13 at 19:33:07, charliea wrote: > Rather than having one big long function, please break this into subfunctions like the memory metric does: https://cs.chromium.org/chromium/src/third_party/catapult/tracing/tracing/met... > > In general, any time you have a long function (I would consider 150 lines *really* long), you should ask yourself if it's absolutely necessary, or if there's any way to break it up. Most of the time, the latter is possible. Done
https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:78: * Creates a histogram suitable for time data. nit: unnecessary jsdoc https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:82: tr.b.Unit.byName.timeDurationInMs_smallerIsBetter); Is smaller actually better for a time histogram? https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:95: * Creates a histogram suitable for energy data. nit: unnecessary jsdoc https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:112: * Creates a histogram suitable for power data. nit: unnecessary jsdoc https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:132: * @param {Object} data - Power data (obtained from getPowerData) Here and a lot of other places: I think that lots of these should be {!Object}, {!tr.v.Histogram}, etc. This indicates that it's not okay to pass null/undefined as a value for that parameter: http://usejsdoc.org/tags-type.html https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:143: /** Outputs histograms to the metrics output (the ValueSet). nit: documentation starts on the second line, like you do elsewhere https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:160: if(timeHist !== undefined) nit: space after if https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:172: * Creates the histograms to keep track of power metrics. nit: think this function documentation is probably superfluous https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:211: * on the star and end time of the RAIL stages. s/star/start https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:255: * Compute the per-frame power metrics and put the results in hists. If you're referring to the variable, it should probably be |hists|. If you're using hists as short for histograms in non-code, don't :-) https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric_test.html (right): https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric_test.html:61: 'successful_load:energy'), 125, 0.5); Has this always been called successful_load? It seems weird to differentiate it from "load", where people might expect to find this metric
https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:78: * Creates a histogram suitable for time data. On 2016/09/14 at 20:49:32, charliea wrote: > nit: unnecessary jsdoc Removing the jusdoc formatting here. (But why is it unnecessary? I thought all the functions were supposed to have jsdocs.) https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:82: tr.b.Unit.byName.timeDurationInMs_smallerIsBetter); On 2016/09/14 at 20:49:33, charliea wrote: > Is smaller actually better for a time histogram? It seems like it would; it means you're completing that task faster. (Do you have to say either bigger is better or smaller is better? Is is possible to say that neither direction is better? Or maybe we should just get rid of the "time" histograms completely - this is an energy and power metric, not a speed metric.) https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:95: * Creates a histogram suitable for energy data. On 2016/09/14 at 20:49:33, charliea wrote: > nit: unnecessary jsdoc See above https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:112: * Creates a histogram suitable for power data. On 2016/09/14 at 20:49:33, charliea wrote: > nit: unnecessary jsdoc See above https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:132: * @param {Object} data - Power data (obtained from getPowerData) On 2016/09/14 at 20:49:33, charliea wrote: > Here and a lot of other places: I think that lots of these should be {!Object}, {!tr.v.Histogram}, etc. > > This indicates that it's not okay to pass null/undefined as a value for that parameter: http://usejsdoc.org/tags-type.html Done https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:143: /** Outputs histograms to the metrics output (the ValueSet). On 2016/09/14 at 20:49:32, charliea wrote: > nit: documentation starts on the second line, like you do elsewhere Done https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:160: if(timeHist !== undefined) On 2016/09/14 at 20:49:32, charliea wrote: > nit: space after if Done https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:172: * Creates the histograms to keep track of power metrics. On 2016/09/14 at 20:49:32, charliea wrote: > nit: think this function documentation is probably superfluous Done https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:211: * on the star and end time of the RAIL stages. On 2016/09/14 at 20:49:32, charliea wrote: > s/star/start Done https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:255: * Compute the per-frame power metrics and put the results in hists. On 2016/09/14 at 20:49:33, charliea wrote: > If you're referring to the variable, it should probably be |hists|. If you're using hists as short for histograms in non-code, don't :-) Done https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric_test.html (right): https://codereview.chromium.org/2333053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric_test.html:61: 'successful_load:energy'), 125, 0.5); On 2016/09/14 at 20:49:33, charliea wrote: > Has this always been called successful_load? It seems weird to differentiate it from "load", where people might expect to find this metric This is testing the "generic" power metrics which take the name of the metric from the title of the RAIL stage, which is "successful load." This just uses whatever the RAIL stage name is. The loading-specific metrics are tested down below (powerMetric_loading_oneInterval) which does look for "load:energy".
https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:20: * TODO(alexandermont): Per-frame power metric will be deprecated once nit: we still use // // // for all multiline comments besides jsdocs. (Also, even though jsdoc can technically be used to document variables and constants, I think we generally use normal docs like above.) https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:30: * Gets the power data (time(ms), energy(J), power(W)) from an given I think you'd convey the same information with: /** * Returns power data for the specified interval in the form: * * { * duration: durationInMs, * energy: energyInJ, * power: powerInW * } */ A couple of notes: - Generally, if a function is sufficiently simple and works mostly through its return value (like this one), it's a good idea to start the doc with "Returns <blah>" - I think it's okay to omit the units from the doc because they're specified in the "of the form" part below. - Like the jsdoc function doc itself, the jsdoc parameter docs are only needed if you feel like those parameters provide necessary context that isn't provided already. In this case, "model" is pretty generally understood within the trace viewer codebase to mean the trace model, and "start" and "end" can probably be figured out by the fact that you say "specified interval" in the function doc. - (Completely optional) I think the "object of the form..." code is a little more readable if you add a line before it. But I admit this is completely personal preference :-) https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:43: var durationInS = tr.b.convertUnit(durationInMs, suggestion (which you can feel free to ignore): you can just inline |end - start| here https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:45: var energyInJ = model.device.powerSeries.getEnergyConsumedInJ( Can you do: var energyInJ = model.device.powerSeries.getEnergyConsumedInJ(start, end); https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:52: * TODO(alexandermont): When LoadExpectation v.1.0 is released, supernit: most version numbers don't have a period between the "v" and the number (e.g. http://www.rocketleaguegame.com/news/category/patch-notes/), which would make this v1.0 https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:55: * RAIL stage to be the TTI, then we may not even need to treat the loading nit: looks like this line might be too long? https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:58: * Gets the intervals of time between navigation event and TTI So this is strange, but TODOs generally don't go inside of jsdocs, but rather above them. That means this would look like: // TODO(alexandermont): When LoadExpexcation v1.0 is released, update // this function to use the new LoadExpectation rather than calling... /** * Returns the interval of time between the navigation event and time * to interactive. */ Also, same comment as above about starting jsdocs with "Returns <blah>" if at all possible https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:61: * @param {tr.b.Model} model - Trace model. nit: as mentioned above, |model| is pretty well understood to mean the trace model within trace viewer, so I think it's okay to leave off the jsdoc parameter docs. https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:145: * @param {!string} title - Brief title of this metric, e.g. "scroll". Will Maybe too long of a line? https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:233: * the loading metric intervals don't correspond exactly to the RAIL stages. is this line too long? https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:243: var afterLoadData = getPowerData_(model, lastLoadTime, model.bounds.max); is this line too long? https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:254: var frameData = getPowerData_(model, currentTime, currentTime + FRAME_MS); is this line too long? https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:273: hists.scrollTimeHist, hists.scrollEnergyHist, hists.scrollPowerHist); same https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:295: // and so need to be computed outside the processInteractionRecord_ loop. same https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric_test.html (right): https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric_test.html:118: rendererProcess.objects.addSnapshot('ptr', 'loading', 'FrameLoader', 300, same
https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:20: * TODO(alexandermont): Per-frame power metric will be deprecated once On 2016/09/15 at 00:19:41, charliea wrote: > nit: we still use > > // > // > // > > for all multiline comments besides jsdocs. (Also, even though jsdoc can technically be used to document variables and constants, I think we generally use normal docs like above.) Done https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:30: * Gets the power data (time(ms), energy(J), power(W)) from an given On 2016/09/15 at 00:19:40, charliea wrote: > I think you'd convey the same information with: > > /** > * Returns power data for the specified interval in the form: > * > * { > * duration: durationInMs, > * energy: energyInJ, > * power: powerInW > * } > */ > > A couple of notes: > > - Generally, if a function is sufficiently simple and works mostly through its return value (like this one), it's a good idea to start the doc with "Returns <blah>" > - I think it's okay to omit the units from the doc because they're specified in the "of the form" part below. > - Like the jsdoc function doc itself, the jsdoc parameter docs are only needed if you feel like those parameters provide necessary context that isn't provided already. In this case, "model" is pretty generally understood within the trace viewer codebase to mean the trace model, and "start" and "end" can probably be figured out by the fact that you say "specified interval" in the function doc. > - (Completely optional) I think the "object of the form..." code is a little more readable if you add a line before it. But I admit this is completely personal preference :-) Done https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:43: var durationInS = tr.b.convertUnit(durationInMs, On 2016/09/15 at 00:19:41, charliea wrote: > suggestion (which you can feel free to ignore): you can just inline |end - start| here Done https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:45: var energyInJ = model.device.powerSeries.getEnergyConsumedInJ( On 2016/09/15 at 00:19:41, charliea wrote: > Can you do: > > var energyInJ = > model.device.powerSeries.getEnergyConsumedInJ(start, end); Done https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:52: * TODO(alexandermont): When LoadExpectation v.1.0 is released, On 2016/09/15 at 00:19:41, charliea wrote: > supernit: most version numbers don't have a period between the "v" and the number (e.g. http://www.rocketleaguegame.com/news/category/patch-notes/), which would make this v1.0 Done https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:55: * RAIL stage to be the TTI, then we may not even need to treat the loading On 2016/09/15 at 00:19:41, charliea wrote: > nit: looks like this line might be too long? It't not too long. This line is 78 characters. https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:58: * Gets the intervals of time between navigation event and TTI On 2016/09/15 at 00:19:41, charliea wrote: > So this is strange, but TODOs generally don't go inside of jsdocs, but rather above them. That means this would look like: > > // TODO(alexandermont): When LoadExpexcation v1.0 is released, update > // this function to use the new LoadExpectation rather than calling... > /** > * Returns the interval of time between the navigation event and time > * to interactive. > */ > > Also, same comment as above about starting jsdocs with "Returns <blah>" if at all possible Done https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:61: * @param {tr.b.Model} model - Trace model. On 2016/09/15 at 00:19:41, charliea wrote: > nit: as mentioned above, |model| is pretty well understood to mean the trace model within trace viewer, so I think it's okay to leave off the jsdoc parameter docs. Done https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:145: * @param {!string} title - Brief title of this metric, e.g. "scroll". Will On 2016/09/15 at 00:19:41, charliea wrote: > Maybe too long of a line? This is also 78 characters. What makes you think this line is too long? https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:233: * the loading metric intervals don't correspond exactly to the RAIL stages. On 2016/09/15 at 00:19:41, charliea wrote: > is this line too long? This line is 78 characters. https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:243: var afterLoadData = getPowerData_(model, lastLoadTime, model.bounds.max); On 2016/09/15 at 00:19:41, charliea wrote: > is this line too long? This line is 77 characters. https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:254: var frameData = getPowerData_(model, currentTime, currentTime + FRAME_MS); On 2016/09/15 at 00:19:41, charliea wrote: > is this line too long? This line is 80 characters. https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:273: hists.scrollTimeHist, hists.scrollEnergyHist, hists.scrollPowerHist); On 2016/09/15 at 00:19:40, charliea wrote: > same This line is 77 characters. https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:295: // and so need to be computed outside the processInteractionRecord_ loop. On 2016/09/15 at 00:19:41, charliea wrote: > same This line is 77 characters. https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric_test.html (right): https://codereview.chromium.org/2333053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric_test.html:118: rendererProcess.objects.addSnapshot('ptr', 'loading', 'FrameLoader', 300, On 2016/09/15 at 00:19:41, charliea wrote: > same This line is 79 characters.
lgtm https://codereview.chromium.org/2333053002/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2333053002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/power_metric.html:50: nit: no need for empty line between the comment and the jsdoc of the function the comment applies to https://codereview.chromium.org/2333053002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/power_metric.html:52: * Returns the intervals of time between navigation event and time nit: looks like "to" can be moved to the previous line https://codereview.chromium.org/2333053002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/power_metric.html:70: // Creates a histogram suitable for time data. nit: these function comments and the two below should all be jsdocs, not regular JS comments. https://codereview.chromium.org/2333053002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/power_metric.html:116: * Stores the power data in data into the given histograms nit: more words can fit on the first line https://codereview.chromium.org/2333053002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/power_metric.html:121: * @param {!tr.v.Histogram} timeHist - Histogram to store energy data. nit: these jsdoc parameters have the wrong names https://codereview.chromium.org/2333053002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/power_metric.html:131: * Outputs histograms to the metrics output (the ValueSet). nit: looks like these lines could be compressed some (as in some words from the previous line can go on this line, so forth). If you use emacs, you can use auto-fill mode to do this for you (alt+q). I'm sure vim has a similar crunching ability. https://codereview.chromium.org/2333053002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/power_metric.html:136: * @param {!tr.b.valueSet} values - Value set to output data into. nit: add blank line between main jsdoc and function parameters, especially because the main doc is so long
(obviously, that last one was an lgtm w/ nits)
The CQ bit was checked by alexandermont@chromium.org
https://codereview.chromium.org/2333053002/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2333053002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/power_metric.html:50: On 2016/09/19 at 17:07:14, charliea wrote: > nit: no need for empty line between the comment and the jsdoc of the function the comment applies to Done https://codereview.chromium.org/2333053002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/power_metric.html:52: * Returns the intervals of time between navigation event and time On 2016/09/19 at 17:07:14, charliea wrote: > nit: looks like "to" can be moved to the previous line Done https://codereview.chromium.org/2333053002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/power_metric.html:70: // Creates a histogram suitable for time data. On 2016/09/19 at 17:07:14, charliea wrote: > nit: these function comments and the two below should all be jsdocs, not regular JS comments. Done https://codereview.chromium.org/2333053002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/power_metric.html:116: * Stores the power data in data into the given histograms On 2016/09/19 at 17:07:14, charliea wrote: > nit: more words can fit on the first line Done https://codereview.chromium.org/2333053002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/power_metric.html:121: * @param {!tr.v.Histogram} timeHist - Histogram to store energy data. On 2016/09/19 at 17:07:14, charliea wrote: > nit: these jsdoc parameters have the wrong names Done https://codereview.chromium.org/2333053002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/power_metric.html:131: * Outputs histograms to the metrics output (the ValueSet). On 2016/09/19 at 17:07:14, charliea wrote: > nit: looks like these lines could be compressed some (as in some words from the previous line can go on this line, so forth). If you use emacs, you can use auto-fill mode to do this for you (alt+q). I'm sure vim has a similar crunching ability. Done https://codereview.chromium.org/2333053002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/power_metric.html:136: * @param {!tr.b.valueSet} values - Value set to output data into. On 2016/09/19 at 17:07:14, charliea wrote: > nit: add blank line between main jsdoc and function parameters, especially because the main doc is so long Done
The patchset sent to the CQ was uploaded after l-g-t-m from charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2333053002/#ps120001 (title: "code review changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...) Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2333053002/#ps140001 (title: "code review changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)
Hmmm: looks like this is some sort of infrastructure flake. Going to try running it through the CQ again.
The CQ bit was checked by charliea@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
Alex, looks like you still have some failures in the loading_metric_test that you'll want to check out: https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2333053002/#ps160001 (title: "revert loading metric test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Put all power metrics in one file. Refactors the power metrics code to put all the power metrics in one file. ========== to ========== Put all power metrics in one file. Refactors the power metrics code to put all the power metrics in one file. Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
