Issue metadata
Sign in to add a comment
|
84%-99.1% regression in smoothness.top_25_smooth at 391215:391246 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
May 4 2016
=== Auto-CCing suspected CL author skyostil@chromium.org === Hi skyostil@chromium.org, the bisect results pointed to your CL below as possibly causing a regression. Please have a look at this info and see whether your CL be related. ===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : scheduler: Only prioritize rAF if it is fast Author : skyostil Commit description: To avoid starving other tasks, only prioritize main thread compositing (i.e., requestAnimationFrame) if there is more than 80% idle time left in a frame. This can reduce animation frame rate on sites which can't keep up with the display refresh rate, but will ensure other tasks (e.g., postMessage) aren't starved as a side-effect. BUG= 600385 Review-Url: https://codereview.chromium.org/1914373004 Cr-Commit-Position: refs/heads/master@{#391221} Commit : 6f3d26fe1385dc74f22bf2cf6bddc6830dd00d6c Date : Tue May 03 13:41:56 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@391214 28.9018 5.3555 5 good chromium@391218 25.2015 1.99516 5 good chromium@391220 27.1706 3.64949 5 good chromium@391221 50.1909 2.0702 5 bad <-- chromium@391222 49.1134 6.14449 5 bad chromium@391230 51.2913 4.6802 5 bad chromium@391246 49.1628 1.83725 5 bad Bisect job ran on: mac_hdd_perf_bisect Bug ID: 609118 Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests smoothness.top_25_smooth Test Metric: frame_times/https___mail.google.com_mail_ Relative Change: 70.10% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_hdd_perf_bisect/builds/516 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9013585093445792448 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5897437219127296 | O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq | X | for more information addressing perf regression bugs. For feedback, | / \ | file a bug with component Tests>AutoBisect. Thank you!
,
May 5 2016
Just added this regression to the bug: bot=chromium-rel-mac-hdd TestSuite=smoothness.gpu_rasterization.top_25_smooth Test=mean_input_event_latency https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg1PKWuAoM Note that mail.google.com regression is prominent.
,
May 5 2016
This is an unfortunate side-effect of the linked patch: gmail's combined main thread compositing work (input handling + BeginMainFrame) takes about 20ms on this machine, so it's not safe to run that at a high priority without potentially starving other work. Since this is main thread scrolling, the scheduler doesn't really have a clear view into which work is essential for the use case and which isn't, so it will treat all work as equal priority. One option would be for gmail to switch to using requestIdleCallback for the other work which is competing against input handling. Another option would be for the scheduler to start treating main thread mouse wheel scrolling as a special case and still apply prioritization to it. +tdresser@ as FYI for the input latency regression.
,
May 6 2016
If we wanted to identify the main thread scrolling case, how would we do it? Some kind of heuristic where we look for event handlers which modify scroll offsets? I doubt it's worth the investment, as we'd prefer folks to move away from main thread scrolling, but it might at least be worth discussing.
,
May 6 2016
In this case I think we could just treat main thread wheel events as gestures where we think it's okay to starve things like timers or network traffic to improve smoothness for the gesture. Right now pretty much all main thread handled input causes us to switch to a super conservative mode where we treat all things equally.
,
May 6 2016
Hmmm, that doesn't sound unreasonable. We already treat touch events this way, don't we?
,
May 6 2016
That's correct. We're gonna revamp some of our use case detection some time next week -- we'll keep this on the list as a potential improvement too.
,
May 6 2016
SGTM!
,
May 10 2016
Sami, should we WontFix this bug, and track the work described in #6 elsewhere, or should we leave this open to track it?
,
May 12 2016
Let me look into what's going on and then update the bug.
,
May 13 2016
Fix here: https://codereview.chromium.org/1977863004/ I'll land it against this bug since gmail is a good representation of the use case it is intending to address.
,
May 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/00b626512ee991e03b8482536583c5ddb073c7b8 commit 00b626512ee991e03b8482536583c5ddb073c7b8 Author: skyostil <skyostil@chromium.org> Date: Mon May 16 12:53:51 2016 scheduler: Add main thread gesture as a detected use case Distinguish between a main thread gesture (e.g., a scroll which is handled on the main thread) and main thread custom input handling (e.g., a touchmove listener which prevents scrolling). In the former case we know the gesture type (scrolling), so we can be a little more aggressive about prioritizing input handling and compositing in order to improve frame rate. BUG= 609118 Review-Url: https://codereview.chromium.org/1977863004 Cr-Commit-Position: refs/heads/master@{#393823} [modify] https://crrev.com/00b626512ee991e03b8482536583c5ddb073c7b8/components/scheduler/renderer/renderer_scheduler.cc [modify] https://crrev.com/00b626512ee991e03b8482536583c5ddb073c7b8/components/scheduler/renderer/renderer_scheduler.h [modify] https://crrev.com/00b626512ee991e03b8482536583c5ddb073c7b8/components/scheduler/renderer/renderer_scheduler_impl.cc [modify] https://crrev.com/00b626512ee991e03b8482536583c5ddb073c7b8/components/scheduler/renderer/renderer_scheduler_impl.h [modify] https://crrev.com/00b626512ee991e03b8482536583c5ddb073c7b8/components/scheduler/renderer/renderer_scheduler_impl_unittest.cc
,
May 18 2016
Looks like the regression is gone: https://chromeperf.appspot.com/group_report?bug_id=609118 (you might need to adjust the cl range.) |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by mustaq@chromium.org
, May 4 2016