Closed Bug 1317697 Opened 8 years ago Closed 7 years ago

Investigate if we can load ExtensionContent.jsm lazily

Categories

(WebExtensions :: General, defect, P1)

Product:
WebExtensions
Component:
General
Type:
defect

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: gkrizsanits, Assigned: kmag)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [e10s-multi:+] triaged)

Attachments

(6 files, 2 obsolete files)

Bug 1317697: Fix reference to undefined property warnings.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1317697: Fix handling of more canceled responses.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1317697: Lazily initialize Schemas.jsm.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1317697: Split ExtensionContent.jsm into a stub process script.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1317697: Split extension page child and content script child code as much as possible.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1317697: Remove things from ExtensionUtils that don't belong there.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details 
To improve content process startup I'm trying to load a few things lazily at startup if it's possible. I wonder if we need everything from ExtensionContent.jsm right from startup or is there anything we can load lazily.
Blocks: 1317304
Whiteboard: [e10s-multi:?]
We can probably load most of it lazily, yeah. We'd at least need some skeleton bootstrap code loaded from the start, though.
Priority: -- → P1
Whiteboard: [e10s-multi:?] → [e10s-multi:?] triaged
Assignee: nobody → kmaglione+bmo
Blocks: 1336380
Whiteboard: [e10s-multi:?] triaged → [e10s-multi:+] triaged
Attachment #8855629 - Attachment is obsolete: true
Attachment #8855629 - Flags: review?(mixedpuppy)
After these changes, sessionrestore overhead is down to somewhere between 1 and 10ms with a simple content script add-on installed, and about 10ms faster than the previous baseline for no_autorestore, since we previously loaded ExtensionContent.jsm even when no extension was installed. ts_paint overhead is likewise down to ~4-20ms (depending on Linux vs. win32), and about a 10-20ms improvement over the previous baseline in e10s. tp5o is about the same as the previous baseline and tp5o responsiveness is improved by ~2-5%.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=21062cd63dd72565a4f9afc35bb4f6f78726096c&newProject=try&newRevision=d381550636c72215f54f9f1bb35a7df450885b55&framework=1&filter=session&showOnlyImportant=0
Comment on attachment 8855627 [details]
Bug 1317697: Fix reference to undefined property warnings.

https://reviewboard.mozilla.org/r/127488/#review131046

logic isn't right
Attachment #8855627 - Flags: review?(mixedpuppy) → review-
Comment on attachment 8855628 [details]
Bug 1317697: Fix handling of more canceled responses.

https://reviewboard.mozilla.org/r/127490/#review131048

The commit message says this fixes something, but the patch merely adds channelId.
Attachment #8855628 - Flags: review?(mixedpuppy) → review-
Comment on attachment 8855628 [details]
Bug 1317697: Fix handling of more canceled responses.

https://reviewboard.mozilla.org/r/127490/#review131048

Correct. Adding the channel ID fixes the canceled response handling for those objects.
Comment on attachment 8855630 [details]
Bug 1317697: Lazily initialize Schemas.jsm.

https://reviewboard.mozilla.org/r/127494/#review131060
Attachment #8855630 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8855648 [details]
Bug 1317697: Remove things from ExtensionUtils that don't belong there.

https://reviewboard.mozilla.org/r/127512/#review131064
Attachment #8855648 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8855951 [details]
Bug 1317697: Update initial process data after permission changes.

https://reviewboard.mozilla.org/r/127828/#review131074

Tests for this are difficult to say the least, r+ with a followup bug to look into that.
Attachment #8855951 - Flags: review?(mixedpuppy) → review+
(In reply to Kris Maglione [:kmag] from comment #31)
> Comment on attachment 8855628 [details]
> Bug 1317697: Fix handling of more canceled responses.
> 
> https://reviewboard.mozilla.org/r/127490/#review131048
> 
> Correct. Adding the channel ID fixes the canceled response handling for
> those objects.

Is there a test that currently covers that (and which one?), if not can we add one?
(In reply to Shane Caraveo (:mixedpuppy) from comment #35)
> (In reply to Kris Maglione [:kmag] from comment #31)
> > Correct. Adding the channel ID fixes the canceled response handling for
> > those objects.
> 
> Is there a test that currently covers that (and which one?), if not can we
> add one?

Not really. This is mostly meant to reduce confusing noise in tests, so it's not meant to be perfect, and it would be hard to test reliably. I could add a test for it, but if it breaks, people will notice the extra noise in their tests pretty quickly.
Blocks: 1355221
Comment on attachment 8855628 [details]
Bug 1317697: Fix handling of more canceled responses.

https://reviewboard.mozilla.org/r/127490/#review131092

Kris explained on irc, this property is checked has a part of abortedResponses, but was missing in these cases.
Attachment #8855628 - Flags: review- → review+
Comment on attachment 8855631 [details]
Bug 1317697: Split ExtensionContent.jsm into a stub process script.

https://reviewboard.mozilla.org/r/127496/#review131128

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_cache.html:70
(Diff revision 5)
>      chromeScript = SpecialPowers.loadChromeScript(() => {
>        addMessageListener("check-script-cache", extensionId => {
>          let {ExtensionManager} = Components.utils.import("resource://gre/modules/ExtensionContent.jsm", {});
>          let ext = ExtensionManager.extensions.get(extensionId);
>  
> +        if (ext) {

Why wouldn't there be an extension here?
Comment on attachment 8855631 [details]
Bug 1317697: Split ExtensionContent.jsm into a stub process script.

https://reviewboard.mozilla.org/r/127496/#review131128

> Why wouldn't there be an extension here?

Because we create the extension instance lazily the first time it's needed, and it's not actually needed in the parent process in this test.
Attachment #8855951 - Attachment is obsolete: true
Comment on attachment 8855627 [details]
Bug 1317697: Fix reference to undefined property warnings.

https://reviewboard.mozilla.org/r/127488/#review132726
Attachment #8855627 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8855631 [details]
Bug 1317697: Split ExtensionContent.jsm into a stub process script.

https://reviewboard.mozilla.org/r/127496/#review133018
Attachment #8855631 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8855632 [details]
Bug 1317697: Split extension page child and content script child code as much as possible.

https://reviewboard.mozilla.org/r/127498/#review133020
Attachment #8855632 - Flags: review?(mixedpuppy) → review+
Depends on: 1357137
Blocks: 1355569
This broke the Bugzilla Socorro Lens extension. Any suggestions how I can fix this are welcome on https://github.com/ashughes1/bugzilla-socorro-lens/issues/20.
Depends on: 1362047
Blocks: webext-perf
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: