Closed Bug 1446250 Opened 6 years ago Closed 6 years ago

Fix pageAction performance regression

Categories

(WebExtensions :: Frontend, defect)

defect
Not set
normal

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 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 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 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)
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/1fb65e18a88f
https://hg.mozilla.org/mozilla-central/rev/563f8e0d9d2d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1449200
No longer depends on: 1447943
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)
Product: Toolkit → WebExtensions
Flags: needinfo?(kmaglione+bmo) → qe-verify-
You need to log in before you can comment on or make changes to this bug.