Bug 235196 - Specify willReadFrequently: true in benchmarks tests that do 2D canvas readbacks
Summary: Specify willReadFrequently: true in benchmarks tests that do 2D canvas readbacks
Status: RESOLVED MOVED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Novosad
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-13 13:28 PST by Justin Novosad
Modified: 2023-06-28 17:54 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.73 KB, patch)
2022-01-13 13:57 PST, Justin Novosad
rniwa: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Novosad 2022-01-13 13:28:02 PST
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.
Comment 1 Justin Novosad 2022-01-13 13:57:56 PST
Created attachment 449103 [details]
Patch
Comment 2 Darin Adler 2022-01-16 08:02:24 PST
Comment on attachment 449103 [details]
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?
Comment 3 Darin Adler 2022-01-16 08:03:09 PST
Changing the benchmark before the websites change seems “cheeky” to me.
Comment 4 Justin Novosad 2022-01-17 11:16:25 PST
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.
Comment 5 Ryosuke Niwa 2022-01-19 00:01:20 PST
(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.
Comment 6 Justin Novosad 2022-01-19 06:40:58 PST
> 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.
Comment 7 Ryosuke Niwa 2022-01-19 21:18:21 PST
Comment on attachment 449103 [details]
Patch

r-ing this patch for now.
Comment 8 Radar WebKit Bug Importer 2022-01-20 13:28:19 PST
<rdar://problem/87846749>
Comment 9 Myles C. Maxfield 2023-06-28 17:02:17 PDT
I think this is a dup of https://github.com/WebKit/MotionMark/pull/1