Recent benchmarks from bug 989373 proved that zones aren't saving as much memory as expected. So since we swichted to CPG (compartment per global) SDK modules became more expensive. On B2G, JSM are using reuseGlobal trick to save ~20MB. I haven't measured yet the single cost of doing the exact same trick for SDK modules, but it allowed me, with various other patches, to drop the devtools footprint in child processes from 9MB to 900k. So let's try to use only one compartment for all devtools modules and see how much we get from such move.

(In reply to Alexandre Poirot (:ochameau) from comment #0) > I haven't measured yet the single cost of doing the exact same trick for SDK > modules, but it allowed me, with various other patches, to drop the devtools > footprint in child processes from 9MB to 900k. That's quite an impressive improvement! :D Great work!

Nice. Does this reduce Firefox (desktop) start-up memory usage too?

(In reply to Nicholas Nethercote [:njn] from comment #4) > Nice. Does this reduce Firefox (desktop) start-up memory usage too? I don't know exactly what happens on desktop. But I imagine we prevent loading any (at least most) of devtools modules, until devtools toolbox is opened. But it may reduce memory when you start opening them! On b2g here is what I measured a gain of -300k for the parent process and -3.1M on app child. It's not really clear why we allocate way less in child than in parent as we load more or less the same thing on both. I measure the USS given by firewatch. master merged compartments Parent process, with settings opened on devtools menu set on "adb only" 46.1 46.2 Parent process, same thing but selected "devtools" (load everything) 51.5=5.4 51.3=+5.1 diff: 0.3 Homescreen, after this (nothing devtools related loaded yet) 9.6 10.2 Homescreen, connected to the memory actor (load everything) 17.7=8.1 15.2=+5 diff: 3.1 Note that I also applied a patch on both test runs to ensure that we do not load devtools in parent process before toggle the setting. We recently regressed that, see bug 1014625. Finally, just for information, here is the list of SDK modules being loaded: resource://gre/modules/devtools/server/main.js resource://gre/modules/devtools/server/actors/common.js resource://gre/modules/devtools/transport/transport.js resource://gre/modules/devtools/DevToolsUtils.js resource://gre/modules/devtools/transport/stream-utils.js resource://gre/modules/devtools/transport/packets.js resource://gre/modules/commonjs/sdk/core/heritage.js resource://gre/modules/devtools/event-emitter.js resource://gre/modules/commonjs/sdk/event/core.js resource://gre/modules/commonjs/sdk/core/namespace.js resource://gre/modules/devtools/deprecated-sync-thenables.js resource://gre/modules/devtools/server/protocol.js resource://gre/modules/commonjs/sdk/event/target.js resource://gre/modules/commonjs/sdk/lang/functional/core.js resource://gre/modules/commonjs/sdk/lang/functional/helpers.js resource://gre/modules/commonjs/sdk/util/object.js resource://gre/modules/commonjs/sdk/util/array.js resource://gre/modules/devtools/server/actors/webbrowser.js resource://gre/modules/devtools/server/actors/root.js resource://gre/modules/devtools/server/actors/script.js resource://gre/modules/devtools/styleinspector/css-logic.js resource://gre/modules/devtools/server/actors/webconsole.js resource://gre/modules/devtools/toolkit/webconsole/utils.js resource://gre/modules/devtools/server/actors/inspector.js resource://gre/modules/devtools/server/actors/string.js resource://gre/modules/commonjs/sdk/platform/xpcom.js resource://gre/modules/commonjs/sdk/util/uuid.js resource://gre/modules/devtools/server/actors/styles.js resource://gre/modules/devtools/server/actors/stylesheets.js resource://gre/modules/devtools/server/actors/highlighter.js resource://gre/modules/devtools/server/actors/call-watcher.js resource://gre/modules/devtools/content-observer.js resource://gre/modules/devtools/server/actors/canvas.js resource://gre/modules/devtools/server/actors/webgl.js resource://gre/modules/devtools/server/actors/webaudio.js resource://gre/modules/devtools/server/actors/styleeditor.js resource://gre/modules/devtools/server/actors/storage.js resource://gre/modules/devtools/async-utils.js resource://gre/modules/devtools/server/actors/gcli.js resource://gre/modules/devtools/gcli/util/util.js resource://gre/modules/devtools/gcli/util/promise.js resource://gre/modules/devtools/server/actors/tracer.js resource://gre/modules/commonjs/sdk/timers.js resource://gre/modules/commonjs/sdk/system/unload.js resource://gre/modules/commonjs/sdk/system/events.js resource://gre/modules/devtools/server/actors/memory.js resource://gre/modules/devtools/server/actors/framerate.js resource://gre/modules/devtools/server/actors/eventlooplag.js resource://gre/modules/devtools/server/actors/layout.js resource://gre/modules/devtools/server/actors/preference.js resource://gre/modules/devtools/server/actors/device.js

(In reply to Nicholas Nethercote [:njn] from comment #4) > Nice. Does this reduce Firefox (desktop) start-up memory usage too? I did a really poor benchmark, looking at RSS via `ps` just after startup, with empty profile. (no gc, just looking at ps right after launch) master: 115160/114500 patch: 118084/118416 There is just few modules being loaded on startup, but unexpectedly, this patch bumps the memory by +2k. I assume it is just some noise? Then I looked at memory after opening the web console on about:home. master: 164632 patch: 145908 And finally, I opened all tools. master: 211504 patch: 199712 So there is a small win when opening tools, -15k when opening the console and -11k when opening all. I would have expected something way bigger as we are loading way more modules on desktop... njn, I end up wondering if RSS in ps is a good way to measure memory easily on desktop??

> njn, I end up wondering if RSS in ps is a good way to measure memory easily > on desktop?? It's not bad, though it is noisy. Another thing you could do is measure in about:memory and look at the size of all the compartments with "devtools" in their names.

New patch, new try: https://tbpl.mozilla.org/?tree=Try&rev=a66bff2bb14a

I tried about:memory, but I had hard time getting meaningful results. The overall resident-unique/explicit numbers don't reflect the gain. Instead I get random results that don't highlight the benefit of this patch. I get significantly variable allocation for startup-cache for example and I wasn't able to disable it. Same thing for runtime/gc, I get variable size. I also got a significantly bigger "XPConnect Compilation Compartment" with my patch (this time it may be related? any idea what is that?). At the end it is still hard to tell what is noise and what is actually related to my patch. Do you have a magic set or pref, profile, env variable or something in order to be able to run about:memory benchmark with more deterministic numbers?

Here is some about memory logs, 4 runs, two on firefox startup: master and with this patch. And two another after opening webconsole on about:home. I toggle OOP in order to better see the gain in the child process, which contain way less noise. This time, on firefox startup, I see a gain of -1.84 MB resisent-unique with 9 compartments merged into one. Then, if you open the webconsole, I got a gain of -2.91 MB resident-unique with 51 compartments merged into one for the parent process. And a gain of -8.00 MB resident-unique with 149 compartmetns merged into one in the tab process. The memory released in the child process better match what I'm seeing on b2g.

Fixed some other failure on tbpl: https://tbpl.mozilla.org/?tree=Try&rev=3029d3918a77

> Do you have a magic set or pref, profile, env variable or something in order > to be able to run about:memory benchmark with more deterministic numbers? No.

New week, new patch: https://tbpl.mozilla.org/?tree=Try&rev=ddf185f496ee

Finally got a green try... I had hard time chasing all usages of `promise` being used without requiring it. Also qrcode module is doing very bad things over here: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/qrcode/decoder/index.js#2568 Which is the main reason why we evaluate each module in its own global set... Hopefully, that's the only module hacking globals so badly. I blacklisted it to load it in its own sandbox. https://tbpl.mozilla.org/?tree=Try&rev=34de7f02756a

Comment on attachment 8435600 [details] [diff] [review] patch Irakli, Can you review the SDK changes? addon-sdk/source/lib/sdk/base64.js addon-sdk/source/lib/toolkit/loader.js Panos, What about looking at all others? Eddy, I'm asking you feedback mostly to let your know about this modification. I don't expect any conflict with workers patches, but you may be happy to see the removal of some magic globals (promise, ChromeWorker). * Global sharing: First, from SDK/loader perspective this patch looks quite bad as there is a reason to use one sandbox per module, that's to have one global set per module. To prevent issues like qrcode one described in previous comment. But unfortunately, the platform code is still expensive regarding new sandboxes/compartments/globals. It should have been fixed with Zones. After CPG (compartment per global) we got a big increase of memory usage and then Zones were meant to shrink it down. But recent measurement in bug 989373 proved that we are not there yet. I also verified this extra cost in this bug with the various benchmark I made against this patch. It allows to save 3MB in each b2g process as well in e10s tabs of firefox. * Magic devtools globals - promise: That's was for the global sharing. Now this patch highlights another issue in loader usage of devtools. We were injecting `promise` as global. And for some reason, this patch makes `const promise = ...;` throw if we keep inserting it as global. So I get rid of this magic promise global and instead expose it as a pseudo module. (I also removed ChromeWorker as it is exposed via require(chrome)) I think it will be a great opportunity to remove some Cu.import for promises and help Eddy in its worker quest ;) * IndexedDB: You will also see various modification regarding indexedDB. That's because initWindowLess doesn't work anymore, as the global object of modules isn't a real global object regarding xpconnect and this call ends up throwing... So instead, I allow exposing indexedDB as a pseudo module. (same thing applies to addDebuggerToGlobal) * DebuggerServer.addActors: There was some magic before this patch... A bunch of globals of main.js were automagically exposed via loadSubScript, whereas *only* DebuggerServer attribute should have been exposed!! For some really unexplained reason, this patch stop the magic. So that I have to explicitely put on DebuggerServer the globals we are expecting to see in actors... Please be kind while reviewing, this is quite significant behavior change, so I'm looking for help in testing and ensuring this patch doesn't regress stuff.

(In reply to Alexandre Poirot (:ochameau) from comment #17) > Also qrcode module is doing very bad things over here: > > http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/qrcode/ > decoder/index.js#2568 > Which is the main reason why we evaluate each module in its own global set... > > Hopefully, that's the only module hacking globals so badly. > I blacklisted it to load it in its own sandbox. I am glad you caught this since strange things would start happening otherwise! (Kind of curious how you noticed it... Just searching for Array and friends...? Or maybe there is some error somewhere.) Really, that entire file is a lesson on terrible style and bad techniques... :( Anyway, I filed bug 1021744 to clean up this one aspect of the file, so it could then be removed from the blacklist.

Comment on attachment 8435600 [details] [diff] [review] patch Review of attachment 8435600 [details] [diff] [review]: ----------------------------------------------------------------- My only worry with this is that we're stuck with subscript loader now that loads things into js object rather than into sandbox. That being said I can live with that. I'm also little unhappy with if statement that decides to make an exception for specific module, I'd rather have that as a part of metadata of the module (but than we won't have a way of reading it ahead of loading I think :() or part of options passed in to loader which will be a list of module identifiers that need to be loaded into their own sandbox. That being said, there's nothing here I can't live with so r+ from me, but would still encourage to address my concerns ;) ::: addon-sdk/source/lib/sdk/base64.js @@ +10,5 @@ > > const { Cu } = require("chrome"); > > // Passing an empty object as second argument to avoid scope's pollution > +var { atob, btoa } = Cu.import("resource://gre/modules/Services.jsm", {}); I'm not sure why do we need to change from const to var, mind putting a comment ? ::: addon-sdk/source/lib/toolkit/loader.js @@ +284,5 @@ > }); > > + let sandbox; > + if (loader.sharedGlobal && > + module.id != "sdk/indexed-db" && I'd be fine (and would actually prefer) to not make an indexed-db module an exception, I don' have anything against exposing indexedDB global props in all sandboxes we create.

Comment on attachment 8435600 [details] [diff] [review] patch Calling Cu.import in DevToolsUtils.js will break the worker debugger. Have you tested this? The xpcshell tests have some tests now that should break if you try to do something like this (in particular, you should try the breakpoint tests).

Comment on attachment 8435600 [details] [diff] [review] patch Review of attachment 8435600 [details] [diff] [review]: ----------------------------------------------------------------- I have a few comments, but it looks good to me overall. I tested on desktop only, I assume you tried both fennec and b2g? (In reply to Alexandre Poirot (:ochameau) from comment #18) > We were injecting `promise` as global. And for > some reason, this patch makes `const promise = ...;` throw if we keep > inserting it as global. So I get rid of this magic promise global and > instead expose it as a pseudo module. I guess that "const promise" throws because you can't declare |promise| again in the same scope. As an alternative to adding require("promise") everywhere, you could just convert any existing "const promise =..." to "let promise =...", right? At one point Eddy had the same arrangement as this patch, but since we are planning to just use native promises everywhere, it made more sense to expose |promise| as a global. Converting would require removing just one line in that case. ::: toolkit/devtools/Loader.jsm @@ +65,5 @@ > +try { > + let { indexedDB } = Cu.Sandbox(this, {wantGlobalProperties:["indexedDB"]}); > + loaderModules.indexedDB = indexedDB; > +} catch(e) { > + // On xpbshell, we can't instanciate indexedDB without crashing Typos: xpcshell, instantiate. @@ +105,5 @@ > "xpcshell-test": "resource://test" > }, > globals: loaderGlobals, > + invisibleToDebugger: this.invisibleToDebugger, > + sharedGlobal: true SrcDirProvider should have "sharedGlobal: true" as well. Otherwise we would be developing in a different configuration than the regular case. ::: toolkit/devtools/server/main.js @@ +304,5 @@ > + let includes = ["Components", "Ci", "Cu", "require", "Services", "DebuggerServer", > + "ActorPool", "DevToolsUtils"]; > + includes.forEach(function (name) { > + this[name] = global[name]; > + }, this); Since DebuggerServer is a singleton, you could just assign directly to DebuggerServer[name], not have to pass |this| to forEach, and use an arrow function as a result. ::: toolkit/devtools/transport/tests/unit/head_dbg.js @@ +186,5 @@ > /** > * Initialize the testing debugger server. > */ > function initTestDebuggerServer() { > + DebuggerServer.registerModule("devtools/server/actors/script", "script", { globalActor: true, tabActor: true }); How does that work and what does it do? registerModule() only takes a single parameter from what I can see.

> Calling Cu.import in DevToolsUtils.js will break the worker debugger. Yes, my const -> let suggestion should avoid that.

(See attachment 8441299 [details] [diff] [review] for interdiff) (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #20) > Comment on attachment 8435600 [details] [diff] [review] > patch > > Review of attachment 8435600 [details] [diff] [review]: > ----------------------------------------------------------------- > > My only worry with this is that we're stuck with subscript loader now that > loads things into js object rather than into sandbox. Hopefully, memory consumption of sandboxes will go down and we would be able to drop this hack... > I'm also little unhappy with if statement that decides to > make an exception for specific module, [...] > part of options passed in to loader which will be a > list of module identifiers that need to be loaded into their own sandbox. I did that and introduced sharedGlobalBlacklist. > ::: addon-sdk/source/lib/sdk/base64.js > @@ +10,5 @@ > > > > const { Cu } = require("chrome"); > > > > // Passing an empty object as second argument to avoid scope's pollution > > +var { atob, btoa } = Cu.import("resource://gre/modules/Services.jsm", {}); > > I'm not sure why do we need to change from const to var, mind putting a > comment ? That's because Devtools loader inject atob and btoa into loader globals... And I don't know why but with this shared global trick, it starts throwing. I don't get why it wasn't throwing before this patch! > ::: addon-sdk/source/lib/toolkit/loader.js > @@ +284,5 @@ > > }); > > > > + let sandbox; > > + if (loader.sharedGlobal && > > + module.id != "sdk/indexed-db" && > > I'd be fine (and would actually prefer) to not make an indexed-db module an > exception, I don' have anything against exposing indexedDB global props in > all sandboxes we create. I let the special indexed-db case because of xpcshell tests. By default, instanciating indexed-db throws on xpcshell. The current code allows not running anything related to indexed db until we do load a module using indexed db.

(In reply to Panos Astithas [:past] from comment #22) > Comment on attachment 8435600 [details] [diff] [review] > patch > > Review of attachment 8435600 [details] [diff] [review]: > ----------------------------------------------------------------- > > I have a few comments, but it looks good to me overall. I tested on desktop > only, I assume you tried both fennec and b2g? Only b2g and desktop, but I'll try to test a fennec tbpl build. > > (In reply to Alexandre Poirot (:ochameau) from comment #18) > > We were injecting `promise` as global. And for > > some reason, this patch makes `const promise = ...;` throw if we keep > > inserting it as global. So I get rid of this magic promise global and > > instead expose it as a pseudo module. > > I guess that "const promise" throws because you can't declare |promise| > again in the same scope. As an alternative to adding require("promise") > everywhere, you could just convert any existing "const promise =..." to "let > promise =...", right? > > At one point Eddy had the same arrangement as this patch, but since we are > planning to just use native promises everywhere, it made more sense to > expose |promise| as a global. Converting would require removing just one > line in that case. Yes, we can convert all const promise and not only devtools modules, but also any SDK modules we are using [1]. I did that for atob, btoa, but it sounds wrong having to modify SDK modules for that and we would be at risk if any SDK developer introduce a new const promise. I'm not sure using global or explicit require will help transitioning to Promise, as the main work will be to convert all `promise` to `Promise` and get rid of any require/Cu.import. [1] http://mxr.mozilla.org/mozilla-central/search?string=const.*promise.*%3D&regexp=1&find=\.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central > ::: toolkit/devtools/server/main.js > @@ +304,5 @@ > > + let includes = ["Components", "Ci", "Cu", "require", "Services", "DebuggerServer", > > + "ActorPool", "DevToolsUtils"]; > > + includes.forEach(function (name) { > > + this[name] = global[name]; > > + }, this); > > Since DebuggerServer is a singleton, you could just assign directly to > DebuggerServer[name], not have to pass |this| to forEach, and use an arrow > function as a result. Updated, now it injects all these symbols on module evaluation. > ::: toolkit/devtools/transport/tests/unit/head_dbg.js > @@ +186,5 @@ > > /** > > * Initialize the testing debugger server. > > */ > > function initTestDebuggerServer() { > > + DebuggerServer.registerModule("devtools/server/actors/script", "script", { globalActor: true, tabActor: true }); > > How does that work and what does it do? registerModule() only takes a single > parameter from what I can see. I mixed up my lazy actor patch during a rebase, I removed that modification...

Comment on attachment 8441306 [details] [diff] [review] patch Oh and I forgot to add, I added a test for the new loader options!

Green try: https://tbpl.mozilla.org/?tree=Try&rev=7332e3e896e2

Comment on attachment 8441306 [details] [diff] [review] patch Review of attachment 8441306 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/main.js @@ +1288,5 @@ > }; > + > +// When using DebuggerServer.addActors, some symbols are expected to be in > +// the scope of actors... until they are all loaded as module, > +// explicitely bind the expected symbols: I think a slightly better phrasing would be: "When using DebuggerServer.addActors, some symbols are expected to be in the scope of the added actor even before the corresponding modules are loaded, so let's explicitly bind the expected symbols here." @@ +1293,5 @@ > +let includes = ["Components", "Ci", "Cu", "require", "Services", "DebuggerServer", > + "ActorPool", "DevToolsUtils"]; > +includes.forEach(name => { > + DebuggerServer[name] = this[name]; > +}); Can you move this before the DebuggerServerConnection declaration so that it's closer to the DebuggerServer initialization?

Comment on attachment 8441306 [details] [diff] [review] patch Review of attachment 8441306 [details] [diff] [review]: ----------------------------------------------------------------- ::: addon-sdk/source/lib/toolkit/loader.js @@ +287,5 @@ > + if (loader.sharedGlobal && > + loader.sharedGlobalBlacklist.indexOf(module.id) == -1) { > + // Create a new object in this sandbox, that will be used as > + // the scope object for this particular module > + sandbox = Cu.createObjectIn(loader.sharedGlobal); Since |sharedGlobal| is a boolean property of |loader|, wouldn't Cu.createObjectIn(loader) be more clear? Or even better (according to [1]): sandbox = new loader.Object(); https://developer.mozilla.org/docs/Components.utils.createObjectIn

Comment on attachment 8441306 [details] [diff] [review] patch Review of attachment 8441306 [details] [diff] [review]: ----------------------------------------------------------------- I looked at the SDK code only and it looks ok. What effect does this have on how the devtools code appears in the browser debugger? ::: addon-sdk/source/lib/toolkit/loader.js @@ +287,5 @@ > + if (loader.sharedGlobal && > + loader.sharedGlobalBlacklist.indexOf(module.id) == -1) { > + // Create a new object in this sandbox, that will be used as > + // the scope object for this particular module > + sandbox = Cu.createObjectIn(loader.sharedGlobal); As past suggested (and with the rename below), new loader.sharedGlobalObject.Object() should work. @@ +303,5 @@ > + invisibleToDebugger: loader.invisibleToDebugger, > + metadata: { > + addonID: loader.id, > + URI: module.uri > + } This block is over-indented @@ +766,5 @@ > + metadata: { > + addonID: options.id, > + URI: "Addon-SDK" > + } > + }); Re-using the same variable name is causing confusion here. Maybe sharedGlobalObject? Is there a reason to not just use the loader's compartment?

Rebased and addressed comments. > I looked at the SDK code only and it looks ok. What effect does this have on > how the devtools code appears in the browser debugger? That's a very good question, I haven't looked at that. It looks like it doesn't change much. I imagine the Debugger API is more based on the notion of "script" than "globals". So that we can still see all module files. We are still using loadSubScript, which most likely help Debugger API identify scripts. > > Re-using the same variable name is causing confusion here. Maybe > sharedGlobalObject? Is there a reason to not just use the loader's > compartment? You are right, it definitely is confusing, renamed to sharedGlobalSandbox. To mean that it is a bit more than just an object. Otherwise, there is a reason to not use loader compartment. That because we want to fine tune the final settings of the sandboxes we are using. (like invisibleToDebuggger, sandbox metadata, ...) loader.js is loaded as a JSM and we can't control all these things via Cu.import. New try: https://tbpl.mozilla.org/?tree=Try&rev=b232bb8a3b2d

Rebased again. Eddy, I had to partially revert http://hg.mozilla.org/mozilla-central/rev/7585a00f7f3e Using magic globals is bad! It happens to not break today because there is some magic behind `const` and sandbox globals. If you already have a global on the sandbox, let's say, `setTimeout` and your later do `const {setTimeout}= ...;`, today it doesn't throw thanks to this magic. But with this optimization, it is no longer magic and it throws! We shouldn't be using any magic global, or the exact same ones than SDK. Otherwise we are going to introduce unexpected conflicts between our magic globals and whatever modules are doing. If we want to expose new globals, we should also expose them in SDK module loaders *and* remove all the conflicting `const` of SDK and devtools codebase at once. In this patch, I'm replacing all magic globals by pseudo modules. So that we never get any unexpected conflict when loading sdk (or 3rd party) modules. Also fixed some tests failures. Here is what should be a green try: https://tbpl.mozilla.org/?tree=Try&rev=0cc783e0a9d8

(In reply to Alexandre Poirot (:ochameau) from comment #33) > Created attachment 8451591 [details] [diff] [review] > patch > > Rebased again. > > Eddy, I had to partially revert > http://hg.mozilla.org/mozilla-central/rev/7585a00f7f3e > Using magic globals is bad! It happens to not break today > because there is some magic behind `const` and sandbox globals. > If you already have a global on the sandbox, > let's say, `setTimeout` and your later do `const {setTimeout}= ...;`, > today it doesn't throw thanks to this magic. > But with this optimization, it is no longer magic and it throws! > > We shouldn't be using any magic global, or the exact same ones than SDK. > Otherwise we are going to introduce unexpected conflicts between our magic > globals > and whatever modules are doing. > If we want to expose new globals, we should also expose them in SDK module > loaders > *and* remove all the conflicting `const` of SDK and devtools codebase at > once. > > In this patch, I'm replacing all magic globals by pseudo modules. > So that we never get any unexpected conflict when loading sdk (or 3rd party) > modules. > > Also fixed some tests failures. > Here is what should be a green try: > https://tbpl.mozilla.org/?tree=Try&rev=0cc783e0a9d8 I moved back and forth between using 'magic' globals and pseudomodules several times before I eventually settled on the former. I've discussed this with several people, including Panos, who seemed to agree with me at the time. I'm not too convinced by your explanation why globals are bad either. If setTimeout is already defined as a global, why would you ever need to do const { setTimeout } = ...? It seems to me that 99% of the time, doing so would be an error, so throwing is actually a good thing. And if you really absolutely must work around it, you can do so by doing something like const { setTimeout: mySetTimeout } = ... I guess my question to you would be why you absolutely need this change. Switching from pseudomodules to globals back to pseudomodules seems like going in circles to me. How would not making this change block you from progressing? Alternatively, you can try to convince me that using globals in this way really *is* a bad idea (giving a concrete example where you really want to require something that's already defined as a global would go a long way in convincing me). That said, I really don't care *that* much how we end up implementing it, as long as we don't break worker debugging (which your patch doesn't, afaict). So I'm not going to fight you too hard on this. However, I want to at least make sure that we're all on the same page here, so we won't end up switching back to globals again a few months later. I hope that sounds reasonable to you. Ping me on irc when you have time. We can probably hash this out in 10 minutes.

Comment on attachment 8451591 [details] [diff] [review] patch Hm. I guess I should have given my feedback here. Instead, I've done so in the previous comment. Please check there! Also cc'ing past because I'd like to know what he has to say on this.

Clarified the magic global story with Eddy on irc and got the green light from Panos. I only see what looks like intermittent on TBPL. I think we are ready to save 3MB \o/

I'd certainly prefer to have globals for things that are either global variables in the web (like |setTimeout|) or almost-but-not-quite like lowercase |promise| (but that's just a temporary thing until we convert to |Promise|). I guess the issue here is that we use non-configurable and non-writable properties in the SDK loader and I understood that it would be hard for Alex to change that everywhere. This is why I settled with using modules instead of globals.

Comment on attachment 8451591 [details] [diff] [review] patch Review of attachment 8451591 [details] [diff] [review]: ----------------------------------------------------------------- Carrying over r+ from comment 31 by :mossop and comment 29 by :past

Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/0b639a4a0de2a1c0b3222884e6f6d6231164a210 Bug 1013997 - Use only one compartment for all devtools modules. r=mossop, r=past