Most SDK add-ons immediately load scores of SDK modules on startup, even if they're not needed immediately. Due to the way most of the SDK is written, it's hard to make a drastic reduction here, but we can still make a pretty significant one.

The loader optimization patches aren't strictly related to the lazy loading, but they came about while I was profiling those changes, and most of them are minor enough that I didn't think they deserved a separate bug. There aren't a lot of areas for huge savings on overhead in the loader code at this point, but the minor optimizations in those patches add up to about a 35% savings (or ~100ms) in the main process, in my testing. The lazy loading changes make a bigger difference in overall startup overhead. With the set of add-ons I tested, I went from around: Main Process Content Process Time spent in require() 5526ms 1808ms loading code 5173ms 1717ms overhead 352ms 91ms to around: Main Process Content Process Time spent in require() 3793ms 943ms loading code 3586ms 887ms overhead 206ms 56ms which is a difference of around: Main Process Content Process Time spent in require() 1733ms 865ms loading code 1587ms 830ms overhead 146ms 35ms I don't think there's much more room to decrease overhead at this point without huge investments of tiem and energy. The most promising change beyond this, I think, would be loading add-on modules into a single sandbox along with system modules. But that should probably be opt-in, at least at the start.

Comment on attachment 8807018 [details] Bug 1314861: Add defineLazyGetter global to SDK loader modules. https://reviewboard.mozilla.org/r/90288/#review90756 Sorry about the review delay. Tests and basic documentation in code are really missing. This function is all but simple, especially due to its polymorphism. Speaking about that, do we really need such complexity? Devtools uses "lazyRequireGetter" with a simplier behavior: lazyRequireGetter(scopeToInjectInto, modulePath, SymbolToExport, BooleanTrueIfTheModuleExportNeedsToBeExported) * Passing the scope object, which in 99% of the usecases is going to be `this`, makes this function less magical. * It requires one call to lazyRequireGetter per symbol to import I found it more than enough in devtools, but SDK is special with its extreme usage of modularity! So I'm open to see lazyRequire simplier, otherwise tests and nice documentation would be enough to move forward.

Comment on attachment 8807018 [details] Bug 1314861: Add defineLazyGetter global to SDK loader modules. https://reviewboard.mozilla.org/r/90288/#review90824 ::: addon-sdk/source/lib/toolkit/loader.js:736 (Diff revision 1) > } > return null; > }); > Loader.resolveURI = resolveURI; > > +let moduleMap = new WeakMap(); I'm not sure there is much value in having this cache. require() already caches modules.

Comment on attachment 8807019 [details] Bug 1314861: Minor optimization: Define globals for shared sandbox modules on the sandbox rather than each module. https://reviewboard.mozilla.org/r/90290/#review90826 There is a typo (Duplicated "Bug Bug") in changeset message. ::: addon-sdk/source/lib/toolkit/loader.js:1101 (Diff revision 1) > }); > > + if (options.sandboxPrototype) { > + // If we were given a sandboxPrototype, we have to define the globals on > + // the sandbox directly. Note that this will not work for callers who > + // depend on being able to add globals after the loader was created. Is there any caller relying on this behavior? It may be good to update Loader() comment about `globals` saying that it can be mutated if sandboxPrototype is passed.

(In reply to Alexandre Poirot [:ochameau] from comment #12) > Tests and basic documentation in code are really missing. > This function is all but simple, especially due to its polymorphism. Yeah, sorry, I guess I wrote this after spending too much time in the SDK, where everything is magical and nothing is documented. > Speaking about that, do we really need such complexity? > Devtools uses "lazyRequireGetter" with a simplier behavior: > lazyRequireGetter(scopeToInjectInto, modulePath, SymbolToExport, > BooleanTrueIfTheModuleExportNeedsToBeExported) > * Passing the scope object, which in 99% of the usecases is going to be > `this`, makes this function less magical. > * It requires one call to lazyRequireGetter per symbol to import > I found it more than enough in devtools, but SDK is special with its extreme > usage of modularity! I would have liked it to be simpler, but unfortunately I couldn't make it much simpler without hugely refactoring most of the SDK. Many (probably most) of the existing calls import at least a half dozen symbols, and rename some of them. It might be a good idea to have a separate function to lazily import the entire module rather than just a single symbol, but I don't think having only one import per call is really doable. Either way, I'll add documentation.

Comment on attachment 8807019 [details] Bug 1314861: Minor optimization: Define globals for shared sandbox modules on the sandbox rather than each module. https://reviewboard.mozilla.org/r/90290/#review90826 > Is there any caller relying on this behavior? > It may be good to update Loader() comment about `globals` saying that it can be mutated if sandboxPrototype is passed. Yeah, both devtools and the SDK rely on it to load the module containing their default globals after the loader has been created. But neither of them set `sandboxPrototype`.

Comment on attachment 8807018 [details] Bug 1314861: Add defineLazyGetter global to SDK loader modules. https://reviewboard.mozilla.org/r/90288/#review90824 > I'm not sure there is much value in having this cache. require() already caches modules. There might not be. I didn't actually do any profiling for that. But even with the other optimizations in this patch set, there's still a fair amount of overhead in resolving modules for each call. I'll check whether this makes any difference.

Comment on attachment 8807020 [details] Bug 1314861: Pre-compute path mapping function to save on runtime lookups. https://reviewboard.mozilla.org/r/90292/#review90840 Isn't this one a premature optimization? It is hard to follow the regexp construction, is that really worth? It sounds surprising such RegExp is significantly faster than what looks like quite simple array/string manipulation. May be all the other patches are making this one insignificant? ::: addon-sdk/source/lib/toolkit/loader.js:732 (Diff revision 1) > // Check for an empty path, an exact match, or a substring match > // with the next character being a forward slash. > - if(stripped === "" || id === stripped || id.startsWith(stripped + "/")) { > - return normalizeExt(id.replace(path, uri)); > + if (path == "") > + patterns.push(""); > + else > + patterns.push(`${escapeMeta(path)}(?=$|/)`); Can't you move `(?=$|/)` to the RegExp constructor? new RegExp(`^(${patterns.join('|')})(/|$)`); ::: addon-sdk/source/lib/toolkit/loader.js:745 (Diff revision 1) > +const resolveURI = iced(function resolveURI(id, mapping) { > + // Do not resolve if already a resource URI > + if (isAbsoluteURI(id)) > + return normalizeExt(id); > + > + return normalizeExt(mapping(id)) I'm not sure it is reviewboard, but it looks like there is an indentation issue.

Comment on attachment 8807021 [details] Bug 1314861: Some trivial optimizations that add up something significant. https://reviewboard.mozilla.org/r/90294/#review90842 It looks like you mixed up patches around requireMap removal. ::: addon-sdk/source/lib/toolkit/loader.js:579 (Diff revision 1) > // avoid complexity we require `baseURI` with a trailing `/`. > const resolve = iced(function resolve(id, base) { > if (!isRelative(id)) > return id; > > - let baseDir = dirname(base); > + var baseDir = dirname(base); var? ::: addon-sdk/source/lib/toolkit/loader.js:631 (Diff revision 1) > let fullId = join(rootURI, modulesDir, id); > > let resolvedPath = (resolveAsFile(fullId) || > resolveAsDirectory(fullId)); > if (resolvedPath) { > - return stripBase(rootURI, resolvedPath); > + return './' + resolvedPath.slice(rootURI.length); You can remove stripBase() function as it was only used from here. ::: addon-sdk/source/lib/toolkit/loader.js (Diff revision 1) > if (isNative) { > - // If a requireMap is available from `generateMap`, use that to > - // immediately resolve the node-style mapping. > - // TODO: write more tests for this use case > - if (requireMap && requireMap[requirer.id]) > - requirement = requireMap[requirer.id][id]; There is some more usages of requireMap in the first lines of Require() and Loader()

Comment on attachment 8807022 [details] Bug 1314861: Add caching to some of the more expensive and repetitive parts of module resolution. https://reviewboard.mozilla.org/r/90296/#review90846 (requireMap removal is dispatches over this changeset) ::: addon-sdk/source/lib/toolkit/loader.js:288 (Diff revision 1) > return false; > } > }), > > + resolutionCache: new DefaultMap(fullId => { > + return resolveId(fullId); You may inline resolveId here as it is only used from here. ::: addon-sdk/source/lib/toolkit/loader.js:310 (Diff revision 1) > + let url = join(rootURI, start); > + > + if (!this.nodeModulesCache.has(url)) > + this.nodeModulesCache.set(url, Array.from(getNodeModulePaths(rootURI, start))); > + > + return this.nodeModulesCache.get(url); Same here, if we are into micro optimization, we wouldn't call map.get when map.has returned false. Also you should be able to use the DefaultMap helper if you move the join(rootURI, start) to the callsite. It looks reasonable as there is only one callsite. ::: addon-sdk/source/lib/toolkit/loader.js:952 (Diff revision 1) > // Resolves `uri` of module using loaders resolve function. > - uri = uri || resolveURI(requirement, mapping); > + if (!uri) { > + if (!mappingCache.has(requirement)) > + mappingCache.set(requirement, resolveURI(requirement, mapping)); > + > + uri = mappingCache.get(requirement); If we are doing micro optimization I would do: if (!has) { uri = resolve(key); map.set(key, uri); } else { uri = map.get(key); } ::: addon-sdk/source/lib/toolkit/loader.js:1153 (Diff revision 1) > // as they are pure implementation detail that no one should rely upon. > let returnObj = { > destructor: { enumerable: false, value: destructor }, > globals: { enumerable: false, value: globals }, > mapping: { enumerable: false, value: mapping }, > + mappingCache: { enumerable: false, value: mappingCache }, You may just pass `value: new Map()` from here instead of having an unused variable.

Comment on attachment 8807023 [details] Bug 1314861: Minor optimization: Avoid the more expensive parts of path joining and normalization where possible. https://reviewboard.mozilla.org/r/90298/#review90754 Thanks for all these patches Kris! ::: addon-sdk/source/lib/toolkit/loader.js:391 (Diff revision 1) > let match = /^((?:resource|file|chrome)\:\/\/[^\/]*|jar:[^!]+!)(.*)/.exec(base); > if (match) { > - return match[1] + normalize(pathJoin(match[2], ...paths)); > + return match[1] + normalize([match[2], ...paths].join("/")); > } > > - return normalize(pathJoin(base, ...paths)); > + return normalize([base, ...paths].join("/")); You can remove pathJoin import (ospath_unix.jsm) from the headers as it is no longer used anywhere. ::: addon-sdk/source/lib/toolkit/loader.js:602 (Diff revision 1) > > - let resolved = join(baseDir, id); > + let resolved; > + if (baseDir.includes(":")) > + resolved = join(baseDir, id); > + else > + resolved = normalize(`${baseDir}/${id}`); I would prefer seing the includes(":") check within join() rather than complexifying the callsites like this! = it would be better to have a more efficient join() rather than making the call sites bypass it

Comment on attachment 8807020 [details] Bug 1314861: Pre-compute path mapping function to save on runtime lookups. https://reviewboard.mozilla.org/r/90292/#review90840 Unfortunately, no, it's not. I tried to avoid this one, but this path lookup function was one of the most significant single chunks of overhead I could find, and this change cut about 25ms off of it in my tests. > Can't you move `(?=$|/)` to the RegExp constructor? > new RegExp(`^(${patterns.join('|')})(/|$)`); Nope. I did try that, but it required a separate special case for the "" special case, which just made things more complicated. > I'm not sure it is reviewboard, but it looks like there is an indentation issue. I think it was a rebase botch from when I removed the `else` in a different commit.

(In reply to Kris Maglione [:kmag] from comment #22) > Comment on attachment 8807020 [details] > Bug 1314861: Pre-compute path mapping function to save on runtime lookups. > > https://reviewboard.mozilla.org/r/90292/#review90840 > > Unfortunately, no, it's not. I tried to avoid this one, but this path lookup > function was one of the most significant single chunks of overhead I could > find, and this change cut about 25ms off of it in my tests. Even with "Bug 1314861: Add caching to some of the more expensive and repetitive parts of module resolution" applied? It sounds very surprising. Did you tried optimizing existing for loop without using regexp? The original code looks so trivial. What is the culprit of it? A simple for-of loop? Usage of replace? startsWith? String concatenation?

(In reply to Alexandre Poirot [:ochameau] from comment #23) > (In reply to Kris Maglione [:kmag] from comment #22) > > Unfortunately, no, it's not. I tried to avoid this one, but this path lookup > > function was one of the most significant single chunks of overhead I could > > find, and this change cut about 25ms off of it in my tests. > > Even with "Bug 1314861: Add caching to some of the more expensive and > repetitive parts of module resolution" applied? It sounds very surprising. > > Did you tried optimizing existing for loop without using regexp? The > original code looks so trivial. What is the culprit of it? A simple for-of > loop? Usage of replace? startsWith? String concatenation? Yes, this is one of the last changes that I tried. Most of the other optimizations only apply to modules that use the node resolver, but this applies to all users, and especially to devtools, which has more paths to check. The overhead here isn't in any one place. Part of it is in the overhead of the for-of loop, but a lot of it is in all of the little things the loop does. Some of it could probably be improved with a simpler pre-processing step, but if we're pre-processing anyway, I think the regexp solution is the simplest. It's also the fastest, given how optimized our regexp engine is.

Added dependency from Bug 1313767 (because two of these patches depends from DefaultWeakMap, that is defined in one of the patches from that bug).

Comment on attachment 8807024 [details] Bug 1314861: Lazily create view for Panels. https://reviewboard.mozilla.org/r/90300/#review108252 This patch looks mostly good (just a small syntax error fixed in the last patch from this patch queue and a small number of nits, related to the inline comments). I'm currently clearing this review request (mostly because of the syntax error fixed in the other patch) ::: addon-sdk/source/lib/sdk/panel.js (Diff revision 1) > detach(styleFor(this)); > > + if (views.has(this)) > - domPanel.dispose(viewFor(this)); > + domPanel.dispose(viewFor(this)); > > - // Release circular reference between view and panel instance. This nit: is the removed comment still valid? ::: addon-sdk/source/lib/sdk/system/events.js:179 (Diff revision 1) > - removeObserver(observer, type); > + removeObserver(observer, type); > - } else { > + } else { > - removeObserverNoShim(observer, type); > + removeObserverNoShim(observer, type); > - } > + } > - } > + } > + } it looks like there is a syntax error here (the promise's `then` misses the closed parentesis) (If I'm not wrong it is actually fixed in the last patch from this patch queue) Also, it would be problably nice to add an inline comment that briefly describe why there is a Promise.resolve here. ::: addon-sdk/source/test/test-addon-extras.js:52 (Diff revision 1) > > let panel = Panel({ > contentURL: "./test-addon-extras.html" > }); > > + // Force the panel view to actually load. It looks like the lazy loading requires every test with a panel to use `getActiveView` to force the panel to be actually loaded, and it is a pity that we have to manually add it to every test (and that only this one has an inline comment that explains its role). can we at least require it only once for test file and add an inline comment where `getActiveView` is imported (at least where it is imported using the regular `require`)? (or the tests need it to be required inside the test case?) ::: addon-sdk/source/test/test-system-events.js:140 (Diff revision 1) > events.emit(type, { data: 1 }); > assert.equal(receivedFromStrong.length, 1, "strong listener invoked"); > assert.equal(receivedFromWeak.length, 1, "weak listener invoked"); > > loader.unload(); > + yield Promise.resolve(); how about adding an inline comment here to explain the reason of this yield? (it it related to the Promise.resolve in the system events, am I right?)

Comment on attachment 8807025 [details] Bug 1314861: Lazily initialize l10n code. https://reviewboard.mozilla.org/r/90302/#review108260 nit, I'm wondering if it would make sense (and it would be possible) to pack more of the `lazyRequire` calls into a single call (I notice that it has different syntaxes for importing an entire module or selected named exports, and so I'm not sure if it is actually possible), or to move and group all the `lazyRequire` calls in a single place (possibly after the statically required modules).

Comment on attachment 8807026 [details] Bug 1314861: Lazily load UI modules, as much as possible. https://reviewboard.mozilla.org/r/90304/#review108266 r+ with comments (in particular the removed `var sidebars = {}`) (also same nit as the above, related to the `lazyRequire`, if it is possible to pack them in a single call or group them together) ::: addon-sdk/source/lib/sdk/ui/frame/model.js:27 (Diff revision 1) > -const { isLocalURL } = require("../../url"); > const { compose } = require("../../lang/functional"); > const { contract } = require("../../util/contract"); > const { id: addonID, data: { url: resolve }} = require("../../self"); > const { Frames } = require("../../input/frame"); > +require("./view"); this require has been moved here from the "ui/frame" module, and I'm pretty sure that the reason is that we want to load it "as late as possible", nevertheless an inline comment above it would be probably nice. ::: addon-sdk/source/lib/sdk/ui/sidebar.js (Diff revision 1) > > const sidebarNS = ns(); > > const WEB_PANEL_BROWSER_ID = 'web-panels-browser'; > > -var sidebars = {}; I guess that this sidebars map was used to keep a reference to the created sidebars to prevent them to be garbage collected (given that it was referenced only where the sidebar is created or disposed), is it not needed anymore?

Comment on attachment 8807027 [details] Bug 1314861: Lazily load most SDK module imports. https://reviewboard.mozilla.org/r/90306/#review108268 This patch also looks almost good, there are some small fixes needed: a module imported twice (both statically and lazily) and a syntax error related to the patch "Bug 1314861: Lazily create view for Panels" from the same patch queue. ::: addon-sdk/source/lib/sdk/remote/parent.js (Diff revision 1) > // Pass a module to resolve the id relatively. > function remoteRequire(id, module = null) { > // Resolve relative to calling module if passed > if (module) > id = moduleResolve(id, module.id); > - let uri = loaderModule.resolveURI(id, pathMapping); This change does not seem to be strictly related to the lazy loading itself, is "resolving the module id into an uri" not necessary anymore here because of the last changes on the SDK loader? ::: addon-sdk/source/lib/sdk/remote/utils.js:8 (Diff revision 1) > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > "use strict"; > > const { Class } = require('../core/heritage'); > const { List, addListItem, removeListItem } = require('../util/list'); > const { emit } = require('../event/core'); it looks like `emit` is imported both statically here and lazily in the line after this. ::: addon-sdk/source/lib/sdk/system/events.js:180 (Diff revision 1) > removeObserverNoShim(observer, type); > } > } > - } > - }) > + }) > + }); This seems to be a fix for a syntax error introduced in the patch "Bug 1314861: Lazily create view for Panels" from the same patch queue. ::: addon-sdk/source/lib/sdk/system/globals.js (Diff revision 1) > - > -// On windows dump does not writes into stdout so cfx can't read thous dumps. > -// To workaround this issue we write to a special file from which cfx will > -// read and print to the console. > -// For more details see: bug-673383 > -exports.dump = stdout.write; I guess that the comment above the removed `exports.dump` is not valid anymore, right? because it was related only to cfx?

Comment on attachment 8807024 [details] Bug 1314861: Lazily create view for Panels. https://reviewboard.mozilla.org/r/90300/#review108252 > It looks like the lazy loading requires every test with a panel to use `getActiveView` to force the panel to be actually loaded, and it is a pity that we have to manually add it to every test (and that only this one has an inline comment that explains its role). > > can we at least require it only once for test file and add an inline comment where `getActiveView` is imported (at least where it is imported using the regular `require`)? (or the tests need it to be required inside the test case?) No, we use a different loader for every task. In any case, it's not worth worrying about, at this point.

Sorry it's taken me so long to get back to this. By the time I got the last reviews, I had way too much else going on. This appears to be a major blocker for QF at this point, though.

Comment on attachment 8807020 [details] Bug 1314861: Pre-compute path mapping function to save on runtime lookups. https://reviewboard.mozilla.org/r/90292/#review131992 ::: addon-sdk/source/lib/toolkit/loader.js:716 (Diff revision 2) > > -const resolveURI = iced(function resolveURI(id, mapping) { > - // Do not resolve if already a resource URI > - if (isAbsoluteURI(id)) > - return normalizeExt(id); > +function compileMapping(mapping) { > + let patterns = []; > + let paths = {}; > + > + let escapeMeta = str => str.replace(/([.\\?+*(){}[\]^$])/g, '\\$1') It looks like this regexp is going to be instanciated everytime we call escapeMeta. We should create it out of escapeMeta, in compileMapping scope looks fine. ::: addon-sdk/source/lib/toolkit/loader.js:744 (Diff revision 2) > + patterns.push(`${escapeMeta(path)}(?=$|/)`); > - } > + } > + > + let pattern = new RegExp(`^(${patterns.join('|')})`); > + > + return id => id.replace(pattern, (m0, m1) => paths[m1]); This is hard to process. return id => { return id.replace(pattern, (m0, m1) => paths[m1]); }; Would help distinguish the two arrow functions. Also a comment saying that it will only replace the first matching pattern sounds like a useful notice. I may sound like a very naive regexp behavior, but ease approaching this complex regexp work. ::: addon-sdk/source/lib/toolkit/loader.js:1103 (Diff revision 2) > let destructor = freeze(Object.create(null)); > > // Make mapping array that is sorted from longest path to shortest path. > let mapping = Object.keys(paths) > .sort((a, b) => b.length - a.length) > .map(path => [path, paths[path]]); This looks tightly related to what we do within compileMapping. It may help getting the big picture by moving that to compileMapping and do `let mapping =compileMapping(paths);`

Comment on attachment 8807020 [details] Bug 1314861: Pre-compute path mapping function to save on runtime lookups. https://reviewboard.mozilla.org/r/90292/#review131992 > It looks like this regexp is going to be instanciated everytime we call escapeMeta. We should create it out of escapeMeta, in compileMapping scope looks fine. Reusing a regexp instance with a /g flag can be a bit risky... but I suppose it shouldn't be a problem in this case. > This looks tightly related to what we do within compileMapping. > It may help getting the big picture by moving that to compileMapping and do `let mapping =compileMapping(paths);` Hm. Yeah, that should definitely be moved into compileMapping now.

Comment on attachment 8807018 [details] Bug 1314861: Add defineLazyGetter global to SDK loader modules. https://reviewboard.mozilla.org/r/90288/#review132558 ::: addon-sdk/source/lib/toolkit/loader.js:488 (Diff revision 2) > + value: lazyRequire.bind(sandbox), > + }; > + descriptors.lazyRequireModule = { > + configurable: true, > + value: lazyRequireModule.bind(sandbox), > + }; SDK loader tries to injects only CommonJS globals. Here you inject into globals something very personal, out of any spec. To be correct I would inject that from SDK custom loader instance: http://searchfox.org/mozilla-central/source/addon-sdk/source/app-extension/bootstrap.js#212 But given the future of SDK, and given that it shouldn't conflict with any existing globals from devtools, feel free to ignore this.

Comment on attachment 8807018 [details] Bug 1314861: Add defineLazyGetter global to SDK loader modules. https://reviewboard.mozilla.org/r/90288/#review132558 > SDK loader tries to injects only CommonJS globals. > Here you inject into globals something very personal, out of any spec. > To be correct I would inject that from SDK custom loader instance: > http://searchfox.org/mozilla-central/source/addon-sdk/source/app-extension/bootstrap.js#212 > > But given the future of SDK, and given that it shouldn't conflict with any existing globals from devtools, feel free to ignore this. This only injects the properties into shared global sandboxes, so add-on modules shouldn't be affected. If you're worried about this affecting devtools, though, I can do something different. But if you're not particularly concerned about that, I'd rather keep it as is until the SDK is gone, since I really don't want to have to rebase more changes onto the lazy loading patches again if I don't have to... We can't actually inject this only into the SDK loader instances, though. Not easily anyway. Since a) there are about a dozen places we create SDK loaders, and b) the devtools load a bunch of these SDK modules, too.

(In reply to Kris Maglione [:kmag] from comment #45) > If you're worried about this affecting > devtools, though, I can do something different. It shouldn't. In devtools, we expose similar helpers with similar names but on a `loader` object: http://searchfox.org/mozilla-central/source/devtools/shared/builtin-modules.js#219-223 So no risk of conflicts. The only issue I already highlighted is that this is similar but not exactly identical. We are most likely going to ignore these new globals and everything will be fine.

Comment on attachment 8807024 [details] Bug 1314861: Lazily create view for Panels. https://reviewboard.mozilla.org/r/90300/#review132960

Comment on attachment 8807027 [details] Bug 1314861: Lazily load most SDK module imports. https://reviewboard.mozilla.org/r/90306/#review132968

https://hg.mozilla.org/integration/mozilla-inbound/rev/7451a3d51a6afa4aadad2409315a72573875f348 Bug 1314861: Add defineLazyGetter global to SDK loader modules. r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/968849e6dfd4a8e2003c5ef829882b01e319dfbc Bug 1314861: Minor optimization: Define globals for shared sandbox modules on the sandbox rather than each module. r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/6de875ca10a71a5cec1c1fba5d96dd59bd256bf8 Bug 1314861: Pre-compute path mapping function to save on runtime lookups. r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/c04697eac158bffa7c1716c0923f6c14dcf00e37 Bug 1314861: Some trivial optimizations that add up something significant. r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/78b85b822ca3fdd13f593e34287ce255bd78c0cd Bug 1314861: Add caching to some of the more expensive and repetitive parts of module resolution. r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/9f207dec7574f3a0645d9a12d060915efb6f830b Bug 1314861: Minor optimization: Avoid the more expensive parts of path joining and normalization where possible. r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/1a5e565c140321de8a7b6e42a77baa91fcf6fdca Bug 1314861: Lazily initialize l10n code. r=rpl https://hg.mozilla.org/integration/mozilla-inbound/rev/e1e096a332662d4721f16ee4614f59e5c97ba7a1 Bug 1314861: Lazily create view for Panels. r=rpl https://hg.mozilla.org/integration/mozilla-inbound/rev/544a9c89137d102ab9939d9fca5f29c851254788 Bug 1314861: Lazily load UI modules, as much as possible. r=rpl https://hg.mozilla.org/integration/mozilla-inbound/rev/b9a1e8ade106a8bce340b1717dea4870543536b9 Bug 1314861: Lazily load most SDK module imports. r=rpl

https://hg.mozilla.org/mozilla-central/rev/7451a3d51a6a https://hg.mozilla.org/mozilla-central/rev/968849e6dfd4 https://hg.mozilla.org/mozilla-central/rev/6de875ca10a7 https://hg.mozilla.org/mozilla-central/rev/c04697eac158 https://hg.mozilla.org/mozilla-central/rev/78b85b822ca3 https://hg.mozilla.org/mozilla-central/rev/9f207dec7574 https://hg.mozilla.org/mozilla-central/rev/1a5e565c1403 https://hg.mozilla.org/mozilla-central/rev/e1e096a33266 https://hg.mozilla.org/mozilla-central/rev/544a9c89137d https://hg.mozilla.org/mozilla-central/rev/b9a1e8ade106

One of the changes in this patch set was to load Panel views only when they're first shown, rather than immediately when the Panel object is created. Extensions that depend on the view documents being loaded eagerly may be affected by that change.

It looks like, between this and bug 1317697, we've got about a 2% improvement on ts_paint e10s, a 2.5-3% improvement on damp e10s, and possibly a 3-5% improvement on tp5o and tp5o responsiveness: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=fac2c174087f&newProject=mozilla-central&newRevision=2b6a66a98e253ba158f3960f1c68ad49b2ebcdb4&framework=1 There were lots of other changes in this range, and some of them probably affected performance too, but these numbers are consistent with the ones I did for the individual landings with a smaller number of retriggers. So I think I'm going to call this a success, even when no add-ons are installed. The majority of the real impact should come when actual SDK add-ons are installed.

(In reply to Kris Maglione [:kmag] from comment #52) > So I think I'm going to call this a success, even when no add-ons are > installed. The majority of the real impact should come when actual SDK > add-ons are installed. That's awesome! Not to rain on the parade though, do we know what the dealio is with the sessionrestore Talos tests? Looks like there were regressions for the e10s case there: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=fac2c174087f&newProject=mozilla-central&newRevision=2b6a66a98e253ba158f3960f1c68ad49b2ebcdb4&framework=1

ni? for comment 53

(In reply to Mike Conley (:mconley) from comment #53) > Not to rain on the parade though, do we know what the dealio is with the > sessionrestore Talos tests? Looks like there were regressions for the e10s > case there: Nope. Those didn't show up in any of the smaller runs I did, so I'm pretty sure they're unrelated. They might be worth looking into separately, but I was assuming that if they were anything more than a blip, they would have triggered a performance alert already.

I think https://github.com/mozilla/gecko-dev/commit/cbfb61ba923bfd32e22f6e5c57491da7dce03e00#diff-8f7679c8efb4c2fab953a23ce2d5ad02L329 generated a regression: https://bugzilla.mozilla.org/show_bug.cgi?id=1358807