Last Comment Bug 836120 - Reduce memory overhead of Sync when it isn't configured
: Reduce memory overhead of Sync when it isn't configured
Status: RESOLVED FIXED
[MemShrink]
:
Product: Cloud Services
Classification: Client Software
Component: Firefox Sync: Backend (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla21
Assigned To: Gregory Szorc [:gps]
:
:
Mentors:
: 825728 (view as bug list)
Depends on:
Blocks: 838717
  Show dependency treegraph
 
Reported: 2013-01-29 17:05 PST by Gregory Szorc [:gps]
Modified: 2013-02-06 09:57 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Don't load sync JS modules unless Sync is configured, v1 (11.33 KB, patch)
2013-01-29 19:25 PST, Gregory Szorc [:gps]
rnewman: review+
Details | Diff | Splinter Review

Description Gregory Szorc [:gps] 2013-01-29 17:05:08 PST
I've been spending a lot of time in about:memory lately and one of the things I noticed was that a lot of JSMs are being loaded by Sync's XPCOM service. However, they really don't do much when Sync isn't configured and thus are just wasting memory.

We should minimize the memory overhead of Sync for profiles not having Sync configured.
Comment 1 Gregory Szorc [:gps] 2013-01-29 19:25:56 PST
Created attachment 707965 [details] [diff] [review]
Don't load sync JS modules unless Sync is configured, v1

This eliminates ~530kb of explicit memory when Sync is not configured by not loading some Sync JS modules. If you count bug 836086, we just freed a total of ~628kb when Sync is not in use!

There is a risk to taking this patch as-is: we no longer load main.js after 10s. This means that the global Weave object will no longer be available unless Sync is configured. Consumers of Weave will need to trigger loading of the Sync service. I documented this in Weave.js and made changes in code where I believe all the initializations of Sync occur.

Now, consumers of Weave should have been doing this already because if they weren't, there was the 10s window between starting the app and accessing Weave where it would have been unavailable. Bug 825728 was one such bug and it is fixed in this patch. Yay!

Not loading main.js (which also imports constants.js) saves ~180kb on my machine. That's a significant portion of the overall savings. And these files are not required unless Sync is running. And, all consumers of Weave should have logic handling the case where it isn't defined anyway. So, I say we go all-in and do this properly - as implemented.

I tested this patch on desktop Firefox and all looks sane. I did not verify the change to /mobile/xul because I do not know how. I looked at TPS and it should work as-is (since it loads main.js explicitly and triggers a Weave.Service access in its init code).

We should still have QA perform a regression of UI components after this patch is applied (just in case).
Comment 2 Richard Newman [:rnewman] 2013-01-29 23:06:45 PST
Comment on attachment 707965 [details] [diff] [review]
Don't load sync JS modules unless Sync is configured, v1

Review of attachment 707965 [details] [diff] [review]:
-----------------------------------------------------------------

I like it. Let's make sure TPS passes… I can imagine there are TPS dependencies here.

::: browser/components/preferences/in-content/sync.js
@@ +43,5 @@
> +      return;
> +    }
> +
> +    let onReady = function () {
> +      Services.obs.removeObserver(onReady, "weave:service:ready");

Also remove the window onUnload handler. No sense keeping that around for the lifetime of the browser session, no?

::: browser/components/preferences/sync.js
@@ +44,5 @@
> +      return;
> +    }
> +
> +    let onReady = function () {
> +      Services.obs.removeObserver(onReady, "weave:service:ready");

Same.

::: services/sync/Weave.js
@@ +21,5 @@
> + * It's worth noting how Sync is lazily loaded. We register a timer that
> + * loads Sync a few seconds after app startup. This is so Sync does not
> + * adversely affect application start time.
> + *
> + * If Sync is not configured, no extra Sync code is loading. If an

s/loading/loaded.
Comment 3 Gregory Szorc [:gps] 2013-01-30 07:03:01 PST
*** Bug 825728 has been marked as a duplicate of this bug. ***
Comment 5 Gregory Szorc [:gps] 2013-01-30 16:15:24 PST
https://hg.mozilla.org/mozilla-central/rev/bb8895c7f091
Comment 6 neil@parkwaycc.co.uk 2013-02-06 09:33:52 PST
Comment on attachment 707965 [details] [diff] [review]
Don't load sync JS modules unless Sync is configured, v1

>-          Cu.import("resource://services-sync/main.js");
>-          if (Weave.Status.checkSetup() != Weave.CLIENT_NOT_CONFIGURED)
>-            Weave.Service;
>+          // We only load more if it looks like Sync is configured.
>+          let prefs = Services.prefs.getBranch(SYNC_PREFS_BRANCH);
>+
>+          if (!prefs.prefHasUserValue("username")) {
>+            return;
>+          }
>+
>+          // We have a username. So, do a more thorough check. This will
>+          // import a number of modules and thus increase memory
>+          // accordingly. We could potentially copy code performed by
>+          // this check into this file if our above code is yielding too
>+          // many false positives.
>+          if (Weave.Status.checkSetup() != Weave.CLIENT_NOT_CONFIGURED) {
>+            this.ensureLoaded();
Unfortunately Weave doesn't exist until this.ensureLoaded() loads it...

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