Issue metadata
Sign in to add a comment
|
17.1% regression in smoothness.sync_scroll.key_mobile_sites_smooth at 397405:397440 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jun 3 2016
=== Auto-CCing suspected CL author alexclarke@chromium.org === Hi alexclarke@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 : Fix a bug with throttling and timer queue suspension Author : alexclarke Commit description: Both the ThrottlingHelper and timer queue suspension (via SuspendTimerQueue/ResumeTimerQueue and SuspendTimerQueueWhenBackgrounded / ResumeTimerQueueWhenForegrounded) call TaskQueue::SetEnabled and it was possible for the ThrottlingHelper to re-enable a suspended queue by mistake. This patch prevents that from happening. BUG= 616052 Review-Url: https://codereview.chromium.org/2028433004 Cr-Commit-Position: refs/heads/master@{#397437} Commit : 22ca5b9bce1453364fdb32979af7175ccf234e15 Date : Thu Jun 02 16:16:41 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@397404 17.3628 0.625985 5 good chromium@397422 17.8173 0.84411 5 good chromium@397431 16.9818 0.137485 5 good chromium@397436 17.4844 0.446388 5 good chromium@397437 20.1416 0.124766 5 bad <-- chromium@397438 20.1985 0.343341 5 bad chromium@397440 20.0538 0.195724 5 bad Bisect job ran on: android_nexus9_perf_bisect Bug ID: 617121 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests smoothness.sync_scroll.key_mobile_sites_smooth Test Metric: frame_times/http___www.deviantart.com_ Relative Change: 15.50% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus9_perf_bisect/builds/1807 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9010877944299339408 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5867369109389312 | 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!
,
Jun 6 2016
I thought the sync scroll tests had been deleted? I kicked off a trace to see what's going on.
,
Jun 24 2016
Looks like the regression is still around. Alex, any luck with the trace?
,
Jul 7 2016
Moving this nonessential bug to the next milestone. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 15 2016
Alex, ping on this. Were you able to get the trace to see what's happening? This test is definitely still reporting data.
,
Jul 19 2016
I wasn't able to tell much from the traces, there's not many categories on. TBH I was under the impression we're not supposed to be monitoring this metric, and in any event it would't be safe to revert that patch. +Tim you're marked as an owner can we close this?
,
Jul 19 2016
The regression is a bit scary. Do you have any reasonable hypothesis as to what could have caused this regression? If you're convinced the change is benign, then feel free to close. It does look a bit fishy to me though.
,
Jul 19 2016
With Primiano's help I was able to run this locally with some extra tracing categories. For most of the scroll both loading and timer tasks are deemed expensive and right at the beginning of the scrolls there's a short period (~280ms) where it's in ExpensiveTaskPolicy::THROTTLE and afterwards ExpensiveTaskPolicy::BLOCK. The patch changes throttling behavior, fixing a bug were throttling wasn't effective for tasks that are on the works queues. Summary of changes: 1. With the patch the scroll seemed shorter (~1500ms vs ~1800ms) 2. A ~185ms loading task gets throttled immediately and doesn't run till much later (after the scrolling). 3. With the patch there are more begin main frames (208 vs 180) and RenderWidgetInputHandler::OnHandleInputEvent(264 vs 241) Why that would regress the benchmark I don't know.
,
Jul 19 2016
If you've got those traces handy, could you upload them?
,
Jul 20 2016
Looks like those got overwritten, but I recorded some new ones this morning (see attached).
,
Jul 20 2016
Tracing reports the average input latency as 25ms for the revert and 20ms for the tip of tree patch. Command used to run the benchmark was: tools/perf/run_benchmark smoothness.sync_scroll.key_mobile_sites_smooth --browser=android-chromium --reset-results --also-run-disabled-tests --output-format=json --story-filter=deviantart
,
Jul 29 2016
I stared at the traces for while, and couldn't come up with anything. Hmph, and we don't have stdio for the runs on the dashboard anymore. I'm wondering if it's just a single long frame, or something like that. In any case, the "after" trace seems fine to me, let's just WontFix. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by rmcilroy@chromium.org
, Jun 3 2016