New issue
Advanced search Search tips

Issue 646361 link

Starred by 1 user
Status: WontFix
Owner:
sa...@chromium.org
Closed: Oct 2016
Cc:
sunn...@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

13.3%-89.1% regression in smoothness.top_25_smooth at 417901:417946

Project Member Reported by kouhei@chromium.org, Sep 13 2016 
See the link to graphs below.
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Sep 13 2016 

Bisect failed: http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_10_11_perf_bisect/builds/895
Failure reason: the build has failed.
Additional errors:
The metric was not found in the test output.
Either of the initial "good" or "bad" revisions failed to be tested or built.
Project Member

Comment 6 by 42576172...@developer.gserviceaccount.com, Sep 24 2016 


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Revert of cc: Remove frame queuing from the scheduler. (patchset #3 id:40001 of https://codereview.chromium.org/2323063004/ )
Author  : sammc
Commit description:
  
Reason for revert:
Broke ChromeScreenshotGrabberTest.TakeScreenshot on  Linux ChromiumOS Tests (dbg)(1): https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/18015.

Original issue's description:
> cc: Remove frame queuing from the scheduler.
>
> CC scheduler has a frame queuing mechanism called "retro frames". This
> has been responsible for a lot of complexity and hard to fix bugs. The
> original intent for adding retro frames was to allow the scheduler to
> handle multiple frames in flight but that goal doesn't seem feasible or
> even desirable any more. This CL removes the retro frames queue and
> instead makes the Scheduler end the previous frame when it receives a
> BeginFrame message.
>
> One surprising behavior was that SyntheticBFS MISSED frames would be
> queued as retro frames and this would convert the synchronous begin
> frame call (inside Scheduler::ProcessScheduledActions) to an
> asynchronous retro frame PostTask. To work around this the Scheduler
> keeps track of a single CancelableClosure that's used for MISSED frames.
>
> The above behavior was also causing the BeginFramesNotFromClient tests
> to work even though there was an extra MISSED frame in the queue. This
> is more elegantly solved in another way by using frame number to advance
> the task runner instead of just running pending tasks.
>
> Lastly SchedulerStateMachine is modified so that it's possible to end
> the previous frame and still have the same behavior as before in the
> commit to active tree (browser compositor) mode.
>
> R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org
> BUG= 602485 ,  644992 
> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
>
> Committed: https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78
> Cr-Commit-Position: refs/heads/master@{#417764}

TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sunnyps@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 602485 ,  644992 

Review-Url: https://codereview.chromium.org/2336493002
Cr-Commit-Position: refs/heads/master@{#417895}
Commit  : 95beb47e50065959ee2f5b43cf431431e32e14cd
Date    : Mon Sep 12 08:37:32 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@417893  141.394  1.08489  5  good
chromium@417894  141.837  1.54837  5  good
chromium@417895  401.254  354.384  5  bad    <--
chromium@417897  257.51   20.4309  5  bad
chromium@417900  255.49   24.1995  5  bad
chromium@417906  257.327  19.2196  5  bad
chromium@417919  253.58   21.8262  5  bad

Bisect job ran on: mac_10_11_perf_bisect
Bug ID: 646361

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests page_cycler_v2_site_isolation.basic_oopif
Test Metric: timeToFirstContentfulPaint_avg/pcv1-warm/http___booking.com
Relative Change: 79.34%
Score: 80.0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_10_11_perf_bisect/builds/921
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9000692089069661600


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

| 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 7 by 42576172...@developer.gserviceaccount.com, Sep 24 2016 


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Revert of cc: Remove frame queuing from the scheduler. (patchset #3 id:40001 of https://codereview.chromium.org/2323063004/ )
Author  : sammc
Commit description:
  
Reason for revert:
Broke ChromeScreenshotGrabberTest.TakeScreenshot on  Linux ChromiumOS Tests (dbg)(1): https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/18015.

Original issue's description:
> cc: Remove frame queuing from the scheduler.
>
> CC scheduler has a frame queuing mechanism called "retro frames". This
> has been responsible for a lot of complexity and hard to fix bugs. The
> original intent for adding retro frames was to allow the scheduler to
> handle multiple frames in flight but that goal doesn't seem feasible or
> even desirable any more. This CL removes the retro frames queue and
> instead makes the Scheduler end the previous frame when it receives a
> BeginFrame message.
>
> One surprising behavior was that SyntheticBFS MISSED frames would be
> queued as retro frames and this would convert the synchronous begin
> frame call (inside Scheduler::ProcessScheduledActions) to an
> asynchronous retro frame PostTask. To work around this the Scheduler
> keeps track of a single CancelableClosure that's used for MISSED frames.
>
> The above behavior was also causing the BeginFramesNotFromClient tests
> to work even though there was an extra MISSED frame in the queue. This
> is more elegantly solved in another way by using frame number to advance
> the task runner instead of just running pending tasks.
>
> Lastly SchedulerStateMachine is modified so that it's possible to end
> the previous frame and still have the same behavior as before in the
> commit to active tree (browser compositor) mode.
>
> R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org
> BUG= 602485 ,  644992 
> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
>
> Committed: https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78
> Cr-Commit-Position: refs/heads/master@{#417764}

TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sunnyps@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 602485 ,  644992 

Review-Url: https://codereview.chromium.org/2336493002
Cr-Commit-Position: refs/heads/master@{#417895}
Commit  : 95beb47e50065959ee2f5b43cf431431e32e14cd
Date    : Mon Sep 12 08:37:32 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N   Good?
chromium@417888  105.799  5.54265  8   good
chromium@417892  105.761  5.07565  12  good
chromium@417894  108.423  5.4094   12  good
chromium@417895  121.787  9.58271  8   bad    <--
chromium@417901  126.581  9.84677  8   bad
chromium@417913  131.357  5.12315  8   bad

Bisect job ran on: mac_retina_perf_bisect
Bug ID: 646361

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests page_cycler_v2.intl_hi_ru
Test Metric: timeToFirstContentfulPaint_avg/pcv1-cold/http___narod.yandex.ru_
Relative Change: 21.50%
Score: 99.8

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_retina_perf_bisect/builds/1681
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9000692103680025632


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

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

Comment 8 by nedngu...@google.com, Oct 5 2016

Owner: sa...@chromium.org
sammc@, can you please check whether your revert cause the regression?

Comment 9 by sa...@chromium.org, Oct 6 2016

Cc: sunn...@chromium.org
Status: WontFix (was: Assigned)
It does appear that the revert regressed performance back to where it was prior to the original CL landing.
►

Sign in to add a comment