Closed Bug 1661123 (sanitizeCSScolors-perf) Opened 5 years ago Closed 5 years ago

LightweightThemeConsumer.jsm:_sanitizeCSSColor slows down first-time about:welcome theme enables

Categories

(Firefox :: Toolbars and Customization, defect, P1)

Product:
Firefox
Component:
Toolbars and Customization
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
82 Branch
Tracking Flags:
Tracking Status
firefox81 + fixed
firefox82 --- fixed

People

(Reporter: dmosedale, Assigned: emilio)

References

Details

Attachments

(1 file)

Bug 1661123 - Make LightWeightThemeConsumer.jsm sanitize colors faster. r=dmose
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
Details | Review

https://searchfox.org/mozilla-central/source/toolkit/modules/LightweightThemeConsumer.jsm#458 has shown up to be a good candidate for speeding up to support making the theme switches in about:welcome meaningfully faster.

Emilio and I chatted about this for a while in Slack, and he has a plan for what looks to be a straightforward fix that should be upliftable to 81 beta.

Flags: needinfo?(emilio)
Summary: replace LightweightThemeConsumer.jsm slows down first about:welcome theme enables with something faster → LightweightThemeConsumer.jsm:_sanitizeCSSColor slows down first-time about:welcome theme enables

Instead of creating an element, flushing styles and getting the computed
value back just to receive, use the existing InspectorUtils.colorToRGBA.

With some refactoring, we can completely get rid of parsing rgba strings
in LightWeightThemeConsumer too, as a benefit. This should be much
faster.

This patch tweaks the InspectorUtils API to allow taking a document, so
that system colors keep working. We could probably get away without
supporting system colors, but it'd technically be a regression, and
since we want this patch to be uplifted, and it's easy, let's avoid
breaking changes.

Flags: needinfo?(emilio)

Just tested the first iteration of this patch -- it was a very large win, cutting the amount of time to initially enable the existing themes almost in half. Thanks so much, Emilio!

Alias: sanitizeCSScolors-perf

[Tracking Requested - why for this release]:

As part of the about:welcome onboarding experiments, first-time users are being offered to choose themes. On older and slower machines today, enabling a theme for the first time is visibly very slow for the existing themes. Furthermore, for 81, we've added Alpenglow, a which is visibly slower to enable than the existing two. The hope is that this patch should almost double the speed of theme selection at low risk.

status-firefox81: --- → affected
status-firefox82: --- → affected
tracking-firefox81: --- → ?
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8fe9ffffccda Make LightWeightThemeConsumer.jsm sanitize colors faster. r=Gijs,jwatt

Backed out changeset 8fe9ffffccda (bug 1661123) for browser_ext_themes_experiment.js failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=mochitest-browser-chrome&fromchange=8fe9ffffccda1f4eddebe0c4dda3edaacbaa9104&tochange=3081e15cb9743821d0e64ecb08836c5cf0a47afb&selectedTaskRun=JwNDK0O6T76wL7Gp_X2UVw.0

Backout link: https://hg.mozilla.org/integration/autoland/rev/3081e15cb9743821d0e64ecb08836c5cf0a47afb

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=314167172&repo=autoland&lineNumber=6937

[task 2020-08-27T11:37:11.977Z] 11:37:11     INFO - TEST-START | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js
[task 2020-08-27T11:37:12.017Z] 11:37:12     INFO - TEST-INFO | started process screenshot
[task 2020-08-27T11:37:12.115Z] 11:37:12     INFO - TEST-INFO | screenshot: exit 0
[task 2020-08-27T11:37:12.115Z] 11:37:12     INFO - Buffered messages logged at 11:37:11
[task 2020-08-27T11:37:12.115Z] 11:37:12     INFO - Entering test bound test_experiment_static_theme
[task 2020-08-27T11:37:12.116Z] 11:37:12     INFO - Extension loaded
[task 2020-08-27T11:37:12.116Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Color property should be unset - 
[task 2020-08-27T11:37:12.116Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be unset - 
[task 2020-08-27T11:37:12.116Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Generic Property should be unset. - 
[task 2020-08-27T11:37:12.116Z] 11:37:12     INFO - Buffered messages logged at 11:37:12
[task 2020-08-27T11:37:12.116Z] 11:37:12     INFO - Testing that current window updated with the experiment applied
[task 2020-08-27T11:37:12.116Z] 11:37:12     INFO - Buffered messages finished
[task 2020-08-27T11:37:12.116Z] 11:37:12     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Color property should be parsed and set. - Got "rgba(255, 0, 255, 1)", expected "rgb(255, 0, 255)"
[task 2020-08-27T11:37:12.116Z] 11:37:12     INFO - Stack trace:
[task 2020-08-27T11:37:12.117Z] 11:37:12     INFO - chrome://mochikit/content/browser-test.js:test_is:1332
[task 2020-08-27T11:37:12.117Z] 11:37:12     INFO - chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:testExperimentApplied:58
[task 2020-08-27T11:37:12.117Z] 11:37:12     INFO - chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:test_experiment_static_theme:100
[task 2020-08-27T11:37:12.117Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be parsed. - 
[task 2020-08-27T11:37:12.117Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be set. - 
[task 2020-08-27T11:37:12.117Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Generic Property should be set. - 
[task 2020-08-27T11:37:12.117Z] 11:37:12     INFO - Testing that new window initialized with the experiment applied
[task 2020-08-27T11:37:12.358Z] 11:37:12     INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-08-27T11:37:12.358Z] 11:37:12     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Color property should be parsed and set. - Got "rgba(255, 0, 255, 1)", expected "rgb(255, 0, 255)"
[task 2020-08-27T11:37:12.358Z] 11:37:12     INFO - Stack trace:
[task 2020-08-27T11:37:12.358Z] 11:37:12     INFO - chrome://mochikit/content/browser-test.js:test_is:1332
[task 2020-08-27T11:37:12.358Z] 11:37:12     INFO - chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:testExperimentApplied:58
[task 2020-08-27T11:37:12.358Z] 11:37:12     INFO - chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:test_experiment_static_theme:105
[task 2020-08-27T11:37:12.358Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be parsed. - 
[task 2020-08-27T11:37:12.359Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be set. - 
[task 2020-08-27T11:37:12.359Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Generic Property should be set. - 
[task 2020-08-27T11:37:12.379Z] 11:37:12     INFO - Testing that both windows unapplied the experiment
[task 2020-08-27T11:37:12.380Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Color property should be unset - 
[task 2020-08-27T11:37:12.380Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be unset - 
[task 2020-08-27T11:37:12.380Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Generic Property should be unset. - 
[task 2020-08-27T11:37:12.380Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Color property should be unset - 
[task 2020-08-27T11:37:12.381Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be unset - 
[task 2020-08-27T11:37:12.381Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Generic Property should be unset. - 
[task 2020-08-27T11:37:12.442Z] 11:37:12     INFO - Leaving test bound test_experiment_static_theme
[task 2020-08-27T11:37:12.442Z] 11:37:12     INFO - Entering test bound test_experiment_dynamic_theme
[task 2020-08-27T11:37:12.442Z] 11:37:12     INFO - Extension loaded
[task 2020-08-27T11:37:12.482Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Color property should be unset - 
[task 2020-08-27T11:37:12.482Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be unset - 
[task 2020-08-27T11:37:12.482Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Generic Property should be unset. - 
[task 2020-08-27T11:37:12.501Z] 11:37:12     INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-08-27T11:37:12.501Z] 11:37:12     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Color property should be parsed and set. - Got "rgba(255, 0, 255, 1)", expected "rgb(255, 0, 255)"
[task 2020-08-27T11:37:12.501Z] 11:37:12     INFO - Stack trace:
[task 2020-08-27T11:37:12.501Z] 11:37:12     INFO - chrome://mochikit/content/browser-test.js:test_is:1332
[task 2020-08-27T11:37:12.501Z] 11:37:12     INFO - chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:testExperimentApplied:197
[task 2020-08-27T11:37:12.501Z] 11:37:12     INFO - chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:test_experiment_dynamic_theme:237
[task 2020-08-27T11:37:12.501Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be parsed. - 
[task 2020-08-27T11:37:12.501Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be set. - 
[task 2020-08-27T11:37:12.502Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Generic Property should be set. - 
[task 2020-08-27T11:37:12.811Z] 11:37:12     INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-08-27T11:37:12.811Z] 11:37:12     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Color property should be parsed and set. - Got "rgba(255, 0, 255, 1)", expected "rgb(255, 0, 255)"
[task 2020-08-27T11:37:12.811Z] 11:37:12     INFO - Stack trace:
[task 2020-08-27T11:37:12.811Z] 11:37:12     INFO - chrome://mochikit/content/browser-test.js:test_is:1332
[task 2020-08-27T11:37:12.811Z] 11:37:12     INFO - chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:testExperimentApplied:197
[task 2020-08-27T11:37:12.811Z] 11:37:12     INFO - chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:test_experiment_dynamic_theme:242
[task 2020-08-27T11:37:12.812Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be parsed. - 
[task 2020-08-27T11:37:12.812Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be set. - 
[task 2020-08-27T11:37:12.812Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Generic Property should be set. - 
[task 2020-08-27T11:37:12.833Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Color property should be unset - 
[task 2020-08-27T11:37:12.833Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be unset - 
[task 2020-08-27T11:37:12.833Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Generic Property should be unset. - 
[task 2020-08-27T11:37:12.834Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Color property should be unset - 
[task 2020-08-27T11:37:12.834Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be unset - 
[task 2020-08-27T11:37:12.834Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Generic Property should be unset. - 
[task 2020-08-27T11:37:12.853Z] 11:37:12     INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-08-27T11:37:12.853Z] 11:37:12     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Color property should be parsed and set. - Got "rgba(255, 0, 255, 1)", expected "rgb(255, 0, 255)"
[task 2020-08-27T11:37:12.853Z] 11:37:12     INFO - Stack trace:
[task 2020-08-27T11:37:12.853Z] 11:37:12     INFO - chrome://mochikit/content/browser-test.js:test_is:1332
[task 2020-08-27T11:37:12.853Z] 11:37:12     INFO - chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:testExperimentApplied:197
[task 2020-08-27T11:37:12.853Z] 11:37:12     INFO - chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:test_experiment_dynamic_theme:268
[task 2020-08-27T11:37:12.854Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be parsed. - 
[task 2020-08-27T11:37:12.854Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be set. - 
[task 2020-08-27T11:37:12.854Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Generic Property should be set. - 
[task 2020-08-27T11:37:12.854Z] 11:37:12     INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-08-27T11:37:12.854Z] 11:37:12     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Color property should be parsed and set. - Got "rgba(255, 0, 255, 1)", expected "rgb(255, 0, 255)"
[task 2020-08-27T11:37:12.854Z] 11:37:12     INFO - Stack trace:
[task 2020-08-27T11:37:12.854Z] 11:37:12     INFO - chrome://mochikit/content/browser-test.js:test_is:1332
[task 2020-08-27T11:37:12.854Z] 11:37:12     INFO - chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:testExperimentApplied:197
[task 2020-08-27T11:37:12.854Z] 11:37:12     INFO - chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js:test_experiment_dynamic_theme:269
[task 2020-08-27T11:37:12.854Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be parsed. - 
[task 2020-08-27T11:37:12.854Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be set. - 
[task 2020-08-27T11:37:12.854Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Generic Property should be set. - 
[task 2020-08-27T11:37:12.894Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Color property should be unset - 
[task 2020-08-27T11:37:12.894Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be unset - 
[task 2020-08-27T11:37:12.894Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Generic Property should be unset. - 
[task 2020-08-27T11:37:12.894Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Color property should be unset - 
[task 2020-08-27T11:37:12.894Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Image property should be unset - 
[task 2020-08-27T11:37:12.894Z] 11:37:12     INFO - TEST-PASS | toolkit/components/extensions/test/browser/browser_ext_themes_experiment.js | Generic Property should be unset. - 
[task 2020-08-27T11:37:12.975Z] 11:37:12     INFO - Leaving test bound test_experiment_dynamic_theme
...
Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4aec4aa2e089 Make LightWeightThemeConsumer.jsm sanitize colors faster. r=Gijs,jwatt
Flags: needinfo?(emilio)
Severity: -- → S3
Priority: -- → P1

https://hg.mozilla.org/mozilla-central/rev/4aec4aa2e089

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox82: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(emilio)

Comment on attachment 9172018 [details]
Bug 1661123 - Make LightWeightThemeConsumer.jsm sanitize colors faster. r=dmose

Beta/Release Uplift Approval Request

  • User impact if declined: see comment 3
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: none
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Simple change to the color sanitization used by the theming code to be faster.
  • String changes made/needed: none
Flags: needinfo?(emilio)
Attachment #9172018 - Flags: approval-mozilla-beta?

Comment on attachment 9172018 [details]
Bug 1661123 - Make LightWeightThemeConsumer.jsm sanitize colors faster. r=dmose

Sounds like a pretty big improvement to a high-visibility change shipping in 81. Approved for 81.0b5.

Attachment #9172018 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: