Looks like sync is Cu.import-ing some modules that may not be immediately needed (or not needed at all). For example PlacesBackups.jsm here http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/bookmarks.js#21 Would be good to check whether other Cu.import(s) in the sync code should really be defineLazyGetterModule

Very likely! This code pre-dates compartments, for one thing :) Very happy to review fixes.

Hi i'd like to work on this bug. How do i begin??

Anirudh: get started by building Firefox for your platform. https://developer.mozilla.org/docs/Mozilla/Developer_guide/Build_Instructions#Getting_started When that's done, start by browsing through the files in /services/sync. You'll see occurrences of Cu.import for modules that aren't necessarily used. This bug is about figuring those out, and switching them to lazyModuleGetters.

Firefox built finished on my platform can you tell me what elese i need to do in order to fix the bug

Ivan: See comment 3.

how do i make a pathc about this bug i mean when i find these files for example what do i do next

Ivan: check out the developer docs here. https://developer.mozilla.org/docs/Mozilla/Developer_guide You'll need to get a checkout of mozilla-central, build, run the Sync xpcshell tests (at a minimum), and use either Mercurial Queues or bookmarks and MozReview to submit a patch for review. Those steps are all thoroughly covered in the guide above. Let us know if you can't figure something out from the docs!

Are these test like the one you do after you make any changes or these are different ?

Just the usual test suites, plus one addition. The point of this bug is to remove things that we suspect aren't needed, and to make other things 'lazy' -- i.e., loaded on demand, not when Sync itself is configured. To verify that change we need to make sure that Sync (and the rest of the browser) still works. That means running the test suites, particularly xpcshell. We should also check that any changes are positive! They should be, but it would be nice to know what gains we achieved. The simplest way to do that is to use about:memory, as described here: https://developer.mozilla.org/en-US/docs/Zombie_compartments#Viewing_live_compartments Start a local build without your change with no pages loaded but with Sync configured. Wait fifteen seconds or so, then load about:memory and see which system compartments are loaded and how much memory Firefox thinks it's using. Then do the same thing with your changes. There should be fewer compartments and less memory usage.

I dont understand i run xpcshell tests but what changes you want me to do

(In reply to Ivan from comment #10) > I dont understand i run xpcshell tests but what changes you want me to do You should read Comment 3.

Comment on attachment 8697735 [details] [diff] [review] I dont know if i did this right please help me There's a lot of stuff in this patch that shouldn't be there, which implies that you did something like `hg diff` to get a patch file. You should go read the developer guide about producing patches for review. But digging in it's possible to see what you're trying to do: --- -Cu.import("resource://gre/modules/XPCOMUtils.jsm"); -Cu.import("resource://gre/modules/Promise.jsm"); -Cu.import("resource://services-sync/constants.js"); -Cu.import("resource://gre/modules/Log.jsm"); -Cu.import("resource://services-sync/util.js"); -Cu.import("resource://services-common/async.js"); +defineLazyModuleGetter(this, "XPCOMUtils.jsm""resource://gre/modules/XPCOMUtils.jsm"); +defineLazyModuleGetter(this, "Promise.jsm""resource://gre/modules/Promise.jsm"); +defineLazyModuleGetter(this, "constants.js""resource://services-sync/constants.js"); +defineLazyModuleGetter(this, "Log.jsm""resource://gre/modules/Log.jsm"); +defineLazyModuleGetter(this, "util.js""resource://services-sync/util.js"); +defineLazyModuleGetter(this, "async.js""resource://services-common/async.js"); --- This is in the right direction, but there are three things to consider. Firstly, lazy module getters work like this: XPCOMUtils.defineLazyModuleGetter(this, "Log", "resource://gre/modules/Log.jsm"); that is, you're defining a thing named "Log" that will transparently replace itself, when first accessed, with the "Log" that you get from importing Log.jsm. Note that it's "Log", not "Log.jsm", because that's the name of the symbol that Log.jsm exports -- you need to actually _look_ at what symbols are exported: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Log.jsm#7 and correlate those with the ones that are used within the importing file. Secondly, we don't want to replace *every* Cu.import with a lazy module getter -- they aren't free. You need to look at each import, and decide one of three things: 1. It's not used at all; someone forgot to remove it, or someone cargo-culted it. Delete the import. 2. It's used immediately when you import and use this file. Leave it. 3. Callers are likely to import this file but not use chunks of it until much later, if at all, and those chunks are the only part that use the symbols imported. Turn this into a lazy module getter. Thirdly, you have to be really careful. Take a look at these lines from your patch: +defineLazyModuleGetter(this, "dentity.js""resource://services-sync/identity.js"); +defineLazyModuleGetter(this, "utils.js""resource://services-common/utils.js"); +defineLazyModuleGetter(this, "utils.js""resource://services-crypto/utils.js"); One typo and two name collisions. This code won't work at all, and would have failed any tests you ran. Bearing those things in mind, give it another shot!

Is this issue still open? Can I take and work on it?

(In reply to arumugaabinesh from comment #14) > Is this issue still open? > Can I take and work on it? Yes - the information already in this bug should be able to get your started.

Ignoring the testing dirs there appear to 219 |Cu.import| calls. Marco (or Richard), do you have any guidance on which ones are worth lazifying?

These can all be easily converted, afaict: PlacesUtils, PlacesBackups, PlacesSyncUtils, Task, NetUtil, FileUtils, Promise, AddonManager, Preferences, osfile, LightweightThemeManager, TelemetryController, Log I don't know well the Sync specific modules and thus I'm not commenting about those. Whether we should do it or not, actually depends on whether these modules are imported and not used in the first 30s after startup, that is a bit hard to guess without instrumentation. But the conversion is "cheap" and the added cost is trivial, so probably worth to do regardless.

(In reply to Marco Bonardo [::mak] from comment #17) > These can all be easily converted, afaict: > PlacesUtils, PlacesBackups, PlacesSyncUtils, Task, NetUtil, FileUtils, > Promise, AddonManager, Preferences, osfile, LightweightThemeManager, > TelemetryController, Log Off the top of my head, of these, the following probably are worth doing: FileUtils, PlacesUtils, PlacesBackups, PlacesSyncUtils, LightweightThemeManager, TelemetryController Most of the rest are used immediately so there's probably no benefit in doing them.

Hi! I created a patch for this bug, I want to have your thoughts on a couple things. First, the attachment above is the about:memory diff of builds with and without the changes in the patch. What I did was the following: 1- Remove unused imports (24 in total). 2- Replace the imports mentioned in a comment above with lazy loaders (9 in total). 3- The file sync-common/util.js in https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/util.js#5 has as exported symbols Async and Services for example. There were many files that imported sync-common/util.js and services-common/async.js The import of sync-common/util.js exports Async from services-common/async.js making the first services-common/async.js redundant (I hope I was clear there :) ) I ran locally the xpcshell tests of services/sync and the tests of services/sync both with 0 fails. That would be all for the description of what I did. Now one thing I wasn't sure how to approach. In the file sync/modules/service.js https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/service.js The imports of resource.js and telemetry.js are used but not through the exported symbols, is this something that should be changed? In a different bug maybe? Thanks in advance!

Comment on attachment 8856180 [details] Bug 986501 - Avoid unnecessary compartments on Sync startup Over to you, Mark.

Comment on attachment 8856180 [details] Bug 986501 - Avoid unnecessary compartments on Sync startup https://reviewboard.mozilla.org/r/128106/#review130828 I'm sorry but I find the patch unrevieawable due to the too many unneeded changes, and some of the modules appear to be removed even if they are used. ::: services/sync/Weave.js (Diff revision 1) > const Cc = Components.classes; > const Ci = Components.interfaces; > const Cu = Components.utils; > > -Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > -Cu.import("resource://gre/modules/Services.jsm"); Why removing these? They are used in the component ::: services/sync/modules/SyncedTabs.jsm:17 (Diff revision 1) > Cu.import("resource://gre/modules/Services.jsm"); > -Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > Cu.import("resource://gre/modules/Task.jsm"); > -Cu.import("resource://gre/modules/Log.jsm"); > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > -Cu.import("resource://gre/modules/PlacesUtils.jsm", this); > Cu.import("resource://services-sync/main.js"); Preserving blame and making easier to review patches is likely more important than alphabetically ordering imports... ::: services/sync/modules/addonutils.js:15 (Diff revision 1) > -Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > Cu.import("resource://gre/modules/Log.jsm"); > Cu.import("resource://services-sync/util.js"); > > +// Lazy imports to prevent unnecessary load on startup. > XPCOMUtils.defineLazyModuleGetter(this, "AddonManager", You removed the XPCOMUtils import? ::: services/sync/modules/bookmark_repair.js:19 (Diff revision 1) > Cu.import("resource://services-sync/constants.js"); > -Cu.import("resource://services-sync/resource.js"); > Cu.import("resource://services-sync/doctor.js"); > +Cu.import("resource://services-sync/resource.js"); > Cu.import("resource://services-sync/telemetry.js"); > -Cu.import("resource://services-common/utils.js"); > +Cu.import("resource://services-sync/util.js"); you removed Services.jsm but it's used later...

to be clear, I stopped reviewing after the fourth file or so, there may be more issues later, but it's really hard to review it since all the imports reordering.

(In reply to Santiago Paez [:tiago] from comment #20) > Created attachment 8856182 [details] > Diff of about:memory before and after changes > > Hi! > > I created a patch for this bug, I want to have your thoughts on a couple > things. Thanks for the patch. I agree with all of mak's comments, so can you please upload a new patch with the absolute minimum needed to solve the bug and with zero "style" changes - that will make it much easier for us to understand exactly what's going on here. > First, the attachment above is the about:memory diff of builds with and > without the changes in the patch. There are changes there which look suspicious - eg, 12 less observers doesn't seem correct. What might be happening is, eg, Sync doesn't actually work with your patch (which seems likely given :mak's comments), which could account for some of these things. IOW, comparing about:memory from 2 builds must be comparing apples with apples - eg, after a successful no-op sync. > 3- The file sync-common/util.js in > https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/util. > js#5 > > has as exported symbols Async and Services for example. There were many > files that imported sync-common/util.js and services-common/async.js > > The import of sync-common/util.js exports Async from > services-common/async.js making the first services-common/async.js redundant > (I hope I was clear there :) ) I think you mean services-sync/util.js? You are probably correct that files which import both those modules probably don't need to import async.js. I hate our module system :( > I ran locally the xpcshell tests of services/sync and the tests of > services/sync both with 0 fails. Note that Sync will need to be manually tested too (ie, with an new actual sync account and verifying it actually works) > In the file sync/modules/service.js > > https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/service. > js > > The imports of resource.js and telemetry.js are used but not through the > exported symbols, is this something that should be changed? In a different > bug maybe? It might well be true that the import of resource.js can be removed. I *think* the import of telemetry.js is probably needed to initialize that module, so will not be able to be made lazy. > Thanks in advance! Thanks for the patch and I look forward to a new version!

Hi Marco and Mark, Thanks for the feedback! I'm sorry about organizing the imports alphabetically, it didn't occur to me that it could make the reviewing process more difficult (I'll keep it in mind for the future). As Marco points out, I removed a lot of XPCOMUtils.jsm, Services.jsm and async.js imports. The reason for that, is that the file service-sync/util.js has this as EXPORTED_SYMBOLS: this.EXPORTED_SYMBOLS = ["XPCOMUtils", "Services", "Utils", "Async", "Svc", "Str"]; Then, when a component imports service-sync/util.js, it already has access to XPCOMUtils, Services and Async. Personally I don't like that service-sync/util.js exports those symbols, and I rather that XPCOMUtils is imported where its needed directly (and not through service-sync/util.js). Mark also pointed out something important, and that is manual testing. I didn't do it before sending the patch, but I did it now. Using a local build (running with the changes in the patch) I was able to sync bookmarks from my desktop to my iPhone, and send tabs from my iPhone to my desktop. This is not by all means a thorough test, but at least I can see that it "works". I couldn't send bookmarks from my iPhone to my desktop, or tabs from my desktop to my iPhone. But I wasn't able to do it even using the "normal-stable" firefox desktop. I'm happy to create a new patch, for sure I'll be removing completely unused imports and adding the lazy imports. As for this conflicting imports I'll wait for your input as for how to proceed. Thanks!

(In reply to Santiago Paez [:tiago] from comment #25) > As Marco points out, I removed a lot of XPCOMUtils.jsm, Services.jsm and > async.js imports. The reason for that, is that the file service-sync/util.js > has this as EXPORTED_SYMBOLS: > > this.EXPORTED_SYMBOLS = ["XPCOMUtils", "Services", "Utils", "Async", "Svc", > "Str"]; This is horrible indeed, mostly because it's hiding code behind curtains. I'm not a peer of Sync, and thus I should not make a decision here, I'm leaving that to Mark. Personally I'd not remove XPCOMUtils and Services imports even if they are exported already. It shouldn't make a difference perf-wise, those 2 specific modules are likely already imported by every single script in Firefox.

Thanks for the explanation. I agree with Mak. For this bug, let's do the min required (which means leaving this duplicate namespace mess around) and use another bug to clean that up.

Comment on attachment 8856180 [details] Bug 986501 - Avoid unnecessary compartments on Sync startup https://reviewboard.mozilla.org/r/128106/#review132722 Removed unused imports and replaced some of them with lazy module loaders.

Comment on attachment 8856180 [details] Bug 986501 - Avoid unnecessary compartments on Sync startup https://reviewboard.mozilla.org/r/128106/#review132722 Made the changes according to what we talked in the bugzilla thread.

Comment on attachment 8856180 [details] Bug 986501 - Avoid unnecessary compartments on Sync startup https://reviewboard.mozilla.org/r/128106/#review133172 I got confused trying to publish this patch, It's asking me for a comment, so... sorry and here it is.

Comment on attachment 8856180 [details] Bug 986501 - Avoid unnecessary compartments on Sync startup https://reviewboard.mozilla.org/r/128106/#review133512

Comment on attachment 8856180 [details] Bug 986501 - Avoid unnecessary compartments on Sync startup Setting the review request flag for mak.

Comment on attachment 8856180 [details] Bug 986501 - Avoid unnecessary compartments on Sync startup Thanks for the patch! If you add "r?mak" to the commit message, MozReview will automatically set the right flags when you push. Easier than manually resetting every time. :-)

Comment on attachment 8856180 [details] Bug 986501 - Avoid unnecessary compartments on Sync startup https://reviewboard.mozilla.org/r/128106/#review133578 This looks great to me and seems thorough, thanks. I've pushed a try run and I'll give it r+ if it looks green.

Comment on attachment 8856180 [details] Bug 986501 - Avoid unnecessary compartments on Sync startup https://reviewboard.mozilla.org/r/128106/#review133608 Try looks green - thanks!

Pushed by mhammond@skippinet.com.au: https://hg.mozilla.org/integration/autoland/rev/30e21f7f07a1 Avoid unnecessary compartments on Sync startup r=markh

(In reply to Mark Hammond [:markh] from comment #36) > Comment on attachment 8856180 [details] > Bug 986501 - Avoid unnecessary compartments on Sync startup > > https://reviewboard.mozilla.org/r/128106/#review133608 > > Try looks green - thanks! Thanks for reviewing the patch and sorry for the review-request-mess. It seem that I still don't have the workflow 100% down :-)