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

Issue 604007 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

25.7%-50.5% regression in smoothness.key_mobile_sites_smooth at 387206:387253

Project Member Reported by rsch...@chromium.org, Apr 15 2016

Issue description

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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg-M_hpAkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg-OP8qQkM


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

android-nexus5X
android-nexus7v2
Project Member

Comment 2 by 42576172...@developer.gserviceaccount.com, Apr 16 2016

Cc: enne@chromium.org
Owner: enne@chromium.org

=== Auto-CCing suspected CL author enne@chromium.org ===

Hi enne@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 : Hook up ui::Compositor to Display's BeginFrameSource
Author  : enne
Commit description:
  
This hooks up the first SurfaceFactoryClient to the real
BeginFrameSource owned by the OnScreenDisplayClient.  This is done by
adding an OutputSurfaceClient::SetBeginFrameSource method.  As the
SurfaceDisplayOutputSurface is also a SurfaceFactoryClient, it can hand
the real begin frame source directly to ui::Compositor's scheduler.

This allows the removal of some of the browser vsync plumbing, but not
all of it.  Once the renderer compositors have been hooked up to use
this path as well, then this and all of the VSyncObserver code can be
ripped out.

The BeginFrameSource is created by the BrowserCompositorOutputSurface
which updates it based on vsync information that it receives.  This
BeginFrameSource is passed to the Display (via OutputSurfaceClient),
which informs the SurfaceManager that the compositor using
that Display should be driven by that BeginFrameSource.  The
SurfaceManager then informs the SurfaceDisplayOutputSurface about
the BeginFrameSource, which passes it into the single thread proxy's
cc::Scheduler for that ui::Compositor's instance.  Plumbing!

The path from SurfaceManager to SurfaceDisplayOutputSurface was
added in https://codereview.chromium.org/1673783004, but the rest
of the plumbing is new to this patch.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review URL: https://codereview.chromium.org/1821863002

Cr-Commit-Position: refs/heads/master@{#387228}
Commit  : 19c108580b99c469ed192066310e3162bf8c196e
Date    : Thu Apr 14 03:37:18 2016


===== TESTED REVISIONS =====
Revision                Mean Value  Std. Dev.   Num Values  Good?
chromium@387214         15.4908     0.777584    5           good
chromium@387227         15.453      1.549306    5           good
chromium@387228         24.1972     1.108553    5           bad         <-
chromium@387229         23.2974     1.418229    5           bad
chromium@387231         23.6124     1.8817      5           bad
chromium@387234         23.4864     0.364056    5           bad
chromium@387240         23.0484     0.625192    5           bad

Bisect job ran on: android_nexus7_perf_bisect
Bug ID: 604007

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --also-run-disabled-tests scheduler.tough_scheduling_cases
Test Metric: mean_main_thread_scroll_latency/mean_main_thread_scroll_latency
Relative Change: 48.79%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus7_perf_bisect/builds/2913
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9015284077926644080


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=604007

| 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!
enne, can you take a look at this? The bisect looks solid.

Comment 4 by enne@chromium.org, Apr 18 2016

Yup, am planning on it.  :)
Cc: siev...@chromium.org

Comment 6 by enne@chromium.org, Apr 18 2016

Looks like this should be addressed by https://codereview.chromium.org/1879833002/.
Project Member

Comment 7 by bugdroid1@chromium.org, Apr 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6414a4526b77dd087581d8a9c1cec50affc66333

commit 6414a4526b77dd087581d8a9c1cec50affc66333
Author: sievers <sievers@chromium.org>
Date: Fri Apr 22 21:33:20 2016

Android: Browser-side scheduling latency tweaks

    - Emit an immediate 'missed' BeginFrame when an
      BFObserver is added instead of waiting for next vsync
    - Remove unneeded requestRender() (=SetNeedsAnimate()) call

    The former helps both the browser compositor and display
    scheduler (now that it uses the same BeginFrameSource).

    Regarding the browser compositor:
    When the omnibox is hiding (or selection handles are showing)
    we need to browser composite with the correct scroll offset
    for a given renderer frame.

    Without this patch we run:
    ScrollBegin VSync SwapCompositorFrame VSync BrowserComposite

    Now we do:
    ScrollBegin VSync SwapCF MissedBF BrowserComposite VSync

    In the next interval, this works because if nothing changed yet,
    the scheduler will defer the BeginMainFrame until there is
    another change in the browser compositor tree (MODE_LATE).

    Calling SetNeedsAnimate()) from CompositorViewHolder when the
    top control offset changes is unnecessary, since this is a
    renderer-driven animation (causes an invalidation in the browser
    compositor tree), so will already cause the necessary
    UpdateLayerTreeHost(). Requesting SetNeedsAnimate()
    causes an extra BeginMainFrame() (SingleThreadProxy+scheduler
    are not particularly good at tracking these between needs_commit
    and needs_animate) and puts us into high latency mode otherwise.
    Fileed bug  crbug.com/602489  to make this more robust in
    SingleThreadProxy and scheduler.

TBR=dtrainor@chromium.org
BUG= 591725 , 602489 , 602485 , 604007 

Review URL: https://codereview.chromium.org/1879833002

Cr-Commit-Position: refs/heads/master@{#389248}

[modify] https://crrev.com/6414a4526b77dd087581d8a9c1cec50affc66333/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
[modify] https://crrev.com/6414a4526b77dd087581d8a9c1cec50affc66333/content/browser/renderer_host/compositor_impl_android.cc

Not all graphs have recovered, e.g., 

ChromiumPerf/android-nexus6/thread_times.tough_scrolling_cases / thread_total_all_cpu_time_per_frame
Project Member

Comment 9 by sheriffbot@chromium.org, Jun 1 2016

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

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -performance-sheriff Performance-Sheriff
Trying a bisect on ChromiumPerf/android-nexus5X/thread_times.tough_scrolling_cases  to see if it's the same root cause.

=== Auto-CCing suspected CL author enne@chromium.org ===

Hi enne@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 : Hook up ui::Compositor to Display's BeginFrameSource
Author  : enne
Commit description:
  
This hooks up the first SurfaceFactoryClient to the real
BeginFrameSource owned by the OnScreenDisplayClient.  This is done by
adding an OutputSurfaceClient::SetBeginFrameSource method.  As the
SurfaceDisplayOutputSurface is also a SurfaceFactoryClient, it can hand
the real begin frame source directly to ui::Compositor's scheduler.

This allows the removal of some of the browser vsync plumbing, but not
all of it.  Once the renderer compositors have been hooked up to use
this path as well, then this and all of the VSyncObserver code can be
ripped out.

The BeginFrameSource is created by the BrowserCompositorOutputSurface
which updates it based on vsync information that it receives.  This
BeginFrameSource is passed to the Display (via OutputSurfaceClient),
which informs the SurfaceManager that the compositor using
that Display should be driven by that BeginFrameSource.  The
SurfaceManager then informs the SurfaceDisplayOutputSurface about
the BeginFrameSource, which passes it into the single thread proxy's
cc::Scheduler for that ui::Compositor's instance.  Plumbing!

The path from SurfaceManager to SurfaceDisplayOutputSurface was
added in https://codereview.chromium.org/1673783004, but the rest
of the plumbing is new to this patch.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review URL: https://codereview.chromium.org/1821863002

Cr-Commit-Position: refs/heads/master@{#387228}
Commit  : 19c108580b99c469ed192066310e3162bf8c196e
Date    : Thu Apr 14 03:37:18 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev   N  Good?
chromium@387100  34.9485  0.195519  5  good
chromium@387175  35.0593  0.214291  5  good
chromium@387213  35.1264  0.315226  5  good
chromium@387223  34.8132  0.317377  8  good
chromium@387226  35.0077  0.242574  8  good
chromium@387227  34.9177  0.307028  5  good
chromium@387228  35.8307  0.294107  5  bad    <--
chromium@387232  36.1411  0.720547  5  bad
chromium@387250  36.0933  0.519026  5  bad

Bisect job ran on: android_nexus5X_perf_bisect
Bug ID: 604007

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests thread_times.tough_scrolling_cases
Test Metric: thread_total_all_cpu_time_per_frame/thread_total_all_cpu_time_per_frame
Relative Change: 3.28%
Score: 99.8

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus5X_perf_bisect/builds/315
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9007707675812718640


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5848655983017984

| 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!
Project Member

Comment 12 by sheriffbot@chromium.org, Jul 13 2016

Labels: -M-53 MovedFrom-53
This issue has been moved once and is lower than Pri-1. Removing the milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
@enne, there are a few graphs that still haven't recovered. Do you think you could take another look at this? Thanks!
Perf sheriff ping: reminder to follow up on possible performance issues
Cc: vmi...@chromium.org
 Issue 606250  has been merged into this issue.
https://chromeperf.appspot.com/report?sid=61aed11e4b64655e83b7c97940f03e3f8066b852b6b8abe419635cd351f0b54f&start_rev=386450&end_rev=423363

This graph eventually regressed a bit more and recovered somewhat, then "improved" to 0, meaning that it stopped working on Sep 9 and nobody noticed (separate bug).

enne, do you have something specific in mind that you still want to do for this bug? If not, this looks to me like it's as Fixed as it's going to get.

Comment 17 by enne@chromium.org, Oct 6 2016

Status: Fixed (was: Assigned)
Yeah, from my end, I think there's nothing else to be done here.

Sign in to add a comment