Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add willReadFrequently: true to Images test #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jrmuizel
Copy link

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

@rniwa
Copy link
Member

rniwa commented Jun 15, 2023

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

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

junov commented Jun 27, 2023

LGTM

@litherum
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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

@jrmuizel
Copy link
Author

@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
Copy link
Contributor

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

@litherum
Copy link
Contributor

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

@kdashg
Copy link

kdashg commented Jul 17, 2023

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.

@smfr smfr added major This is a "major" change per the Governance rules test change Change to a test, or new test labels Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major This is a "major" change per the Governance rules test change Change to a test, or new test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants