Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1066983002:
Improve hit-testing telemetry test (Closed)
DescriptionImprove hit-testing telemetry test
Wait for 'polymer-ready' event instead of hard-coded 2 seconds. Pay selector
cost once to ensure iterations are only capturing the cost of hit-testing.
Note that this CL is expected to reduce thread times on the dashboard.
BUG=472632
Committed: https://crrev.com/894a75d9fc52dd3280469df6406b3096743e7a09
Cr-Commit-Position: refs/heads/master@{#324313}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 16 (3 generated)
majidvp@chromium.org changed reviewers: + rbyers@chromium.org
LGTM Have you tried running this locally on Android a handful of times with and without your patch? Just wondering how the changes are expected to impact the results (both absolute due to less JS running per tap, and variability due to different waiting behavior). Ideally you'd include a telemetry report showing the results (for a single target device) of ~50 runs with and without this patch.
On 2015/04/07 21:44:24, Rick Byers wrote: > LGTM > > Have you tried running this locally on Android a handful of times with and > without your patch? Just wondering how the changes are expected to impact the > results (both absolute due to less JS running per tap, and variability due to > different waiting behavior). Ideally you'd include a telemetry report showing > the results (for a single target device) of ~50 runs with and without this > patch. Here is the impact of patch on the benchmark on Nexus 5 (50 runs): https://chromium.googlecode.com/issues/attachment?aid=4726320006000&name=resu... AFAICT it does get rid of flakiness.
On 2015/04/08 15:37:05, majidvp wrote: > On 2015/04/07 21:44:24, Rick Byers wrote: > > LGTM > > > > Have you tried running this locally on Android a handful of times with and > > without your patch? Just wondering how the changes are expected to impact the > > results (both absolute due to less JS running per tap, and variability due to > > different waiting behavior). Ideally you'd include a telemetry report showing > > the results (for a single target device) of ~50 runs with and without this > > patch. > > Here is the impact of patch on the benchmark on Nexus 5 (50 runs): > https://chromium.googlecode.com/issues/attachment?aid=4726320006000&name=resu... > > AFAICT it does get rid of flakiness. Awesome, the results look great - thank you! Still LGTM of course
majidvp@chromium.org changed reviewers: + nednguyen@google.com
On 2015/04/08 15:46:51, Rick Byers wrote: > On 2015/04/08 15:37:05, majidvp wrote: > > On 2015/04/07 21:44:24, Rick Byers wrote: > > > LGTM > > > > > > Have you tried running this locally on Android a handful of times with and > > > without your patch? Just wondering how the changes are expected to impact > the > > > results (both absolute due to less JS running per tap, and variability due > to > > > different waiting behavior). Ideally you'd include a telemetry report > showing > > > the results (for a single target device) of ~50 runs with and without this > > > patch. > > > > Here is the impact of patch on the benchmark on Nexus 5 (50 runs): > > > https://chromium.googlecode.com/issues/attachment?aid=4726320006000&name=resu... > > > > AFAICT it does get rid of flakiness. > > Awesome, the results look great - thank you! > > Still LGTM of course +nednguyen for OWNER's review
https://codereview.chromium.org/1066983002/diff/1/tools/perf/page_sets/key_hi... File tools/perf/page_sets/key_hit_test_cases.py (right): https://codereview.chromium.org/1066983002/diff/1/tools/perf/page_sets/key_hi... tools/perf/page_sets/key_hit_test_cases.py:39: 'Action_TapAction') Why do you create an interaction record here?
On 2015/04/08 16:27:26, nednguyen wrote: > https://codereview.chromium.org/1066983002/diff/1/tools/perf/page_sets/key_hi... > File tools/perf/page_sets/key_hit_test_cases.py (right): > > https://codereview.chromium.org/1066983002/diff/1/tools/perf/page_sets/key_hi... > tools/perf/page_sets/key_hit_test_cases.py:39: 'Action_TapAction') > Why do you create an interaction record here? Besides that question, this is lgtm. Please run the perf tryjob ThreadTimesKeyHitTestCases on android & linux and make sure those bots are green before you land.
https://codereview.chromium.org/1066983002/diff/1/tools/perf/page_sets/key_hi... File tools/perf/page_sets/key_hit_test_cases.py (right): https://codereview.chromium.org/1066983002/diff/1/tools/perf/page_sets/key_hi... tools/perf/page_sets/key_hit_test_cases.py:39: 'Action_TapAction') On 2015/04/08 16:27:26, nednguyen wrote: > Why do you create an interaction record here? It was there and I left it as is. Should I remove it?
https://codereview.chromium.org/1066983002/diff/1/tools/perf/page_sets/key_hi... File tools/perf/page_sets/key_hit_test_cases.py (right): https://codereview.chromium.org/1066983002/diff/1/tools/perf/page_sets/key_hi... tools/perf/page_sets/key_hit_test_cases.py:39: 'Action_TapAction') On 2015/04/08 18:32:30, majidvp wrote: > On 2015/04/08 16:27:26, nednguyen wrote: > > Why do you create an interaction record here? > > It was there and I left it as is. Should I remove it? Thanks, I didn't notice that it was here before. Looks like the thread_time benchmark that use this page care about the thread time metrics that happen during the tap. I would leave this here unless the original page author says otherwise.
On 2015/04/08 16:28:32, nednguyen wrote: > On 2015/04/08 16:27:26, nednguyen wrote: > > > https://codereview.chromium.org/1066983002/diff/1/tools/perf/page_sets/key_hi... > > File tools/perf/page_sets/key_hit_test_cases.py (right): > > > > > https://codereview.chromium.org/1066983002/diff/1/tools/perf/page_sets/key_hi... > > tools/perf/page_sets/key_hit_test_cases.py:39: 'Action_TapAction') > > Why do you create an interaction record here? > > Besides that question, this is lgtm. Please run the perf tryjob > ThreadTimesKeyHitTestCases on android & linux and make sure those bots are green > before you land. Green on both linux and android: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus6_p... http://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bisec...
The CQ bit was checked by majidvp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1066983002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/894a75d9fc52dd3280469df6406b3096743e7a09 Cr-Commit-Position: refs/heads/master@{#324313} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
