New issue
Advanced search Search tips

Issue 646377 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

8.5%-136.2% regression in smoothness.sync_scroll.key_mobile_sites_smooth at 418007:418078

Project Member Reported by kouhei@chromium.org, Sep 13 2016

Issue description

See the link to graphs below.
 
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Sep 14 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 : Remove uses of external begin frame sources
Author  : enne
Commit description:
  
With this change, all output surfaces in the renderer provide begin
frame sources directly rather than via an additional parameter to
the LayerTreeHost.

The functional difference between these two paths is that the external
begin frame source exists for the lifetime of the compositor, whereas
the output surface begin frame source is only provided between
BindToClient / DetachFromClient.  The cc::Scheduler is already robust
to these changes and the browser compositor already uses this logic.

The output surface path is more useful in the browser where the output
surface may change which begin frame source it is providing.  However,
this patch just modifies the renderer to use the same path as the
browser so there's only one way of doing things.

In this patch, any time a CompositorOutputSurface or
SynchronousCompositorOutputSurface would be used, RenderThreadImpl
passes it the CompositorExternalBeginFrameSource that would have been
used as the external begin frame source parameter.

The other cases of output surface creation do not need an external begin
frame source.  Layout tests composite synchronously without a
cc::Scheduler and so do not need one.  ui::OutputSurface for mus
renderers internally creates its own default begin frame source (for now).
Blimp doesn't create an output surface so no longer needs special
case code for an external begin frame source that it also doesn't use.

The external begin frame source setting and plumbing is not cleaned up
in this patch and will be done in a followup.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2299003002
Cr-Commit-Position: refs/heads/master@{#418040}
Commit  : 077ba4863c28cbcb1049c24148aeae09f1dfa646
Date    : Mon Sep 12 21:01:34 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev   N   Good?
chromium@418039  10.4307  1.17512   12  good
chromium@418040  25.5896  0.454617  5   bad    <--
chromium@418041  24.7934  1.07537   5   bad
chromium@418042  23.5301  6.25257   8   bad
chromium@418044  25.3215  0.383454  8   bad
chromium@418049  22.6951  5.93939   8   bad
chromium@418058  22.6756  6.46314   12  bad

Bisect job ran on: android_nexus5X_perf_bisect
Bug ID: 646377

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: first_gesture_scroll_update_latency/Blogger
Relative Change: 85.72%
Score: 99.9

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


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

| 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 4 by bugdroid1@chromium.org, Sep 20 2016

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

commit bfdea7f114aded7866981b8bfe3edd0c75155f12
Author: enne <enne@chromium.org>
Date: Tue Sep 20 01:09:27 2016

cc: Remove estimated parent draw time from renderer

This is a display scheduler only concept now that unified begin frame is
turned on.  Renderers can just use the deadline passed to them from the
top level begin frame source.

This fixes a regression from https://codereview.chromium.org/2299003002
where Android was using the incorrect parent estimated draw time of
none and so thought it had less time for frames than it did.

R=brianderson@chromium.org
BUG= 646377 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2352823002
Cr-Commit-Position: refs/heads/master@{#419617}

[modify] https://crrev.com/bfdea7f114aded7866981b8bfe3edd0c75155f12/cc/scheduler/scheduler.cc
[modify] https://crrev.com/bfdea7f114aded7866981b8bfe3edd0c75155f12/cc/scheduler/scheduler.h
[modify] https://crrev.com/bfdea7f114aded7866981b8bfe3edd0c75155f12/cc/scheduler/scheduler_unittest.cc
[modify] https://crrev.com/bfdea7f114aded7866981b8bfe3edd0c75155f12/cc/test/fake_layer_tree_host_impl_client.h
[modify] https://crrev.com/bfdea7f114aded7866981b8bfe3edd0c75155f12/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/bfdea7f114aded7866981b8bfe3edd0c75155f12/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/bfdea7f114aded7866981b8bfe3edd0c75155f12/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/bfdea7f114aded7866981b8bfe3edd0c75155f12/cc/trees/proxy_impl.cc
[modify] https://crrev.com/bfdea7f114aded7866981b8bfe3edd0c75155f12/cc/trees/proxy_impl.h
[modify] https://crrev.com/bfdea7f114aded7866981b8bfe3edd0c75155f12/cc/trees/single_thread_proxy.cc
[modify] https://crrev.com/bfdea7f114aded7866981b8bfe3edd0c75155f12/cc/trees/single_thread_proxy.h

Comment 5 by enne@chromium.org, Sep 22 2016

Status: Fixed (was: Assigned)

Sign in to add a comment