Content script seems to be reported as something that dramatically slow down Firefox when executing Talos tests: - between 28 and 46% regression on tp5o responsiveness - 17 to 25% on ts_paint - 18 to 24% on session restore - 11% regression on tp5o - 6% on devtools - 5% regression on tpaint Full results over here: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=4df5cda574b9&newProject=try&newRevision=6e1bfd15a211&framework=1 (Remember to check the "Show only important changes" checkbox) Here is how I made this test: https://hg.mozilla.org/try/rev/44e57e6dfd8f704293493b62654e4c0ef14d1897 It simply bundles a web extension xpi with this manifest: { ... "content_scripts": [{ "matches": ["<all_urls>"], "js": [ "content-script.js" ] }] } And content-script.js is an empty file. I'm obviously expecting a regression to happen when using <all_urls> match pattern, but not that big. Such performance impact looks scary for users having addons with content scripts. It also prevents suggesting using Web extension for building new firefox features.

I think this is being caused by the generation of the API namespace objects for thousands of documents. I've actually been worrying about this lately, especially in combination with the out-of-process add-ons changes, so I'm looking into it.

It's not clear if you come to that point thanks to profile data, but it could be interesting to see the profiler output on these tests and see what is really slowing things down? I pushed a new try run following this doc: https://wiki.mozilla.org/Buildbot/Talos/Profiling https://treeherder.mozilla.org/#/jobs?repo=try&revision=357ef52754c8&selectedJob=29928594 The test ran, many failed due to the profile command I imagine. I tried looking at one run in tp5o, but I haven't seen any JS code running in the profile, but I don't know much about talos and even less in cleopatra. You can find the profile data by clicking on each individual run, in the Job details panel, there is link to files like "profile_tp5o_scroll.sps.zip"

No, I haven't done any direct profiling at this point, only re-run talos tests in a few configurations to confirm that injecting a content script into all URLs is indeed the problem. At this point, though, it doesn't really matter. It's clear enough that there's way too much overhead involved in creating those namespaces. If there's still a problem when the obvious issues are fixed, I'll look into profiling.

:andym pointed me here for an issue with lastpass 4.x beta from AMO causing tab switch hangs and the slow script popdown to appear. I got a profile: https://cleopatra.io/#report=59496d2c2608ae860a484b633dee5dd6e168e5bc

That's not related. This only shows up in real use when huge numbers of windows are being created. It's hard to make much of that profile, either way. Nearly all of the jank in it is from GeckoProfiler itself. Most of the rest is from the Tracking Protection Experiment, uBlock, and HTTPS Everywhere. The jank that *is* from LastPass is from the SDK version.

Seems like my suspicions were probably correct: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a9d88cc1fc8c&newProject=try&newRevision=6e1bfd15a211&framework=1

(In reply to Kris Maglione [:kmag] from comment #6) > Seems like my suspicions were probably correct: > https://treeherder.mozilla.org/perf.html#/ > compare?originalProject=try&originalRevision=a9d88cc1fc8c&newProject=try&newR > evision=6e1bfd15a211&framework=1 I'm confused. What are the two revisions being compared here? It looks like the "base" revision has the laziness changes but doesn't load an extension, while the "new" revision loads an extension but doesn't have the laziness changes.

(In reply to Bill McCloskey (:billm) from comment #11) > I'm confused. What are the two revisions being compared here? It looks like > the "base" revision has the laziness changes but doesn't load an extension, > while the "new" revision loads an extension but doesn't have the laziness > changes. Yeah, I screwed up the rebase for that try run. I'm currently waiting for a build with both the changes and the extension: https://treeherder.mozilla.org/#/jobs?repo=try&revision=188be7a86cbba32aa8c3eb5ef6cb57c4f565b7d2

Comment on attachment 8805889 [details] Bug 1312690: Cleanup. https://reviewboard.mozilla.org/r/89498/#review88966 ::: toolkit/components/extensions/Extension.jsm (Diff revision 1) > - // If we get a null object before reaching the requested path > - // (e.g. the API object is returned only on particular kind of contexts instead > - // of based on WebExtensions permissions, like it happens for the devtools APIs), > - // stop searching and return undefined. > - // TODO(robwu): This should never be reached. If an API is not available for > - // a context, it should be declared as such in the schema and enforced by > - // `shouldInject`, for instance using the same logic that is used to opt-in > - // to APIs in content scripts. > - // If this check is kept, then there is a discrepancy between APIs depending > - // on whether it is generated locally or remotely: Non-existing local APIs > - // are excluded in `shouldInject` by this check, but remote APIs do not have > - // this information and will therefore cause the schema API generator to > - // create an API that proxies to a non-existing API implementation. Should this start throwing now if its meant to be unreachable?

Comment on attachment 8805889 [details] Bug 1312690: Cleanup. https://reviewboard.mozilla.org/r/89498/#review88966 > Should this start throwing now if its meant to be unreachable? Yeah, I'm going to handle that in a follow-up, once I'm sure it doesn't break devtools APIs. For now, I just wanted to get rid of the distracting wall of text.

Comment on attachment 8805890 [details] Bug 1312690: Lazily initialize schema bindings. https://reviewboard.mozilla.org/r/89500/#review88986 ::: toolkit/components/extensions/ExtensionUtils.jsm:2096 (Diff revision 1) > const stylesheetMap = new DefaultMap(url => { > let uri = NetUtil.newURI(url); > return styleSheetService.preloadSheet(uri, styleSheetService.AGENT_SHEET); > }); > > +function defineLazyGetter(object, prop, getter) { This doesn't appear to be used here? ::: toolkit/components/extensions/Schemas.jsm:67 (Diff revision 1) > } > }); > }); > } > > +function defineLazyGetter(object, prop, getter) { A comment explaining that this differs from the XPCOMUtils version in its handling of cross-compartment wrapping would be helpful. ::: toolkit/components/extensions/Schemas.jsm:1975 (Diff revision 1) > return ns.permissions.some(perm => wrapperFuncs.hasPermission(perm)); > } > return true; > }, > > + defineLazyGetter, I don't see where this gets used? ::: toolkit/components/extensions/Schemas.jsm:1988 (Diff revision 1) > * interface, which runs the actual functionality of the generated API. > */ > inject(dest, wrapperFuncs) { > let context = new InjectionContext(wrapperFuncs); > > - for (let [namespace, ns] of this.namespaces) { > + let nestedNamespaces = new DeepMap(); I think this would be marginally easier to follow if you move this to right before the for loop that populates it. ::: toolkit/components/extensions/Schemas.jsm:2005 (Diff revision 1) > + let apiImpl = context.getImplementation(ns.name, name); > + entry.inject(apiImpl, [ns.name], name, obj, context); > } > } > > // Remove the namespace object if it is empty nit: revise this to something like "Only inject this entry if it is not empty" ::: toolkit/components/extensions/Schemas.jsm:2039 (Diff revision 1) > + > + if (ns.name.includes(".")) { > + let path = ns.name.split("."); > + let leafName = path.pop(); > > - // Copy the apiObj as the final nested level. > + let parent = path.reduce((ns, prop) => ns.get(prop), I think this would also be slightly more readable if you put the reduce into a method on DeepMap

Comment on attachment 8805890 [details] Bug 1312690: Lazily initialize schema bindings. https://reviewboard.mozilla.org/r/89500/#review89006 Whoops, hit publish too fast, most of the comments are style-nits looks good with removal of the unused code.

Comment on attachment 8805890 [details] Bug 1312690: Lazily initialize schema bindings. https://reviewboard.mozilla.org/r/89500/#review88986 > This doesn't appear to be used here? It just wound up in the wrong patch after I did some rebasing. It's used in the subsequent patches. > A comment explaining that this differs from the XPCOMUtils version in its handling of cross-compartment wrapping would be helpful. Good point. The name should probably reflect that too... > I don't see where this gets used? Subsequent patches. > I think this would be marginally easier to follow if you move this to right before the for loop that populates it. Yeah. For some reason I was thinking it was used by one of the helper functions. > nit: revise this to something like "Only inject this entry if it is not empty" Well, it does actually wind up getting removed, when we return undefined. The getter has already been injected at this point. > I think this would also be slightly more readable if you put the reduce into a method on DeepMap Hm. Yeah, I suppose it would.

Comment on attachment 8805890 [details] Bug 1312690: Lazily initialize schema bindings. https://reviewboard.mozilla.org/r/89500/#review88986 > It just wound up in the wrong patch after I did some rebasing. It's used in the subsequent patches. Okay so how does it differ from the XPCOMUtils version? From a quick glance it looks like it handles writing before reading which the existing one doesn't and it binds this to the containing object when the callback is invoked?

Comment on attachment 8805890 [details] Bug 1312690: Lazily initialize schema bindings. https://reviewboard.mozilla.org/r/89500/#review88986 > Okay so how does it differ from the XPCOMUtils version? From a quick glance it looks like it handles writing before reading which the existing one doesn't and it binds this to the containing object when the callback is invoked? The main difference is that it defines the result on `this` rather than on the object it was passed, so that it can be defined on a prototype rather than on each individual instance.

Comment on attachment 8805891 [details] Bug 1312690: Lazily initialize extension APIs. https://reviewboard.mozilla.org/r/89502/#review89106 ::: browser/components/extensions/ext-utils.js:116 (Diff revision 1) > + close() { > + this.closePopup(); > + } What's the connection between this and the rest of the patch? ::: toolkit/components/extensions/Extension.jsm:278 (Diff revision 1) > this.currentMessageManager = xulBrowser.messageManager; > this._docShellTracker = new BrowserDocshellFollower(xulBrowser, > this.onBrowserChange.bind(this)); > - this.principal_ = principal; > > - this.apiObj = {}; > + Object.defineProperty(this, "principal", { Any reason not to rename the parameter and just set `this.principal = principal_;` ::: toolkit/components/extensions/Extension.jsm:316 (Diff revision 1) > super.unload(); > Management.emit("proxy-context-unload", this); > } > } > > +defineLazyGetter(ProxyContext.prototype, "apiObj", function() { From just reading the code this doesn't seem right, it looks like the lazy getter will store apiObj/sandbox right on the ProxyContext prototype rather than on individual instances of ProxyContext. But its late, I must be misunderstanding...?

Comment on attachment 8805891 [details] Bug 1312690: Lazily initialize extension APIs. https://reviewboard.mozilla.org/r/89502/#review89106 > From just reading the code this doesn't seem right, it looks like the lazy getter will store apiObj/sandbox right on the ProxyContext prototype rather than on individual instances of ProxyContext. > But its late, I must be misunderstanding...? Okay your comment on the other patch and this one crossed in mid-air. First off, are you going to move defineLazyGetter in ExtensionUtils over to this patch? Regardless, I think that (like the version in Schemas), it should at the very least have a comment explaining the difference and ideally a different name so readers less familiar with the details than you don't get easily confused.

Comment on attachment 8805891 [details] Bug 1312690: Lazily initialize extension APIs. https://reviewboard.mozilla.org/r/89502/#review89106 > What's the connection between this and the rest of the patch? With the lazy instantiation, no context is created for windows where no API is touched, so we need to listen for extension shutdown directly rather than listening for a page-close event. The page-close event was kind of dodgy, anyway, and not OOP compatible, so this change was coming regardless. > Any reason not to rename the parameter and just set `this.principal = principal_;` The problem isn't the parameter name. It's that `principal` is defined as a getter on the base class. > Okay your comment on the other patch and this one crossed in mid-air. > First off, are you going to move defineLazyGetter in ExtensionUtils over to this patch? > Regardless, I think that (like the version in Schemas), it should at the very least have a comment explaining the difference and ideally a different name so readers less familiar with the details than you don't get easily confused. Nope, it stores it on `this`, which will be an object instance rather than the prototype.

Actual valid talos run: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=6e1bfd15a211&newProject=try&newRevision=114070058f56&framework=1&showOnlyImportant=1

Comment on attachment 8805891 [details] Bug 1312690: Lazily initialize extension APIs. https://reviewboard.mozilla.org/r/89502/#review89728 ::: toolkit/components/extensions/ExtensionUtils.jsm:2182 (Diff revision 2) > + * Importantly, this means that a lazy getter defined on an object > + * prototype will be invoked separately for each object instance that > + * it's accessed on. > + * > + * @param {object} object > + * The object on which to define the getter. I know you mentioned it above but for extreme pedantry, have this say "The prototype or object on which to ..." ?

Comment on attachment 8805892 [details] Bug 1312690: Lazily initialize binding implementation objects. https://reviewboard.mozilla.org/r/89504/#review89812

(In reply to Kris Maglione [:kmag] from comment #32) > Actual valid talos run: > > https://treeherder.mozilla.org/perf.html#/ > compare?originalProject=try&originalRevision=6e1bfd15a211&newProject=try&newR > evision=114070058f56&framework=1&showOnlyImportant=1 \o/ Looks promising!

Comment on attachment 8806550 [details] Bug 1312690: Load content scripts asynchronously when possible. https://reviewboard.mozilla.org/r/89946/#review89816 ::: toolkit/components/extensions/ExtensionContent.jsm:186 (Diff revision 1) > runSafeSyncWithoutClone(winUtils.removeSheetUsingURIString, url, winUtils.AUTHOR_SHEET); > } > } > }, > > - tryInject(window, sandbox, shouldRun) { > + tryInject(window, sandbox, shouldRun, when) { While you're here, could you add documentation about this method (the meanings of its parameters in particular) here?

Comment on attachment 8806551 [details] Bug 1312690: Create content script sandboxes in same zone as content window. https://reviewboard.mozilla.org/r/89948/#review89818 This seems like a good thing to do but I'm not sure how its connected to this bug? Is it just that it reduces inefficiency in GC when using content scripts?

Comment on attachment 8806552 [details] Bug 1312690: Speed up MatchPattern matching. https://reviewboard.mozilla.org/r/89950/#review89820 ::: toolkit/modules/addons/MatchPattern.jsm:113 (Diff revision 1) > }; > > MatchPattern.prototype = { > // |uri| should be an nsIURI. > matches(uri) { > - for (let matcher of this.matchers) { > + return this.mathers.some(matcher => matcher.matches(uri)); looks like typos here and below, how is this passing tests?

Comment on attachment 8806551 [details] Bug 1312690: Create content script sandboxes in same zone as content window. https://reviewboard.mozilla.org/r/89948/#review89818 And memory allocation in general, yes.

Comment on attachment 8806552 [details] Bug 1312690: Speed up MatchPattern matching. https://reviewboard.mozilla.org/r/89950/#review89820 > looks like typos here and below, how is this passing tests? Heh. Indeed. I think I must have introduced the typo when fixing a merge conflict.

Comment on attachment 8806553 [details] Bug 1312690: Remove now-useless .matches() call from tryInject. https://reviewboard.mozilla.org/r/89952/#review89826 Can you give more context here? Its not clear to me why this is now-useless

Comment on attachment 8806554 [details] Bug 1312690: Add some stupid microoptimizations. https://reviewboard.mozilla.org/r/89954/#review89828

Comment on attachment 8806553 [details] Bug 1312690: Remove now-useless .matches() call from tryInject. https://reviewboard.mozilla.org/r/89952/#review89826 We check whether the script matches before attaching it to the context now, so we don't get any `tryInject` calls where it doesn't match.

Comment on attachment 8806553 [details] Bug 1312690: Remove now-useless .matches() call from tryInject. https://reviewboard.mozilla.org/r/89952/#review89840

Comment on attachment 8806552 [details] Bug 1312690: Speed up MatchPattern matching. https://reviewboard.mozilla.org/r/89950/#review89842

Comment on attachment 8806550 [details] Bug 1312690: Load content scripts asynchronously when possible. https://reviewboard.mozilla.org/r/89946/#review89848 Thanks for the clarification. I think this code overall would be a little simpler to read and understand if `shouldRun` was replaced with a boolean flag called `exactState` or something and the logic for `isWhenBeforeOrSame()` just moved inside `tryInject()` and was run conditionally based on that flag. But that's a nit, feel free to take it or leave it.

Comment on attachment 8806550 [details] Bug 1312690: Load content scripts asynchronously when possible. https://reviewboard.mozilla.org/r/89946/#review89848 It would definitely be nice, but it's another bug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/daeb765e4bf6fa3a16f6382f3a6e33b3282a3948 Bug 1312690: Cleanup. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/533878a315ea839c15f48185dda9c9cd4f3ca2a8 Bug 1312690: Lazily initialize schema bindings. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/b253db09499c1fb2af39193ac61eaa29e63f496e Bug 1312690: Lazily initialize extension APIs. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/308aaa7e7a0800d12386c2d8cc2b20f0cccd6e91 Bug 1312690: Lazily initialize binding implementation objects. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/a75b8c557c65d588f8e20fece25c32b68988cc70 Bug 1312690: Load content scripts asynchronously when possible. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/14b7fabb7657bfe987ec0cc2b07d9d784001addd Bug 1312690: Create content script sandboxes in same zone as content window. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/11f20c7b030cb1e08f191bc29b520162d4a9971c Bug 1312690: Speed up MatchPattern matching. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/12ef342043e97b556c794efc3cf6f2f4bdf46a5d Bug 1312690: Remove now-useless .matches() call from tryInject. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/5e3d6fb19a2b91400d3baa26c868da621df8e700 Bug 1312690: Add some stupid microoptimizations. r=aswan

https://hg.mozilla.org/mozilla-central/rev/daeb765e4bf6 https://hg.mozilla.org/mozilla-central/rev/533878a315ea https://hg.mozilla.org/mozilla-central/rev/b253db09499c https://hg.mozilla.org/mozilla-central/rev/308aaa7e7a08 https://hg.mozilla.org/mozilla-central/rev/a75b8c557c65 https://hg.mozilla.org/mozilla-central/rev/14b7fabb7657 https://hg.mozilla.org/mozilla-central/rev/11f20c7b030c https://hg.mozilla.org/mozilla-central/rev/12ef342043e9 https://hg.mozilla.org/mozilla-central/rev/5e3d6fb19a2b

I ran Talos again with these patches landed and there is still an impact: https://treeherder.mozilla.org/perf.html#/compare?originalProject=autoland&originalRevision=45ed47afd518&newProject=try&newRevision=bd55a48ea16a&framework=1 - sessionrestore 15% - ts_paint 14% - tp5o responsiveness 9% - devtools 4% These patches reduced the regression a lot, especially on tp5o responsiveness. But content scripts still have a significant impact on talos numbers. It doesn't seem to have improved ts_paint. Should I open another bug?

I'm a bit skeptical of some of the numbers. I'd like to see a comparison against a base run on try rather than autoland. But, yes, feel free to file another bug once you have that. Thanks!