1391310 - context.wrapPromise is too slow

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Kris Maglione [:kmag]]

The main issue is a bunch of spread args/rest args round trips that the JIT doesn't handle very well, combined with lots of cross-compartment wrapper overhead, and even more X-Ray overhead.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1391310: Optimize runSafe/wrapPromise to avoid wrapper/spread arg/rest arg overhead.

Lots of little bits of overhead add up to a significant amount of overhead
over the many, many times this function is called.

Review commit: https://reviewboard.mozilla.org/r/169748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169748/

Comment 2

[Tomislav Jovanovic :zombie]

Comment on attachment 8898374 [details]
Bug 1391310: Optimize runSafe/wrapPromise to avoid wrapper/spread arg/rest arg overhead.

https://reviewboard.mozilla.org/r/169748/#review175318

::: toolkit/components/extensions/ExtensionCommon.jsm:252
(Diff revision 1)
>  
>    get principal() {
>      throw new Error("Not implemented");
>    }
>  
> -  runSafe(...args) {
> +  runSafe(callback, ...args) {

nit: it looks like there are maybe 2 other callers of this, so perhaps totally replace all with `applySafe`?

::: toolkit/components/extensions/ExtensionCommon.jsm:268
(Diff revision 1)
>      } else if (!this.active) {
>        Cu.reportError("context.runSafe called while context is inactive");
>      } else {
> -      return runSafeSync(this, ...args);
> +      try {
> +        let {cloneScope} = this;
> +        args = args.map(arg => Cu.cloneInto(arg, cloneScope));

The array will be in a different scope from the arguments.

::: toolkit/components/extensions/ExtensionCommon.jsm:439
(Diff revision 1)
>     *
>     * @returns {Promise|undefined} If callback is null, a promise object
>     *     belonging to the target scope. Otherwise, undefined.
>     */
>    wrapPromise(promise, callback = null) {
> -    let runSafe = this.runSafe.bind(this);
> +    let runSafe = this.applySafe.bind(this);

nit: rename to `applySafe`, the diff below was confusing for a bit (though in part likely because before coffee ;)

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8898374 [details]
Bug 1391310: Optimize runSafe/wrapPromise to avoid wrapper/spread arg/rest arg overhead.

https://reviewboard.mozilla.org/r/169748/#review175318

> The array will be in a different scope from the arguments.

That's not a problem. Reflect.apply is in the same scope as the array, so we want the array to be in our scope, and the arguments to be in the callee scope.

Comment 4

[Tomislav Jovanovic :zombie]

Comment on attachment 8898374 [details]
Bug 1391310: Optimize runSafe/wrapPromise to avoid wrapper/spread arg/rest arg overhead.

https://reviewboard.mozilla.org/r/169748/#review175318

> That's not a problem. Reflect.apply is in the same scope as the array, so we want the array to be in our scope, and the arguments to be in the callee scope.

Yeah, that was a comment to myself, figured it out later but missed to drop it. ;)

Comment 5

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e849e7a75c15c43e5b10618a381d79a05a72aaa
Bug 1391310: Optimize runSafe/wrapPromise to avoid wrapper/spread arg/rest arg overhead. r=zombie

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0e849e7a75c1

Comment 7

[marius.santa]

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.