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

Issue 814219 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

3.2%-13.3% regression in system_health.memory_mobile at 537300:537429

Project Member Reported by alexclarke@chromium.org, Feb 21 2018

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Feb 21 2018

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=814219

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


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

android-webview-nexus5X
android-webview-nexus6
chromium-rel-mac12
Project Member

Comment 2 by 42576172...@developer.gserviceaccount.com, Feb 21 2018

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/13b98af7840000
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Feb 22 2018

Cc: chrishtr@chromium.org asvitk...@chromium.org dalecur...@chromium.org piman@chromium.org enne@chromium.org khushals...@chromium.org
Owner: khushals...@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/13b98af7840000

cc: Serialize bitmap backed images for OOP raster. by khushalsagar@chromium.org
https://chromium.googlesource.com/chromium/src/+/73760a33887fc8333fe071dd0e5e8c17891e9518

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions
 Issue 814217  has been merged into this issue.
Issue 814008 has been merged into this issue.
Cc: briander...@chromium.org
 Issue 813987  has been merged into this issue.
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, Feb 23 2018

📍 Pinpoint job started.
https://chromeperf.appspot.com/job/14d22c20440000
https://chromium-review.googlesource.com/c/chromium/src/+/932942 addresses the smoothness regression and I suspect will fix the memory regressions too. It was causing us to re-upload and cache copies of the same image.
Project Member

Comment 9 by 42576172...@developer.gserviceaccount.com, Feb 23 2018

📍 Found a significant difference after 1 commit.
https://chromeperf.appspot.com/job/14d22c20440000

canvas: Ensure stable content ids for canvas image snapshots. by khushalsagar@chromium.org
https://chromium-review.googlesource.com/c/chromium/src/+/932942/4

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions
Issue 814025 has been merged into this issue.
Project Member

Comment 11 by bugdroid1@chromium.org, Feb 26 2018

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

commit 637c7485329228146d8af0555f8473cc9dc21e5b
Author: Khushal <khushalsagar@chromium.org>
Date: Mon Feb 26 21:54:16 2018

cc: Ensure correct scaling for non-lazy images in GPU cache.

When using a non-lazy (bitmap) image in the GPU cache, we assume the
upload to be scaled at the mip-level required for draw, when we
actually use the original image for the upload. Fix this by considering
non-lazy scaled images as lazy generated. This also ensures we use
lower gpu memory, by caching downscaled uploads.

R=ericrk@chromium.org

Bug:  815045 ,  814219 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I4b7d3f4edbcf0e5ba5574512f662a3fdf53cabd6
Reviewed-on: https://chromium-review.googlesource.com/935441
Reviewed-by: Zhenyao Mo <zmo@chromium.org>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Commit-Queue: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539276}
[modify] https://crrev.com/637c7485329228146d8af0555f8473cc9dc21e5b/cc/tiles/gpu_image_decode_cache.cc
[modify] https://crrev.com/637c7485329228146d8af0555f8473cc9dc21e5b/cc/tiles/gpu_image_decode_cache_unittest.cc
[modify] https://crrev.com/637c7485329228146d8af0555f8473cc9dc21e5b/content/test/gpu/gpu_tests/pixel_expectations.py
[modify] https://crrev.com/637c7485329228146d8af0555f8473cc9dc21e5b/content/test/gpu/gpu_tests/pixel_test_pages.py

Project Member

Comment 12 by bugdroid1@chromium.org, Feb 26 2018

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

commit cace4d4608929f1febdba2a1535c111945afe1fd
Author: Khushal <khushalsagar@chromium.org>
Date: Mon Feb 26 22:06:25 2018

canvas: Ensure stable content ids for canvas image snapshots.

When taking a snapshot from an SkSurface, it internally maintains a
cached copy of the SkImage which is invalidated only when the canvas
contents change. This ensures re-using cached texture uploads from this
image on subsequent uses.

However, when building a PaintImage using this SkImage we generate a
new content id for each draw, resulting in a cache miss on cc's decode
cache and re-upload with color conversion. Ensure that we use a stable
PaintImage::ContentId for the same SkImage.

R=junov@chromium.org

Bug:  814226 ,  814219 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ie1d8ebb40ccf98593dac3e6f65ca705054e6d90b
Reviewed-on: https://chromium-review.googlesource.com/932942
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: Justin Novosad <junov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539280}
[modify] https://crrev.com/cace4d4608929f1febdba2a1535c111945afe1fd/cc/paint/paint_image.cc
[modify] https://crrev.com/cace4d4608929f1febdba2a1535c111945afe1fd/cc/paint/paint_image.h
[modify] https://crrev.com/cace4d4608929f1febdba2a1535c111945afe1fd/third_party/WebKit/Source/platform/graphics/CanvasResourceProvider.cpp
[modify] https://crrev.com/cace4d4608929f1febdba2a1535c111945afe1fd/third_party/WebKit/Source/platform/graphics/CanvasResourceProvider.h

Project Member

Comment 13 by bugdroid1@chromium.org, Feb 27 2018

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

commit 4c1f61ac59a284ef92af29b01b4a9c561c33a56d
Author: Henrik Boström <hbos@chromium.org>
Date: Tue Feb 27 12:08:14 2018

Revert "cc: Ensure correct scaling for non-lazy images in GPU cache."

This reverts commit 637c7485329228146d8af0555f8473cc9dc21e5b.

Reason for revert: Suspected culprit of consistent Linux MSan Tests failures of GpuImageDecodeCacheTest.NonLazyImageUploadDownscaled since
https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20MSan%20Tests/builds/8195

Original change's description:
> cc: Ensure correct scaling for non-lazy images in GPU cache.
> 
> When using a non-lazy (bitmap) image in the GPU cache, we assume the
> upload to be scaled at the mip-level required for draw, when we
> actually use the original image for the upload. Fix this by considering
> non-lazy scaled images as lazy generated. This also ensures we use
> lower gpu memory, by caching downscaled uploads.
> 
> R=​ericrk@chromium.org
> 
> Bug:  815045 ,  814219 
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> Change-Id: I4b7d3f4edbcf0e5ba5574512f662a3fdf53cabd6
> Reviewed-on: https://chromium-review.googlesource.com/935441
> Reviewed-by: Zhenyao Mo <zmo@chromium.org>
> Reviewed-by: Eric Karl <ericrk@chromium.org>
> Commit-Queue: Khushal <khushalsagar@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#539276}

TBR=zmo@chromium.org,khushalsagar@chromium.org,ericrk@chromium.org

Change-Id: I441305f1ebfa962b3a78e5fa53f2169996fae196
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  815045 ,  814219 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/939361
Reviewed-by: Henrik Boström <hbos@chromium.org>
Commit-Queue: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539421}
[modify] https://crrev.com/4c1f61ac59a284ef92af29b01b4a9c561c33a56d/cc/tiles/gpu_image_decode_cache.cc
[modify] https://crrev.com/4c1f61ac59a284ef92af29b01b4a9c561c33a56d/cc/tiles/gpu_image_decode_cache_unittest.cc
[modify] https://crrev.com/4c1f61ac59a284ef92af29b01b4a9c561c33a56d/content/test/gpu/gpu_tests/pixel_expectations.py
[modify] https://crrev.com/4c1f61ac59a284ef92af29b01b4a9c561c33a56d/content/test/gpu/gpu_tests/pixel_test_pages.py

Components: Internals>Compositing>Images
All regressions are fixed except the one on soundcloud. The page has a case of an unaccelerated canvas being used with cc running gpu rasterization. The canvas is updated every frame, it rasters into a bitmap which is pushed into a recording to cc, which is uploaded using gpu discardable. Prior to this change, it was happening in skia.

I think skia has a lower gpu cache limit than what gpu discardable has. Everything is unpinned but stays in the gpu discardable cache till it reaches capacity. I'm not sure if this is something that needs to be fixed. In theory, the behaviour is similar to what we have with cpu discardable. ericrk@, piman@, thoughts?
Project Member

Comment 16 by bugdroid1@chromium.org, Apr 16 2018

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

commit a9d29a7aecf3042132247b323f79703605a272c0
Author: Khushal <khushalsagar@google.com>
Date: Mon Apr 16 19:39:43 2018

cc: Actively purge cached uploads for bitmaps from Canvas2D.

When using an unaccelerated canvas with a compositor using Gpu raster,
each canvas update is rasterized to a bitmap pushed into a paint
recording. This bitmap is uploaded and cached by cc's
GpuImageDecodeCache. Since the canvas is frequently updated, it is
common for the Gpu cache to use and keep the discardable texture memory
to capacity.

Avoid the above situation by actively deleting canvas snapshots in the
Gpu cache. PaintImages from a canvas instance use a stable
PaintImage::Id and update the PaintImage::ContentId when an updated
snapshot is created. This patch ensures that the Gpu cache keeps the
max 2 ContentId entries for the same PaintImage::Id (pending and active
tree), effectively ensuring we only have the last 2 canvas snapshots.

Note that BitmapImage, for encoded images in blink, also uses an updated
ContentId as more data is fetched for an image. This change will also
ensure that we evict previous partially decoded frames for the same
image.

For play:media:soundcloud test case on my linux machine, this reduced
cc's cache size from 236M to 30M.

R=enne@chromium.org, ericrk@chromium.org

Bug:  814219 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I888155ae5504ece6ab73981f320515478d87917d
Reviewed-on: https://chromium-review.googlesource.com/987194
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551078}
[modify] https://crrev.com/a9d29a7aecf3042132247b323f79703605a272c0/cc/paint/paint_image.h
[modify] https://crrev.com/a9d29a7aecf3042132247b323f79703605a272c0/cc/test/skia_common.cc
[modify] https://crrev.com/a9d29a7aecf3042132247b323f79703605a272c0/cc/test/skia_common.h
[modify] https://crrev.com/a9d29a7aecf3042132247b323f79703605a272c0/cc/tiles/gpu_image_decode_cache.cc
[modify] https://crrev.com/a9d29a7aecf3042132247b323f79703605a272c0/cc/tiles/gpu_image_decode_cache.h
[modify] https://crrev.com/a9d29a7aecf3042132247b323f79703605a272c0/cc/tiles/gpu_image_decode_cache_unittest.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a9d29a7aecf3042132247b323f79703605a272c0

commit a9d29a7aecf3042132247b323f79703605a272c0
Author: Khushal <khushalsagar@google.com>
Date: Mon Apr 16 19:39:43 2018

cc: Actively purge cached uploads for bitmaps from Canvas2D.

When using an unaccelerated canvas with a compositor using Gpu raster,
each canvas update is rasterized to a bitmap pushed into a paint
recording. This bitmap is uploaded and cached by cc's
GpuImageDecodeCache. Since the canvas is frequently updated, it is
common for the Gpu cache to use and keep the discardable texture memory
to capacity.

Avoid the above situation by actively deleting canvas snapshots in the
Gpu cache. PaintImages from a canvas instance use a stable
PaintImage::Id and update the PaintImage::ContentId when an updated
snapshot is created. This patch ensures that the Gpu cache keeps the
max 2 ContentId entries for the same PaintImage::Id (pending and active
tree), effectively ensuring we only have the last 2 canvas snapshots.

Note that BitmapImage, for encoded images in blink, also uses an updated
ContentId as more data is fetched for an image. This change will also
ensure that we evict previous partially decoded frames for the same
image.

For play:media:soundcloud test case on my linux machine, this reduced
cc's cache size from 236M to 30M.

R=enne@chromium.org, ericrk@chromium.org

Bug:  814219 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I888155ae5504ece6ab73981f320515478d87917d
Reviewed-on: https://chromium-review.googlesource.com/987194
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551078}
[modify] https://crrev.com/a9d29a7aecf3042132247b323f79703605a272c0/cc/paint/paint_image.h
[modify] https://crrev.com/a9d29a7aecf3042132247b323f79703605a272c0/cc/test/skia_common.cc
[modify] https://crrev.com/a9d29a7aecf3042132247b323f79703605a272c0/cc/test/skia_common.h
[modify] https://crrev.com/a9d29a7aecf3042132247b323f79703605a272c0/cc/tiles/gpu_image_decode_cache.cc
[modify] https://crrev.com/a9d29a7aecf3042132247b323f79703605a272c0/cc/tiles/gpu_image_decode_cache.h
[modify] https://crrev.com/a9d29a7aecf3042132247b323f79703605a272c0/cc/tiles/gpu_image_decode_cache_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment