Closed Bug 1397448 Opened 7 years ago Closed 7 years ago

Lots of HTTP connections result in poor performance caused by MessageChannel.jsm

Categories

(WebExtensions :: Request Handling, enhancement)

enhancement
Not set
normal

Tracking

(Performance Impact:high, firefox57 fixed)

RESOLVED FIXED
mozilla57
Performance Impact high
Tracking Status
firefox57 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

+++ This bug was initially created as a clone of Bug #1396856 +++

Splitting off a separate bug for the MessageChannel.jsm overhead. There might be a bit more we can do about this in the short term, but the biggest improvements would probably come from removing cross-compartment wrapper overhead and WebIDLifying the MessageManager bindings.
Blocks: 1386895
Kris, do you think you can get this bug fixed by Sept 15.  If not, I will change it to P2 for post 57.
Whiteboard: [qf] → [qf:p1]
I'm going to do some profiling with the patches from bug 1186409 applied, and see if there are any easy wins. If there are, I'll make whatever improvements I can.

I think it might be better to just focus on converting message managers to WebIDL (bug 888600), if there's any chance of landing that in 57. I don't think it would be especially risky, but it would still be a fairly large patchset.
Depends on: 1381961
Comment on attachment 8906184 [details]
Bug 1397448: Part 2 - Speed up about:addon child frame checks.

https://reviewboard.mozilla.org/r/177944/#review182952
Attachment #8906184 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8906183 [details]
Bug 1397448: Part 1 - Generate WebRequest message objects in WebRequest.jsm.

https://reviewboard.mozilla.org/r/177942/#review182954
Attachment #8906183 - Flags: review?(mixedpuppy) → review+
These changes shave off about another 20% of the overhead in my tests. The big white elephant in the room is still the message manager bindings. The XPConnect overhead from those shows up in several different places in all of the profiles I've done. And it doesn't help that we wind up having to separately create the bindings in at least three different compartments (sometimes 4) just to send one message.
Comment on attachment 8906223 [details]
Bug 1397448: Part 6 - Cache messageManager for MessageManagerProxy.

https://reviewboard.mozilla.org/r/177978/#review182998

::: toolkit/components/extensions/ExtensionUtils.jsm:494
(Diff revision 1)
> -        value: target,
> -        configurable: true,
> -        writable: true,
> -      });
>      } else {
> +      this.messageManager = target.messageManager;

addListeners is also doing this.

::: toolkit/components/extensions/ExtensionUtils.jsm:539
(Diff revision 1)
>    /**
>     * @property {nsIMessageSender|null} messageManager
>     *        The message manager that is currently being proxied. This
>     *        may change during the life of the proxy object, so should
>     *        not be stored elsewhere.
>     */

comment no longer makes sense here

::: toolkit/components/extensions/ExtensionUtils.jsm:619
(Diff revision 1)
>     */
>    addListeners(target) {
>      target.addEventListener("SwapDocShells", this);
>  
> +    this.eventTarget = target;
> +    this.messageManager = target.messageManager;

Didn't setting this.messageManager already happen above?  Or is addListeners called otherwise?

::: toolkit/components/extensions/ExtensionUtils.jsm:634
(Diff revision 1)
>     *
>     * @param {Element} target
>     *        The target element.
>     */
>    removeListeners(target) {
>      target.removeEventListener("SwapDocShells", this);

Should this.eventTarget be null'd?
Attachment #8906223 - Flags: review?(mixedpuppy)
Comment on attachment 8906223 [details]
Bug 1397448: Part 6 - Cache messageManager for MessageManagerProxy.

https://reviewboard.mozilla.org/r/177978/#review182998

> addListeners is also doing this.

Eh. It wasn't when I wrote this line.

> comment no longer makes sense here

Yes it does. The semantics are the same, the only difference is that we cache the property whenever it changes rather than retrieving it from the <browser> every time.

> Didn't setting this.messageManager already happen above?  Or is addListeners called otherwise?

It's also called from "SwapDocShells" event listeners.

> Should this.eventTarget be null'd?

It could be, but it's handled by `addListeners` or `dispose` already.
Comment on attachment 8906223 [details]
Bug 1397448: Part 6 - Cache messageManager for MessageManagerProxy.

https://reviewboard.mozilla.org/r/177978/#review182998

> Yes it does. The semantics are the same, the only difference is that we cache the property whenever it changes rather than retrieving it from the <browser> every time.

What I meant is that is seems out of place with the getter removed.
Comment on attachment 8906223 [details]
Bug 1397448: Part 6 - Cache messageManager for MessageManagerProxy.

https://reviewboard.mozilla.org/r/177978/#review183010
Attachment #8906223 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8906223 [details]
Bug 1397448: Part 6 - Cache messageManager for MessageManagerProxy.

https://reviewboard.mozilla.org/r/177978/#review182998

> What I meant is that is seems out of place with the getter removed.

JSDoc doesn't have any particular syntax support for properties, and neither do JS classes. So it doesn't really matter where the doc comment is, as long as it's within the class definition and it has the right name.
Comment on attachment 8906185 [details]
Bug 1397448: Part 3 - Reduce the number of promise callbacks created in MessageChannel.

https://reviewboard.mozilla.org/r/177946/#review183020

::: toolkit/components/extensions/MessageChannel.jsm
(Diff revision 2)
>      });
>      deferred.sender = recipient;
>      deferred.messageManager = target;
>      deferred.channelId = channelId;
>  
> -    this._addPendingResponse(deferred);

`_addPendingResponse()` is now unused, so please remove it.  Also, it has a significant jsdoc, so you should also move it somewhere appropriate.
Attachment #8906185 - Flags: review?(tomica) → review+
Comment on attachment 8906186 [details]
Bug 1397448: Part 4 - Use a simpler message broker for response messages.

https://reviewboard.mozilla.org/r/177948/#review183024

::: toolkit/components/extensions/MessageChannel.jsm:224
(Diff revision 2)
>      }
>    }
>  }
>  
>  /**
> + * A simplified subclass of FilteringMessageManager that only supports

nit:  these should really be named `*Broker`, and it probably makes sense for the simpler class to be the base.

::: toolkit/components/extensions/MessageChannel.jsm:237
(Diff revision 2)
> +    }
> +  }
> +
> +  addHandler(messageName, handler) {
> +    if (DEBUG && this.handlers.has(messageName)) {
> +      throw new Error(`Handler already registered for response ID ${messageName}`);

Why throw only in DEBUG builds?  Or is the `.has` check that hot here?

::: toolkit/components/extensions/MessageChannel.jsm:239
(Diff revision 2)
> +
> +  addHandler(messageName, handler) {
> +    if (DEBUG && this.handlers.has(messageName)) {
> +      throw new Error(`Handler already registered for response ID ${messageName}`);
> +    }
> +    this.handlers.set(messageName, handler);

So we created a new Set for each `sendMessage` call?  Uh.
Attachment #8906186 - Flags: review?(tomica) → review+
Comment on attachment 8906187 [details]
Bug 1397448: Part 5 - Make uniqueProcessID a lexically scoped string.

https://reviewboard.mozilla.org/r/177950/#review183026
Attachment #8906187 - Flags: review?(tomica) → review+
Comment on attachment 8906186 [details]
Bug 1397448: Part 4 - Use a simpler message broker for response messages.

https://reviewboard.mozilla.org/r/177948/#review183024

> nit:  these should really be named `*Broker`, and it probably makes sense for the simpler class to be the base.

Eh, that's how I originally named them, but Bill asked me to rename them.

> Why throw only in DEBUG builds?  Or is the `.has` check that hot here?

Because the check is relatively expensive and this is a hot code path.
Comment on attachment 8906185 [details]
Bug 1397448: Part 3 - Reduce the number of promise callbacks created in MessageChannel.

https://reviewboard.mozilla.org/r/177946/#review183020

> `_addPendingResponse()` is now unused, so please remove it.  Also, it has a significant jsdoc, so you should also move it somewhere appropriate.

Yes, I left it there because I didn't feel like rewriting the comment and the comments that referenced it :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9322f67548f0d2a7f82bbaadf69b22dc7049558c
Bug 1397448: Part 1 - Generate WebRequest message objects in WebRequest.jsm. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6aa1cb3ad1116d755f0bd712f990e239e57df77
Bug 1397448: Part 2 - Speed up about:addon child frame checks. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc7cd6e70a267dcca38e50450ded275e48cbc788
Bug 1397448: Part 3 - Reduce the number of promise callbacks created in MessageChannel. r=zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdacd79919672692979ea0ad20d100861c46a2fe
Bug 1397448: Part 4 - Use a simpler message broker for response messages. r=zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/09cd911ffca7302090d78b6d068066dee1ee7540
Bug 1397448: Part 5 - Make uniqueProcessID a lexically scoped string. r=zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/10629cc177b4a3925d5547bf94df550c36b1b155
Bug 1397448: Part 6 - Cache messageManager for MessageManagerProxy. r=mixedpuppy
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)
Flags: needinfo?(kmaglione+bmo) → qe-verify-
Blocks: 1457477
Product: Toolkit → WebExtensions
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.