Closed
Bug 1446250
Opened 6 years ago
Closed 6 years ago
Fix pageAction performance regression
Categories
(WebExtensions :: Frontend, defect)
WebExtensions
Frontend
Tracking
(firefox61 fixed)
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
Details
Attachments
(2 files)
The migration to Photon page actions removed nearly all of our pageAction update optimizations and caused a massive performance regression. It needs to be fixed.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8959431 [details] Bug 1446250: Part 1 - Optimize Photon PageAction update performance. https://reviewboard.mozilla.org/r/228244/#review234206 ::: browser/base/content/browser-pageActions.js:527 (Diff revision 1) > + if (urlbarButton) { > + urlbarButton.setAttribute("aria-label", title); > } > + > // tooltiptext falls back to the title, so update it, too. > - this._updateActionTooltip(action); > + this._updateActionTooltip(action, undefined, title, urlbarButton); move the update tooltip into the if statement ::: browser/modules/PageActions.jsm:803 (Diff revision 1) > - * state. If the property does not exist in the window's state, or if > - * no window is given, then the global value is returned. > - * @return The property value. > + * If true, always returns a window-specific properties object. > + * If a properties object does not exist for the given window, > + * one is created and cached. > + * @returns {object} > */ > - _getProperty(name, browserWindow) { > + _getProperties(window, forceWindowSpecific) { nit: forceWindowSpecific = false per doc above? I know.
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8959432 [details] Bug 1446250: Part 2 - Optimize/reduce calls into the Photon PageAction API. https://reviewboard.mozilla.org/r/228246/#review234224
Attachment #8959432 -
Flags: review?(mixedpuppy) → review+
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8959431 [details] Bug 1446250: Part 1 - Optimize Photon PageAction update performance. https://reviewboard.mozilla.org/r/228244/#review234348 I mostly have a lot of questions. Also, for future archaeologists, it would be nice to have a comment on the bug where "nearly all of our pageAction update optimizations" happened (ie bug reference), and to link this to whatever bug the screenshot work and/or talos regression was documented in. Right now I feel like I'm reviewing in the dark, essentially. ::: browser/base/content/browser-pageActions.js:466 (Diff revision 1) > * The action to update. > * @param propertyName (string, optional) > * The name of the property to update. If not given, then DOM nodes > * will be updated to reflect the current values of all properties. > */ > - updateAction(action, propertyName = null) { > + updateAction(action, propertyName = null, value) { Update the doc comment too, please. ::: browser/base/content/browser-pageActions.js:514 (Diff revision 1) > if (!title) { > // `title` is a required action property, but the bookmark action's is an > // empty string since its actual title is set via > // BookmarkingUI.updateBookmarkPageMenuItem(). The purpose of this early > // return is to ignore that empty title. > return; > } Does this still work? Why? ::: browser/base/content/browser-pageActions.js:535 (Diff revision 1) > - _updateActionTooltip(action) { > - let node = this.urlbarButtonNodeForActionID(action.id); > + _updateActionTooltip(action, tooltip = action.getTooltip(window), > + title = action.getTitle(window), Don't need a title here if a tooltip has been passed, so could leave that out and in the function itself do something like: ```js tooltip = tooltip || title; if (typeof tooltip == "undefined") { tooltip = action.getTitle(window); } ``` Note that both before and after these changes, I'm a little curious about add-ons or other consumers that explicitly want to set the tooltip to empty string. Doesn't seem like that's possible unless both `getTooltip` and `getTitle` return `""`. Maybe we want a follow-up for that, if that's actually a problem (hard for me to say about webextensions). ::: browser/modules/PageActions.jsm:726 (Diff revision 1) > + > + _createIconProperties(urls) { > + if (urls && typeof urls == "object") { > + let props = this._iconProperties.get(urls); > + if (!props) { > + props = Object.freeze({ Why the object.freeze here and below? ::: browser/modules/PageActions.jsm:735 (Diff revision 1) > + return Object.freeze({ > + "--pageAction-image-16px": null, > + "--pageAction-image-32px": urls ? escapeCSSURL(urls) : null, > + }); Why don't we store this one in `_iconProperties` ?
Attachment #8959431 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 6•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8959431 [details] Bug 1446250: Part 1 - Optimize Photon PageAction update performance. https://reviewboard.mozilla.org/r/228244/#review234348 The optimizations were added in a few different bugs over time. I can find you the specifics if you want. They were removed in [1]. But essentially: - We need to update the pageAction values on every location change, and since location changes happen often (e.g., on every tab switch), the work we do for the update needs to be as cheap as possible. It also needs to be as cheap as possible during startup. - The most expensive part of the update is calculating the icon values, both in terms of calculating the correct icon URL for different sizes, and in terms of generating/escaping the CSS property values. The old pageAction code handled this by caching generated CSS properties for a given URLs object, and making sure the exact same URLs object was always used for the same icon values. The Photon migration removed these caches, and actually caused us to calculate the preferred sizes twice for every update, with a bunch of duplicate window-specific value look-ups and garbage string generations in-between. - Beyond that, just looking up the stored values for a property can be expensive. The optimized pageAction code had one object containing all of the values needed to update the icon for a given tab, and it was only looked up once for any update. The Photon version... is complicated. For one tab update, the value winds up being looked up at least a dozen times for every tab switch, and the look-up for global values requires generating property strings, which aside from being constructed need to be atomized and then eventually GCed. - And, of course, the previous pageAction code looked up the icon that it was updating only once for every location change, and set relatively cheap DOM attributes in an animation frame callback. In the Photon code, each property change requires generating *at least* two element ID strings, one constructed update method name (which needs to be atomized and GCed in the end, and can't be JIT optimized), 3 for-of loops, and a half dozen map look-ups. So, basically, what was once an extremely cheap operation (and almost free in the common case) became extremely expensive. [1]: https://searchfox.org/mozilla-central/diff/37cb69c21b066708206113df1fd2c6d5c957f501/browser/components/extensions/ext-pageAction.js#120 > Does this still work? Why? It works the same as it did before. I'm not sure why it wouldn't. > Don't need a title here if a tooltip has been passed, so could leave that out and in the function itself do something like: > > ```js > tooltip = tooltip || title; > if (typeof tooltip == "undefined") { > tooltip = action.getTitle(window); > } > ``` > > Note that both before and after these changes, I'm a little curious about add-ons or other consumers that explicitly want to set the tooltip to empty string. Doesn't seem like that's possible unless both `getTooltip` and `getTitle` return `""`. Maybe we want a follow-up for that, if that's actually a problem (hard for me to say about webextensions). My impresion was that an empty tooltip was disallowed in the same way as an empty title. For the WebExtension case, at least, tooltip and title are the same. If there are other consumers that want empty tooltips, I suppose treating an empty string specially might make sense, but that's a different bug. > Why the object.freeze here and below? Just as a precaution, since we return the objects from a public API, and we really don't want them to be changed, especially when they're cached. > Why don't we store this one in `_iconProperties` ? Well, for a few reasons: 1) This is much cheaper to compute than the object version. 2) This case doesn't matter for the WebExtension case, since we always pass objects, and that was the case that particularly needed optimization. 3) `_iconProperties` is a `WeakMap`, so that its cached values are freed whenever the original icon URLs object is freed. Strings can't be used as `WeakMap` keys, so we'd need a strong map of generated objects for any URL string used ever. That seems like a footgun waiting to happen, and probably not worth the effort.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8959431 [details] Bug 1446250: Part 1 - Optimize Photon PageAction update performance. https://reviewboard.mozilla.org/r/228244/#review235070 Thanks for fixing this.
Attachment #8959431 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 10•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fb65e18a88ff8ba18013aecc4b0f7e32c4bf6d6 Bug 1446250: Part 1 - Optimize Photon PageAction update performance. r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/563f8e0d9d2dbf0eec8cb3f4afa0272f873e52ff Bug 1446250: Part 2 - Optimize/reduce calls into the Photon PageAction API. r=mixedpuppy
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1fb65e18a88f https://hg.mozilla.org/mozilla-central/rev/563f8e0d9d2d
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 12•6 years ago
|
||
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(kmaglione+bmo)
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(kmaglione+bmo) → qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•