Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2104303002:
Create a metric that combines v8, memory, and responsiveness metrics. (Closed)
DescriptionCreate a metric that combines v8, memory, and responsiveness metrics.
We need it to port ooronline_tbm to TBMv2.
This patch also changes the frame time discrepancy to be absolute,
so that we are consistent with TBMv1.
BUG=chromium:621035
Patch Set 1 #
Total comments: 3
Patch Set 2 : add todo #Patch Set 3 : revert discrepancy metric change #Messages
Total messages: 13 (5 generated)
Description was changed from ========== Create a metric that combines v8, memory, and responsiveness metrics. This is needed for porting oortonline_tbm to TBMv2 (crbug.com/621035). ========== to ========== Create a metric that combines v8, memory, and responsiveness metrics. This is needed for porting oortonline_tbm to TBMv2. BUG=621035 ==========
Description was changed from ========== Create a metric that combines v8, memory, and responsiveness metrics. This is needed for porting oortonline_tbm to TBMv2. BUG=621035 ========== to ========== Create a metric that combines v8, memory, and responsiveness metrics. We need it to port ooronline_tbm to TBMv2. This patch also changes the frame time discrepancy to be absolute, so that we are consistent with TBMv1. BUG=621035 ==========
ulan@chromium.org changed reviewers: + benjhayden@chromium.org, petrcermak@chromium.org
ptal https://codereview.chromium.org/2104303002/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/responsiveness_metric.html (left): https://codereview.chromium.org/2104303002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/responsiveness_metric.html:52: var absolute = false; Ben, any reason why frame time discrepancy is relative in TBMv2? It is absolute in TBMv1. Is this change OK for existing benchmarks?
LGTM with one comment and assuming Ben is fine with the responsiveness metric modification. Thanks, Petr https://codereview.chromium.org/2104303002/diff/1/tracing/tracing/metrics/v8/... File tracing/tracing/metrics/v8/v8_metrics.html (right): https://codereview.chromium.org/2104303002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/v8_metrics.html:24: function v8AndMemoryAndResponsivenessMetrics(values, model) { Can you please add a TODO to remove this compound metric once support for multiple TBMv2 metics is added (https://groups.google.com/a/chromium.org/forum/#!topic/telemetry-announce/8_8...). Something along the lines of: // TODO(ulan): Remove this uber-metric once metric_runner supports multiple TBMv2 metics (https://github.com/catapult-project/catapult/issues/2430).
Description was changed from ========== Create a metric that combines v8, memory, and responsiveness metrics. We need it to port ooronline_tbm to TBMv2. This patch also changes the frame time discrepancy to be absolute, so that we are consistent with TBMv1. BUG=621035 ========== to ========== Create a metric that combines v8, memory, and responsiveness metrics. We need it to port ooronline_tbm to TBMv2. This patch also changes the frame time discrepancy to be absolute, so that we are consistent with TBMv1. BUG=chromium:621035 ==========
Thanks! https://codereview.chromium.org/2104303002/diff/1/tracing/tracing/metrics/v8/... File tracing/tracing/metrics/v8/v8_metrics.html (right): https://codereview.chromium.org/2104303002/diff/1/tracing/tracing/metrics/v8/... tracing/tracing/metrics/v8/v8_metrics.html:24: function v8AndMemoryAndResponsivenessMetrics(values, model) { On 2016/06/29 20:26:07, petrcermak wrote: > Can you please add a TODO to remove this compound metric once support for > multiple TBMv2 metics is added > (https://groups.google.com/a/chromium.org/forum/#!topic/telemetry-announce/8_8...). > Something along the lines of: > > // TODO(ulan): Remove this uber-metric once metric_runner supports multiple > TBMv2 metics (https://github.com/catapult-project/catapult/issues/2430). Done.
IIRC, the reason that we use relative discrepancy here is because it is bounded and normalized, which makes it easy to add to a histogram, which is the key difference between TBMv1 and TBMv2. Absolute discrepancy is unbounded, so what should the bounds of the histogram be set to? The durations of animations can vary widely from under a second to many many seconds, but we want to be able to distinguish between "relatively" different discrepancies for short animations, so that limits the bin size which correspondingly increases the bin count. Or maybe we don't care about janks longer than 1 second, so maybe appropriate settings for a histogram of absolute discrepancies would be range=[0, 1000] ms, binCount=100? Would it be possible to move the |absolute| flag flip to another CL so we can add a bunch of folks to talk about it without blocking the v8 metrics? What is the timeline on metric_runner supporting multiple metrics? I thought that was RSN.
Also, the DISCREPANCY_NUMERIC_BUILDER's unit would need to change to ms for absolute discrepancy.
eakuefner@chromium.org changed reviewers: + eakuefner@chromium.org
not lgtm This doesn't need to happen after https://codereview.chromium.org/2110683010 is landed; please wait for that and then just combine them in your benchmark.
On 2016/06/30 19:54:33, eakuefner wrote: > not lgtm > > This doesn't need to happen after https://codereview.chromium.org/2110683010 is > landed; please wait for that and then just combine them in your benchmark. Thanks. I will wait. Closing this issue without landing. > Would it be possible to move the |absolute| flag flip to another CL so we can > add a bunch of folks to talk about it without blocking the v8 metrics? Yes. I will upload it in a different CL. > Or maybe we don't care about janks longer than 1 second, so maybe appropriate > settings for a histogram of absolute discrepancies would be range=[0, 1000] ms, > binCount=100? That sounds good to me. Absolute discrepancy has two advantages: 1. Its unit is ms, so it is easier to understand. 2. It is less noisy because we are not adding noise from the total execution time.
Message was sent while issue was closed.
> Yes. I will upload it in a different CL. Done: https://codereview.chromium.org/2113253002/ |
