New issue
Advanced search Search tips

Issue 868369 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

6.6%-128.3% regression in rendering.mobile at 576464:577094

Project Member Reported by pmeenan@chromium.org, Jul 27

Issue description

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

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


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

android-nexus5x-perf
Cc: junov@chromium.org
Owner: junov@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/14ce7a6ba40000

Make OffscreenCanvas 2d contexts use CanvasResource by junov@chromium.org
https://chromium.googlesource.com/chromium/src/+/79531c77e491dc9536c985d6c4f5dcd8c6bb48b8
0.1621 → 0.2596 (+0.09752)

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions
Owner: fs...@chromium.org
Cc: -junov@chromium.org
Cc: fs...@chromium.org
Components: Blink>Canvas Blink>Paint
Owner: ----
Status: Available (was: Assigned)
I'm having a very hard time reproducing those tests... wpr_go doesn't compile/run, and this was after chasing around the wprgo file for one hour.
Status: WontFix (was: Available)
The microsoft.snow benchmark is a new one from the looks of it and it kicked up a bunch of noise.  I don't think it has settled into a predictable baseline yet.
FYI, this is not a new benchmark. The benchmarks were recently reorganized into rendering.desktop and rendering.mobile. This one came from smoothness.tough_canvas_tests.

For historical data on this test, I think this is the correct link: https://chromeperf.appspot.com/report?sid=8e4e7451a8e9acfd5e997662396e9ad4ce6c10a2fb3343c891439df8ad1010b4

You can also run it outside the telemetry framework here: 
https://testdrive-archive.azurewebsites.net/Performance/LetItSnow/Default.html
Owner: fs...@chromium.org
Status: Assigned (was: WontFix)
Cc: -pmeenan@chromium.org
 Issue 868898  has been merged into this issue.
Although I can repro this between stable and canary, I can't repro around the pinpointed CL. Trying a new pinpoint.

Also, trying a proper bisect between M68 and ToT.
📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/16f73a42640000
Labels: Needs-Bisect
The pinpoint is broken. That's not the cause.
We can see the regression between Stable (M68) and Canary (M70).
Could please a tester bisect android for us please?

The site is:
https://testdrive-archive.azurewebsites.net/Performance/LetItSnow/Default.html

and in the broken version, the snowflakes are very slow.
Labels: -Needs-Bisect
Status: Started (was: Assigned)
Was able to proper bisect.
Cc: khushals...@chromium.org
Khushal,
the problem seems be that on:
https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/graphics/canvas_resource_provider.cc?type=cs&sq=package:chromium&g=0&l=723

we are now falling back to the GPU ImageDecodeCache that is acting WAY slower than the SharedCCDecodeCache.
Do you have any idea why this is so and what we could do about it?

Could you capture a "cc" trace for this that might point out what part has gotten slower? My intuition is that it could be this code again (https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/graphics/canvas_resource_provider.cc?l=541), it stashes a ref for every draw you make with an image and on my machine this page was making ~247 drawImage calls every update.
And this stashing is done only for the gpu cache, not sw. Since that is where doing a constant lock/unlock of the same image in repeated draws was causing a perf hit.
here it is.
trace_snowandroid.json.gz
1.3 MB Download
Cc: ericrk@chromium.org
There are GpuImageDecodeCache::UploadImage for every draw call, makes it look like we're getting a cache miss each time. Eric's debugging locally to see why that is.
It definitely looks like we're hitting the GPU Image decode cache both before and after "Make OffscreenCanvas 2d contexts use CanvasResource", so that change doesn't appear to have impacted which cache we use. I see requests to "GpuImageDecodeCache::GetDecodedImageForDraw" before and after the change.

The difference seems to be that after the change listed above, we re-upload images on every call to GpuImageDecodeCache::GetDecodedImageForDraw, rather than having a cache hit.

I wonder if the change above modified something around IDs for bitmaps being drawn to the canvas - if we no longer have stable IDs the caching layer doesn't know it can re-use an image.

For instance, could the code change in OffscreenCanvasRenderingContext2D::TransferToImageBitmap cause something like this, assuming each snowflake is drawn via a canvas then captured for re-drawing to the larger canvas?

Will log some IDs to confirm.
https://chromium-review.googlesource.com/c/chromium/src/+/1180423 should address the root cause of this bug as well.
Project Member

Comment 24 by bugdroid1@chromium.org, Aug 21

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

commit f93f8b7c3d3080586deba2a1a7f6b47bcfc4e340
Author: Khushal <khushalsagar@chromium.org>
Date: Tue Aug 21 01:12:26 2018

blink/canvas: Ensure constant PaintImage::ContentId for canvas snapshots.

Its important for Canvas snapshots to have a constant
PaintImage::ContentId to get a cache-hit in cc's ImageDecodeCache. The
logic was missing the case where the CanvasResourceProvider has a valid
ContextProvider but is still using software raster, thus creating
bitmaps which were being allocated new ids.

R=fserb@chromium.org

Bug:  868369 ,  872117 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Iadc76f35483c0aed9c31aba38a7808aee1dc8863
Reviewed-on: https://chromium-review.googlesource.com/1180423
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584588}
[modify] https://crrev.com/f93f8b7c3d3080586deba2a1a7f6b47bcfc4e340/third_party/blink/renderer/platform/graphics/canvas_resource_provider.cc
[modify] https://crrev.com/f93f8b7c3d3080586deba2a1a7f6b47bcfc4e340/third_party/blink/renderer/platform/graphics/canvas_resource_provider.h

Project Member

Comment 25 by bugdroid1@chromium.org, Aug 21

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

commit bb3941cc8f025f1a63a1134e7e8085e5b41c820b
Author: Khushal <khushalsagar@chromium.org>
Date: Tue Aug 21 03:32:06 2018

blink/canvas: Ensure constant PaintImage::ContentId for canvas snapshots.

Its important for Canvas snapshots to have a constant
PaintImage::ContentId to get a cache-hit in cc's ImageDecodeCache. The
logic was missing the case where the CanvasResourceProvider has a valid
ContextProvider but is still using software raster, thus creating
bitmaps which were being allocated new ids.

R=​fserb@chromium.org

Bug:  868369 ,  872117 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Iadc76f35483c0aed9c31aba38a7808aee1dc8863
Reviewed-on: https://chromium-review.googlesource.com/1180423
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Khushal <khushalsagar@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#584588}(cherry picked from commit f93f8b7c3d3080586deba2a1a7f6b47bcfc4e340)
Reviewed-on: https://chromium-review.googlesource.com/1182921
Reviewed-by: Krishna Govind <govind@chromium.org>
Cr-Commit-Position: refs/branch-heads/3529@{#2}
Cr-Branched-From: 6e0e7cd9bf10a61f443ba63115e86e9204d061ae-refs/heads/master@{#584585}
[modify] https://crrev.com/bb3941cc8f025f1a63a1134e7e8085e5b41c820b/third_party/blink/renderer/platform/graphics/canvas_resource_provider.cc
[modify] https://crrev.com/bb3941cc8f025f1a63a1134e7e8085e5b41c820b/third_party/blink/renderer/platform/graphics/canvas_resource_provider.h

Status: Fixed (was: Started)
Cc: schenney@chromium.org vamshi.kommuri@chromium.org
 Issue 868683  has been merged into this issue.
Project Member

Comment 28 by bugdroid1@chromium.org, Sep 8

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

commit 8b8f1dbacda50770d9afa2f72da765cc6d27441a
Author: Khushal <khushalsagar@chromium.org>
Date: Sat Sep 08 02:08:08 2018

blink/canvas: Ensure constant PaintImage::ContentId for canvas snapshots.

Its important for Canvas snapshots to have a constant
PaintImage::ContentId to get a cache-hit in cc's ImageDecodeCache. The
logic was missing the case where the CanvasResourceProvider has a valid
ContextProvider but is still using software raster, thus creating
bitmaps which were being allocated new ids.

R=​fserb@chromium.org

Bug:  868369 ,  872117 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Iadc76f35483c0aed9c31aba38a7808aee1dc8863
Reviewed-on: https://chromium-review.googlesource.com/1180423
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Khushal <khushalsagar@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#584588}(cherry picked from commit f93f8b7c3d3080586deba2a1a7f6b47bcfc4e340)
Reviewed-on: https://chromium-review.googlesource.com/1214753
Reviewed-by: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#912}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/8b8f1dbacda50770d9afa2f72da765cc6d27441a/third_party/blink/renderer/platform/graphics/canvas_resource_provider.cc
[modify] https://crrev.com/8b8f1dbacda50770d9afa2f72da765cc6d27441a/third_party/blink/renderer/platform/graphics/canvas_resource_provider.h

Sign in to add a comment