New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 617121 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Last visit > 30 days ago
Closed: Jul 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

17.1% regression in smoothness.sync_scroll.key_mobile_sites_smooth at 397405:397440

Project Member Reported by rmcilroy@chromium.org, Jun 3 2016

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=617121

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgnKmzuwoM


Bot(s) for this bug's original alert(s):

android-nexus9
Cc: alexclarke@chromium.org
Owner: alexclarke@chromium.org

=== 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!
I thought the sync scroll tests had been deleted?  I kicked off a trace to see what's going on.
Looks like the regression is still around. Alex, any luck with the trace?
Project Member

Comment 5 by sheriffbot@chromium.org, Jul 7 2016

Labels: -M-53 M-54 MovedFrom-53
Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Alex, ping on this. Were you able to get the trace to see what's happening? This test is definitely still reporting data.
Cc: tdres...@chromium.org
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?

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.
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.


If you've got those traces handy, could you upload them?
Looks like those got overwritten, but I recorded some new ones this morning (see attached).
tot.html
4.4 MB View Download
revert.html
4.5 MB View Download
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
Status: WontFix (was: Assigned)
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