Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3001843002:
Add TBMv2 metrics for WebVR. (Closed)
DescriptionAdd TBMv2 metrics for WebVR.
All the metrics for WebVR in this patch depend on trace counter, we will
add metrics depend on trace events later.
BUG=chromium:726906
Review-Url: https://codereview.chromium.org/3001843002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/4db994232ee1547aea6b19f33f2436723ba3221c
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed Ben's comments #
Total comments: 7
Patch Set 3 : Addressed Ben's comments #
Messages
Total messages: 19 (10 generated)
Description was changed from ========== Add TBMv2 metrics for WebVR. All the metrics for WebVR in this patch depend on trace counter, we will add metrics depend on trace events later. ========== to ========== Add TBMv2 metrics for WebVR. All the metrics for WebVR in this patch depend on trace counter, we will add metrics depend on trace events later. ==========
leilei@chromium.org changed reviewers: + benjhayden@chromium.org, nednguyen@chromium.org
Description was changed from ========== Add TBMv2 metrics for WebVR. All the metrics for WebVR in this patch depend on trace counter, we will add metrics depend on trace events later. ========== to ========== Add TBMv2 metrics for WebVR. All the metrics for WebVR in this patch depend on trace counter, we will add metrics depend on trace events later. BUG=726906 ==========
leilei@chromium.org changed reviewers: + nednguyen@google.com
Friendly ping.
Thanks, this looks good and straightforward! Just a few nits. Also, do you have a benchmark with stories for this metric yet? If so, can you run it and share the results.html so we can see how this plays out with real data? If not, can you run the metric over at least one real trace and share the results.html, or vulcanize the trace json into an html file? You can use tracing/bin/run_metric to produce a results.html from a trace, or tracing/bin/trace2html to vulcanize a trace. https://github.com/catapult-project/catapult/blob/master/tracing/bin/README.md https://codereview.chromium.org/3001843002/diff/1/tracing/tracing/metrics/vr/... File tracing/tracing/metrics/vr/webvr_metric.html (right): https://codereview.chromium.org/3001843002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/webvr_metric.html:19: { 'name': 'webvr_fps', Please move the open brace to the previous line. https://codereview.chromium.org/3001843002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/webvr_metric.html:38: const nameToSamples = {}; Please use an ES6 Map instead. https://codereview.chromium.org/3001843002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/webvr_metric.html:39: for (const s of counter.series) { Please rename 's' to 'series' for clarity. https://codereview.chromium.org/3001843002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/webvr_metric.html:46: for (const [serieName, samples] of Object.entries(nameToSamples)) { 'serie' is not a word. 'Series' is both singular and plural.
On 2017/08/23 17:28:06, benjhayden wrote: > Thanks, this looks good and straightforward! > Just a few nits. > Also, do you have a benchmark with stories for this metric yet? > If so, can you run it and share the results.html so we can see how this plays > out with real data? > If not, can you run the metric over at least one real trace and share the > results.html, or vulcanize the trace json into an html file? > You can use tracing/bin/run_metric to produce a results.html from a trace, or > tracing/bin/trace2html to vulcanize a trace. > https://github.com/catapult-project/catapult/blob/master/tracing/bin/README.md > Yes, I have a benchmark with stories for this metric locally, go/webvr-results is the link to the result.html file generated by telemetry. > https://codereview.chromium.org/3001843002/diff/1/tracing/tracing/metrics/vr/... > File tracing/tracing/metrics/vr/webvr_metric.html (right): > > https://codereview.chromium.org/3001843002/diff/1/tracing/tracing/metrics/vr/... > tracing/tracing/metrics/vr/webvr_metric.html:19: { 'name': 'webvr_fps', > Please move the open brace to the previous line. > > https://codereview.chromium.org/3001843002/diff/1/tracing/tracing/metrics/vr/... > tracing/tracing/metrics/vr/webvr_metric.html:38: const nameToSamples = {}; > Please use an ES6 Map instead. > > https://codereview.chromium.org/3001843002/diff/1/tracing/tracing/metrics/vr/... > tracing/tracing/metrics/vr/webvr_metric.html:39: for (const s of counter.series) > { > Please rename 's' to 'series' for clarity. > > https://codereview.chromium.org/3001843002/diff/1/tracing/tracing/metrics/vr/... > tracing/tracing/metrics/vr/webvr_metric.html:46: for (const [serieName, samples] > of Object.entries(nameToSamples)) { > 'serie' is not a word. 'Series' is both singular and plural.
Description was changed from ========== Add TBMv2 metrics for WebVR. All the metrics for WebVR in this patch depend on trace counter, we will add metrics depend on trace events later. BUG=726906 ========== to ========== Add TBMv2 metrics for WebVR. All the metrics for WebVR in this patch depend on trace counter, we will add metrics depend on trace events later. ==========
https://codereview.chromium.org/3001843002/diff/1/tracing/tracing/metrics/vr/... File tracing/tracing/metrics/vr/webvr_metric.html (right): https://codereview.chromium.org/3001843002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/webvr_metric.html:19: { 'name': 'webvr_fps', On 2017/08/23 17:28:06, benjhayden wrote: > Please move the open brace to the previous line. Done. https://codereview.chromium.org/3001843002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/webvr_metric.html:38: const nameToSamples = {}; On 2017/08/23 17:28:06, benjhayden wrote: > Please use an ES6 Map instead. Done. https://codereview.chromium.org/3001843002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/webvr_metric.html:39: for (const s of counter.series) { On 2017/08/23 17:28:06, benjhayden wrote: > Please rename 's' to 'series' for clarity. Done. https://codereview.chromium.org/3001843002/diff/1/tracing/tracing/metrics/vr/... tracing/tracing/metrics/vr/webvr_metric.html:46: for (const [serieName, samples] of Object.entries(nameToSamples)) { On 2017/08/23 17:28:06, benjhayden wrote: > 'serie' is not a word. 'Series' is both singular and plural. Done.
Description was changed from ========== Add TBMv2 metrics for WebVR. All the metrics for WebVR in this patch depend on trace counter, we will add metrics depend on trace events later. ========== to ========== Add TBMv2 metrics for WebVR. All the metrics for WebVR in this patch depend on trace counter, we will add metrics depend on trace events later. BUG=726906 ==========
Thanks! Just 3 final suggestions then lgtm. https://codereview.chromium.org/3001843002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/vr/webvr_metric.html (right): https://codereview.chromium.org/3001843002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/vr/webvr_metric.html:16: // Maps VR trace counters to histogram. Please change this to an ES6 Map. https://codereview.chromium.org/3001843002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/vr/webvr_metric.html:19: 'name': 'webvr_fps', No need to put quotes around dictionary keys that are valid variable names. https://codereview.chromium.org/3001843002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/vr/webvr_metric.html:21: 'options': {'description': 'WebVR frame per second'} The default bin boundaries for the count unit are exponential from 1 to 1000 with 20 bins. I recommend specifying custom bin boundaries in options here that are linear from 20 to 120, with 25 bins: binBoundaries: tr.v.HistogramBinBoundaries.createLinear(20, 120, 25), This will only make it easier to see variation within and between histograms, no other effect.
Description was changed from ========== Add TBMv2 metrics for WebVR. All the metrics for WebVR in this patch depend on trace counter, we will add metrics depend on trace events later. BUG=726906 ========== to ========== Add TBMv2 metrics for WebVR. All the metrics for WebVR in this patch depend on trace counter, we will add metrics depend on trace events later. BUG=chromium:726906 ==========
https://codereview.chromium.org/3001843002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/vr/webvr_metric.html (right): https://codereview.chromium.org/3001843002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/vr/webvr_metric.html:16: // Maps VR trace counters to histogram. On 2017/08/28 05:25:54, benjhayden wrote: > Please change this to an ES6 Map. Done. https://codereview.chromium.org/3001843002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/vr/webvr_metric.html:19: 'name': 'webvr_fps', On 2017/08/28 05:25:54, benjhayden wrote: > No need to put quotes around dictionary keys that are valid variable names. I tried to remove quotes around dictionary keys, however it will be treated as variable name instead of a string, and I got undefined error for the key. https://codereview.chromium.org/3001843002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/vr/webvr_metric.html:21: 'options': {'description': 'WebVR frame per second'} On 2017/08/28 05:25:54, benjhayden wrote: > The default bin boundaries for the count unit are exponential from 1 to 1000 > with 20 bins. > I recommend specifying custom bin boundaries in options here that are linear > from 20 to 120, with 25 bins: > binBoundaries: tr.v.HistogramBinBoundaries.createLinear(20, 120, 25), > This will only make it easier to see variation within and between histograms, no > other effect. Done.
lgtm, thanks! https://codereview.chromium.org/3001843002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/vr/webvr_metric.html (right): https://codereview.chromium.org/3001843002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/vr/webvr_metric.html:19: 'name': 'webvr_fps', On 2017/08/29 at 00:13:39, Lei Lei wrote: > On 2017/08/28 05:25:54, benjhayden wrote: > > No need to put quotes around dictionary keys that are valid variable names. > > I tried to remove quotes around dictionary keys, however it will be treated as variable name instead of a string, and I got undefined error for the key. Sorry, my fault. Dictionary keys don't need to be quoted, but Map keys do.
The CQ bit was checked by leilei@chromium.org
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": 40001, "attempt_start_ts": 1504027266548400,
"parent_rev": "4a8edf340d6a045709e89b326efa07dc07b71c6d", "commit_rev":
"4db994232ee1547aea6b19f33f2436723ba3221c"}
Message was sent while issue was closed.
Description was changed from ========== Add TBMv2 metrics for WebVR. All the metrics for WebVR in this patch depend on trace counter, we will add metrics depend on trace events later. BUG=chromium:726906 ========== to ========== Add TBMv2 metrics for WebVR. All the metrics for WebVR in this patch depend on trace counter, we will add metrics depend on trace events later. BUG=chromium:726906 Review-Url: https://codereview.chromium.org/3001843002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
