Add willReadFrequently: true to Images test

[jrmuizel]

We expect authors to use this path in the wild so we should prefer measuring it.

[rniwa]

Do we have any data to back up the claim that this is what authors are doing?

[jrmuizel]

Chrome warns about doing multiple getImageData() calls on canvases: "Canvas2D: Multiple readback operations using getImageData are faster with the willReadFrequently attribute set to true. See: https://html.spec.whatwg.org/multipage/canvas.html#concept-canvas-will-read-frequently"

People seem to notice and fix this warning: https://github.com/paperjs/paper.js/issues/1999 https://github.com/paperjs/paper.js/issues/2000 https://github.com/paperjs/paper.js/issues/2003

Here are some other relatively high profile uses of willReadFrequently: https://github.com/pixijs/pixijs/blob/356abaaad852b248f0aa3f6873f5d7d2a56e3a50/packages/text/src/TextMetrics.ts#L57 https://github.com/mapbox/mapbox-gl-js/blob/696b013cdf34725cb63484dcf8f2e5a4f4af0c9c/src/util/browser.js#L48 https://github.com/plotly/plotly.js/blob/ac2ce6619180a231233e0e38cf7a0de80d1df2de/src/traces/image/plot.js#L95 https://github.com/openlayers/openlayers/blob/f3a5cd0d3e7056ff070527171de9586bcb59eb39/src/ol/DataTile.js#L61 https://github.com/fabricjs/fabric.js/blob/833754f3b21dcf4c7aad13eb751b4f3994c324b4/src/canvas/SelectableCanvas.ts#L1169 https://github.com/gorhill/uBlock/blob/f04f13e855c2ef136076c1785e2c52f8859e1d42/platform/common/vapi-background.js#L772 https://github.com/darkreader/darkreader/blob/c435f517f295aa503fc8e3fa2e6815b00bcd0eb3/src/inject/dynamic-theme/image.ts#L81

[junov]

LGTM

[litherum]

This should be a separate subtest, I think. I don't think we should modify the existing subtest - it intentionally doesn't test the willReadFrequently path

[jrmuizel]

Is the current non-willReadFrequently path worth being included in the set of things being tested?

i.e. do you have evidence that it's commonly hit and improving it makes a difference for users?

[litherum]

Well,

1. The default value for willReadFrequently is false
2. willReadFrequently was only added 2 years ago, so any content written before then wouldn't have specified it

If the request is for us to gather telemetry for all the times getContext() is called without willReadFrequently explicitly set to true, we can do that, but the above sure seems compelling that the non-willReadFrequently path is at least important enough to continue testing.

[jrmuizel]

The request would be for telemetry for frequent reading without willReadFrequently vs frequent reading with willReadFrequently

[jrmuizel]

@junov, in https://bugs.webkit.org/show_bug.cgi?id=235196#c6 you mention adding instrumentation to measure feature adoption. Do you have the results of that instrumentation that you can share?

[litherum]

(FYI: Us gathering telemetry will take about a year)

[litherum]

I also don't understand the resistance to making a separate test for this. What's the downside?

[kdashg]

I think the resistance is that we don't really want to incentivize spending engineering on performance optimizations for a path that we actively discourage authors from using.

We likely want to track the willReadFrequently:false testcase individually in our own regression testing, but I'd prefer not to encourage engineering investment in something that, should an author wish to, is loudly warned about and easily fixed.

I think we're not totally against having tests for both, but that I feel like we'd prefer to weight them as something like 30:1 for willReadFrequently:true vs false. In practice, this is akin to not having the willReadFrequently:false/default case.