New issue
Advanced search Search tips

Issue 617427 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

8.1%-730.8% regression in tab_switching.five_blank_pages at 397717:397782

Project Member Reported by sullivan@chromium.org, Jun 5 2016

Issue description

See the link to graphs below.
 
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 : mac: ensure ui::Compositor exists for visible RWHVs
Author  : enne
Commit description:
  
RenderWidgetHostViewMac is careful about creating ui::Compositor and so
currently only does this in Show and SwapCompositorFrame.  However, if
it the RenderWidgetHostView is created while the RenderWidgetHost is
visible, then Show will early out and never create a ui::Compositor.

This causes problems with begin frame scheduling, which wants to have
the Display (which is created as part of creating the ui::Compositor's
output surface currently) own the real begin frame source that drives
everything.  In that world, if no ui::Compositor exists, no frames ever
are sent or swapped.  So, the ui::Compositor needs to be created first.

Also, if the ui::Compositor is correctly created during Show (or the
constructor), then there's no need to ensure that it exists during
swap compositor frame, and so that can be safely removed.

R=asvitkine@chromium.org,ccameron@chromium.org

Review-Url: https://codereview.chromium.org/2005013002
Cr-Commit-Position: refs/heads/master@{#397755}
Commit  : 9af6c23da7070f9423d5cf25856ed27f7d5d8e5e
Date    : Fri Jun 03 18:13:13 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@397752  1207.4   15.9154  5  good
chromium@397754  1207.0   25.632   5  good
chromium@397755  10036.8  79.4084  5  bad    <--
chromium@397756  9877.0   271.48   5  bad
chromium@397760  9985.6   236.139  5  bad

Bisect job ran on: mac_10_10_perf_bisect
Bug ID: 617427

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests tab_switching.tough_energy_cases
Test Metric: idle_wakeups_total/idle_wakeups_total
Relative Change: 727.03%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_10_10_perf_bisect/builds/2130
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9010740138134741536


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

| 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 3 by bugdroid1@chromium.org, Jun 6 2016

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

commit d5d349f0ff52e0a478ae9b807d35c0511e828fa5
Author: enne <enne@chromium.org>
Date: Mon Jun 06 22:30:00 2016

Revert of mac: ensure ui::Compositor exists for visible RWHVs (patchset #12 id:220001 of https://codereview.chromium.org/2005013002/ )

Reason for revert:
Causes blank tabs, tab switching perf tests

BUG= 617497 , 617427 

Original issue's description:
> mac: ensure ui::Compositor exists for visible RWHVs
>
> RenderWidgetHostViewMac is careful about creating ui::Compositor and so
> currently only does this in Show and SwapCompositorFrame.  However, if
> it the RenderWidgetHostView is created while the RenderWidgetHost is
> visible, then Show will early out and never create a ui::Compositor.
>
> This causes problems with begin frame scheduling, which wants to have
> the Display (which is created as part of creating the ui::Compositor's
> output surface currently) own the real begin frame source that drives
> everything.  In that world, if no ui::Compositor exists, no frames ever
> are sent or swapped.  So, the ui::Compositor needs to be created first.
>
> Also, if the ui::Compositor is correctly created during Show (or the
> constructor), then there's no need to ensure that it exists during
> swap compositor frame, and so that can be safely removed.
>
> R=asvitkine@chromium.org,ccameron@chromium.org
>
> Committed: https://crrev.com/9af6c23da7070f9423d5cf25856ed27f7d5d8e5e
> Cr-Commit-Position: refs/heads/master@{#397755}

TBR=phajdan.jr@chromium.org,asvitkine@chromium.org,ccameron@chromium.org,danakj@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.

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

[modify] https://crrev.com/d5d349f0ff52e0a478ae9b807d35c0511e828fa5/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/d5d349f0ff52e0a478ae9b807d35c0511e828fa5/content/browser/renderer_host/render_widget_host_view_mac_editcommand_helper_unittest.mm
[modify] https://crrev.com/d5d349f0ff52e0a478ae9b807d35c0511e828fa5/content/content_tests.gypi
[modify] https://crrev.com/d5d349f0ff52e0a478ae9b807d35c0511e828fa5/content/public/test/test_renderer_host.cc
[modify] https://crrev.com/d5d349f0ff52e0a478ae9b807d35c0511e828fa5/content/test/BUILD.gn
[modify] https://crrev.com/d5d349f0ff52e0a478ae9b807d35c0511e828fa5/ui/accelerated_widget_mac/window_resize_helper_mac.cc
[modify] https://crrev.com/d5d349f0ff52e0a478ae9b807d35c0511e828fa5/ui/accelerated_widget_mac/window_resize_helper_mac.h

Status: Fixed (was: Assigned)
Regression recovered after revert - thanks enne!

Sign in to add a comment