Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3010693002:
[vr] Add metric to measure frame cycle times (Closed)
Description[vr] Add metric to measure frame cycle times
BUG=chromium:726906
Review-Url: https://codereview.chromium.org/3010693002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/49460e08b5918b6f67f1ba15cc06df60383d9ba4
Patch Set 1 #
Total comments: 18
Patch Set 2 : Apply review feedback #
Total comments: 2
Patch Set 3 : Apply review feedback #Patch Set 4 : Rebased on ToT #
Messages
Total messages: 24 (11 generated)
tiborg@chromium.org changed reviewers: + bsheedy@chromium.org, leilei@chromium.org
Hi Lei, hi Brian, PTAL. Thanks, Tibor
Description was changed from ========== [vr] Add metric to measure frame cycle times BUG=chromium:726906 ========== to ========== [vr] Add metric to measure frame cycle times BUG=chromium:726906 ==========
tiborg@chromium.org changed reviewers: - bsheedy@chromium.org
lgtm
tiborg@chromium.org changed reviewers: + benjhayden@chromium.org
Hi Ben, Could you please take a look at this CL? Thank you! Cheers, Tibor
What happens when these metrics regress? Do you want to add any Diagnostics like Breakdowns or RelatedEventSets to help diagnose regressions? I can help design diagnostics if you have questions. https://github.com/catapult-project/catapult/blob/master/docs/how-to-write-me... https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... File tracing/tracing/metrics/vr/frame_cycle_duration_metric.html (right): https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric.html:16: const FRAME_CYCLE_EVENTS = new Map([ This would be more readable as code instead of data. It would be cleaner to call Histogram.addSample() directly in the event loop instead of building an intermediate data structure. You can use a helper function to avoid duplicating Histogram names. It's correct to use a Map to map from event titles to histogram instances, but the values have structure so they should be objects (JS dictionaries), which you can destructure. The ES6 Map API is map.get(key), map.set(key, value), for (const [key, value] of map). function createHistograms(histograms, name, options) { return { wall: histograms.createHistogram(name + '_wall', tr.b.Unit.byName.timeDurationInMs_smallerIsBetter, [], options), cpu: histograms.createHistogram(name + '_cpu', tr.b.Unit.byName.timeDurationInMs_smallerIsBetter, [], options), }; } const histogramsByEventTitle = new Map(); histogramsByEventTitle.set('VrShellGl::DrawFrame', createHistograms(histograms, 'draw_frame', {description: '...'})); histogramsByEventTitle.set('VrShellGl::AcquireFram', createHistograms(histograms, 'acquire_frame', {description: '...'})); const {wall: wallHist, cpu: cpuHist} = histogramsByEventTitle.get(event.title); wallHist.addSample(event.duration); cpuHist.addSample(event.cpuDuration); https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric.html:62: for (const event of model.getDescendantEvents()) { Metric performance doesn't matter terribly much when running benchmarks on the waterfall, but users can and do run metrics interactively in trace viewer and telemetry directly. It's also nice to be good stewards of the bots when possible. Instead of iterating over many thousands of events, please consider the process[es], thread[s], and event container[s] that could contain your events. We provide the model helper system to make this a bit easier. The powerMetric demonstrates finding the browser and renderer processes and their main threads: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr... Please let me know if you have any questions about finding your events efficiently. https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric.html:74: FRAME_CYCLE_EVENTS.get(title).get('name') + ' (CPU Duration)', Histogram names should not contain spaces or parens. https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... File tracing/tracing/metrics/vr/frame_cycle_duration_metric_test.html (right): https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric_test.html:15: tr.b.unittest.testSuite(function() { I highly recommend reading go/unit-test-practices https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric_test.html:18: function createSlice(duration, currentTime) { This can be inlined into the sole caller. https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric_test.html:35: durations.forEach(function(duration) { Please use for..of instead of forEach: for (const duration of durations) { } https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric_test.html:45: function createModel(durations) { This can be inlined into the sole caller. https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric_test.html:56: function getValues(name, histograms) { Callers should call getHistogramNamed directly with the full histogram name instead of using this helper function. https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric_test.html:63: function getAvg(durations) { This doesn't look necessary. All callers can be replaced with literals.
Thanks for the review! https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... File tracing/tracing/metrics/vr/frame_cycle_duration_metric.html (right): https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric.html:16: const FRAME_CYCLE_EVENTS = new Map([ On 2017/08/30 18:38:39, benjhayden wrote: > This would be more readable as code instead of data. > It would be cleaner to call Histogram.addSample() directly in the event loop > instead of building an intermediate data structure. > You can use a helper function to avoid duplicating Histogram names. > It's correct to use a Map to map from event titles to histogram instances, but > the values have structure so they should be objects (JS dictionaries), which you > can destructure. > The ES6 Map API is map.get(key), map.set(key, value), for (const [key, value] of > map). > > function createHistograms(histograms, name, options) { > return { > wall: histograms.createHistogram(name + '_wall', > tr.b.Unit.byName.timeDurationInMs_smallerIsBetter, [], options), > cpu: histograms.createHistogram(name + '_cpu', > tr.b.Unit.byName.timeDurationInMs_smallerIsBetter, [], options), > }; > } > > const histogramsByEventTitle = new Map(); > histogramsByEventTitle.set('VrShellGl::DrawFrame', createHistograms(histograms, > 'draw_frame', {description: '...'})); > histogramsByEventTitle.set('VrShellGl::AcquireFram', > createHistograms(histograms, 'acquire_frame', {description: '...'})); > > const {wall: wallHist, cpu: cpuHist} = histogramsByEventTitle.get(event.title); > wallHist.addSample(event.duration); > cpuHist.addSample(event.cpuDuration); Oh, these a great suggestions! Makes it much cleaner. Added. https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric.html:62: for (const event of model.getDescendantEvents()) { On 2017/08/30 18:38:39, benjhayden wrote: > Metric performance doesn't matter terribly much when running benchmarks on the > waterfall, but users can and do run metrics interactively in trace viewer and > telemetry directly. It's also nice to be good stewards of the bots when > possible. > Instead of iterating over many thousands of events, please consider the > process[es], thread[s], and event container[s] that could contain your events. > We provide the model helper system to make this a bit easier. > The powerMetric demonstrates finding the browser and renderer processes and > their main threads: > https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr... > > Please let me know if you have any questions about finding your events > efficiently. That sounds very good! Unfortunately, the tracing events happen on an unnamed thread we started in the browser process. I now iterate through all unnamed threads in the browser process to find the right events. I can try to give the thread a name and reference it by this name in a follow up CL. https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric.html:74: FRAME_CYCLE_EVENTS.get(title).get('name') + ' (CPU Duration)', On 2017/08/30 18:38:39, benjhayden wrote: > Histogram names should not contain spaces or parens. Done. https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... File tracing/tracing/metrics/vr/frame_cycle_duration_metric_test.html (right): https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric_test.html:15: tr.b.unittest.testSuite(function() { On 2017/08/30 18:38:39, benjhayden wrote: > I highly recommend reading go/unit-test-practices Very good read! Would you also suggest splitting the tests for the individual events into separate functions? https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric_test.html:18: function createSlice(duration, currentTime) { On 2017/08/30 18:38:39, benjhayden wrote: > This can be inlined into the sole caller. Done. https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric_test.html:35: durations.forEach(function(duration) { On 2017/08/30 18:38:39, benjhayden wrote: > Please use for..of instead of forEach: > for (const duration of durations) { > } Done. https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric_test.html:45: function createModel(durations) { On 2017/08/30 18:38:39, benjhayden wrote: > This can be inlined into the sole caller. Done. https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric_test.html:56: function getValues(name, histograms) { On 2017/08/30 18:38:39, benjhayden wrote: > Callers should call getHistogramNamed directly with the full histogram name > instead of using this helper function. Done. https://codereview.chromium.org/3010693002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/frame_cycle_duration_metric_test.html:63: function getAvg(durations) { On 2017/08/30 18:38:39, benjhayden wrote: > This doesn't look necessary. All callers can be replaced with literals. Done.
Thanks! One last nit then lgtm. And do let me know if you want to talk about diagnostics at some point. https://codereview.chromium.org/3010693002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/vr/frame_cycle_duration_metric.html (right): https://codereview.chromium.org/3010693002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/vr/frame_cycle_duration_metric.html:53: const glThreads = helper.process.findAllThreadsMatching(function(thread) { You can use an arrow function here thread => !thread.name
Hi Ben The current plan for regression is to set some kind of alert if these metrics become larger than a certain amount of milliseconds. If that happens we would go back to the histogram timeline and check which CL is the most likely culprit. This may be a naive approach and I'm happy if you could give advice on this. Here, I have a general question: how do we map a histogram to a CL, git commit, CQ position? Thank you! Cheers, Tibor https://codereview.chromium.org/3010693002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/vr/frame_cycle_duration_metric.html (right): https://codereview.chromium.org/3010693002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/vr/frame_cycle_duration_metric.html:53: const glThreads = helper.process.findAllThreadsMatching(function(thread) { On 2017/08/30 22:57:48, benjhayden wrote: > You can use an arrow function here > thread => !thread.name Done.
On 2017/09/01 at 15:18:55, tiborg wrote: > Hi Ben > > The current plan for regression is to set some kind of alert if these metrics become larger than a certain amount of milliseconds. If that happens we would go back to the histogram timeline and check which CL is the most likely culprit. This may be a naive approach and I'm happy if you could give advice on this. Here, I have a general question: how do we map a histogram to a CL, git commit, CQ position? > > Thank you! > > Cheers, > Tibor > > https://codereview.chromium.org/3010693002/diff/20001/tracing/tracing/metrics... > File tracing/tracing/metrics/vr/frame_cycle_duration_metric.html (right): > > https://codereview.chromium.org/3010693002/diff/20001/tracing/tracing/metrics... > tracing/tracing/metrics/vr/frame_cycle_duration_metric.html:53: const glThreads = helper.process.findAllThreadsMatching(function(thread) { > On 2017/08/30 22:57:48, benjhayden wrote: > > You can use an arrow function here > > thread => !thread.name > > Done. I scheduled a VC on Tuesday if that works for you.
Thank you, Ben! Will commit for now and will add the Diagnostics in a follow-up CL if necessary.
The CQ bit was checked by tiborg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from leilei@chromium.org, benjhayden@chromium.org Link to the patchset: https://codereview.chromium.org/3010693002/#ps40001 (title: "Apply review feedback")
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 Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...) 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 tiborg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benjhayden@chromium.org, leilei@chromium.org Link to the patchset: https://codereview.chromium.org/3010693002/#ps60001 (title: "Rebased on ToT")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1504298020680470,
"parent_rev": "1cd370fd20bb2d7d7b22d23efe5886f24e068f52", "commit_rev":
"49460e08b5918b6f67f1ba15cc06df60383d9ba4"}
Message was sent while issue was closed.
Description was changed from ========== [vr] Add metric to measure frame cycle times BUG=chromium:726906 ========== to ========== [vr] Add metric to measure frame cycle times BUG=chromium:726906 Review-Url: https://codereview.chromium.org/3010693002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
