Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2351963006:
Add range of interest support to cpuTimeMetric. (Closed)
DescriptionAdd range of interest support to cpuTimeMetric.
This CL is mostly clean up. It has several small logic fixes:
* Ignore slices that don't have cpuDuration.
* When there is a rangeOfInterest, calculate the overlap between that and a
slice to determine how much of a slice's cpuDuration counts towards the total.
* Add test for rangeOFInterest path [and fix errors].
BUG=chromium:640312
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/a8deb272b550b60794e1c376f58d6240ddbc5da3
Patch Set 1 #
Total comments: 15
Patch Set 2 : comments from nednguyen and charliea. #
Total comments: 4
Patch Set 3 : threads -> cores #
Total comments: 5
Patch Set 4 : Comments from ben. #
Total comments: 22
Patch Set 5 : Add test, comments, etc. #
Messages
Total messages: 50 (28 generated)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
erikchen@chromium.org changed reviewers: + nednguyen@google.com
nednguyen: Please review.
nednguyen@google.com changed reviewers: + charliea@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/20 22:26:44, nednguyen wrote: ping - nednguyen, can you review given that you were the one that asked to see these changes?
I am patching your CL & found the results of cpuTimeMetric on sfgates.json trace is empty. +Ben, Ethan: is this bug in metrics side panel? https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/cpu_time_metric.html (right): https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/cpu_time_metric.html:44: slice.start,slice.end); nits: space after the comma. +Charlie: can we enable eslint spacing check? https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/cpu_time_metric.html:69: 0.01, 50, 200) Is 50 realistic? That would means around 50 threads, all busy at the same time for the whole second?
https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/cpu_time_metric.html (right): https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/cpu_time_metric.html:44: slice.start,slice.end); Filed as https://github.com/catapult-project/catapult/issues/2836 https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/cpu_time_metric.html:52: threadCpuTime += slice.cpuDuration * multiplier; Why wouldn't this just be: threadCpuTime += intersection.range; Doesn't |intersection| already contain the common interval between |slice| and |rangeOfInterest|? It seems like we can get away without this multiplier. https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/cpu_time_metric.html:68: var boundaries = tr.v.HistogramBinBoundaries.createExponential( Could you please move this up to the top of the file into a constant, along with some rationalization about why the values you chose make for good boundaries? Something like: // Use a lower bound of zero for the metric boundaries (when no CPU time // is consumed) and an upper bound of 50 (fifty threads are all active // for the entire time). var CPU_TIME_PERCENTAGE_BOUNDARIES = tr.v.HistogramBinBoundaries.createExponential( 0.01, 50, 200); Also, wouldn't we want the lower bound of these bins to be zero (something doesn't occupy any CPU time during the range of interest). https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/cpu_time_metric.html:69: 0.01, 50, 200) On 2016/09/21 at 20:27:29, nednguyen wrote: > Is 50 realistic? That would means around 50 threads, all busy at the same time for the whole second? I'd argue that the upper bound *shouldn't* be realistic - it should be something a decent amount above what you'd expect to see. https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/cpu_time_metric.html:70: var cpuTimeNumeric = new tr.v.Histogram('cpu_time_percentage', Please move 'cpu_time_percentage' to next line with other params https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/cpu_time_metric.html:73: 'Percent CPU utilization, normalized against a single core. Can be ' + nit: indent of four spaces (this line and next) https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/cpu_time_metric.html:74: 'greater than 100%'; nit: add trailing . Also, please add explanation of why it can be greater than 100%
https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/cpu_time_metric.html (right): https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/cpu_time_metric.html:44: slice.start,slice.end); On 2016/09/21 20:27:29, nednguyen wrote: > nits: space after the comma. > > +Charlie: can we enable eslint spacing check? Done. https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/cpu_time_metric.html:52: threadCpuTime += slice.cpuDuration * multiplier; On 2016/09/21 20:44:14, charliea wrote: > Why wouldn't this just be: > > threadCpuTime += intersection.range; > > Doesn't |intersection| already contain the common interval between |slice| and > |rangeOfInterest|? It seems like we can get away without this multiplier. that makes the assumption that wall time == cpu time. https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/cpu_time_metric.html:68: var boundaries = tr.v.HistogramBinBoundaries.createExponential( On 2016/09/21 20:44:14, charliea wrote: > Could you please move this up to the top of the file into a constant, along with > some rationalization about why the values you chose make for good boundaries? > Something like: > > // Use a lower bound of zero for the metric boundaries (when no CPU time > // is consumed) and an upper bound of 50 (fifty threads are all active > // for the entire time). > var CPU_TIME_PERCENTAGE_BOUNDARIES = > tr.v.HistogramBinBoundaries.createExponential( > 0.01, 50, 200); Done > > Also, wouldn't we want the lower bound of these bins to be zero (something > doesn't occupy any CPU time during the range of interest). Can't use zero for exponential histogram. https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/cpu_time_metric.html:70: var cpuTimeNumeric = new tr.v.Histogram('cpu_time_percentage', On 2016/09/21 20:44:14, charliea wrote: > Please move 'cpu_time_percentage' to next line with other params Done. https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/cpu_time_metric.html:73: 'Percent CPU utilization, normalized against a single core. Can be ' + On 2016/09/21 20:44:14, charliea wrote: > nit: indent of four spaces (this line and next) Done. https://codereview.chromium.org/2351963006/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/cpu_time_metric.html:74: 'greater than 100%'; On 2016/09/21 20:44:14, charliea wrote: > nit: add trailing . > > Also, please add explanation of why it can be greater than 100% Done.
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
https://codereview.chromium.org/2351963006/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/cpu_time_metric.html (right): https://codereview.chromium.org/2351963006/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:20: tr.v.HistogramBinBoundaries.createExponential(0.01, 50, 200); I still think 50 here mean 50 cores, not 50 threads. If the machine has 1 core, even if it has 100 threads, the total thread time / wall time still <= 1. I would say 16 cores is a better upper bounds. However, it's not the end of the world if the bound is a little loose, so it's up to you whether you want to make the change.
https://codereview.chromium.org/2351963006/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/cpu_time_metric.html (right): https://codereview.chromium.org/2351963006/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:20: tr.v.HistogramBinBoundaries.createExponential(0.01, 50, 200); On 2016/09/21 22:43:25, nednguyen wrote: > I still think 50 here mean 50 cores, not 50 threads. If the machine has 1 core, > even if it has 100 threads, the total thread time / wall time still <= 1. > > I would say 16 cores is a better upper bounds. However, it's not the end of the > world if the bound is a little loose, so it's up to you whether you want to make > the change. My local machine has 24 cores. I imagine in the not too distant future, machines with have a lot more. Again, I don't see the harm in having a larger upper bound.
https://codereview.chromium.org/2351963006/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/cpu_time_metric.html (right): https://codereview.chromium.org/2351963006/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:20: tr.v.HistogramBinBoundaries.createExponential(0.01, 50, 200); On 2016/09/21 22:47:20, erikchen wrote: > On 2016/09/21 22:43:25, nednguyen wrote: > > I still think 50 here mean 50 cores, not 50 threads. If the machine has 1 > core, > > even if it has 100 threads, the total thread time / wall time still <= 1. > > > > I would say 16 cores is a better upper bounds. However, it's not the end of > the > > world if the bound is a little loose, so it's up to you whether you want to > make > > the change. > > My local machine has 24 cores. I imagine in the not too distant future, machines > with have a lot more. Again, I don't see the harm in having a larger upper > bound. Ok. Though can you update the comment to "(fifty cores are all active ...)"?
https://codereview.chromium.org/2351963006/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/cpu_time_metric.html (right): https://codereview.chromium.org/2351963006/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:20: tr.v.HistogramBinBoundaries.createExponential(0.01, 50, 200); On 2016/09/21 22:50:17, nednguyen wrote: > On 2016/09/21 22:47:20, erikchen wrote: > > On 2016/09/21 22:43:25, nednguyen wrote: > > > I still think 50 here mean 50 cores, not 50 threads. If the machine has 1 > > core, > > > even if it has 100 threads, the total thread time / wall time still <= 1. > > > > > > I would say 16 cores is a better upper bounds. However, it's not the end of > > the > > > world if the bound is a little loose, so it's up to you whether you want to > > make > > > the change. > > > > My local machine has 24 cores. I imagine in the not too distant future, > machines > > with have a lot more. Again, I don't see the harm in having a larger upper > > bound. > > Ok. Though can you update the comment to "(fifty cores are all active ...)"? Done.
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
benjhayden@chromium.org changed reviewers: + benjhayden@chromium.org
2 nits then lgtm https://codereview.chromium.org/2351963006/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/cpu_time_metric.html (right): https://codereview.chromium.org/2351963006/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:52: var sliceDuration = slice.end - slice.start; Can't you just use slice.duration? https://codereview.chromium.org/2351963006/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:75: var cpuTimeNumeric = new tr.v.Histogram( Rename to cpuTimeHistogram or cpuTimeHist :-)
@nedn: Unable to repro: https://screenshot.googleplex.com/nvbSLqDEsHr.png
https://codereview.chromium.org/2351963006/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/cpu_time_metric.html (right): https://codereview.chromium.org/2351963006/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:52: var sliceDuration = slice.end - slice.start; On 2016/09/21 23:45:53, benjhayden wrote: > Can't you just use slice.duration? Done. https://codereview.chromium.org/2351963006/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:75: var cpuTimeNumeric = new tr.v.Histogram( On 2016/09/21 23:45:54, benjhayden wrote: > Rename to cpuTimeHistogram or cpuTimeHist :-) Done.
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
On 2016/09/21 23:56:12, benjhayden wrote: > @nedn: Unable to repro: https://screenshot.googleplex.com/nvbSLqDEsHr.png Oh, I just check it on Chrome canary and it works. So it only affects Chrome version 53.0.2785.116 (Mac) :-(
Sorry that I didn't notice this earlier, but the way we are computing cpu metric
of chrome can totally be off due to the fact that the collected trace can be the
combination of multiple trace sources: chrome, telemetry, android systrace,...
I think the code should be updated so that:
1) We only sum the thread time of chrome processes. There is chrome_helper that
would help you with querying only chrome process.
2) Using "model.bounds.range" can be a bit questionable, as we can totally get
into situation like this:
[ Battor trace start ] [ Battor
trace end]
[ Chrome starts] [chrome traces ]... [ chrome end]
Since the model.bounds.range is the range of all combined collected traces, it
can include time outside of the browser, which would render this chrome metric
meaningless. We need to keep track of the time chrome actually starts, end &
divide the cpuDuration by chrome's duration.
Again, I am really sorry that I didn't catch this problem earlier :-(
https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/cpu_time_metric.html (right):
https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/cpu_time_metric.html:68: rangeOfInterest :
model.bounds.range;
this is a bug:
var normalizationRange = rangeOfInterest ? rangeOfInterest.duration :
model.bounds.range;
Can you update the test coverage so that it would catch this bug?
https://codereview.chromium.org/2351963006/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/cpu_time_metric.html (right): https://codereview.chromium.org/2351963006/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:15: // Use a lower bound of zero for the metric boundaries (when no CPU time Isn't it a lower bound of .01? https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/cpu_time_metric.html (right): https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:15: // Use a lower bound of zero for the metric boundaries (when no CPU time Isn't it a lower bound of .01? https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:31: function cpuTimeMetric(values, model, optOptions) { nit: this should be opt_options, not optOptions https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:32: var rangeOfInterest = optOptions ? optOptions.rangeOfInterest : undefined; If you make this: var rangeOfInterest = opt_options ? opt_options.rangeOfInterest : model.bounds; Then I think a lot of the branching in this function drops away. https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:41: thread.sliceGroup.topLevelSlices.forEach(function(slice) { My vote would be to create a temporary range based on the slice's duration: var sliceRange = tr.b.Range.fromExplicitRange(slice.start, slice.end); then below, it would be: if (rangeOfInterest && !rangeOfInterest.findIntersection(sliceRange).isEmpty) return; ... and var intersection = rangeOfInterest.findIntersection(sliceRange); ... multiplier = intersection.duration / slice.duration; https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:48: var multiplier = 1; It took me a little while to figure out what "multiplier" does here: it might be a good idea to give it a better name (it's not very specific). I couldn't come up with anything better than "fractionOfSliceInsideRangeOfInterest". Maybe abbreviate RangeOfInterest to ROI? Not sure. (If you do this, you can probably get rid of the "Calculate..." comment, which would be good too. https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:50: var intersection = rangeOfInterest.findExplicitIntersectionDuration( nit: I'm not a huge fan of the fact that intersection is a scalar but the name definitely sounds like it could be a range. One thing to do would be to make this "intersectionDuration", but that's kind of long. My personal preference is listed above (at function(slice)) https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:52: var sliceDuration = slice.duration; nit: probably isn't necessary to create a temp variable for slice.duration https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:53: if (sliceDuration > 0) { I don't think this is necessary, right? a sliceDuration shouldn't ever be *less* than zero: at lowest, it'll just be zero. Given this, I think that you can do: fractionOfSliceInsideROI = intersection.duration / slice.duration; https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:59: threadCpuTime += slice.cpuDuration * multiplier; It'd probably be a good idea to add a comment about the assumption this relies on: // We assume that, if a slice doesn't lie entirely inside of the range of interest, // then the CPU time is evenly distributed inside of the slice. https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:67: var normalizationRange = rangeOfInterest ? (no longer necessary if you default the range of interest to model.bounds)
Also: could you make the CL title more specific? Maybe something like "Add range of interest support to cpuTimeMetric"
Description was changed from ========== Minor modifications to cpu time metric. If there is a rangeOfInterest, make sure to use a multiplier to only look at CPU usage during that rangeOfInterest. Use percentage as the unit, rather than milliseconds. BUG=chromium:640312 ========== to ========== Minor modifications to cpu time metric. This CL is mostly clean up. It has several small logic fixes: * Ignore slices that don't have cpuDuration. * When there is a rangeOfInterest, calculate the overlap between that and a slice to determine how much of a slice's cpuDuration counts towards the total. * Add test for rangeOFInterest path [and fix errors]. BUG=chromium:640312 ==========
Description was changed from ========== Minor modifications to cpu time metric. This CL is mostly clean up. It has several small logic fixes: * Ignore slices that don't have cpuDuration. * When there is a rangeOfInterest, calculate the overlap between that and a slice to determine how much of a slice's cpuDuration counts towards the total. * Add test for rangeOFInterest path [and fix errors]. BUG=chromium:640312 ========== to ========== Add range of interest support to cpuTimeMetric This CL is mostly clean up. It has several small logic fixes: * Ignore slices that don't have cpuDuration. * When there is a rangeOfInterest, calculate the overlap between that and a slice to determine how much of a slice's cpuDuration counts towards the total. * Add test for rangeOFInterest path [and fix errors]. BUG=chromium:640312 ==========
Description was changed from ========== Add range of interest support to cpuTimeMetric This CL is mostly clean up. It has several small logic fixes: * Ignore slices that don't have cpuDuration. * When there is a rangeOfInterest, calculate the overlap between that and a slice to determine how much of a slice's cpuDuration counts towards the total. * Add test for rangeOFInterest path [and fix errors]. BUG=chromium:640312 ========== to ========== Add range of interest support to cpuTimeMetric. This CL is mostly clean up. It has several small logic fixes: * Ignore slices that don't have cpuDuration. * When there is a rangeOfInterest, calculate the overlap between that and a slice to determine how much of a slice's cpuDuration counts towards the total. * Add test for rangeOFInterest path [and fix errors]. BUG=chromium:640312 ==========
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
All comments have been addressed. nednguyen: I'm going to fix your issue about detecting Chrome processes in a separate CL. I've made a note of this [and another future fix] at https://bugs.chromium.org/p/chromium/issues/detail?id=640312#c12. https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/cpu_time_metric.html (right): https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:15: // Use a lower bound of zero for the metric boundaries (when no CPU time On 2016/09/22 17:54:29, charliea wrote: > Isn't it a lower bound of .01? Done. https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:31: function cpuTimeMetric(values, model, optOptions) { On 2016/09/22 17:54:29, charliea wrote: > nit: this should be opt_options, not optOptions Done. https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:32: var rangeOfInterest = optOptions ? optOptions.rangeOfInterest : undefined; On 2016/09/22 17:54:29, charliea wrote: > If you make this: > > var rangeOfInterest = opt_options ? opt_options.rangeOfInterest : model.bounds; > > Then I think a lot of the branching in this function drops away. Done. https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:41: thread.sliceGroup.topLevelSlices.forEach(function(slice) { On 2016/09/22 17:54:29, charliea wrote: > My vote would be to create a temporary range based on the slice's duration: > > var sliceRange = tr.b.Range.fromExplicitRange(slice.start, slice.end); > > then below, it would be: > > if (rangeOfInterest && > !rangeOfInterest.findIntersection(sliceRange).isEmpty) > return; > > ... and > > var intersection = rangeOfInterest.findIntersection(sliceRange); > ... > multiplier = intersection.duration / slice.duration; Done. https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:48: var multiplier = 1; On 2016/09/22 17:54:29, charliea wrote: > It took me a little while to figure out what "multiplier" does here: it might be > a good idea to give it a better name (it's not very specific). I couldn't come > up with anything better than "fractionOfSliceInsideRangeOfInterest". Maybe > abbreviate RangeOfInterest to ROI? Not sure. > > (If you do this, you can probably get rid of the "Calculate..." comment, which > would be good too. Went with fractionOfSliceInsideRangeOfInterest https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:50: var intersection = rangeOfInterest.findExplicitIntersectionDuration( On 2016/09/22 17:54:28, charliea wrote: > nit: I'm not a huge fan of the fact that intersection is a scalar but the name > definitely sounds like it could be a range. One thing to do would be to make > this "intersectionDuration", but that's kind of long. My personal preference is > listed above (at function(slice)) Done. https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:52: var sliceDuration = slice.duration; On 2016/09/22 17:54:29, charliea wrote: > nit: probably isn't necessary to create a temp variable for slice.duration Done. https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:53: if (sliceDuration > 0) { On 2016/09/22 17:54:29, charliea wrote: > I don't think this is necessary, right? a sliceDuration shouldn't ever be *less* > than zero: at lowest, it'll just be zero. Given this, I think that you can do: > > fractionOfSliceInsideROI = intersection.duration / slice.duration; It could be equal to zero. https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:59: threadCpuTime += slice.cpuDuration * multiplier; On 2016/09/22 17:54:29, charliea wrote: > It'd probably be a good idea to add a comment about the assumption this relies > on: > > // We assume that, if a slice doesn't lie entirely inside of the range of > interest, > // then the CPU time is evenly distributed inside of the slice. Done. https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:67: var normalizationRange = rangeOfInterest ? On 2016/09/22 17:54:28, charliea wrote: > (no longer necessary if you default the range of interest to model.bounds) Done. https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/cpu_time_metric.html:68: rangeOfInterest : model.bounds.range; On 2016/09/22 01:36:09, nednguyen wrote: > this is a bug: > > var normalizationRange = rangeOfInterest ? rangeOfInterest.duration : > model.bounds.range; > > > Can you update the test coverage so that it would catch this bug? > Done.
On 2016/09/22 21:38:10, erikchen wrote: > All comments have been addressed. > > nednguyen: I'm going to fix your issue about detecting Chrome processes in a > separate CL. I've made a note of this [and another future fix] at > https://bugs.chromium.org/p/chromium/issues/detail?id=640312#c12. > > https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... > File tracing/tracing/metrics/system_health/cpu_time_metric.html (right): > > https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... > tracing/tracing/metrics/system_health/cpu_time_metric.html:15: // Use a lower > bound of zero for the metric boundaries (when no CPU time > On 2016/09/22 17:54:29, charliea wrote: > > Isn't it a lower bound of .01? > > Done. > > https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... > tracing/tracing/metrics/system_health/cpu_time_metric.html:31: function > cpuTimeMetric(values, model, optOptions) { > On 2016/09/22 17:54:29, charliea wrote: > > nit: this should be opt_options, not optOptions > > Done. > > https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... > tracing/tracing/metrics/system_health/cpu_time_metric.html:32: var > rangeOfInterest = optOptions ? optOptions.rangeOfInterest : undefined; > On 2016/09/22 17:54:29, charliea wrote: > > If you make this: > > > > var rangeOfInterest = opt_options ? opt_options.rangeOfInterest : > model.bounds; > > > > Then I think a lot of the branching in this function drops away. > > Done. > > https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... > tracing/tracing/metrics/system_health/cpu_time_metric.html:41: > thread.sliceGroup.topLevelSlices.forEach(function(slice) { > On 2016/09/22 17:54:29, charliea wrote: > > My vote would be to create a temporary range based on the slice's duration: > > > > var sliceRange = tr.b.Range.fromExplicitRange(slice.start, slice.end); > > > > then below, it would be: > > > > if (rangeOfInterest && > > !rangeOfInterest.findIntersection(sliceRange).isEmpty) > > return; > > > > ... and > > > > var intersection = rangeOfInterest.findIntersection(sliceRange); > > ... > > multiplier = intersection.duration / slice.duration; > > Done. > > https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... > tracing/tracing/metrics/system_health/cpu_time_metric.html:48: var multiplier = > 1; > On 2016/09/22 17:54:29, charliea wrote: > > It took me a little while to figure out what "multiplier" does here: it might > be > > a good idea to give it a better name (it's not very specific). I couldn't come > > up with anything better than "fractionOfSliceInsideRangeOfInterest". Maybe > > abbreviate RangeOfInterest to ROI? Not sure. > > > > (If you do this, you can probably get rid of the "Calculate..." comment, which > > would be good too. > > Went with fractionOfSliceInsideRangeOfInterest > > https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... > tracing/tracing/metrics/system_health/cpu_time_metric.html:50: var intersection > = rangeOfInterest.findExplicitIntersectionDuration( > On 2016/09/22 17:54:28, charliea wrote: > > nit: I'm not a huge fan of the fact that intersection is a scalar but the name > > definitely sounds like it could be a range. One thing to do would be to make > > this "intersectionDuration", but that's kind of long. My personal preference > is > > listed above (at function(slice)) > > Done. > > https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... > tracing/tracing/metrics/system_health/cpu_time_metric.html:52: var sliceDuration > = slice.duration; > On 2016/09/22 17:54:29, charliea wrote: > > nit: probably isn't necessary to create a temp variable for slice.duration > > Done. > > https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... > tracing/tracing/metrics/system_health/cpu_time_metric.html:53: if (sliceDuration > > 0) { > On 2016/09/22 17:54:29, charliea wrote: > > I don't think this is necessary, right? a sliceDuration shouldn't ever be > *less* > > than zero: at lowest, it'll just be zero. Given this, I think that you can do: > > > > fractionOfSliceInsideROI = intersection.duration / slice.duration; > > It could be equal to zero. > > https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... > tracing/tracing/metrics/system_health/cpu_time_metric.html:59: threadCpuTime += > slice.cpuDuration * multiplier; > On 2016/09/22 17:54:29, charliea wrote: > > It'd probably be a good idea to add a comment about the assumption this relies > > on: > > > > // We assume that, if a slice doesn't lie entirely inside of the range of > > interest, > > // then the CPU time is evenly distributed inside of the slice. > > Done. > > https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... > tracing/tracing/metrics/system_health/cpu_time_metric.html:67: var > normalizationRange = rangeOfInterest ? > On 2016/09/22 17:54:28, charliea wrote: > > (no longer necessary if you default the range of interest to model.bounds) > > Done. > > https://codereview.chromium.org/2351963006/diff/60001/tracing/tracing/metrics... > tracing/tracing/metrics/system_health/cpu_time_metric.html:68: rangeOfInterest : > model.bounds.range; > On 2016/09/22 01:36:09, nednguyen wrote: > > this is a bug: > > > > var normalizationRange = rangeOfInterest ? rangeOfInterest.duration : > > model.bounds.range; > > > > > > Can you update the test coverage so that it would catch this bug? > > > > Done. lgtm, I agree that could be a substantial fix & should be done separately
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benjhayden@chromium.org Link to the patchset: https://codereview.chromium.org/2351963006/#ps80001 (title: "Add test, comments, etc.")
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 ========== Add range of interest support to cpuTimeMetric. This CL is mostly clean up. It has several small logic fixes: * Ignore slices that don't have cpuDuration. * When there is a rangeOfInterest, calculate the overlap between that and a slice to determine how much of a slice's cpuDuration counts towards the total. * Add test for rangeOFInterest path [and fix errors]. BUG=chromium:640312 ========== to ========== Add range of interest support to cpuTimeMetric. This CL is mostly clean up. It has several small logic fixes: * Ignore slices that don't have cpuDuration. * When there is a rangeOfInterest, calculate the overlap between that and a slice to determine how much of a slice's cpuDuration counts towards the total. * Add test for rangeOFInterest path [and fix errors]. BUG=chromium:640312 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
Description was changed from ========== Add range of interest support to cpuTimeMetric. This CL is mostly clean up. It has several small logic fixes: * Ignore slices that don't have cpuDuration. * When there is a rangeOfInterest, calculate the overlap between that and a slice to determine how much of a slice's cpuDuration counts towards the total. * Add test for rangeOFInterest path [and fix errors]. BUG=chromium:640312 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ========== to ========== Add range of interest support to cpuTimeMetric. This CL is mostly clean up. It has several small logic fixes: * Ignore slices that don't have cpuDuration. * When there is a rangeOfInterest, calculate the overlap between that and a slice to determine how much of a slice's cpuDuration counts towards the total. * Add test for rangeOFInterest path [and fix errors]. BUG=chromium:640312 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
