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

Issue 883507 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

2.5%-936.3% regression in system_health.memory_desktop at 589898:589923

Project Member Reported by ushesh@chromium.org, Sep 12

Issue description

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

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=44b3f1730efd95d90a6265641310b9e9b9273cabc8be355a9caf3b8c6b73d320


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

mac-10_12_laptop_low_end-perf
mac-10_13_laptop_high_end-perf

system_health.memory_desktop - Benchmark documentation link:
  https://bit.ly/system-health-benchmarks
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/14f296d7640000

Roll src/third_party/skia c55b114a75a0..43b882bceda7 (1 commits) by chromium-autoroll@skia-public.iam.gserviceaccount.com
https://chromium.googlesource.com/chromium/src/+/2e841144a020510228b854b8db796bd4a0de25a2
9.214e+06 → 9.355e+07 (+8.433e+07)

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Benchmark documentation link:
  https://bit.ly/system-health-benchmarks
Owner: bsalomon@chromium.org
The roll has only one commit.
Remove purging of GPU resources based on flush counts.
Owner: bsalo...@google.com
Cc: bsalo...@google.com
Owner: ericrk@chromium.org
Hey Eric, could you point me to a place where I could add a periodic call to GrContext::performDeferredCleanup to free up not-recently-used GPU resources from Skia's cache? We removed the flush-count based auto-purging.
Not sure how heavy weight deferred cleanup is. If running it after each raster task (image upload or tile) is acceptable, I'd recommend adding it here, when num_clients_busy_ is 0:
https://cs.chromium.org/chromium/src/components/viz/common/gpu/context_cache_controller.cc?rcl=7afcf404e94fdb656f79229d1d3593eb08adc5d2&l=111

Additionally, for the new OOP-Raster path, you could add a call here:
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gr_cache_controller.cc?rcl=7afcf404e94fdb656f79229d1d3593eb08adc5d2&l=25

Both of these functions handle a delayed idle cleanup task, but I'd ignore that for the purposes of this change and just run performDeferredCleanup immediately, as we're never guaranteed to become idle, and idle cleanup is for heavier weight things.
Cc: -bsalo...@google.com ericrk@chromium.org
Owner: bsalo...@google.com
Thanks! The cost of this function is basically glDeleteTextures and delete. If it's called regularly the number of texture deletes/C++ deletes should be pretty small.
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 18

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

commit 274eb4f85c39b59a6edc3c5431f93b7b8d2aaf10
Author: Brian Salomon <bsalomon@google.com>
Date: Tue Sep 18 02:41:42 2018

Perform Skia's deferred cleanup in ContextCacheController::ClientBecameBusy

Bug:  883507 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I76f00baa70afd29585f2d4f3a71aa2a05126a4a9
Reviewed-on: https://chromium-review.googlesource.com/1224674
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591922}
[modify] https://crrev.com/274eb4f85c39b59a6edc3c5431f93b7b8d2aaf10/components/viz/common/gpu/context_cache_controller.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Sep 18

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

commit 30012f55014078976cfc728e40569cdd8ef2e51f
Author: Brian Salomon <bsalomon@google.com>
Date: Tue Sep 18 03:07:41 2018

Delete old GrContext resources in ScheduleGrContextCleanup.

Bug:  883507 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I0918e4b87265acc7367f07e3f7b95c4d8d67851c
Reviewed-on: https://chromium-review.googlesource.com/1224676
Reviewed-by: Eric Karl <ericrk@chromium.org>
Commit-Queue: Brian Salomon <bsalomon@google.com>
Cr-Commit-Position: refs/heads/master@{#591926}
[modify] https://crrev.com/30012f55014078976cfc728e40569cdd8ef2e51f/gpu/command_buffer/service/gr_cache_controller.cc

Issue 883811 has been merged into this issue.
📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/15547670e40000
📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/1655a42f640000
Project Member

Comment 17 by bugdroid1@chromium.org, Sep 24

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

commit 2780b2574ea3648e79cc7d8f5d072e830d53aed3
Author: Brian Salomon <bsalomon@google.com>
Date: Mon Sep 24 08:11:02 2018

Reduce time thresholds for purging GrContext resources to 15s.

These were initially set to 30s. However, memory usage on perf bots was
not reduced to the previous values from before the GrContext flush count
mechanism was removed.

Bug:  883507 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: Iccf8c622f1aa3c2592e0f3f147a772878d7f8e72
Reviewed-on: https://chromium-review.googlesource.com/1236175
Reviewed-by: Eric Karl <ericrk@chromium.org>
Commit-Queue: Brian Salomon <bsalomon@google.com>
Cr-Commit-Position: refs/heads/master@{#593489}
[modify] https://crrev.com/2780b2574ea3648e79cc7d8f5d072e830d53aed3/components/viz/common/gpu/context_cache_controller.cc
[modify] https://crrev.com/2780b2574ea3648e79cc7d8f5d072e830d53aed3/gpu/command_buffer/service/gr_cache_controller.cc

Status: Fixed (was: Assigned)
With the new threshold graphs are back at previous levels.

Sign in to add a comment