The B2G main process has 100 system compartments and wastes half (5mb) of its JS heap [1]. I expect we'd see a significant win if we could merge these compartments. I'd prefer to do a system-level fix rather than asking us to merge the .js/.jsm's because we share many of these files with Firefox, so they should be handled with care. Bobby, how hard would it be to ask the system to load files X, Y, and Z into the same compartment? [1] attachment 668326 [details]. Save this file, decompress it, and open it via the "read reports from file" button at the bottom of about:memory.

> open it > via the "read reports from file" button at the bottom of about:memory. BTW, you need a fairly recent Nightly or dev build to do this.

(In reply to Justin Lebar [:jlebar] from comment #0) > Bobby, how hard would it be to ask the system to load files X, Y, and Z into > the same compartment? Given CPG, it would mean that they'd all be in the global and have the same scope. This goes contrary to the assumptions built into JSMs. Merging them in postprocessing would be a major footgun IMO. It seems like a more productive (and widely-applicable) solution would be to reduce the footprint of those compartments. If we're really wasting large portions of various compartments, we should find a way to make those arenas smaller (and have them grow / allocate more on demand).

> It seems like a more productive (and widely-applicable) solution would be to reduce the > footprint of those compartments. We have bugs (basically bug 759585, whose summary does not entirely reflect the issue), but it strikes me as unlikely that we'll get them done for B2G v1. But maybe someone just needs to spend some time on it. I guess I was wondering if we have any other options. > Given CPG, it would mean that they'd all be in the global and have the same scope. This > goes contrary to the assumptions built into JSMs. Merging them in postprocessing would be > a major footgun IMO. I see. The alternative is to manually merge these files (either make them all one file, or use #includes), which has the same effect of causing all the code to share a global, but also is annoying to write and potentially complicated to undo.

Yeah. We could add some hack in the mozJSComponentLoader that allows JSMs to be imported in such a way that they share globals, but I'm kind of queasy about building in support for violating one of the explicit guarantees (isolation) promised by the JSM model. Given that, I'd be in favor of some kind post-processed OmniJSM, I guess. We should really just make compartments not waste memory, though.

Bill, this is the most obvious JS-related b2g memory issue I see. Do you think you could work on this? It's not clear how we should fix it (whether by manually merging JSMs or doing bug 759585 or something else), but it looks like we need to do something...

(In reply to Justin Lebar [:jlebar] from comment #0) > The B2G main process has 100 system compartments and wastes half (5mb) of > its JS heap [1]. > How are many compartments causing us to waste JS heap? Are most/many of the compartments oversized for the allocations happening within them? Or is it overhead of the compartments themselves? (Or something else.) If the former, could we cut the default size way down, to 1K or something?

> Are most/many of the compartments oversized for the allocations happening within them? Yes, numerically, the vast majority meet this criterion. > If the former, could we cut the default size way down, to 1K or something? Part of the problem is that aiui the things which are causing this waste are directly mmap'ed, so can't be smaller than 4kb. We have (potentially) more than one such thing per compartment. But I forget exactly how this works, so maybe Bill can fill in details.

I'd like to get bug 759585 done by the end of the year. Is that soon enough for B2G? Here's a quick summary. Two GC things must be on different pages if they are from different compartments or if they are of different types. This allows us to very quickly tell what compartment something comes from as well as its type, because we can store that data in the page's header. However, it has a cost. There are about 15 different types: shapes, scripts, type objects, and objects with a varying number of inline slots. A small compartment will typically have a few objects of each type (i.e., more than one but fewer than ten), so it needs 15 pages, each of which is mostly empty. This became a much bigger problem with CPG, when the number of system compartments went from one to several hundred. The solution in bug 759585 is to allow compartments to share pages, and to obtain the compartment of a given object by a different mechanism.

Basically, having all of the following is bad: (a) lots of small compartments (b) ~20 kinds of GC thing (c) each arena is 4KB (d) each arena can only hold one kind of GC thing (e) each arena is owned by a single compartment The worst case scenario is a compartment that has 1 instance of each GC thing; it will use up ~80KB of heap to store those ~20 GC things. This bug is about changing (a). Bug 759585 is about changing (e). IIRC Luke tried changing (c) so that each arena was 2KB when doing CPG, but then decided against it. (Maybe it didn't make much difference?) I'm not sure how hard changing (b) or (d) would be, though my guess is "quite".

> I'd like to get bug 759585 done by the end of the year. Is that soon enough for B2G? Unlikely. B2G is shipping FF18, according to a newsgroup message I just read. Now that I think of it, the fact that we have to land any changes we want to make on Aurora severely limits our options in terms of large JS changes. It's quite unfortunate that we didn't have any time to optimize the platform before we branched, but that's a battle I fought and lost long ago.

Well, if we need to do something really simple and low risk, the easiest would be to reduce the number of allocation kinds. There are trade-offs, though. - We could turn off background finalization. That would eliminate six allocation kinds, but it would make GCs slower. I'm not sure how badly this would affect b2g. - We could eliminate all the JS object classes except one, maybe the one with 4 inline slots. This would cause us to use more memory in some cases, but I think it would be a savings overall. It would also slow down the JITs a little. We'd only want to do this for b2g though. Also, it's crazy that we can't make any platform changes for b2g. Is there any possibility of revising that policy? Where are these decisions made?

(In reply to Justin Lebar [:jlebar] from comment #10) > Now that I think of it, the fact that we have to land any changes we want to > make on Aurora severely limits our options in terms of large JS changes. That's by design. > It's quite unfortunate that we didn't have any time to optimize the platform > before we branched, but that's a battle I fought and lost long ago. There's nothing to gain by arguing here, but what do you think we've been doing for the last 10+ months? ;) (In reply to Bill McCloskey (:billm) from comment #11) > Also, it's crazy that we can't make any platform changes for b2g. Is there > any possibility of revising that policy? Where are these decisions made? We can and are ... but since we're stabilizing, the changes need to be landable on aurora as well.

> (In reply to Bill McCloskey (:billm) from comment #11) > > Also, it's crazy that we can't make any platform changes for b2g. Is there > > any possibility of revising that policy? Where are these decisions made? > > We can and are ... but since we're stabilizing, the changes need to be > landable on aurora as well. I've always thought aurora was for bugfixes only. In the few cases I've seen someone ask for approval to land an optimization (for memory or speed) on aurora, it's been rejected. Does that mean no optimizations, or have I misunderstood the policy?

Yes, that's generally true. However, the "bug" / "optimization" space is not black and white. The non-blocking-basecamp+ requests will be triaged.

(In reply to Bill McCloskey (:billm) from comment #11) > - We could turn off background finalization. That would eliminate six > allocation kinds, but it would make GCs slower. I'm not sure how badly this > would affect b2g. Background finalization is already disabled for single-core CPUs, right? I think multi-core CPUs are a fairly high end thing for phones, so maybe this already isn't going to be enabled for B2G? (I have no actual knowledge of what the hardware is, that's just a guess.)

(In reply to Bill McCloskey (:billm) from comment #13) > I've always thought aurora was for bugfixes only. In the few cases I've seen > someone ask for approval to land an optimization (for memory or speed) on > aurora, it's been rejected. Does that mean no optimizations, or have I > misunderstood the policy? We have been less strict with Firefox for Android for some time and have been uplifting even some feature work and AFAIK even platform adjustments through the channels. I think we can make the argument to do the same for B2G in certain cases - but we should try to minimize the risk for desktop and Android as much as possible. You probably should ask release-drivers how we're going to play that.

> I think multi-core CPUs are a fairly high end thing for phones, so maybe this already isn't going to > be enabled for B2G? (I have no actual knowledge of what the hardware is, that's just a guess.) Our CPUs are multicore, yes.

(In reply to Justin Lebar [:jlebar] from comment #17) > > I think multi-core CPUs are a fairly high end thing for phones, so maybe this already isn't going to > > be enabled for B2G? (I have no actual knowledge of what the hardware is, that's just a guess.) > > Our CPUs are multicore, yes. Ack, sorry. Single core! Single core.

Well, the number of cores only determines whether we do background allocation, not background finalization. So we'd still take a hit by turning it off. Anyway, I filed bug 799519 to reduce the number of allocation kinds.

(In reply to Nicholas Nethercote [:njn] from comment #9) > IIRC Luke tried changing (c) so that each arena was 2KB when doing CPG, but > then decided against it. (Maybe it didn't make much difference?) It made a real difference (I forget exactly how much) and was a single-line change when I first did the experiment. When I tried to do it again later, we ran into the problem that the arena decommit logic wants arenas to be page-sized. I seem to recall that all the proposed workarounds were kinda gross.

We could also try to share arenas between system compartments.

(In reply to Gregor Wagner [:gwagner] from comment #21) > We could also try to share arenas between system compartments. But we infer the compartment of an object by examining its arena, right? I don't see how we'd overcome that ambiguity.

(In reply to Bobby Holley (:bholley) from comment #22) > (In reply to Gregor Wagner [:gwagner] from comment #21) > > We could also try to share arenas between system compartments. > > But we infer the compartment of an object by examining its arena, right? I > don't see how we'd overcome that ambiguity. Right, the current lookup wouldn't work. It might still make sense to add a new pointer per object for tiny system compartments and avoid many half-empty arenas.

Do we have a path forward here?

Is the plan to optimize the JS object kinds to reduce wasted heap?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #25) > Is the plan to optimize the JS object kinds to reduce wasted heap? I thought njn largely discredited that idea in bug 799519.

Yeah, I'm struggling to see how things will improve on the JS side of this -- i.e. making compartments take up less space -- in the short-term. As for reducing the number of compartments, is there scope for some hacky manual concatenation of .jsm files or something? That's not nice, but it's my best idea :(

(In reply to Nicholas Nethercote [:njn] from comment #27) > Yeah, I'm struggling to see how things will improve on the JS side of this > -- i.e. making compartments take up less space -- in the short-term. > > As for reducing the number of compartments, is there scope for some hacky > manual concatenation of .jsm files or something? That's not nice, but it's > my best idea :( Perhaps preprocess B2G's JSMs into one and somehow alias that file to the different file names? That way we get meaningful JS stacks for a DEBUG build (or a build that doesn't have that particular preprocessing turned on). (On a somewhat related note, I wish there was a declarative way for a JSM consumer to declare which symbols it wants to introduce into its local scope.)

jlebar will produce a list of compartments currently being used so people can look through it and identify if any compartments are being loaded unnecessarily.

Here are all the system compartments in the B2G main process, for a recent build. > 16.94 MB (24.15%) -- js-non-window > 13.80 MB (19.67%) -- compartments > 12.66 MB (18.04%) -- non-window-global > 11.03 MB (15.72%) -- (92 tiny) > 0.61 MB (00.87%) [anonymous sandbox] (from: resource:///modules/devtools/dbg-server.jsm:39)) > 0.45 MB (00.65%) resource://gre/modules/XPCOMUtils.jsm) > 0.40 MB (00.57%) resource://gre/modules/CSPUtils.jsm) > 0.38 MB (00.54%) resource://gre/modules/Webapps.jsm) > 0.36 MB (00.52%) jar:file:///system/b2g/omni.ja!/components/WifiWorker.js) > 0.36 MB (00.51%) jar:file:///system/b2g/omni.ja!/components/SettingsManager.js) > 0.34 MB (00.48%) jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js) > 0.34 MB (00.48%) jar:file:///system/b2g/omni.ja!/components/BrowserElementParent.js) > 0.34 MB (00.48%) resource://gre/modules/UserAgentOverrides.jsm) > 0.29 MB (00.41%) resource://gre/modules/XPIProvider.jsm) > 0.27 MB (00.38%) jar:file:///system/b2g/omni.ja!/components/RILContentHelper.js) > 0.23 MB (00.33%) jar:file:///system/b2g/omni.ja!/components/AppProtocolHandler.js) > 0.23 MB (00.33%) jar:file:///system/b2g/omni.ja!/components/contentSecurityPolicy.js) > 0.21 MB (00.31%) jar:file:///system/b2g/omni.ja!/components/SystemMessageInternal.js) > 0.20 MB (00.29%) jar:file:///system/b2g/omni.ja!/components/DOMWifiManager.js) > 0.19 MB (00.27%) resource://gre/modules/SettingsChangeNotifier.jsm) > 0.17 MB (00.24%) jar:file:///system/b2g/omni.ja!/components/Webapps.js) > 0.16 MB (00.23%) resource://gre/modules/AddonManager.jsm) > 0.12 MB (00.18%) resource://gre/modules/WspPduHelper.jsm) > 0.12 MB (00.17%) jar:file:///system/b2g/omni.ja!/components/ConsoleAPI.js) > 0.12 MB (00.17%) resource://gre/modules/ActivitiesService.jsm) > 0.12 MB (00.16%) resource://gre/modules/AppsUtils.jsm) > 0.11 MB (00.16%) resource://gre/modules/ril_consts.js) > 0.11 MB (00.16%) jar:file:///system/b2g/omni.ja!/components/nsHandlerService.js) > 0.11 MB (00.15%) resource://gre/modules/devtools/dbg-client.jsm) > 0.11 MB (00.15%) jar:file:///system/b2g/omni.ja!/components/TelemetryPing.js) > 0.11 MB (00.15%) jar:file:///system/b2g/omni.ja!/components/NetworkManager.js) > 0.10 MB (00.14%) resource://gre/modules/IndexedDBHelper.jsm) > 0.10 MB (00.14%) resource://gre/modules/ctypes.jsm) > 0.10 MB (00.14%) resource://gre/modules/DOMRequestHelper.jsm) > 0.10 MB (00.14%) resource://gre/modules/LightweightThemeManager.jsm) > 0.10 MB (00.14%) resource:///modules/services-common/log4moz.js) > 0.09 MB (00.13%) jar:file:///system/b2g/omni.ja!/components/SmsDatabaseService.js) > 0.09 MB (00.13%) resource://gre/modules/ObjectWrapper.jsm) > 0.09 MB (00.13%) resource://gre/modules/AlarmService.jsm) > 0.09 MB (00.13%) jar:file:///system/b2g/omni.ja!/components/SettingsService.js) > 0.09 MB (00.13%) resource://gre/modules/NetworkStatsService.jsm) > 0.09 MB (00.12%) jar:file:///system/b2g/omni.ja!/components/nsPrompter.js) > 0.08 MB (00.12%) resource://gre/modules/ContactDB.jsm) > 0.08 MB (00.12%) jar:file:///system/b2g/omni.ja!/components/nsUpdateTimerManager.js) > 0.08 MB (00.12%) resource://gre/modules/NetUtil.jsm) > 0.08 MB (00.11%) resource://gre/modules/BrowserElementPromptService.jsm) > 0.08 MB (00.11%) resource://gre/modules/NetworkStatsDB.jsm) > 0.08 MB (00.11%) resource://gre/modules/systemlibs.js) > 0.08 MB (00.11%) resource://gre/modules/accessibility/AccessFu.jsm) > 0.08 MB (00.11%) resource://gre/modules/DOMFMRadioParent.jsm) > 0.08 MB (00.11%) jar:file:///system/b2g/omni.ja!/components/MmsService.js) > 0.08 MB (00.11%) jar:file:///system/b2g/omni.ja!/components/messageWakeupService.js) > 0.07 MB (00.11%) resource://gre/modules/PluginProvider.jsm) > 0.07 MB (00.11%) jar:file:///system/b2g/omni.ja!/components/SystemMessageManager.js) > 0.07 MB (00.10%) resource://gre/modules/Payment.jsm) > 0.07 MB (00.10%) resource://gre/modules/Geometry.jsm) > 0.07 MB (00.10%) jar:file:///system/b2g/omni.ja!/components/MozKeyboard.js) > 0.07 MB (00.10%) resource://gre/modules/ContactService.jsm) > 0.07 MB (00.10%) resource://gre/modules/PermissionsInstaller.jsm) > 0.07 MB (00.10%) resource://gre/modules/AddonLogging.jsm) > 0.07 MB (00.10%) resource://specialpowers/MockFilePicker.jsm) > 0.07 MB (00.10%) resource://gre/modules/accessibility/Utils.jsm) > 0.07 MB (00.10%) resource://gre/modules/SettingsQueue.jsm) > 0.07 MB (00.10%) resource://gre/modules/accessibility/TouchAdapter.jsm) > 0.07 MB (00.10%) resource://gre/modules/ConsoleAPIStorage.jsm) > 0.07 MB (00.09%) jar:file:///system/b2g/omni.ja!/components/DirectoryProvider.js) > 0.07 MB (00.09%) resource://gre/modules/SettingsDB.jsm) > 0.07 MB (00.09%) jar:file:///system/b2g/omni.ja!/components/ProcessGlobal.js) > 0.06 MB (00.09%) jar:file:///system/b2g/omni.ja!/components/NetworkStatsManager.js) > 0.06 MB (00.09%) resource://gre/modules/WapPushManager.js) > 0.06 MB (00.09%) resource://gre/modules/CertUtils.jsm) > 0.06 MB (00.09%) jar:file:///system/b2g/omni.ja!/components/addonManager.js) > 0.06 MB (00.09%) resource://gre/modules/Services.jsm) > 0.06 MB (00.09%) jar:file:///system/b2g/omni.ja!/components/marionettecomponent.js) > 0.06 MB (00.09%) chrome://marionette/content/marionette-elements.js) > 0.06 MB (00.09%) resource://gre/modules/FileUtils.jsm) > 0.06 MB (00.09%) resource://gre/modules/AlarmDB.jsm) > 0.06 MB (00.09%) jar:file:///system/b2g/omni.ja!/components/ActivityProxy.js) > 0.06 MB (00.09%) jar:file:///system/b2g/omni.ja!/components/AppsService.js) > 0.06 MB (00.09%) resource://gre/modules/devtools/_Promise.jsm) > 0.06 MB (00.09%) jar:file:///system/b2g/omni.ja!/components/nsDefaultCLH.js) > 0.06 MB (00.09%) jar:file:///system/b2g/omni.ja!/components/ActivitiesGlue.js) > 0.06 MB (00.09%) resource:///modules/devtools/dbg-server.jsm) > 0.06 MB (00.09%) jar:file:///system/b2g/omni.ja!/components/jsconsole-clhandler.js) > 0.06 MB (00.08%) resource://gre/modules/PermissionSettings.jsm) > 0.06 MB (00.08%) resource://gre/modules/PermissionPromptHelper.jsm) > 0.06 MB (00.08%) resource://gre/modules/wap_consts.js) > 0.05 MB (00.07%) resource://gre/modules/PrivateBrowsingUtils.jsm) > 0.05 MB (00.07%) resource://gre/modules/jsdebugger.jsm) > 0.05 MB (00.07%) chrome://global/content/bindings/general.xml) > 0.04 MB (00.06%) chrome://global/content/bindings/popup.xml) > 0.04 MB (00.06%) oz-nullprincipal:{a6ab571e-05b9-486f-bd03-01c0baec8d30}) > 0.04 MB (00.06%) oz-nullprincipal:{5e35df73-e88b-4d5f-abe0-1f289a9bf733}) > 0.04 MB (00.05%) ull-principal) > 0.02 MB (00.04%) chrome://global/content/bindings/scrollbar.xml) > 0.02 MB (00.04%) chrome://global/content/platformHTMLBindings.xml) > 0.87 MB (01.24%) chrome://browser/content/shell.xul) > 0.76 MB (01.08%) app://system.gaiamobile.org/index.html)

And here are all the system compartments in the browser process. > 37.51 MB (100.0%) -- explicit > 19.21 MB (51.20%) ++ window-objects > 7.41 MB (19.76%) -- js-non-window > 3.79 MB (10.11%) -- compartments > 2.75 MB (07.32%) -- non-window-global > 0.37 MB (00.99%) compartment([System Principal]) > 0.25 MB (00.67%) jar:file:///system/b2g/omni.ja!/components/ConsoleAPI.js) > 0.24 MB (00.63%) resource://gre/modules/XPCOMUtils.jsm) > 0.22 MB (00.60%) resource://gre/modules/ConsoleAPIStorage.jsm) > 0.21 MB (00.55%) resource://gre/modules/BrowserElementPromptService.jsm) > 0.21 MB (00.55%) jar:file:///system/b2g/omni.ja!/components/SiteSpecificUserAgent.js) > 0.20 MB (00.54%) resource://gre/modules/UserAgentOverrides.jsm) > 0.20 MB (00.52%) resource://gre/modules/Geometry.jsm) > 0.19 MB (00.51%) jar:file:///system/b2g/omni.ja!/components/DirectoryProvider.js) > 0.09 MB (00.23%) jar:file:///system/b2g/omni.ja!/components/nsPrompter.js) > 0.08 MB (00.22%) jar:file:///system/b2g/omni.ja!/components/BrowserElementParent.js) > 0.06 MB (00.16%) resource://gre/modules/Services.jsm) > 0.06 MB (00.16%) jar:file:///system/b2g/omni.ja!/components/ProcessGlobal.js) > 0.05 MB (00.14%) resource://gre/modules/PrivateBrowsingUtils.jsm) > 0.05 MB (00.12%) chrome://global/content/bindings/videocontrols.xml) > 0.04 MB (00.11%) ++ compartment(moz-nullprincipal:{41d27a3c-8a3f-4d6c-bcc2-588e789bf5da}) > 0.04 MB (00.11%) chrome://global/content/bindings/scale.xml) > 0.04 MB (00.11%) chrome://global/content/bindings/button.xml) > 0.04 MB (00.11%) chrome://global/content/bindings/general.xml) > 0.04 MB (00.10%) chrome://global/content/bindings/text.xml) > 0.04 MB (00.10%) chrome://global/content/bindings/progressmeter.xml) > 0.04 MB (00.10%) ++ compartment(null-principal)

> > 0.34 MB (00.48%) resource://gre/modules/UserAgentOverrides.jsm) Is B2G using per-site UA support? Sounds like this could go. > > 0.29 MB (00.41%) resource://gre/modules/XPIProvider.jsm) I think there's a bug on turning this off. > > 0.16 MB (00.23%) resource://gre/modules/AddonManager.jsm) This could go too. > > 0.11 MB (00.16%) resource://gre/modules/ril_consts.js) Maybe this can be inlined into other RIL modules? > > 0.11 MB (00.16%) jar:file:///system/b2g/omni.ja!/components/nsHandlerService.js) > > 0.10 MB (00.14%) resource://gre/modules/ctypes.jsm) Does b2g need ctypes? > > 0.10 MB (00.14%) resource://gre/modules/LightweightThemeManager.jsm) Or Personas? > > 0.09 MB (00.12%) jar:file:///system/b2g/omni.ja!/components/nsPrompter.js) Does BrowserElementPromptService.jsm supplant this? > > 0.07 MB (00.10%) resource://gre/modules/Geometry.jsm) What is including this? > > 0.07 MB (00.10%) jar:file:///system/b2g/omni.ja!/components/MozKeyboard.js) > > 0.07 MB (00.10%) resource://gre/modules/AddonLogging.jsm) Doesn't seem necessary. > > 0.07 MB (00.10%) resource://specialpowers/MockFilePicker.jsm) Definitely doesn't seem necessary! > > 0.07 MB (00.10%) resource://gre/modules/accessibility/Utils.jsm) Does b2g even have working a11y? > > 0.06 MB (00.09%) jar:file:///system/b2g/omni.ja!/components/addonManager.js) Same as other addon stuff. > > 0.06 MB (00.09%) jar:file:///system/b2g/omni.ja!/components/marionettecomponent.js) > > 0.06 MB (00.09%) chrome://marionette/content/marionette-elements.js) These should not be necessary! > > 0.06 MB (00.09%) jar:file:///system/b2g/omni.ja!/components/nsDefaultCLH.js) I don't think we should need command line handlers for b2g, but maybe I'm wrong.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #32) > > > 0.34 MB (00.48%) resource://gre/modules/UserAgentOverrides.jsm) > > Is B2G using per-site UA support? Sounds like this could go. Yes we use that. > > > 0.29 MB (00.41%) resource://gre/modules/XPIProvider.jsm) > > I think there's a bug on turning this off. > > > > 0.16 MB (00.23%) resource://gre/modules/AddonManager.jsm) > > This could go too. This will go once https://bugzilla.mozilla.org/show_bug.cgi?id=794228 is on aurora. > > > > 0.10 MB (00.14%) resource://gre/modules/ctypes.jsm) > > Does b2g need ctypes? Yes, for the RIL and wifi. > > > > 0.07 MB (00.10%) resource://specialpowers/MockFilePicker.jsm) > > Definitely doesn't seem necessary! yep, I wonder what loads that! > > > > 0.07 MB (00.10%) resource://gre/modules/accessibility/Utils.jsm) > > Does b2g even have working a11y? Yes, we have some early support. > > > 0.06 MB (00.09%) jar:file:///system/b2g/omni.ja!/components/marionettecomponent.js) > > > 0.06 MB (00.09%) chrome://marionette/content/marionette-elements.js) > > These should not be necessary! > > > > 0.06 MB (00.09%) jar:file:///system/b2g/omni.ja!/components/nsDefaultCLH.js) > > I don't think we should need command line handlers for b2g, but maybe I'm > wrong. We use it at least on desktop, but we should look into disabling that on device.

How hard would it to be to try a really aggressive solution here like what philikon proposes in comment 28?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #34) > How hard would it to be to try a really aggressive solution here like what > philikon proposes in comment 28? My main concern with this approach is that we'd be using different scoping on desktop and B2G for the same files. If there are any collisions between the files, we won't see it on Tinderbox or in desktop builds, and we have very little test coverage on B2G. Maybe that worry is unfounded, and maybe we don't have a choice. I don't know. If we're going to go down this route, my preference would again be what I suggested in comment 1 -- that is, I'd prefer to address this by loading these files into the same compartment (via a Gecko change), because that lets us maintain the lazy-loading properties of JSMs. But I don't know how hard that would be.

(In reply to Justin Lebar [:jlebar] from comment #35) > If we're going to go down this route, my preference would again be what I > suggested in comment 1 -- that is, I'd prefer to address this by loading > these files into the same compartment (via a Gecko change), because that > lets us maintain the lazy-loading properties of JSMs. But I don't know how > hard that would be. Just to reiterate what Bobby said, this is difficult. Ever since CPG landed, we've added assumptions all over the JS engine and the browser that if two objects are in the same compartment, then they have the same global object. Given how deeply embedded this invariant is now, it would be a ton of work and cause lots of regressions if we broke it. We could use the same global object for all the JSMs. But I think that would have all the same collision issues that you're worried about.

> We could use the same global object for all the JSMs. But I think that would have all the same > collision issues that you're worried about. Yes, exactly. My point is that if we are going to accept using the same global object for all these JSMs and having the potential for collisions -- which happens if we manually merge the script files or if we do what I suggested in comment 1 -- it seems to me that we might as well alter Gecko to stick all the JSMs into the same compartment, since at least that still gives us lazy loading. I understand that basically undoing CPG is hard. But would what I'm suggesting be hard?

We can avoid collision issues in preprocessing by having the script wrap the concatenated files in their own JS scopes. The exported symbols by definition can't conflict.

(In reply to Justin Lebar [:jlebar] from comment #37) > I understand that basically undoing CPG is hard. But would what I'm > suggesting be hard? Sorry, I misunderstood. I don't think that would be very hard.

FWIW, I care much more that this has an owner than I care how we fix this.

Bill, can you take this?

I think khuey is going to try to hack something together that merges some of the modules together. A JS engine fix would be nice though :)

If we're going to merge JSMs together, Kyle's probably a better person to do it. I'll be happy to help with any JS engine or xpconnect work that we need, though.

That's probably less risky. We'll just have to learn to live with the taste of vom in our mouths.

I have a mostly working thing here. I'll put up patches tomorrow after I spend some time atoning for my sins.

What kind of savings are you seeing? 5MB+ washes out a lot of vom ...

Steps (1) Reboot (2) Unlock lock screen (3) Wait for homescreen to load Gathered from $ python tools/get_about_memory.py --minimize 23.32 MB (100.0%) -- js-main-runtime ├──19.96 MB (85.62%) -- compartments │ ├───9.00 MB (38.60%) -- gc-heap │ │ ├──4.65 MB (19.95%) ── unused-gc-things

After 9.50 MB (100.0%) -- js-main-runtime ├──5.01 MB (52.67%) -- compartments │ ├──2.65 MB (27.87%) -- gc-heap │ │ ├──1.06 MB (11.12%) ── unused-gc-things [2] \o/ \o/ So we cut 3.5MB off unused-gc-things, and reduced the size of the main heap by *14* MB!

Holy Christmas. Android will want this too.

OK, for main process we go from 23.32 MB (100.0%) -- js-main-runtime ├──19.96 MB (85.62%) -- compartments │ ├───9.00 MB (38.60%) -- gc-heap │ │ ├──4.65 MB (19.95%) ── unused-gc-things to 12.29 MB (100.0%) -- js-main-runtime ├───9.24 MB (75.17%) -- compartments │ ├──5.08 MB (41.31%) -- gc-heap │ │ ├──2.35 MB (19.11%) ── unused-gc-things For the homescreen process, we go from 14.06 MB (100.0%) -- js-main-runtime ├───8.26 MB (58.75%) -- compartments │ ├──5.35 MB (38.07%) -- gc-heap │ │ ├──3.11 MB (22.13%) ── unused-gc-things [2] to 9.50 MB (100.0%) -- js-main-runtime ├──5.01 MB (52.67%) -- compartments │ ├──2.65 MB (27.87%) -- gc-heap │ │ ├──1.06 MB (11.12%) ── unused-gc-things [2]

After repeating the same experiment one more time with these patches I get b2g process 11.92 MB (100.0%) -- js-main-runtime ├───8.89 MB (74.60%) -- compartments │ ├──5.10 MB (42.79%) -- gc-heap │ │ ├──2.44 MB (20.50%) ── unused-gc-things homescreen 9.50 MB (100.0%) -- js-main-runtime ├──5.01 MB (52.67%) -- compartments │ ├──2.65 MB (27.87%) -- gc-heap │ │ ├──1.06 MB (11.12%) ── unused-gc-things [2] so the numbers look pretty stable.

For the Main Process measurements, After that: - analysis-temporary: 5.3 MiB - unused-gc-things: 2.3 MiB - objects: 0.8 MiB - shapes: 0.6 MiB - string-chars: 0.5 MiB - object slots/elems: 0.5 MiB - shapes-extra: 0.4 MiB - cross-compartment-wrappers: 0.2 MiB - runtime: 0.2 MiB The temporary-analysis and unused-gc-things wins were expected. (And bug 804891 would have got most of the temporary-analysis improvement.) The other changes are less expected. Kyle thinks that the change in objects (and thus shapes) suggests that we have lots of cross-compartment wrapper objects. (The "cross-compartment-wrappers" entry just measures the hash table, not the actual wrapper objects.) I could separate the measurement of wrapper objects if that seems useful -- i.e. add "gc-heap/objects/wrapper", much like we have "gc-heap/objects/function".

Another thing: for the Homescreen app, every measurement is reported twice. E.g.: 53.69 MB ── resident [2] 156.42 MB ── vsize [2] Why on earth does that happen? Looking at the JSON, we have both this: { "kind": 2, "description": "Memory mapped by the process...", "process": "Homescreen (pid 355)", "amount": 29151232, "units": 0, "path": "resident" }, and this: { "kind": 2, "description": "Memory mapped by the process...", "process": "Homescreen (pid 355)", "amount": 29597696, "units": 0, "path": "resident" }, The "amount" field differs slightly between the two, so it looks like we're making two separate measurements. Could there be a problem in the IPC used to trigger the measurements from the main process?

> Could there be a problem in the IPC used to trigger the measurements from the main process? It could be that we're asking the child to send a memory report to the parent through the normal about:memory request-child-memory-report thing, and then we're also asking that child process to directly dump its memory report. If someone can run get_about_memory.py --keep-individual-reports and post all the .gz files, that would be helpful to understand what's happening.

> It could be that we're asking the child to send a memory report to the > parent through the normal about:memory request-child-memory-report thing, But that should only happen if you load about:memory, and that's not possible on B2G... ---- More comparisons: Main Process, before and after: 68.11 MB (100.0%) ++ rss 60.44 MB (100.0%) ++ pss 194.98 MB (100.0%) ++ size 60.03 MB (100.0%) ++ rss 52.27 MB (100.0%) ++ pss 179.14 MB (100.0%) ++ size Pretty good! Perhaps not as good as the "explicit" comparisons. Homescreen process, before and after: 53.88 MB (100.0%) ++ rss 38.50 MB (100.0%) ++ pss 156.42 MB (100.0%) ++ size 56.12 MB (100.0%) ++ rss 40.65 MB (100.0%) ++ pss 143.51 MB (100.0%) ++ size rss and pss went up? These measurements are suspect because of the above-mentioned double-counting, but still... hmm.

It's a completely different phone with these patches. It's going to be difficult negotiating blocking-basecamp+ vs. approval-aurora here, but I think a prereq is landable patches. So we should focus on that first.

> It's a completely different phone with these patches. Great! AIUI the white unagi phones that everyone has in MV have 512 MiB of RAM, whereas the final phone will only have 256 MiB. Is that right?

The unagi physically has 512Mb. However, if you have the kernel-2 update applied, then it reconfigures the memory to look like a phone with only 256 Mb. You can check by doing: adb shell cat /proc/meminfo | grep MemTotal If you get something like MemTotal: 184096 kB then your unagi is configured for 256 Mb.

We'll be shipping a kernel update that artificially limits the unagis to 256MB.

> If you get something like > > MemTotal: 184096 kB > > then your unagi is configured for 256 Mb. Thanks for the detailed response! Is there a good way to communicate this to all the dogfooders? I think it would be useful for people to know if their phone is using 256 or 512 MiB when gauging the performance.

Can we please have this conversation somewhere else?

Alex, Bhavana, The patches here aren't ready to land quite yet, but I want to go ahead and this on the tracking radar. The approval-aurora decision here is going to be dicey and controversial. The downside is - the patch to .jsm's is huge and touches a lot of shared code However - the patch should be semantics preserving for Firefox. There's a continued risk of regressions, but those should only be felt by b2g. So b2g "gets what it pays for". The motivation for landing these patches is, - saves around 10MB of memory in b2g in a simple testcase, and possibly more in realistic scenarios - this is the biggest remaining known win for b2g v1 - that ~10MB makes b2g feel like a "different phone", in the sense of improved UX from fewer applications dying in the background. I can't formalize this yet, but am starting in [1]. Thanks! [1] https://wiki.mozilla.org/B2G/Memory_acceptance_criteria

(Sorry, wrong flag.)

In support of comment 66, this will make a huge difference for Android if enabled. We should consult with the Android drivers whether they want this as well.

This set of patches is green on try with the pref turned off. We decided it was better to change the world to maintain consistency, rather than try to isolate the JS loaded in b2g from the JS loaded elsewhere.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #66) > However > - the patch should be semantics preserving for Firefox. There's a > continued risk of regressions, but those should only be felt by b2g. So b2g > "gets what it pays for". Could you go into any more detail about why the regressions would most likely be limited to B2G, and what a regression on desktop/mobile could look like? (In reply to Andreas Gal :gal from comment #68) > In support of comment 66, this will make a huge difference for Android if > enabled. We should consult with the Android drivers whether they want this > as well. B2G would be the only real motivation for taking this patch on Aurora 18, since this isn't a critical issue for Android. To decrease the risk of regression, we should be moving on reviewing these patches very quickly.

Comment on attachment 675759 [details] [diff] [review] Part 3: Fix the world to put properties on the 'this' object. Review of attachment 675759 [details] [diff] [review]: ----------------------------------------------------------------- This scares and terrifies me. r+

(In reply to Alex Keybl [:akeybl] from comment #73) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #66) > > However > > - the patch should be semantics preserving for Firefox. There's a > > continued risk of regressions, but those should only be felt by b2g. So b2g > > "gets what it pays for". > > Could you go into any more detail about why the regressions would most > likely be limited to B2G, and what a regression on desktop/mobile could look > like? We're requiring the way symbols are exported from JSMs and XPCOM JS components to be slightly different. Semantically there is no difference to Firefox and all other products that don't flip the pref introduced by these patches. If somebody submits a patch to Firefox that uses the "old" way of exporting symbols, Firefox and other products will not break. B2G might. That's what Chris is referring to. I don't see much possibility for Firefox to regress from these patches, given that the try runs are green.

Comment on attachment 675756 [details] [diff] [review] Part 1: Add a pref to the js loader that allows loading things in the same global. Review of attachment 675756 [details] [diff] [review]: ----------------------------------------------------------------- To be honest, this scares me quite a bit. This changes the semantics of JS components in subtle ways (I know that I actually have a patch that will be broken by this) and makes them all share a global. Can we at least put that global into strict mode to avoid mistaken assignment to non-declared variables being stuck on the global? ::: js/xpconnect/loader/mozJSComponentLoader.cpp @@ +569,5 @@ > mModules.Put(spec, entry); > > // Set the location information for the new global, so that tools like > // about:memory may use that information > + //xpc::SetLocationForGlobal(entry->global, spec); Here and where you do this below need to be sorted out. @@ +615,2 @@ > { > + nsresult rv = NS_OK; Might as well bump this declaration down to before the do_GetService call. @@ +657,5 @@ > + JSAutoCompartment ac(aCx, obj); > + > + if (aReuseLoaderGlobal) { > + // If we're reusing the loader global, we don't actually use the > + // global, but rather we use a different object. We use a different object as the this object? @@ +664,5 @@ > + > + obj = newObj; > + } > + > + *aRealFile = false; Nit: newline before the comment. @@ +702,5 @@ > // Expose the URI from which the script was imported through a special > // variable that we insert into the JSM. > + JSString *exposedUri = JS_NewStringCopyN(aCx, nativePath.get(), > + nativePath.Length()); > + if (!JS_DefineProperty(aCx, obj, "__URI__", Wouldn't it be more natural to expose this as a parameter to the function that we create now that we can? @@ +717,5 @@ > + JSObject **aObject, > + char **aLocation, > + jsval *exception) > +{ > + nsresult rv; Please move this declaration down to the first use. @@ +755,5 @@ > + if (!mReuseLoaderGlobal) { > + rv = ReadCachedScript(cache, cachePath, cx, mSystemPrincipal, > + &script); > + } > + else { Nit, here and everywhere else: in this file else cuddles with the previous brace: } else { ... } @@ +788,5 @@ > .setFileAndLine(nativePath.get(), 1) > + .setSourcePolicy(mReuseLoaderGlobal ? > + JS::CompileOptions::NO_SOURCE : > + JS::CompileOptions::LAZY_SOURCE); > + js::RootedObject rootedObject(cx, obj); Why doesn't this have to be up where we get obj from PrepareObjectForLocation? @@ +947,5 @@ > } > // Propagate the exception, if one exists. Also, don't leave the stale > // exception on this context. > JS_SetOptions(cx, oldopts); > + if ((!script && !function) && exception) { No need for the extra parentheses. @@ +1014,4 @@ > return NS_ERROR_OUT_OF_MEMORY; > } > > + JS_AddNamedObjectRoot(cx, aObject, *aLocation); Perhaps in a followup: we could probably use the shared global to root the functions and objects instead of using named roots. ::: js/xpconnect/loader/mozJSComponentLoader.h @@ +56,5 @@ > nsresult ReallyInit(); > void UnloadModules(); > > + JSObject* PrepareObjectForLocation(JSCLContextHelper& aCx, > + nsIFile* aComponentFile, Uber-nit: trailing whitespace here. ::: js/xpconnect/loader/mozJSLoaderUtils.cpp @@ +39,5 @@ > nsresult > +ReadCachedFunction(StartupCache* cache, nsACString &uri, JSContext *cx, > + nsIPrincipal *systemPrincipal, JSFunction **functionp) > +{ > + return NS_ERROR_FAILURE; Is this really intended? @@ +77,5 @@ > +nsresult > +WriteCachedFunction(StartupCache* cache, nsACString &uri, JSContext *cx, > + nsIPrincipal *systemPrincipal, JSFunction *function) > +{ > + return NS_ERROR_FAILURE; Ditto? ::: js/xpconnect/loader/mozJSLoaderUtils.h @@ +24,5 @@ > > nsresult > +ReadCachedFunction(mozilla::scache::StartupCache* cache, nsACString &uri, > + JSContext *cx, nsIPrincipal *systemPrincipal, > + JSFunction **function); It seems like the parameters are playing pong with the *s and &s. I see that this is copy/pasted, so I guess it's OK, but it'd be nice if at least the new functions were consistent in where the decorators went. ::: modules/libpref/src/init/all.js @@ +3811,5 @@ > // they are handled separately. This pref is only read once at startup: > // a restart is required to enable a new value. > pref("network.activity.blipIntervalMilliseconds", 0); > > +pref("jsloader.reuseGlobal", false); This also needs a big comment explaining what it does.

> It's a completely different phone with these patches. I keep wondering how much of this is due to the reduction in memory consumption, and how much of it is due to a reduction in work involving cross-compartment wrappers. It could be that the latter is significant but it's only really noticeable on B2G because the devices have such low-end CPUs.

Can we please announce this change on dev-platform? And also, is there a way to check/warn if someone exports globals from an jsm in the future?

(In reply to Blake Kaplan (:mrbkap) from comment #76) > Comment on attachment 675756 [details] [diff] [review] > Part 1: Add a pref to the js loader that allows loading things in the same > global. > > Review of attachment 675756 [details] [diff] [review]: > ----------------------------------------------------------------- > > To be honest, this scares me quite a bit. This changes the semantics of JS > components in subtle ways (I know that I actually have a patch that will be > broken by this) and makes them all share a global. Can we at least put that > global into strict mode to avoid mistaken assignment to non-declared > variables being stuck on the global? Not until we make all of our JS code strict-safe. > ::: js/xpconnect/loader/mozJSComponentLoader.cpp > @@ +569,5 @@ > > mModules.Put(spec, entry); > > > > // Set the location information for the new global, so that tools like > > // about:memory may use that information > > + //xpc::SetLocationForGlobal(entry->global, spec); > > Here and where you do this below need to be sorted out. Fixed. > @@ +615,2 @@ > > { > > + nsresult rv = NS_OK; > > Might as well bump this declaration down to before the do_GetService call. Done. > @@ +657,5 @@ > > + JSAutoCompartment ac(aCx, obj); > > + > > + if (aReuseLoaderGlobal) { > > + // If we're reusing the loader global, we don't actually use the > > + // global, but rather we use a different object. > > We use a different object as the this object? Fixed. > @@ +664,5 @@ > > + > > + obj = newObj; > > + } > > + > > + *aRealFile = false; > > Nit: newline before the comment. Done. > @@ +702,5 @@ > > // Expose the URI from which the script was imported through a special > > // variable that we insert into the JSM. > > + JSString *exposedUri = JS_NewStringCopyN(aCx, nativePath.get(), > > + nativePath.Length()); > > + if (!JS_DefineProperty(aCx, obj, "__URI__", > > Wouldn't it be more natural to expose this as a parameter to the function > that we create now that we can? Yes, if we're actually sending this at all, which we don't think we are. > @@ +717,5 @@ > > + JSObject **aObject, > > + char **aLocation, > > + jsval *exception) > > +{ > > + nsresult rv; > > Please move this declaration down to the first use. Done. > @@ +755,5 @@ > > + if (!mReuseLoaderGlobal) { > > + rv = ReadCachedScript(cache, cachePath, cx, mSystemPrincipal, > > + &script); > > + } > > + else { > > Nit, here and everywhere else: in this file else cuddles with the previous > brace: > > } else { > ... > } Done. > @@ +788,5 @@ > > .setFileAndLine(nativePath.get(), 1) > > + .setSourcePolicy(mReuseLoaderGlobal ? > > + JS::CompileOptions::NO_SOURCE : > > + JS::CompileOptions::LAZY_SOURCE); > > + js::RootedObject rootedObject(cx, obj); > > Why doesn't this have to be up where we get obj from > PrepareObjectForLocation? I have no idea, I just copy/pasted. > @@ +947,5 @@ > > } > > // Propagate the exception, if one exists. Also, don't leave the stale > > // exception on this context. > > JS_SetOptions(cx, oldopts); > > + if ((!script && !function) && exception) { > > No need for the extra parentheses. Fixed. > @@ +1014,4 @@ > > return NS_ERROR_OUT_OF_MEMORY; > > } > > > > + JS_AddNamedObjectRoot(cx, aObject, *aLocation); > > Perhaps in a followup: we could probably use the shared global to root the > functions and objects instead of using named roots. Yeah. > ::: js/xpconnect/loader/mozJSComponentLoader.h > @@ +56,5 @@ > > nsresult ReallyInit(); > > void UnloadModules(); > > > > + JSObject* PrepareObjectForLocation(JSCLContextHelper& aCx, > > + nsIFile* aComponentFile, > > Uber-nit: trailing whitespace here. Fixed. > ::: js/xpconnect/loader/mozJSLoaderUtils.cpp > @@ +39,5 @@ > > nsresult > > +ReadCachedFunction(StartupCache* cache, nsACString &uri, JSContext *cx, > > + nsIPrincipal *systemPrincipal, JSFunction **functionp) > > +{ > > + return NS_ERROR_FAILURE; > > Is this really intended? Added comments. > @@ +77,5 @@ > > +nsresult > > +WriteCachedFunction(StartupCache* cache, nsACString &uri, JSContext *cx, > > + nsIPrincipal *systemPrincipal, JSFunction *function) > > +{ > > + return NS_ERROR_FAILURE; > > Ditto? > > ::: js/xpconnect/loader/mozJSLoaderUtils.h > @@ +24,5 @@ > > > > nsresult > > +ReadCachedFunction(mozilla::scache::StartupCache* cache, nsACString &uri, > > + JSContext *cx, nsIPrincipal *systemPrincipal, > > + JSFunction **function); > > It seems like the parameters are playing pong with the *s and &s. I see that > this is copy/pasted, so I guess it's OK, but it'd be nice if at least the > new functions were consistent in where the decorators went. > > ::: modules/libpref/src/init/all.js > @@ +3811,5 @@ > > // they are handled separately. This pref is only read once at startup: > > // a restart is required to enable a new value. > > pref("network.activity.blipIntervalMilliseconds", 0); > > > > +pref("jsloader.reuseGlobal", false); > > This also needs a big comment explaining what it does. Done.

Comment on attachment 676373 [details] [diff] [review] Part 1: Add a pref to the js loader that allows loading things in the same global (comments addressed). Review of attachment 676373 [details] [diff] [review]: ----------------------------------------------------------------- Please file followups on the strict mode thing and the __LOCATION__ thing.

Backed out after two followups on suspicion of mochitest-{2,4}, xpcshell, ... failures (though we've had quite a few busted pushes due to this not compiling initially, so not overly sure as to what caused them): https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a145ded68994 https://hg.mozilla.org/integration/mozilla-inbound/rev/ad1d720d82b7

Doesn't look like this was at fault for the orange. Relanded. https://hg.mozilla.org/integration/mozilla-inbound/rev/67cb43bb8865

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #82) > Doesn't look like this was at fault for the orange. Relanded. > > https://hg.mozilla.org/integration/mozilla-inbound/rev/67cb43bb8865 This change breaks B2G Marionette, waiting for port forwarding till timeout.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #83) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #82) > > Doesn't look like this was at fault for the orange. Relanded. > > > > https://hg.mozilla.org/integration/mozilla-inbound/rev/67cb43bb8865 > > This change breaks B2G Marionette, waiting for port forwarding till timeout. Yeah, 100% sure. 1) checkout head right before this patch from inbound: marionette works fine 2) apply this patch: marionette timeout 3) apply this patch but don't enable it in b2g/app/b2g.js: marionette works fine

Backout for B2G Marionette bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/5bf3abe91210

Here is the console log captured when Marionette hangs and never processes test cases until timeout.

Rebase to inbound tip, Marionette problem still not solved.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #86) > Here is the console log captured when Marionette hangs and never processes > test cases until timeout. I think the console log might not help a lot. We had this problem before[1]. Some config script typo caused ENABLE_MARIONETTE undefined so failed all marionette tests with exactly the same assertion error as what we have now. `wait_for_port` is defined in testing/marionette/client/marionette/emulator.py. It opens a socket stream and waits for in-emulator marionette's response. So, the problem here is, why B2G Marionette stops functioning as normal with this patch. Besides, is it a B2G-only problem? I've pushed a change[2] to try server to verify it. [1]: https://github.com/mozilla-b2g/gonk-misc/issues/40 [2]: https://tbpl.mozilla.org/?tree=Try&rev=9f7634f82eb2

Hi Vicamo, The marionette tests complete successfully for me on 5/5 runs on inbound 7781dba5ed5e. Can you provide more details about why you backed out the patch here?

Try run for 9f7634f82eb2 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=9f7634f82eb2 Results (out of 10 total builds): success: 4 failure: 6 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vyang@mozilla.com-9f7634f82eb2

(In reply to Chris Jones [:cjones] [:warhammer] from comment #89) > Hi Vicamo, > > The marionette tests complete successfully for me on 5/5 runs on inbound > 7781dba5ed5e. Can you provide more details about why you backed out the > patch here? ./build.sh && ./test.sh marionette /home/vicamo/WorkSpace/mozilla/b2g/qemu-arm/gecko/dom/sms/tests/marionette/test_getmessage.js 1) checkout 553fb59b9ca0, run above command, marionette tests pass 2) checkout 67cb43bb8865, run above command, marionette hangs 3) edit b2g/app/b2g.js, set preference "jsloader.reuseGlobal" to false, run above command, marionette tests pass again.

You should have flipped the pref rather than back out a 700 kb patch. Relanding this is going to be a PITA.

Try run for 9f7634f82eb2 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=9f7634f82eb2 Results (out of 11 total builds): success: 4 failure: 7 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vyang@mozilla.com-9f7634f82eb2

I've relanded this with the pref turned off. https://hg.mozilla.org/integration/mozilla-inbound/rev/5ce71981e005 Please don't back out several hundred KB patches when you know that you can flip a pref to fix the problem.

Thanks, Kyle. I filed bug 807478 for re-enabling the pref. Let's move the discussion there and let this bug move towards RESOLVED/FIXED.

> > It's a completely different phone with these patches. > > I keep wondering how much of this is due to the reduction in memory > consumption, and how much of it is due to a reduction in work involving > cross-compartment wrappers. If the improvement is much larger on a 256 MiB phone than a 512 MiB phone, that indicates it's the reduction in memory consumption that makes the difference. But if we still see a significant improvement on a 512 MiB phone, then that indicates the cross-compartent overhead reduction is a factor.

[Approval Request Comment] Bug caused by (feature/regressing bug #): N/A User impact if declined: We need this to meet B2G quality goals. Testing completed (on m-c, etc.): We've landed this on m-c and seen no fallout with the pref turned off (the Desktop/Fennec case). Risk to taking this patch (and alternatives if risky): Relatively low risk compared to the size of the patch. Most of the changes are trivial mechanical changes, the rest is controlled by a pref that's only set on B2G. String or UUID changes made by this patch: N/A

Comment on attachment 680123 [details] [diff] [review] Consolidated patch for Aurora low risk patch , preffed on only for b2g, approving on aurora .

(In reply to bhavana bajaj [:bajaj] from comment #100) > Comment on attachment 680123 [details] [diff] [review] > low risk patch , preffed on only for b2g, approving on aurora . @@ -587,9 +587,11 @@ pref("browser.prompt.allowNative", false // they are handled separately. This pref is only read once at startup: // a restart is required to enable a new value. pref("network.activity.blipIntervalMilliseconds", 250); // Send some sites a custom user-agent. pref("general.useragent.override.facebook.com", "\(Mobile#(Android; Mobile"); pref("general.useragent.override.youtube.com", "\(Mobile#(Android; Mobile"); +pref("jsloader.reuseGlobal", false); + pref("jsloader.reuseGlobal", true); Doesn’t the line below override the new addition?

(In reply to John Drinkwater (:beta) from comment #101) > // Send some sites a custom user-agent. > pref("general.useragent.override.facebook.com", "\(Mobile#(Android; > Mobile"); > pref("general.useragent.override.youtube.com", "\(Mobile#(Android; Mobile"); > > +pref("jsloader.reuseGlobal", false); > + > pref("jsloader.reuseGlobal", true); > > > Doesn’t the line below override the new addition? Yes, good catch. The line below exists because bug 807478 was already landed. So the new addition is superfluous.

https://hg.mozilla.org/releases/mozilla-aurora/rev/658d1cf59d38 I turned the pref off on Aurora, because turning it on currently turns the tree quite orange.

> I turned the pref off on Aurora, because turning it on currently turns the tree quite > orange. Are the dependencies in this bug what we need to turn it on for B2G v1, or is there something else?

I don't know yet. It looked marionette related, but the patches in bug 807478 supposedly landed on Aurora.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #105) > I don't know yet. It looked marionette related, but the patches in bug > 807478 supposedly landed on Aurora. Can you either mark this as ff-18:affected or file a new bug, so we don't lose track of this?

(In reply to Justin Lebar [:jlebar] from comment #106) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #105) > > I don't know yet. It looked marionette related, but the patches in bug > > 807478 supposedly landed on Aurora. > > Can you either mark this as ff-18:affected or file a new bug, so we don't > lose track of this? Bug 810719.

Can we undo this ugliness now that zones have landed?

If someone shows that replacing this with zones retains the same memory usage characteristics, yes. It's not obvious that it will though, because this also eliminates CCWs that zones do not.

It would sure be interesting to measure this.

(In reply to Bobby Holley (:bholley) from comment #108) > Can we undo this ugliness now that zones have landed? I filed bug 989373 for this.