836120 - Reduce memory overhead of Sync when it isn't configured

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

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]]

Attachment: 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]]

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 4

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/bb8895c7f091

Comment 5

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/bb8895c7f091

Comment 6

[[e-mail]]

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...