Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2696053002:
Make estimatedInputLatency metric more friendly for traces without TTI. (Closed)
DescriptionMake estimatedInputLatency metric more friendly for traces without TTI.
Currently the metric throws an error if the trace does not have TTI
or has multiple TTIs. This makes the metric hard to use since one
usually does not know whether a benchmark has a single TTI or not.
Now instead of throwing the metric skips such traces.
The EIL for interactive time is produced only if the trace has
a single TTI. Once https://crbug.com/692112 is fixed, interactive EIL
will be produced also for traces with multiple TTIs.
Apart from that, the patch adds a new metric that computes
the EIL for the whole timeline.
BUG=chromium:625701
Review-Url: https://codereview.chromium.org/2696053002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/1ebfddc4d6f29a3588fc29b3456921dc83d25cf5
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address Tim's comments #
Total comments: 4
Patch Set 3 : Rewrite JSDoc, fix typo. #Patch Set 4 : Simplify histogram creation #
Total comments: 2
Patch Set 5 : Fix typo #
Total comments: 6
Patch Set 6 : Address Ben's comment #
Messages
Total messages: 31 (14 generated)
Description was changed from ========== Make estimatedInputLatency metric more friendly for traces without TTI. Currently the metric throws an error if the trace does not have TTI or has multiple TTIs. This makes the metric hard to use since one usually does not know whether a benchmark has a single TTI or not. Now instead of throwing an error, the metric will computed the estimated input latency for the whole timeline. The interactive estimated input latency is produced only if the trace has single TTI. BUG=catapult:# ========== to ========== Make estimatedInputLatency metric more friendly for traces without TTI. Currently the metric throws an error if the trace does not have TTI or has multiple TTIs. This makes the metric hard to use since one usually does not know whether a benchmark has a single TTI or not. Now instead of throwing an error, the metric will compute the estimated input latency for the whole timeline. The interactive estimated input latency is produced only if the trace has single TTI. BUG=catapult:# ==========
Description was changed from ========== Make estimatedInputLatency metric more friendly for traces without TTI. Currently the metric throws an error if the trace does not have TTI or has multiple TTIs. This makes the metric hard to use since one usually does not know whether a benchmark has a single TTI or not. Now instead of throwing an error, the metric will compute the estimated input latency for the whole timeline. The interactive estimated input latency is produced only if the trace has single TTI. BUG=catapult:# ========== to ========== Make estimatedInputLatency metric more friendly for traces without TTI. Currently the metric throws an error if the trace does not have TTI or has multiple TTIs. This makes the metric hard to use since one usually does not know whether a benchmark has a single TTI or not. Now the metric computes the EIL for the whole timeline. The EIL for interactive time is produced only if the trace has a single TTI. Once https://crbug.com/692112 is fixed, interactive EIL will be produced also for traces with multiple TTIs. BUG=chromium:625701 ==========
ulan@chromium.org changed reviewers: + nednguyen@google.com, tdresser@chromium.org
PTAL.
The description is a bit confusing. "Now the metric computes the EIL for the whole timeline." Aren't you adding a new metric, not modifying an existing one? https://codereview.chromium.org/2696053002/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2696053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:48: * It must be either 'interactive' or 'total'. Nit: JSdoc isn't supposed to contain whitespace formatting, IIUC. https://codereview.chromium.org/2696053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:50: function createHistogramForEQT_(label) { Should this take a string, or an enum? We should at least have some kind of assertion that the label is valid. https://codereview.chromium.org/2696053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:64: label === 'total' ? '' : ` in ${label} time.`; I find this hard to read. "The maximum EQT ... for a given renderer in interactive time." Perhaps instead of "in interactive" time, this should be "while the page is interactive"? Can we add the window description after, to keep the text in order? Or even just put the raw string explicitly for each case.
Description was changed from ========== Make estimatedInputLatency metric more friendly for traces without TTI. Currently the metric throws an error if the trace does not have TTI or has multiple TTIs. This makes the metric hard to use since one usually does not know whether a benchmark has a single TTI or not. Now the metric computes the EIL for the whole timeline. The EIL for interactive time is produced only if the trace has a single TTI. Once https://crbug.com/692112 is fixed, interactive EIL will be produced also for traces with multiple TTIs. BUG=chromium:625701 ========== to ========== Make estimatedInputLatency metric more friendly for traces without TTI. Currently the metric throws an error if the trace does not have TTI or has multiple TTIs. This makes the metric hard to use since one usually does not know whether a benchmark has a single TTI or not. Now instead of throwing, the metric skips such traces. The EIL for interactive time is produced only if the trace has a single TTI. Once https://crbug.com/692112 is fixed, interactive EIL will be produced also for traces with multiple TTIs. Apart from that, the patch adds a new metric that computes the EIL for the whole timeline. BUG=chromium:625701 ==========
Description was changed from ========== Make estimatedInputLatency metric more friendly for traces without TTI. Currently the metric throws an error if the trace does not have TTI or has multiple TTIs. This makes the metric hard to use since one usually does not know whether a benchmark has a single TTI or not. Now instead of throwing, the metric skips such traces. The EIL for interactive time is produced only if the trace has a single TTI. Once https://crbug.com/692112 is fixed, interactive EIL will be produced also for traces with multiple TTIs. Apart from that, the patch adds a new metric that computes the EIL for the whole timeline. BUG=chromium:625701 ========== to ========== Make estimatedInputLatency metric more friendly for traces without TTI. Currently the metric throws an error if the trace does not have TTI or has multiple TTIs. This makes the metric hard to use since one usually does not know whether a benchmark has a single TTI or not. Now instead of throwing the metric skips such traces. The EIL for interactive time is produced only if the trace has a single TTI. Once https://crbug.com/692112 is fixed, interactive EIL will be produced also for traces with multiple TTIs. Apart from that, the patch adds a new metric that computes the EIL for the whole timeline. BUG=chromium:625701 ==========
Thanks. I updated the CL description. https://codereview.chromium.org/2696053002/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2696053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:48: * It must be either 'interactive' or 'total'. On 2017/02/14 20:03:40, tdresser wrote: > Nit: JSdoc isn't supposed to contain whitespace formatting, IIUC. Other metrics have whitespace in similar cases (e.g. https://cs.chromium.org/chromium/src/third_party/catapult/tracing/tracing/met...) https://codereview.chromium.org/2696053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:50: function createHistogramForEQT_(label) { On 2017/02/14 20:03:40, tdresser wrote: > Should this take a string, or an enum? We should at least have some kind of > assertion that the label is valid. I made the values into constants and added check. https://codereview.chromium.org/2696053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:64: label === 'total' ? '' : ` in ${label} time.`; On 2017/02/14 20:03:41, tdresser wrote: > I find this hard to read. "The maximum EQT ... for a given renderer in > interactive time." > > Perhaps instead of "in interactive" time, this should be "while the page is > interactive"? > > Can we add the window description after, to keep the text in order? > > Or even just put the raw string explicitly for each case. I made the string explicit in each case.
LGTM https://codereview.chromium.org/2696053002/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2696053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:48: * It must be either 'interactive' or 'total'. On 2017/02/14 20:37:29, ulan wrote: > On 2017/02/14 20:03:40, tdresser wrote: > > Nit: JSdoc isn't supposed to contain whitespace formatting, IIUC. > Other metrics have whitespace in similar cases (e.g. > https://cs.chromium.org/chromium/src/third_party/catapult/tracing/tracing/met...) I'll defer to charliea@ on this. The Google JS style guide says "don't rely on whitespace to format JSDoc", and we claim to align with the Google JS style guide. https://codereview.chromium.org/2696053002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2696053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:50: * It must be either 'interactive' or 'total'. Should we expect people to pass the constants, or string literals? https://codereview.chromium.org/2696053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:68: ' for a given renderer while the page is interavive'; interavive -> interactive
tdresser@chromium.org changed reviewers: + charliea@chromium.org
+charliea@, to comment on whitespace nit.
Thanks for adding charliea@. I rewrote the JSDoc to avoid leading whitespaces, but I am still curious whether to indent the multiline param@ description or not. https://codereview.chromium.org/2696053002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2696053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:50: * It must be either 'interactive' or 'total'. On 2017/02/14 21:01:35, tdresser wrote: > Should we expect people to pass the constants, or string literals? I rewrote this part to make clear that constant should be used. Also removed leading whitespaces. https://codereview.chromium.org/2696053002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:68: ' for a given renderer while the page is interavive'; On 2017/02/14 21:01:35, tdresser wrote: > interavive -> interactive Done.
ulan@chromium.org changed reviewers: - nednguyen@google.com
charliea@, besides the JSDoc style issue, may I ask you for owner's stamp?
I simplified histogram creation in the latest patch set. Now createHistogramForEQT_ just accepts histogram name and description.
Thanks, this is better. Still LGTM. https://codereview.chromium.org/2696053002/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2696053002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:48: * It must be either 'interactive' or 'total'. On 2017/02/14 21:01:35, tdresser wrote: > On 2017/02/14 20:37:29, ulan wrote: > > On 2017/02/14 20:03:40, tdresser wrote: > > > Nit: JSdoc isn't supposed to contain whitespace formatting, IIUC. > > Other metrics have whitespace in similar cases (e.g. > > > https://cs.chromium.org/chromium/src/third_party/catapult/tracing/tracing/met...) > > I'll defer to charliea@ on this. > > The Google JS style guide says "don't rely on whitespace to format JSDoc", and > we claim to align with the Google JS style guide. Sorry, this is correct, I missed the note on this in the style guide. https://codereview.chromium.org/2696053002/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2696053002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:48: * @param {string} desctiption Description of the histogram. description
> > The Google JS style guide says "don't rely on whitespace to format JSDoc", and > > we claim to align with the Google JS style guide. > Sorry, this is correct, I missed the note on this in the style guide. I just read the note. Good to know, thank you for checking :) https://codereview.chromium.org/2696053002/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2696053002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:48: * @param {string} desctiption Description of the histogram. On 2017/02/15 15:47:04, tdresser wrote: > description Done.
ulan@chromium.org changed reviewers: + benjhayden@chromium.org
ulan@chromium.org changed reviewers: - benjhayden@chromium.org
Gentle ping for owner's stamp, charliea@. Please lmk if you have a lot in your review queue, I will redirect.
benjhayden@chromium.org changed reviewers: + benjhayden@chromium.org
1 nit and 2 requests for followup CLs then lgtm https://codereview.chromium.org/2696053002/diff/80001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2696053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:46: * Returns a histogram for EQT with the given name and description. You can change this to an @returns tag like @returns {!tr.v.Histogram} and the rest of the information in this line is implied by the function name and parameters. https://codereview.chromium.org/2696053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:88: tr.e.chrome.maxExpectedQueueingTimeInSlidingWindow( If most callers of this function pass a Range.min and a Range.max, then you can modify the function to just take a Range instead in a followup CL. https://codereview.chromium.org/2696053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:112: tr.metrics.MetricRegistry.register(estimatedInputLatency); Can you add "Metric" to the end of the metric name in a followup CL?
Thank you, landing. https://codereview.chromium.org/2696053002/diff/80001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2696053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:46: * Returns a histogram for EQT with the given name and description. On 2017/02/17 19:33:30, benjhayden wrote: > You can change this to an @returns tag like > @returns {!tr.v.Histogram} > and the rest of the information in this line is implied by the function name and > parameters. Done. https://codereview.chromium.org/2696053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:88: tr.e.chrome.maxExpectedQueueingTimeInSlidingWindow( On 2017/02/17 19:33:30, benjhayden wrote: > If most callers of this function pass a Range.min and a Range.max, then you can > modify the function to just take a Range instead in a followup CL. Thanks, will change in a followup. https://codereview.chromium.org/2696053002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:112: tr.metrics.MetricRegistry.register(estimatedInputLatency); On 2017/02/17 19:33:30, benjhayden wrote: > Can you add "Metric" to the end of the metric name in a followup CL? Yep, my another CL already does it. I will land it after landing this CL.
The CQ bit was checked by ulan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, benjhayden@chromium.org Link to the patchset: https://codereview.chromium.org/2696053002/#ps90001 (title: "Address Ben's comment")
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": 90001, "attempt_start_ts": 1487360789343530,
"parent_rev": "5252580c24aa6c4bc322a19c9ad03cd2c94f1382", "commit_rev":
"1ebfddc4d6f29a3588fc29b3456921dc83d25cf5"}
Message was sent while issue was closed.
Description was changed from ========== Make estimatedInputLatency metric more friendly for traces without TTI. Currently the metric throws an error if the trace does not have TTI or has multiple TTIs. This makes the metric hard to use since one usually does not know whether a benchmark has a single TTI or not. Now instead of throwing the metric skips such traces. The EIL for interactive time is produced only if the trace has a single TTI. Once https://crbug.com/692112 is fixed, interactive EIL will be produced also for traces with multiple TTIs. Apart from that, the patch adds a new metric that computes the EIL for the whole timeline. BUG=chromium:625701 ========== to ========== Make estimatedInputLatency metric more friendly for traces without TTI. Currently the metric throws an error if the trace does not have TTI or has multiple TTIs. This makes the metric hard to use since one usually does not know whether a benchmark has a single TTI or not. Now instead of throwing the metric skips such traces. The EIL for interactive time is produced only if the trace has a single TTI. Once https://crbug.com/692112 is fixed, interactive EIL will be produced also for traces with multiple TTIs. Apart from that, the patch adds a new metric that computes the EIL for the whole timeline. BUG=chromium:625701 Review-Url: https://codereview.chromium.org/2696053002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:90001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
Sorry about the delay, ulan. Defer to benjhayden@ and tdresser@ for their stamps.
Message was sent while issue was closed.
On 2017/02/17 20:27:07, charliea wrote: > Sorry about the delay, ulan. Defer to benjhayden@ and tdresser@ for their > stamps. No problem, landed the CL :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
