putImageData ~4x performance regression between chrome 63 and 64(beta)
Reported by
jf.pamb...@gmail.com,
Jan 15 2018
|
||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36 Steps to reproduce the problem: 1. Go to https://jsfiddle.net/Lmqjpgee/6/ 2. Open the console 2. Press 'Run' 3. wait until the test completes (about 15 seconds). The number logged in the console is the average time needed to draw a 2000x2000 image over 1000 iteration. 4. Compare results between chrome 63 and 64. What is the expected behavior? Similar performance in chrome 63 and 64. What went wrong? About 4x performance regression. On my system, that translates to 2.23 ms/it to 9.35 ms/it. Did this work before? N/A Chrome version: 64.0.3282.85 Channel: beta OS Version: Flash Version: Although 2000x2000 may seem like a large image, the performance regression is constant across canvas sizes.
,
Jan 16 2018
jf.pambrun@ Thanks for the issue. Able to reproduce this issue on Windows 10 and Ubuntu 14.04 the latest Canary 65.0.3322.3 and Beta 64.0.3282.85 by following the steps mentioned in the original comment. Bisect Information: =================== Good Build: 64.0.3262.0 (Revision - 514703) Bad Build: 64.0.3263.0 (Revision - 515052) Will update Changelog tomorrow by executing the per-revision bisect script, hence adding Needs-Bisect label. Thanks..
,
Jan 16 2018
,
Jan 17 2018
,
Jan 17 2018
Could be due to : https://chromium.googlesource.com/chromium/src/+/5b07996e178c5f4e153e771d48ab839867fa7440
,
Jan 17 2018
// Adding to comment #2. Bisect Information: =================== Good Build: 64.0.3262.0 (Revision - 514703) Bad Build: 64.0.3263.0 (Revision - 515052) On executing the per-revision bisect script, below is the changelog URL. Changelog URL: --------------- https://chromium.googlesource.com/chromium/src/+log/6c7d1f3f662c2471077dc79c6d2e8224fa452c44..5b07996e178c5f4e153e771d48ab839867fa7440 From the above Changelog URL, suspecting the below change for this issue. Reviewed-on: https://chromium-review.googlesource.com/758562 Adding ReleaseBlock-Stable as this is a recent Regression. Please feel free to remove the same if this is not applicable. Thanks...
,
Jan 17 2018
Able to repro this issue on Mac OS 10.12.6 as well.
,
Jan 22 2018
junov@ Gentle Ping! This issue is marked as RB-Stable, could you please let us know is there any latest update available on this issue. Thanks!
,
Jan 23 2018
,
Jan 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7243aeeab61b2599e180f0cb4ddbba13daf323bf commit 7243aeeab61b2599e180f0cb4ddbba13daf323bf Author: Justin Novosad <junov@chromium.org> Date: Wed Jan 24 22:35:46 2018 Fix performance regression in putImageData This CL returns to the simple direct blit code path we had before. In CL https://chromium-review.googlesource.com/c/chromium/src/+/758562 the implementation of Canvas2DLayerBridge::WritePixels was changed to go through the PaintCanvas interface, which required making an extra copy and force the pixels through an alternate color correction code path that handles alpha blending correctly. It turns out that this detour is unnecessary since alpha blending does not come into play in putImageData since it is a straight blit. BUG= 802081 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I3af99cc56745f6db52921282d905f73981675926 Reviewed-on: https://chromium-review.googlesource.com/883770 Reviewed-by: Fernando Serboncini <fserb@chromium.org> Commit-Queue: Justin Novosad <junov@chromium.org> Cr-Commit-Position: refs/heads/master@{#531723} [modify] https://crrev.com/7243aeeab61b2599e180f0cb4ddbba13daf323bf/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
,
Jan 24 2018
,
Jan 24 2018
This bug requires manual review: Request affecting a post-stable build Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 25 2018
Tested this issue on Windows 10, Mac OS 10.12.6 and Ubuntu 14.04 on the latest Chrome Build 66.0.3331.0 by following the steps mentioned in the original comment. On navigating to the given jsfiddle page, and clicking on Run button, can see the behavior as in M63 builds Attached is the screen shot for reference. Hence adding TE verified labels as the fix is working as intended. Thanks..
,
Jan 25 2018
Thanks Justin - how safe is this merge? Please note that we've already started ramp-up for M64 to stable. Is this critical enough for 64 or can we aim for 65 instead?
,
Jan 25 2018
Very safe. It is basically a partial revert. Living with the regression for one release cycle probably isn't the end of the world since this probably does not affect a very large number of web sites. Unfortunately we don't collect usage stats for putImageData in Chrome.
,
Jan 25 2018
Does this impact users as well, or more just testing? So for sites using putImageData, does that mean a 4X regression faced by users?
,
Jan 25 2018
Speaking for myself, I am building a medical image viewer and I am trying to render reformatted images at 60fps. I have about 12-13ms to render and draw up to a 4 megapixel canvas. That put operation used to take ~1ms, but now that 5 or 6 effectively reducing my render budget/performance in half. It's possible that many webgl applications are also affected, although it might be using a different path.
,
Jan 25 2018
Yeah, so of the few web sites that are impacted, some are going to suffer substantially. I am very confident that this fix won't regress rendering functionality, since we have extensive test coverage for that, and because the fix just reverts to a clone of the previous code path of putImageData.
,
Jan 25 2018
Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 25 2018
Pls merge your change to M65 branch 3325 ASAP so we can pick it up for next M65 dev release. Thank you.
,
Jan 26 2018
Approving merge for M64. Branch:3282 We don't have a respin planned yet, and we probably won't respin for this specifically. However, let's merge it in branch.
,
Jan 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8592577dae81184d048c39d4f03afeb2028ac358 commit 8592577dae81184d048c39d4f03afeb2028ac358 Author: Justin Novosad <junov@chromium.org> Date: Fri Jan 26 19:16:08 2018 Fix performance regression in putImageData This CL returns to the simple direct blit code path we had before. In CL https://chromium-review.googlesource.com/c/chromium/src/+/758562 the implementation of Canvas2DLayerBridge::WritePixels was changed to go through the PaintCanvas interface, which required making an extra copy and force the pixels through an alternate color correction code path that handles alpha blending correctly. It turns out that this detour is unnecessary since alpha blending does not come into play in putImageData since it is a straight blit. BUG= 802081 TBR=junov@chromium.org (cherry picked from commit 7243aeeab61b2599e180f0cb4ddbba13daf323bf) Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I3af99cc56745f6db52921282d905f73981675926 Reviewed-on: https://chromium-review.googlesource.com/883770 Reviewed-by: Fernando Serboncini <fserb@chromium.org> Commit-Queue: Justin Novosad <junov@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#531723} Reviewed-on: https://chromium-review.googlesource.com/889340 Reviewed-by: Justin Novosad <junov@chromium.org> Cr-Commit-Position: refs/branch-heads/3325@{#121} Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} [modify] https://crrev.com/8592577dae81184d048c39d4f03afeb2028ac358/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
,
Jan 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0186bfee26ca0bc9d6eff1e80203f84319aec437 commit 0186bfee26ca0bc9d6eff1e80203f84319aec437 Author: Justin Novosad <junov@chromium.org> Date: Fri Jan 26 19:18:04 2018 Fix performance regression in putImageData This CL returns to the simple direct blit code path we had before. In CL https://chromium-review.googlesource.com/c/chromium/src/+/758562 the implementation of Canvas2DLayerBridge::WritePixels was changed to go through the PaintCanvas interface, which required making an extra copy and force the pixels through an alternate color correction code path that handles alpha blending correctly. It turns out that this detour is unnecessary since alpha blending does not come into play in putImageData since it is a straight blit. BUG= 802081 TBR=junov@chromium.org (cherry picked from commit 7243aeeab61b2599e180f0cb4ddbba13daf323bf) Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I3af99cc56745f6db52921282d905f73981675926 Reviewed-on: https://chromium-review.googlesource.com/883770 Reviewed-by: Fernando Serboncini <fserb@chromium.org> Commit-Queue: Justin Novosad <junov@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#531723} Reviewed-on: https://chromium-review.googlesource.com/889410 Reviewed-by: Justin Novosad <junov@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#600} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [modify] https://crrev.com/0186bfee26ca0bc9d6eff1e80203f84319aec437/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
,
Jan 26 2018
,
Jan 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2a7396d958ac598a7c11607ea90e40811eb0fe4d commit 2a7396d958ac598a7c11607ea90e40811eb0fe4d Author: Justin Novosad <junov@chromium.org> Date: Sat Jan 27 02:02:21 2018 Fix build failure from change 0186bfee26ca0bc9d6eff1e80203f84319aec437 This change does without the helper method CanvasResourceProvider:: WritePixels, which exists in ToT, by simply in-lining the helper method at the call site. BUG= 802081 TBR=fserb@chromium.org Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I4d17934202154e0ed977dd0c7da1c0d1829e8a46 Reviewed-on: https://chromium-review.googlesource.com/890281 Reviewed-by: Justin Novosad <junov@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#603} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [modify] https://crrev.com/2a7396d958ac598a7c11607ea90e40811eb0fe4d/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
,
Jan 30 2018
Tested this issue on Windows 10, Mac OS 10.12.6 and Ubuntu 14.04 on the latest Chrome Build 65.0.3325.31 by following the steps mentioned in the original comment. Navigated to the given jsfiddle page, and hit the Run button and observed the Devtools -> Console tab, can see the behavior as in M-63 builds Attached is the screen shot for reference. Hence adding TE verified labels as the fix is working as intended. Thanks..
,
Feb 1 2018
Tested this issue on Windows 10, Mac OS 10.12.6 and Ubuntu 14.04 on the latest M-64 Chrome Build 64.0.3282.140 by following the steps mentioned in the original comment. On navigating to the given jsfiddle page -> Opening Devtools-Console tab, and clicking on Run button, can see the behavior as in M63 builds Attached is the screen shot for reference. Hence adding TE verified labels as the fix is working as intended. Thanks.. |
||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||
Comment 1 by jklot...@gmail.com
, Jan 15 2018