Issue metadata
Sign in to add a comment
|
6.6%-128.3% regression in rendering.mobile at 576464:577094 |
||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jul 27
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/14ce7a6ba40000
,
Jul 27
📍 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
,
Jul 27
,
Jul 27
,
Jul 30
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.
,
Jul 31
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.
,
Aug 1
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
,
Aug 1
,
Aug 2
,
Aug 6
Issue 868898 has been merged into this issue.
,
Aug 15
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/16f73a42640000
,
Aug 15
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.
,
Aug 16
📍 Job complete. See results below. https://pinpoint-dot-chromeperf.appspot.com/job/16f73a42640000
,
Aug 16
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.
,
Aug 16
Was able to proper bisect.
,
Aug 17
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?
,
Aug 17
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.
,
Aug 17
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.
,
Aug 17
here it is.
,
Aug 17
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.
,
Aug 17
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.
,
Aug 17
https://chromium-review.googlesource.com/c/chromium/src/+/1180423 should address the root cause of this bug as well.
,
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
,
Aug 21
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
,
Aug 21
,
Aug 23
,
Sep 8
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 |
|||||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Jul 27