New issue
Advanced search Search tips

Issue 643251 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

16%-24.1% regression in memory.top_10_mobile_stress at 415482:415528

Project Member Reported by mustaq@chromium.org, Sep 1 2016

Issue description

See the link to graphs below.
 
Cc: ericrk@chromium.org
Owner: ericrk@chromium.org

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

Hi ericrk@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 : Refactor client visibility handling
Author  : ericrk
Commit description:
  
Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, visibility handling now uses ScopedVisibility objects
which will dcheck if visible > not visible updates are not paired.

This change also introduces a new ContextCacheController object which
handles trimming context caches based on visibility. This simplifies
handling in clients of the ContextProvider and removes this logic
from ContextSupport.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2278283003
Cr-Commit-Position: refs/heads/master@{#415491}
Commit  : f6c4fb6b0b77f35f094cb73c49b0bfe22d3f4461
Date    : Tue Aug 30 23:29:09 2016


===== TESTED REVISIONS =====
Revision         Mean      Std Dev  N  Good?
chromium@415481  71242875  6420639  5  good
chromium@415487  71290060  4851116  5  good
chromium@415490  73733243  6050343  5  good
chromium@415491  90772603  3063128  5  bad    <--
chromium@415492  91102904  5820671  5  bad
chromium@415493  93205299  4658978  5  bad
chromium@415505  92418867  4450739  5  bad
chromium@415528  89194496  3437387  5  bad

Bisect job ran on: android_nexus9_perf_bisect
Bug ID: 643251

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests memory.top_10_mobile_stress
Test Metric: background-memory:chrome:all_processes:reported_by_os:gpu_memory:proportional_resident_size_avg/after_http_www_baidu_com_s_word_google
Relative Change: 25.20%
Score: 99.5

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus9_perf_bisect/builds/2080
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9002705132967091328


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

| 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!
ericrk, could you comment on whether the bisect above seems correct to blame your CL as the source of the regression?

Comment 5 by ericrk@chromium.org, Sep 13 2016

Sorry, am OOO until the 19th. It makes sense that my CL might cause memory regressions, but should only do so in cases where we were incorrectly freeing memory before (and thrashing caches). I'll need to run the test and check whether this regression is good - we were clearing things incorrectly before - or if my CL is causing us to keep resources where we shouldn't. I'll prioritize this once I get back.

Comment 6 by ericrk@chromium.org, Sep 20 2016

Status: Started (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 20 2016

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

commit d3bd7f0bacd5e2141ddb9f08335a9157da367b14
Author: ericrk <ericrk@chromium.org>
Date: Tue Sep 20 22:29:01 2016

Flush contexts after LayerTreeHostImpl cleanup

Currently, we do not flush our GL contexts after cleaning up
LayerTreeHostImpl (LTHI) resources. This delays the time it takes for
these deletes to reach the GPU process, and can allow driver caches to
grow unnecessarily.

This CL does a ShallowFlushCHROMIUM after freeing LTHI resources, which
reduces cache growth on some platforms (observed on N9).

R=vmpstr
BUG= 643251 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

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

[modify] https://crrev.com/d3bd7f0bacd5e2141ddb9f08335a9157da367b14/cc/trees/layer_tree_host_impl.cc

Comment 8 by ericrk@chromium.org, Sep 21 2016

Status: Fixed (was: Started)
With the CL in #7, the metric has recovered. Closing out.

Comment 9 by ericrk@chromium.org, Sep 21 2016

Labels: Merge-Request-54
The CL which caused this regression has been merged to 54 - requesting merge for the fix.
Status: Started (was: Fixed)

Comment 11 by dimu@chromium.org, Sep 21 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member

Comment 12 by bugdroid1@chromium.org, Sep 22 2016

Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c850e8a84cb5c6effd3d12ab76d85ac5194e3187

commit c850e8a84cb5c6effd3d12ab76d85ac5194e3187
Author: Eric Karl <ericrk@chromium.org>
Date: Thu Sep 22 19:38:40 2016

Flush contexts after LayerTreeHostImpl cleanup

Currently, we do not flush our GL contexts after cleaning up
LayerTreeHostImpl (LTHI) resources. This delays the time it takes for
these deletes to reach the GPU process, and can allow driver caches to
grow unnecessarily.

This CL does a ShallowFlushCHROMIUM after freeing LTHI resources, which
reduces cache growth on some platforms (observed on N9).

R=vmpstr
BUG= 643251 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2358593002
Cr-Commit-Position: refs/heads/master@{#419874}
(cherry picked from commit d3bd7f0bacd5e2141ddb9f08335a9157da367b14)

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

Cr-Commit-Position: refs/branch-heads/2840@{#494}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/c850e8a84cb5c6effd3d12ab76d85ac5194e3187/cc/trees/layer_tree_host_impl.cc

Status: Fixed (was: Started)
Cc: mustaq@chromium.org
 Issue 643250  has been merged into this issue.
Project Member

Comment 15 by bugdroid1@chromium.org, Oct 27 2016

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

commit c850e8a84cb5c6effd3d12ab76d85ac5194e3187
Author: Eric Karl <ericrk@chromium.org>
Date: Thu Sep 22 19:38:40 2016

Flush contexts after LayerTreeHostImpl cleanup

Currently, we do not flush our GL contexts after cleaning up
LayerTreeHostImpl (LTHI) resources. This delays the time it takes for
these deletes to reach the GPU process, and can allow driver caches to
grow unnecessarily.

This CL does a ShallowFlushCHROMIUM after freeing LTHI resources, which
reduces cache growth on some platforms (observed on N9).

R=vmpstr
BUG= 643251 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2358593002
Cr-Commit-Position: refs/heads/master@{#419874}
(cherry picked from commit d3bd7f0bacd5e2141ddb9f08335a9157da367b14)

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

Cr-Commit-Position: refs/branch-heads/2840@{#494}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/c850e8a84cb5c6effd3d12ab76d85ac5194e3187/cc/trees/layer_tree_host_impl.cc

Sign in to add a comment