Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2698003004:
Add a function that computes contribution of selected events to EQT. (Closed)
DescriptionAdd a function that computes contribution of selected events to EQT.
This function will be used later to add metrics that track impact of
Chrome components on EQT. For example, impact of V8 on EQT.
We define the contribution as the maximum expected queueing time in
the sliding time window of size 500ms (WINDOW_SIZE_MS) for the trace
that is modified as follows:
- from each top-level task remove all subevents except the selected
events.
- removing subevents shrinks a task by shifting its end time closer
to the start time. The start time does not change.
BUG=chromium:625701
Review-Url: https://codereview.chromium.org/2698003004
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/0cc3f7ab57ee2fab3930f80fa6c74bab20838728
Patch Set 1 #Patch Set 2 : fix indent #
Total comments: 16
Patch Set 3 : Address comments Tim's comments #
Total comments: 2
Patch Set 4 : Add "Metric" suffix. #Patch Set 5 : Rebase #
Total comments: 2
Patch Set 6 : Fix comment #
Messages
Total messages: 24 (8 generated)
ulan@chromium.org changed reviewers: + benjhayden@chromium.org, tdresser@chromium.org
This the last missing piece for EIL metric. Once this lands we can start tracking EIL impact of various chrome components. This CL depends on https://codereview.chromium.org/2696053002/ tdresser@, please review the algorithm and logic. benjhayden@, please review style for owner's stamp.
Sorry, I know we talked about this approach at a high level, but I'm forgetting the details. There are two possible approach here: • EQT - EQT without type X (The one you take here) • EQT for tasks of type X Isn't the second approach better, at least if we're using a sliding window? If we have a trace with two windows of equal length, one of which is dominated by tasks of type X, the first will report that tasks of type X are irrelevant, but the second won't. https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:126: * latency (EIL). We define the contribution as the difference between the EIL Our naming is pretty inconsistent, but we should be using "Expected Queueing Time" here, not Estimated Input Latency. The long term plan is to have Estimated Input Latency factor in the likelihood of handling events on the compositor thread. I should have commented on this before, at some point we should go back and make sure this naming is consistent.
I'll defer to Tim on the metric design and Charlie on the style issues if he feels differently. https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:155: ' for a given renderer while the page is interactive.' + Mixing plain strings with string formats feels dangerous to me since the ticks look so similar to quotes, though it hasn't been discussed one way or another afaik. https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:160: if (rendererHelper.isChromeTracingUI) continue; Please move the body of this loop to a helper function so that the functions fit on a screen. https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:162: .filter(slice => slice.duration > 0 && !containsForcedGC_(slice)) IIRC, tracing prefers for loops instead of chaining meta-functions like this. https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:170: let shrunkenTasks = tasks.map(task => { Is there a reason to use the block arrow function instead of the simpler arrow function like this? let shrunkenTasks = tasks.map(task => ({ start: task.start, end: task.end - task.shrinkAmount, }));
https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:155: ' for a given renderer while the page is interactive.' + On 2017/02/15 at 19:16:50, benjhayden wrote: > Mixing plain strings with string formats feels dangerous to me since the ticks look so similar to quotes, though it hasn't been discussed one way or another afaik. Nevermind, sorry. :-) Charlie says that eslint might require this.
> There are two possible approach here: > • EQT - EQT without type X (The one you take here) > • EQT for tasks of type X > Isn't the second approach better, at least if we're using a > sliding window? Yes, I like the second approach better. It cannot distinguish - a task of type X appearing inside a long top-level task vs - the same task of type X appearing inside a short top-level task. But that is probably OK. I think we discussed the following two approaches (but I might have misunderstood): • EQT - EQT without type X • take a concrete sliding window that realizes the maximum EQT and compute the duration of type X in that window. In any case, thank you for the suggestion. I implemented it in the latest patch set. https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:126: * latency (EIL). We define the contribution as the difference between the EIL On 2017/02/15 18:39:37, tdresser wrote: > Our naming is pretty inconsistent, but we should be using "Expected Queueing > Time" here, not Estimated Input Latency. The long term plan is to have Estimated > Input Latency factor in the likelihood of handling events on the compositor > thread. > > I should have commented on this before, at some point we should go back and make > sure this naming is consistent. Done. I changed the metric names and all references to "Expected Qeueuing Time". I will rename the file name in a separate CL.
https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:155: ' for a given renderer while the page is interactive.' + On 2017/02/15 19:25:32, benjhayden wrote: > On 2017/02/15 at 19:16:50, benjhayden wrote: > > Mixing plain strings with string formats feels dangerous to me since the ticks > look so similar to quotes, though it hasn't been discussed one way or another > afaik. > > Nevermind, sorry. :-) > Charlie says that eslint might require this. Acknowledged. https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:155: ' for a given renderer while the page is interactive.' + On 2017/02/15 19:16:50, benjhayden wrote: > Mixing plain strings with string formats feels dangerous to me since the ticks > look so similar to quotes, though it hasn't been discussed one way or another > afaik. Acknowledged. https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:160: if (rendererHelper.isChromeTracingUI) continue; On 2017/02/15 19:16:50, benjhayden wrote: > Please move the body of this loop to a helper function so that the functions fit > on a screen. The latest version of the function is much smaller. Does it fit your screen? https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:162: .filter(slice => slice.duration > 0 && !containsForcedGC_(slice)) On 2017/02/15 19:16:50, benjhayden wrote: > IIRC, tracing prefers for loops instead of chaining meta-functions like this. I think that functional style is cleaner, but if you have strong opinion about it, I will use a loop here. https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:170: let shrunkenTasks = tasks.map(task => { On 2017/02/15 19:16:50, benjhayden wrote: > Is there a reason to use the block arrow function instead of the simpler arrow > function like this? > let shrunkenTasks = tasks.map(task => ({ > start: task.start, > end: task.end - task.shrinkAmount, > })); I would also prefer your suggestion, but I get lint error: "168:26 error Expected block statement surrounding arrow body arrow-body-style"
Acks and 1 question, then lgtm. https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:160: if (rendererHelper.isChromeTracingUI) continue; On 2017/02/15 at 20:48:04, ulan wrote: > On 2017/02/15 19:16:50, benjhayden wrote: > > Please move the body of this loop to a helper function so that the functions fit > > on a screen. > The latest version of the function is much smaller. Does it fit your screen? Yes, thanks! https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:162: .filter(slice => slice.duration > 0 && !containsForcedGC_(slice)) On 2017/02/15 at 20:48:04, ulan wrote: > On 2017/02/15 19:16:50, benjhayden wrote: > > IIRC, tracing prefers for loops instead of chaining meta-functions like this. > I think that functional style is cleaner, but if you have strong opinion about it, I will use a loop here. I'm conflicted. I personally also love metafunctions, but I feel like I received strong push-back against them in the past. Maybe these callbacks are simple enough. https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:170: let shrunkenTasks = tasks.map(task => { On 2017/02/15 at 20:48:03, ulan wrote: > On 2017/02/15 19:16:50, benjhayden wrote: > > Is there a reason to use the block arrow function instead of the simpler arrow > > function like this? > > let shrunkenTasks = tasks.map(task => ({ > > start: task.start, > > end: task.end - task.shrinkAmount, > > })); > > I would also prefer your suggestion, but I get lint error: > "168:26 error Expected block statement surrounding arrow body arrow-body-style" Ah, good to know! https://codereview.chromium.org/2698003004/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2698003004/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:203: expectedQueueingTime, Is there a reason the metric name doesn't end in "Metric" like all the others?
Thank you! https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2698003004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:162: .filter(slice => slice.duration > 0 && !containsForcedGC_(slice)) On 2017/02/15 21:20:26, benjhayden wrote: > On 2017/02/15 at 20:48:04, ulan wrote: > > On 2017/02/15 19:16:50, benjhayden wrote: > > > IIRC, tracing prefers for loops instead of chaining meta-functions like > this. > > I think that functional style is cleaner, but if you have strong opinion about > it, I will use a loop here. > > I'm conflicted. I personally also love metafunctions, but I feel like I received > strong push-back against them in the past. > Maybe these callbacks are simple enough. Yep, I also think that the callbacks are simple. Let's keep them unless someone else speaks up. :) https://codereview.chromium.org/2698003004/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2698003004/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:203: expectedQueueingTime, On 2017/02/15 21:20:26, benjhayden wrote: > Is there a reason the metric name doesn't end in "Metric" like all the others? Done.
Rebased to TOT. This CL doesn't have pending dependencies anymore and is ready to land after review.
Description was changed from ========== Add a function that computes contribution of selected events to EIL. This function will be used later to add metrics that track impact of Chrome components on EIL. For example, impact of V8 on EIL. We define the contribution of the selected events to the EIL as the difference between the EIL of the original trace and the EIL of a modified trace where the durations of the selected events are set to zero. Thus it shows how much the EIL could be improved if the selected events would be optimized to run in 0 ms. BUG=chromium:625701 ========== to ========== Add a function that computes contribution of selected events to EQT. This function will be used later to add metrics that track impact of Chrome components on EQT. For example, impact of V8 on EQT. We define the contribution as the maximum expected queueing time in the sliding time window of size 500ms (WINDOW_SIZE_MS) for the trace that is modified as follows: - from each top-level task remove all subevents except the selected events. - removing subevents shrinks a task by shifting its end time closer to the start time. The start time does not change. BUG=chromium:625701 ==========
Tim, can I land this?
LGTM with nit. https://codereview.chromium.org/2698003004/diff/80001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2698003004/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:118: * that satisfy the given predicate. Based on https://github.com/catapult-project/catapult/issues/3239, we should either add an @returns annotation, or remove the extra * causing this to be interpreted as jsdoc.
Thank you. https://codereview.chromium.org/2698003004/diff/80001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/estimated_input_latency_metric.html (right): https://codereview.chromium.org/2698003004/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/estimated_input_latency_metric.html:118: * that satisfy the given predicate. On 2017/02/21 15:12:58, tdresser wrote: > Based on https://github.com/catapult-project/catapult/issues/3239, we should > either add an @returns annotation, or remove the extra * causing this to be > interpreted as jsdoc. Done.
The CQ bit was checked by ulan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benjhayden@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2698003004/#ps100001 (title: "Fix comment")
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 Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...)
The CQ bit was checked by ulan@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": 100001, "attempt_start_ts": 1487759257525040,
"parent_rev": "e56eec86141aea22d87356ea1118a892f03c3e84", "commit_rev":
"0cc3f7ab57ee2fab3930f80fa6c74bab20838728"}
Message was sent while issue was closed.
Description was changed from ========== Add a function that computes contribution of selected events to EQT. This function will be used later to add metrics that track impact of Chrome components on EQT. For example, impact of V8 on EQT. We define the contribution as the maximum expected queueing time in the sliding time window of size 500ms (WINDOW_SIZE_MS) for the trace that is modified as follows: - from each top-level task remove all subevents except the selected events. - removing subevents shrinks a task by shifting its end time closer to the start time. The start time does not change. BUG=chromium:625701 ========== to ========== Add a function that computes contribution of selected events to EQT. This function will be used later to add metrics that track impact of Chrome components on EQT. For example, impact of V8 on EQT. We define the contribution as the maximum expected queueing time in the sliding time window of size 500ms (WINDOW_SIZE_MS) for the trace that is modified as follows: - from each top-level task remove all subevents except the selected events. - removing subevents shrinks a task by shifting its end time closer to the start time. The start time does not change. BUG=chromium:625701 Review-Url: https://codereview.chromium.org/2698003004 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
