Last Comment Bug 798491 - Reduce number of system compartments in B2G main process
: Reduce number of system compartments in B2G main process
Status: RESOLVED FIXED
[MemShrink:P1][slim:<5MB]
:
Product: Firefox OS
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P1 major (vote)
: ---
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
Depends on: 807860 807862 809666 810139 810987
Blocks: slim-fast 1122978 764220 799519 807104 807478 807698 810719 835886
  Show dependency treegraph
 
Reported: 2012-10-05 11:52 PDT by Justin Lebar (not reading bugmail)
Modified: 2015-01-17 15:51 PST (History)
34 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed


Attachments
Part 1: Add a pref to make the JS component loader load everything in the same global (31.30 KB, patch)
2012-10-25 08:58 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Part 2: Fix the world (653.98 KB, patch)
2012-10-25 08:59 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Before these patches (1.19 MB, text/plain)
2012-10-25 14:47 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details
After (903.56 KB, text/plain)
2012-10-25 14:49 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details
Part 1, uncorrupted (31.30 KB, patch)
2012-10-25 14:52 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Part 2, rebased on f35ce23ddfc8 (654.65 KB, patch)
2012-10-25 14:52 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Part 1: Add a pref to the js loader that allows loading things in the same global. (31.63 KB, patch)
2012-10-26 17:02 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Part 2: If the pref from the previous patch is set, don't assume we have our own global. (1.94 KB, patch)
2012-10-26 17:03 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
mrbkap: review+
Details | Diff | Splinter Review
Part 3: Fix the world to put properties on the 'this' object. (655.23 KB, patch)
2012-10-26 17:04 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
philipp: review+
Details | Diff | Splinter Review
Part 1: Add a pref to the js loader that allows loading things in the same global (comments addressed). (31.98 KB, patch)
2012-10-29 15:59 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
mrbkap: review+
Details | Diff | Splinter Review
Marionette timesout (1.37 KB, text/plain)
2012-10-31 02:14 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details
Add an option to stick all chrome JSMs/JS components in the same compartment (691.22 KB, patch)
2012-10-31 02:54 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Consolidated patch for Aurora (678.64 KB, patch)
2012-11-09 10:11 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
khuey: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2012-10-05 11:52:41 PDT
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.
Comment 1 Nicholas Nethercote [:njn] 2012-10-05 14:44:10 PDT
> 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.
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-10-08 01:18:49 PDT
(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).
Comment 3 Justin Lebar (not reading bugmail) 2012-10-08 06:23:16 PDT
> 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.
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2012-10-08 06:44:07 PDT
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.
Comment 5 Justin Lebar (not reading bugmail) 2012-10-08 19:53:49 PDT
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...
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-08 19:59:56 PDT
(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?
Comment 7 Justin Lebar (not reading bugmail) 2012-10-08 20:03:54 PDT
> 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.
Comment 8 Bill McCloskey (:billm) 2012-10-08 20:34:08 PDT
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.
Comment 9 Nicholas Nethercote [:njn] 2012-10-08 20:34:49 PDT
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".
Comment 10 Justin Lebar (not reading bugmail) 2012-10-08 20:39:30 PDT
> 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.
Comment 11 Bill McCloskey (:billm) 2012-10-08 21:18:48 PDT
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?
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-08 21:27:38 PDT
(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.
Comment 13 Bill McCloskey (:billm) 2012-10-08 21:37:00 PDT
> (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?
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-08 21:47:27 PDT
Yes, that's generally true.  However, the "bug" / "optimization" space is not black and white.  The non-blocking-basecamp+ requests will be triaged.
Comment 15 Andrew McCreight [:mccr8] 2012-10-09 05:10:52 PDT
(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.)
Comment 16 Robert Kaiser 2012-10-09 06:27:07 PDT
(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.
Comment 17 Justin Lebar (not reading bugmail) 2012-10-09 07:34:45 PDT
> 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.
Comment 18 Justin Lebar (not reading bugmail) 2012-10-09 07:35:01 PDT
(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.
Comment 19 Bill McCloskey (:billm) 2012-10-09 08:36:06 PDT
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.
Comment 20 Luke Wagner [:luke] 2012-10-09 08:44:29 PDT
(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.
Comment 21 Gregor Wagner [:gwagner] 2012-10-09 08:47:23 PDT
We could also try to share arenas between system compartments.
Comment 22 Bobby Holley (:bholley) (busy with Stylo) 2012-10-09 08:51:03 PDT
(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.
Comment 23 Gregor Wagner [:gwagner] 2012-10-09 09:16:01 PDT
(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.
Comment 24 Justin Lebar (not reading bugmail) 2012-10-16 07:14:16 PDT
Do we have a path forward here?
Comment 25 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-16 13:56:25 PDT
Is the plan to optimize the JS object kinds to reduce wasted heap?
Comment 26 Justin Lebar (not reading bugmail) 2012-10-16 14:02:15 PDT
(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.
Comment 27 Nicholas Nethercote [:njn] 2012-10-16 15:04:18 PDT
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 :(
Comment 28 Philipp von Weitershausen [:philikon] 2012-10-16 15:19:58 PDT
(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.)
Comment 29 Nicholas Nethercote [:njn] 2012-10-16 16:44:53 PDT
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.
Comment 30 Justin Lebar (not reading bugmail) 2012-10-16 17:32:33 PDT
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)
Comment 31 Justin Lebar (not reading bugmail) 2012-10-16 17:34:33 PDT
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)
Comment 32 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-16 17:48:48 PDT
> >         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.
Comment 33 [:fabrice] Fabrice Desré 2012-10-16 17:53:40 PDT
(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.
Comment 34 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-18 09:17:39 PDT
How hard would it to be to try a really aggressive solution here like what philikon proposes in comment 28?
Comment 35 Justin Lebar (not reading bugmail) 2012-10-18 11:16:02 PDT
(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.
Comment 36 Bill McCloskey (:billm) 2012-10-18 11:22:30 PDT
(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.
Comment 37 Justin Lebar (not reading bugmail) 2012-10-18 11:27:37 PDT
> 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?
Comment 38 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-18 11:38:43 PDT
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.
Comment 39 Bill McCloskey (:billm) 2012-10-18 11:42:32 PDT
(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.
Comment 40 Justin Lebar (not reading bugmail) 2012-10-22 13:16:56 PDT
FWIW, I care much more that this has an owner than I care how we fix this.
Comment 41 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-22 14:13:58 PDT
Bill, can you take this?
Comment 42 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-10-22 14:56:03 PDT
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 :)
Comment 43 Bill McCloskey (:billm) 2012-10-22 15:03:47 PDT
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.
Comment 44 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-22 15:07:19 PDT
That's probably less risky.  We'll just have to learn to live with the taste of vom in our mouths.
Comment 45 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-23 20:42:40 PDT
I have a mostly working thing here.  I'll put up patches tomorrow after I spend some time atoning for my sins.
Comment 46 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-23 23:04:15 PDT
What kind of savings are you seeing?  5MB+ washes out a lot of vom ...
Comment 47 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-25 08:58:33 PDT
Created attachment 675147 [details] [diff] [review]
Part 1: Add a pref to make the JS component loader load everything in the same global
Comment 48 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-25 08:59:14 PDT
Created attachment 675149 [details] [diff] [review]
Part 2: Fix the world
Comment 49 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-25 14:47:19 PDT
Created attachment 675308 [details]
Before these patches

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
Comment 50 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-25 14:49:56 PDT
Created attachment 675313 [details]
After

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!
Comment 51 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-25 14:52:22 PDT
Created attachment 675314 [details] [diff] [review]
Part 1, uncorrupted
Comment 52 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-25 14:52:54 PDT
Created attachment 675315 [details] [diff] [review]
Part 2, rebased on f35ce23ddfc8
Comment 53 Andreas Gal :gal 2012-10-25 14:56:57 PDT
Holy Christmas. Android will want this too.
Comment 54 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-25 14:57:30 PDT
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]
Comment 55 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-25 15:06:11 PDT
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.
Comment 56 Nicholas Nethercote [:njn] 2012-10-25 15:53:49 PDT
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".
Comment 57 Nicholas Nethercote [:njn] 2012-10-25 15:56:14 PDT
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?
Comment 58 Justin Lebar (not reading bugmail) 2012-10-25 16:04:41 PDT
> 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.
Comment 59 Nicholas Nethercote [:njn] 2012-10-25 16:12:10 PDT
> 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.
Comment 60 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-26 00:25:37 PDT
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.
Comment 61 Nicholas Nethercote [:njn] 2012-10-26 11:11:47 PDT
> 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?
Comment 62 Dave Hylands [:dhylands] 2012-10-26 11:52:33 PDT
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.
Comment 63 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-26 12:23:03 PDT
We'll be shipping a kernel update that artificially limits the unagis to 256MB.
Comment 64 Nicholas Nethercote [:njn] 2012-10-26 12:33:52 PDT
> 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.
Comment 65 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-26 12:35:03 PDT
Can we please have this conversation somewhere else?
Comment 66 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-26 16:44:44 PDT
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
Comment 67 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-26 16:45:23 PDT
(Sorry, wrong flag.)
Comment 68 Andreas Gal :gal 2012-10-26 16:56:26 PDT
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.
Comment 69 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-26 17:02:35 PDT
Created attachment 675756 [details] [diff] [review]
Part 1: Add a pref to the js loader that allows loading things in the same global.
Comment 70 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-26 17:03:27 PDT
Created attachment 675757 [details] [diff] [review]
Part 2: If the pref from the previous patch is set, don't assume we have our own global.
Comment 71 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-26 17:04:09 PDT
Created attachment 675759 [details] [diff] [review]
Part 3: Fix the world to put properties on the 'this' object.
Comment 72 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-26 17:05:00 PDT
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.
Comment 73 Alex Keybl [:akeybl] 2012-10-29 08:12:23 PDT
(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 74 Philipp von Weitershausen [:philikon] 2012-10-29 11:08:08 PDT
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+
Comment 75 Philipp von Weitershausen [:philikon] 2012-10-29 11:39:38 PDT
(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 76 Blake Kaplan (:mrbkap) 2012-10-29 14:47:15 PDT
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.
Comment 77 Nicholas Nethercote [:njn] 2012-10-29 14:53:41 PDT
> 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.
Comment 78 Andreas Gal :gal 2012-10-29 14:54:16 PDT
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?
Comment 79 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-29 15:59:58 PDT
Created attachment 676373 [details] [diff] [review]
Part 1: Add a pref to the js loader that allows loading things in the same global (comments addressed).

(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 80 Blake Kaplan (:mrbkap) 2012-10-29 16:24:18 PDT
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.
Comment 81 Ed Morley [:emorley] 2012-10-30 10:04:42 PDT
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
Comment 82 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-30 12:43:19 PDT
Doesn't look like this was at fault for the orange.  Relanded.

https://hg.mozilla.org/integration/mozilla-inbound/rev/67cb43bb8865
Comment 83 Vicamo Yang [:vicamo][:vyang] 2012-10-31 01:01:44 PDT
(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.
Comment 84 Vicamo Yang [:vicamo][:vyang] 2012-10-31 01:11:03 PDT
(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
Comment 85 Vicamo Yang [:vicamo][:vyang] 2012-10-31 01:36:37 PDT
Backout for B2G Marionette bustage:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bf3abe91210
Comment 86 Vicamo Yang [:vicamo][:vyang] 2012-10-31 02:14:40 PDT
Created attachment 676947 [details]
Marionette timesout

Here is the console log captured when Marionette hangs and never processes test cases until timeout.
Comment 87 Vicamo Yang [:vicamo][:vyang] 2012-10-31 02:54:24 PDT
Created attachment 676951 [details] [diff] [review]
Add an option to stick all chrome JSMs/JS components in the same compartment

Rebase to inbound tip, Marionette problem still not solved.
Comment 88 Vicamo Yang [:vicamo][:vyang] 2012-10-31 03:10:25 PDT
(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
Comment 89 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-31 03:39:01 PDT
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?
Comment 90 Mozilla RelEng Bot 2012-10-31 04:30:33 PDT
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
Comment 91 Vicamo Yang [:vicamo][:vyang] 2012-10-31 05:45:21 PDT
(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.
Comment 92 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-31 06:24:08 PDT
You should have flipped the pref rather than back out a 700 kb patch.  Relanding this is going to be a PITA.
Comment 93 Mozilla RelEng Bot 2012-10-31 09:00:43 PDT
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
Comment 94 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-31 09:23:16 PDT
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.
Comment 95 Philipp von Weitershausen [:philikon] 2012-10-31 14:20:12 PDT
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.
Comment 96 Nicholas Nethercote [:njn] 2012-10-31 14:46:55 PDT
> > 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.
Comment 97 Ed Morley [:emorley] 2012-11-01 06:55:16 PDT
https://hg.mozilla.org/mozilla-central/rev/5ce71981e005
Comment 98 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-11-09 10:11:22 PST
Created attachment 680123 [details] [diff] [review]
Consolidated patch for Aurora
Comment 99 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-11-09 10:14:50 PST
[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 100 bhavana bajaj [:bajaj] 2012-11-09 15:34:03 PST
Comment on attachment 680123 [details] [diff] [review]
Consolidated patch for Aurora

low risk patch , preffed on only for b2g, approving on aurora .
Comment 101 John Drinkwater (:beta) 2012-11-10 02:58:32 PST
(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?
Comment 102 Philipp von Weitershausen [:philikon] 2012-11-10 09:01:50 PST
(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.
Comment 103 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-11-11 12:59:25 PST
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.
Comment 104 Justin Lebar (not reading bugmail) 2012-11-11 14:04:07 PST
> 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?
Comment 105 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-11-11 14:07:57 PST
I don't know yet.  It looked marionette related, but the patches in bug 807478 supposedly landed on Aurora.
Comment 106 Justin Lebar (not reading bugmail) 2012-11-11 15:35:06 PST
(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?
Comment 107 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-11-11 15:44:27 PST
(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.
Comment 108 Bobby Holley (:bholley) (busy with Stylo) 2013-04-22 07:17:33 PDT
Can we undo this ugliness now that zones have landed?
Comment 109 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-04-22 07:23:23 PDT
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.
Comment 110 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2013-05-09 11:22:24 PDT
It would sure be interesting to measure this.
Comment 111 Bobby Holley (:bholley) (busy with Stylo) 2014-03-28 09:15:35 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.