235196 2022-01-13 13:28:02 -0800 Specify willReadFrequently: true in benchmarks tests that do 2D canvas readbacks 2023-06-28 17:54:52 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified RESOLVED MOVED https://bugs.webkit.org/show_bug.cgi?id=244117 https://bugs.webkit.org/show_bug.cgi?id=249021 InRadar P2 Normal --- 1 junov junov ap cdumez darin ews-watchlist jbedard jonlee mmaxfield rniwa sabouhallawa webkit-bug-importer oldest_to_newest 1830681 0 junov 2022-01-13 13:28:02 -0800 The Images test of MotionMark does a lot of calls to getImageData, and therefore should use the willReadFrequently context creation attribute (see: https://github.com/whatwg/html/commit/d3e5732abdec1a73f716d1fd90f69313c4ddd54c ). Various browsers (Firefox, Edge, Chrome) in the near future will be removing heuristics that chose between CPU and GPU based rendering in order to make the browser's behavior more predictable. For web apps to retain good readback performance after these heuristics are removed, they will need to specify willReadFrequently at context creation time. For MotionMark to continue to provide fair comparisons of browser performance, it should use the willReadFrequently attribute when appropriate. 1830703 1 449103 junov 2022-01-13 13:57:56 -0800 Created attachment 449103 Patch 1831298 2 449103 darin 2022-01-16 08:02:24 -0800 Comment on attachment 449103 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449103&action=review > PerformanceTests/ChangeLog:12 > + this new API feature. That doesn’t make sense to me. The benchmark is intended as a proxy for performance on websites; changes in browsers should not result in changes to the benchmark. Changes to websites could, though. Has this change to the benchmark been discussed somewhere? Did the people discussing it consider the issue I am raising here? 1831299 3 darin 2022-01-16 08:03:09 -0800 Changing the benchmark before the websites change seems “cheeky” to me. 1831533 4 junov 2022-01-17 11:16:25 -0800 This change has not been discussed with anyone who Maintains MotionMark. We can discuss it here. Here is a summary of the situation: Many browser have heuristics to switch between between GPU and CPU-based rasterization of 2d canvas content. The reason for this is that GPU acceleration does not always perform better than without GPU acceleration, mostly because GPU readbacks are expensive, especially when they need to be synchronous, as is required by the getImageData and toDataURL methods. The problem with these heuristics is that they sometimes cause glitchy behavior when switching between GPU and CPU rendering on the fly. For example, anti-aliased edges might have a slightly different appearance. The sudden change in appearance may feel glitchy to users and the behaviour is sometimes surprising to web developers. Also, these heuristics result in performance characteristics that are surprising. For example: why is the first call to getImageData slower than the second one? To make the platform saner, it was proposed that these heuristic should be removed, and replaced by am API that gives developer explicit control in order to eliminate surprises. The new API feature is the willReadFrequently context creation attribute: https://html.spec.whatwg.org/multipage/canvas.html#concept-canvas-will-read-frequently Removing the heuristics will negatively affect the performance of web sites that call getImageData repeatedly without specifying {willReadFrequently: true} at context creation time. Therefore, there will be a devrel push to raise awareness of this issue so that Web devs can make the change before the heuristic removal goes live. This proposed change to MotionMark would assure continuity in performance for the benchmark. Basically it would assure that the benchmark continues to measure the same code path as before, which is the desirable code path. 1831954 5 rniwa 2022-01-19 00:01:20 -0800 (In reply to Justin Novosad from comment #4) > > To make the platform saner, it was proposed that these heuristic should be > removed, and replaced by am API that gives developer explicit control in > order to eliminate surprises. The new API feature is the willReadFrequently > context creation attribute: > > https://html.spec.whatwg.org/multipage/canvas.html#concept-canvas-will-read- > frequently > > Removing the heuristics will negatively affect the performance of web sites > that call getImageData repeatedly without specifying {willReadFrequently: > true} at context creation time. Therefore, there will be a devrel push to > raise awareness of this issue so that Web devs can make the change before > the heuristic removal goes live. > > This proposed change to MotionMark would assure continuity in performance > for the benchmark. Basically it would assure that the benchmark continues > to measure the same code path as before, which is the desirable code path. The benchmarks should measure the realistic browser performance. Making use of this new API in MotionMark should only be done after the vast majority of websites, NOT just the most popular websites, that use getImageData have adopted this new API. Otherwise, MotionMark will give unrealistically favorable numbers for browsers that implement such an new API but don't implement the aforementioned heuristics. 1832016 6 junov 2022-01-19 06:40:58 -0800 > Making use of this new API in MotionMark should only be done after the vast > majority of websites, NOT just the most popular websites, that use > getImageData have adopted this new API. Fair enough. I will add browser instrumentation to measure feature adoption. I will chime in at a later date once adoption is strong. 1832345 7 449103 rniwa 2022-01-19 21:18:21 -0800 Comment on attachment 449103 Patch r-ing this patch for now. 1832552 8 webkit-bug-importer 2022-01-20 13:28:19 -0800 <rdar://problem/87846749> 1964144 9 mmaxfield 2023-06-28 17:02:17 -0700 I think this is a dup of https://github.com/WebKit/MotionMark/pull/1 449103 2022-01-13 13:57:56 -0800 2022-01-19 21:18:21 -0800 Patch bug-235196-20220113165756.patch text/plain 1770 junov SW5kZXg6IFBlcmZvcm1hbmNlVGVzdHMvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFBlcmZvcm1h bmNlVGVzdHMvQ2hhbmdlTG9nCShyZXZpc2lvbiAyODc5OTIpCisrKyBQZXJmb3JtYW5jZVRlc3Rz L0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDIyLTAxLTEzICBK dXN0aW4gTm92b3NhZCAgPGp1bm92QGdvb2dsZS5jb20+CisKKyAgICAgICAgU3BlY2lmeSB3aWxs UmVhZEZyZXF1ZW50bHk6IHRydWUgaW4gTW90aW9uTWFyayBJbWFnZXMgdGVzdAorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjM1MTk2CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhpcyBjaGFuZ2UgYWRkcyB0 aGUgd2lsbFJlYWRGcmVxdWVudCAyZCBjb250ZXh0IGNyZWF0aW9uCisgICAgICAgIGF0dHJpYnV0 ZSB0byB0aGUgTW90aW9uTWFyayBJbWFnZXMgdGVzdCBpbiBvcmRlciB0byBlbnN1cmUKKyAgICAg ICAgdGhhdCBwZXJmb3JtYW5jZSBjaGFyYWN0ZXJpc3RpY3MgcmVtYWluIGNvbnNpc3RlbnQgd2hl biB2YXJpb3VzCisgICAgICAgIGJyb3dzZXJzIHN0YXJ0IHJlbW92aW5nIHBlcmZvcm1hbmNlIGhl dXJpc3RpY3MgaW4gZmF2b3Igb2YKKyAgICAgICAgdGhpcyBuZXcgQVBJIGZlYXR1cmUuCisKKyAg ICAgICAgKiBNb3Rpb25NYXJrL3Rlc3RzL21hc3Rlci9yZXNvdXJjZXMvaW1hZ2UtZGF0YS5qczoK KyAgICAgICAgKGFuaW1hdGUpOiBBZGRlZCB3aWxsUmVhZEZyZXF1ZW50bHk6IHRydWUgdG8gMmQg Y29udGV4dCBjcmVhdGlvbgorCiAyMDIxLTEyLTE3ICBSb2IgQnVpcyAgPHJidWlzQGlnYWxpYS5j b20+CiAKICAgICAgICAgW2Nzcy1jb250YWluXVtQZXJmb3JtYW5jZSB0ZXN0XSBBZGQgdGVzdCBj b250YWluLXBhaW50LXRleHQtbm93cmFwLmh0bWwKSW5kZXg6IFBlcmZvcm1hbmNlVGVzdHMvTW90 aW9uTWFyay90ZXN0cy9tYXN0ZXIvcmVzb3VyY2VzL2ltYWdlLWRhdGEuanMKPT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQot LS0gUGVyZm9ybWFuY2VUZXN0cy9Nb3Rpb25NYXJrL3Rlc3RzL21hc3Rlci9yZXNvdXJjZXMvaW1h Z2UtZGF0YS5qcwkocmV2aXNpb24gMjg3OTg5KQorKysgUGVyZm9ybWFuY2VUZXN0cy9Nb3Rpb25N YXJrL3Rlc3RzL21hc3Rlci9yZXNvdXJjZXMvaW1hZ2UtZGF0YS5qcwkod29ya2luZyBjb3B5KQpA QCAtMTQzLDcgKzE0Myw3IEBAIHZhciBJbWFnZURhdGFTdGFnZSA9IFV0aWxpdGllcy5jcmVhdGVT dWIKICAgICBhbmltYXRlOiBmdW5jdGlvbih0aW1lRGVsdGEpIHsKICAgICAgICAgZm9yICh2YXIg aSA9IDA7IGkgPCB0aGlzLl9vZmZzZXRJbmRleDsgKytpKSB7CiAgICAgICAgICAgICB2YXIgZWxl bWVudCA9IHRoaXMudGVzdEVsZW1lbnRzW2ldOwotICAgICAgICAgICAgdmFyIGNvbnRleHQgPSBl bGVtZW50LmdldENvbnRleHQoIjJkIik7CisgICAgICAgICAgICB2YXIgY29udGV4dCA9IGVsZW1l bnQuZ2V0Q29udGV4dCgiMmQiLCB7d2lsbFJlYWRGcmVxdWVudGx5OiB0cnVlfSk7CiAKICAgICAg ICAgICAgIC8vIEdldCBpbWFnZSBkYXRhCiAgICAgICAgICAgICB2YXIgaW1hZ2VEYXRhID0gY29u dGV4dC5nZXRJbWFnZURhdGEoMCwgMCwgdGhpcy5pbWFnZVdpZHRoLCB0aGlzLmltYWdlSGVpZ2h0 KTsK