There are currently some optimizations for image surfaces that we perform in imgFrame::Optimize() that are of dubious value. I conjecture that the single color optimization is rarely used on the modern web but it adds significant complexity to our code and is totally untested. Similarly the optimization to R5G6B5 surface formats on Android applies only to old devices with a 16-bit display and is unlikely to be used much in the wild. I'd like to rip out both of these optimizations. To determine if it's OK to do that, we should add some telemetry probes to see how often these optimizations are actually used.
Created attachment 8764509 [details] [diff] [review] Add telemetry for image optimizations that may no longer be useful. Here's the patch. Note that we only mark the single color optimization as used if the image is bigger than 1x1, because for images which have only one pixel there's no memory usage benefit to applying the optimization.
Attachment #8764509 - Flags: review?(edwin) → review+
Comment on attachment 8764509 [details] [diff] [review] Add telemetry for image optimizations that may no longer be useful. I don't think you need to record the "false" value for the _TO_565 case, do you? true will probably be sufficient. Please expand on your histogram descriptions to record *what* and *when* a value is recorded. So e.g. "Records true each time a decoded image is optimized to a single color". Since you're planning on making a specific decision here (to remove or keep these optimizations), rather than long-term monitoring, you should make these probes expire, typically in 6 months. So expires_in_version: 54. data-review=me with those changes
Attachment #8764509 - Flags: feedback?(benjamin) → feedback+
Created attachment 8765097 [details] [diff] [review] Add telemetry for image optimizations that may no longer be useful. Updated the patch to incorporate bsmedberg's feedback.
Attachment #8764509 - Attachment is obsolete: true
Try job here. Should be ready to go if the job is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b58ffbfe3b17
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/6a0632dcd94b Add telemetry for image optimizations that may no longer be useful. r=edwin
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Preliminary results suggest that on Nightly, these features are literally *never* used. *Zero* uses. We should uplift this to aurora and beta ASAP. If nobody's using them on aurora or beta either, I'm ripping this code out.
Comment on attachment 8765097 [details] [diff] [review] Add telemetry for image optimizations that may no longer be useful. Approval Request Comment [Feature/regressing bug #]: N/A [User impact if declined]: This just adds new telemetry. [Describe test coverage new/current, TreeHerder]: N/A [Risks and why]: This is very low risk code that just makes us report two new telemetry values related to image decoding. The telemetry values measure the usage of features which we have good reason to believe - especially because of the preliminary data from the Nightly population - are rarely or never used, and should be removed from the codebase because they are a significant maintenance burden and have no test coverage. It'd be very helpful to get numbers on this from the beta population in particular ASAP. [String/UUID change made/needed]: None.
status-firefox48: --- → affected
status-firefox49: --- → affected
Comment on attachment 8765097 [details] [diff] [review] Add telemetry for image optimizations that may no longer be useful. Sure, let's do it Should be in 48 beta 5
status-firefox49: affected → fixed
status-firefox48: affected → fixed
Backed out from beta for build bustage https://hg.mozilla.org/releases/mozilla-beta/rev/0b4f2a4e4229 https://treeherder.mozilla.org/logviewer.html#?job_id=1252153&repo=mozilla-beta
status-firefox48: fixed → affected
Rebased and pushed to beta: https://hg.mozilla.org/releases/mozilla-beta/rev/e8999eb7311e
status-firefox48: affected → fixed
The initial results for the single color optimization are unambiguous: *nobody* is using it. Zero uses on nightly, aurora, or beta. It's time to remove it. For the 565 optimization, the situation is less clear. There are ~460 telemetry pings reporting that it's in use on nightly, all of them on Linux. However, there are none on aurora or beta. I think we should hold off on removing that optimization until we see what happens on release. Benjamin, can we tell in more detail what OS the IMAGE_OPTIMIZE_TO_565_USED pings are coming from? In particular, can we distinguish between Linux, Android, and B2G? (I'm guess all of those show up as Linux.)
You can do that using custom queries, sure. Ask Roberto Vitillo or the the fhr-dev mailing list questions and they can help you.
Now that the new telemetry has hit release, I think we can be pretty confident that the 565 optimization can be removed. We've gotten 14 hits on release so far, meaning a total of 14 images have been optimized to 565. That's not buying us anything meaningful.
You need to log in before you can comment on or make changes to this bug.