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)
WebExtensions
Request Handling
Tracking
(Performance Impact:high, firefox57 fixed)
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
(Blocks 1 open bug)
Details
Attachments
(6 files)
(deleted),
text/x-review-board-request
|
mixedpuppy
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
mixedpuppy
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
zombie
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
zombie
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
zombie
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
mixedpuppy
:
review+
|
Details |
+++ 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.
Comment 1•7 years ago
|
||
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]
Assignee | ||
Comment 2•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
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 9•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 10•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 12•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•7 years ago
|
||
mozreview-review-reply |
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 21•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 22•7 years ago
|
||
mozreview-review-reply |
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 23•7 years ago
|
||
mozreview-review |
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 24•7 years ago
|
||
mozreview-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 25•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 26•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 27•7 years ago
|
||
mozreview-review-reply |
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 :)
Assignee | ||
Comment 28•7 years ago
|
||
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
Comment 29•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9322f67548f0
https://hg.mozilla.org/mozilla-central/rev/b6aa1cb3ad11
https://hg.mozilla.org/mozilla-central/rev/bc7cd6e70a26
https://hg.mozilla.org/mozilla-central/rev/fdacd7991967
https://hg.mozilla.org/mozilla-central/rev/09cd911ffca7
https://hg.mozilla.org/mozilla-central/rev/10629cc177b4
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 30•7 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)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(kmaglione+bmo) → qe-verify-
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Updated•3 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•