Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2871573006:
Gracefully handle floating-point precision errors in the EQT metric. (Closed)
DescriptionGracefully handle floating-point precision errors in the EQT metric.
The following example from a real trace shows that the top-level tasks
can have overlap of about 0.2ms due to precision errors:
task1: start=25851.0181016922 end=25851.0481016922,
task2: start=25851.0251016616 end=34496.0291013717.
This patch bumps up the threshold for task overlap detection in the
EQT metric and adjusts the task endpoints to remove the overlap caused
by precision errors.
Review-Url: https://codereview.chromium.org/2871573006
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/23c9f59f4c6bba6a92e5bcae0a1a3d626db5dee3
Patch Set 1 #
Total comments: 17
Patch Set 2 : Address comments #Patch Set 3 : fix typo #Patch Set 4 : rebase #
Messages
Total messages: 19 (8 generated)
ulan@chromium.org changed reviewers: + tdresser@chromium.org
ptal
Looks good. I just want to confirm our approach to throwing vs recovering all the time, then I'll stamp. https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... File tracing/tracing/extras/chrome/estimated_input_latency.html (right): https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency.html:311: let sortedTasks = tasks.slice().sort((a, b) => a.start - b.start); const (The binding never changes, although the contents are mutated) https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency.html:313: for (let i = 1; i < sortedTasks.length; i++) { If we're cleaning things up, might as well make this range based too. for (const sortedTask of sortedTasks) { } https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency.html:321: const PRECISION_MS = 0.1; Where are we pulling this number from? Should we just always do the adjustment you're doing below? https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency.html:327: let midpoint = (sortedTasks[i - 1].end + sortedTasks[i].start) / 2; const https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency.html:335: let endpoints = []; const https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency.html:349: let slidingWindow = new SlidingWindow( const https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency.html:352: for (let t of endpoints) { const https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... File tracing/tracing/extras/chrome/estimated_input_latency_test.html (right): https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency_test.html:368: // end-points for overlaping. overlapping
Thank you for review. https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... File tracing/tracing/extras/chrome/estimated_input_latency.html (right): https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency.html:311: let sortedTasks = tasks.slice().sort((a, b) => a.start - b.start); On 2017/05/09 12:16:15, tdresser wrote: > const (The binding never changes, although the contents are mutated) Done. https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency.html:313: for (let i = 1; i < sortedTasks.length; i++) { On 2017/05/09 12:16:15, tdresser wrote: > If we're cleaning things up, might as well make this range based too. > > for (const sortedTask of sortedTasks) { > } I need access to the previous task in the loop. https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency.html:321: const PRECISION_MS = 0.1; On 2017/05/09 12:16:15, tdresser wrote: > Where are we pulling this number from? Should we just always do the adjustment > you're doing below? The number is arbitrary based on the observed precision error in the sample in the comment above. I would like some guard here to catch real overlapping inputs. For example, overlap of 1ms and more is likely to be a bug in the caller of the function. https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency.html:327: let midpoint = (sortedTasks[i - 1].end + sortedTasks[i].start) / 2; On 2017/05/09 12:16:15, tdresser wrote: > const Done. https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency.html:335: let endpoints = []; On 2017/05/09 12:16:15, tdresser wrote: > const It is reassigned in line 344 below. https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency.html:349: let slidingWindow = new SlidingWindow( On 2017/05/09 12:16:15, tdresser wrote: > const Done. https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency.html:352: for (let t of endpoints) { On 2017/05/09 12:16:14, tdresser wrote: > const Done.
LGTM https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... File tracing/tracing/extras/chrome/estimated_input_latency.html (right): https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency.html:313: for (let i = 1; i < sortedTasks.length; i++) { On 2017/05/09 17:23:42, ulan wrote: > On 2017/05/09 12:16:15, tdresser wrote: > > If we're cleaning things up, might as well make this range based too. > > > > for (const sortedTask of sortedTasks) { > > } > > I need access to the previous task in the loop. Acknowledged. https://codereview.chromium.org/2871573006/diff/1/tracing/tracing/extras/chro... tracing/tracing/extras/chrome/estimated_input_latency.html:335: let endpoints = []; On 2017/05/09 17:23:42, ulan wrote: > On 2017/05/09 12:16:15, tdresser wrote: > > const > It is reassigned in line 344 below. Acknowledged.
The CQ bit was checked by ulan@google.com
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 Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
ulan@chromium.org changed reviewers: + benjhayden@chromium.org - ulan@google.com
+benjhayden@ for owner's stamp.
On 2017/05/10 at 09:25:55, ulan wrote: > +benjhayden@ for owner's stamp. LGTM Looks like you need to rebase on top of my no-var, prefer-const work?
On 2017/05/10 18:21:30, benjhayden wrote: > On 2017/05/10 at 09:25:55, ulan wrote: > > +benjhayden@ for owner's stamp. > > LGTM > > Looks like you need to rebase on top of my no-var, prefer-const work? Thanks, rebased
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/2871573006/#ps60001 (title: "rebase")
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": 1494498573378080,
"parent_rev": "6adf18ab78fe7f20638206d2ca6ecd3d58efd9c9", "commit_rev":
"23c9f59f4c6bba6a92e5bcae0a1a3d626db5dee3"}
Message was sent while issue was closed.
Description was changed from ========== Gracefully handle floating-point precision errors in the EQT metric. The following example from a real trace shows that the top-level tasks can have overlap of about 0.2ms due to precision errors: task1: start=25851.0181016922 end=25851.0481016922, task2: start=25851.0251016616 end=34496.0291013717. This patch bumps up the threshold for task overlap detection in the EQT metric and adjusts the task endpoints to remove the overlap caused by precision errors. ========== to ========== Gracefully handle floating-point precision errors in the EQT metric. The following example from a real trace shows that the top-level tasks can have overlap of about 0.2ms due to precision errors: task1: start=25851.0181016922 end=25851.0481016922, task2: start=25851.0251016616 end=34496.0291013717. This patch bumps up the threshold for task overlap detection in the EQT metric and adjusts the task endpoints to remove the overlap caused by precision errors. Review-Url: https://codereview.chromium.org/2871573006 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... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
