sync: preliminary support for deferred SyncBackendHost initialization

chrome/browser/sync/profile_sync_service.cc

Index: chrome/browser/sync/profile_sync_service.cc
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index b649c14e32d39f0e7eb634bf6dd11cba7c4b9633..b5d4ef18a354f5f29db9bbf04365e82852a22551 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -297,11 +297,20 @@ void ProfileSyncService::TryStart() {
   // but we haven't completed sync setup, we try to start sync anyway, since
   // it's possible we crashed/shutdown after logging in but before the backend
   // finished initializing the last time.
-  if (!HasSyncSetupCompleted() && !setup_in_progress_ && !auto_start_enabled_)
-    return;
-
-  // All systems Go for launch.
-  StartUp();
+  //
+  // However, the only time we actually need to start sync _immediately_ is if
+  // the user is in the process of setting up and configuring (e.g, we need to
+  // fetch account details like encryption state to populate UI). Otherwise,
+  // for performance reasons / maximizing parallelism at chrome startup, we
+  // defer the heavy lifting for sync initialization until things have calmed
+  // down.  See DeferStartUp().
+  if (HasSyncSetupCompleted() || auto_start_enabled_)
+    StartUp(STARTUP_DEFER_BACKEND);
+
+  // We haven't completed sync setup. Start immediately if the user explicitly
+  // kicked this off.
+  if (setup_in_progress_)
+    StartUp(STARTUP_IMMEDIATE);

[Andrew T Wilson (Slow), 2013/02/18 10:08:15]

I have two concerns:

1) this is going to delay the initial sync on ChromeOS.
2) this relies on the UI layer calling SetSetupInProgress(true) even if they
just want to sync with the default settings. I don't remember any more if this
is true on all platforms - it may be. It *seems* to be true on desktop from my
scan of SyncSetupHandler and the OneClick* classes.

Another thing we could do is always do STARTUP_IMMEDIATE until we've had at
least one completed sync - that would address the first concern at least.

I'm slightly nervous about the fact that sync_initialized() will stay false.
This will have some impact on SigninTracker and SyncSetupHandler, and this code
will break in subtle ways if there are any paths through the code that don't
call SetSetupInProgress(), so if there's anything we can do to enforce that the
UI always calls this (like add a DCHECK to SetSyncSetupCompleted() if
SetupInProgress() != true) that'd be good.

But overall I think this is a good approach if we can make the dependency on
SetupInProgress() more explicit/enforced somehow.

[tim (not reviewing), 2013/02/21 03:50:54]

(1) is an excellent observation. I rejiggered the if condition to only defer if
sync setup has completed. That seems to make sense. (We don't *always* want to
startup_immediate otherwise, just as the original code didn't always call
StartUp, but yeah.)

Re (2), my goal was to maintain the same constraints as today, and I think the
code as written now does this.  Before, if setup had not completed, and setup in
progress was not set (and we're not on an auto start platform), then we would
*not* call StartUp.  The same is true with the new code, unless you're on an
auto start platfrom.  Right?

 }
 
 void ProfileSyncService::StartSyncingWithServer() {
@@ -467,8 +476,7 @@ void ProfileSyncService::OnSyncConfigureRetry() {
   NotifyObservers();
 }
 
-
-void ProfileSyncService::StartUp() {
+void ProfileSyncService::StartUp(StartUpDeferred deferred) {
   // Don't start up multiple times.
   if (backend_.get()) {
     DVLOG(1) << "Skipping bringing up backend host.";
@@ -487,6 +495,33 @@ void ProfileSyncService::StartUp() {
         sync_prefs_.GetSpareBootstrapToken());
   }
 #endif
+
+  if (!sync_global_error_.get()) {

[Nicolas Zea, 2013/02/19 19:01:06]

why is this moved up here?

[tim (not reviewing), 2013/02/21 03:50:54]

The only thing I'm trying to defer is backend initialization, and in general I
want to spin up the UI related bits.  I thought it was a more logical ordering
of operations this way than having the possibly deferred call in between.  I
don't think this can have any bad side effects, especially since from a
serialized operations on the UI thread point of view I have not changed any
ordering (the backend would have to post stuff to get picked up by the global
error tracking, so this code here still "happens before" that would).

+#if !defined(OS_ANDROID)
+    sync_global_error_.reset(new SyncGlobalError(this, signin()));
+#endif
+    GlobalErrorServiceFactory::GetForProfile(profile_)->AddGlobalError(
+        sync_global_error_.get());
+    AddObserver(sync_global_error_.get());
+  }
+
+  if (deferred == STARTUP_DEFER_BACKEND &&
+      CommandLine::ForCurrentProcess()->
+          HasSwitch(switches::kSyncEnableDeferredStartup)) {
+    return;

[Nicolas Zea, 2013/02/19 19:01:06]

I'd like to see a timestamp set here, so we can record the average time spent
deferring startup in a histogram

[tim (not reviewing), 2013/02/21 03:50:54]

Good idea.  I didn't really want to add more state to PSS tracking the deferred
state though, I'm trying to limit that to the closure that will trigger
StartUpSlowBackendComponents.  So I just added a parameter to tell whether we
were originally requested to start up deferred, and if so, use start_up_time_ as
the timestamp. This sound OK?

+  }
+
+  StartUpSlowBackendComponents();
+}
+
+void ProfileSyncService::StartUpSlowBackendComponents() {
+  // Don't start up multiple times.
+  if (backend_.get()) {
+    DVLOG(1) << "Skipping bringing up backend host.";
+    return;
+  }
+
+  DCHECK(IsSyncEnabledAndLoggedIn());
   CreateBackend();
 
   // Initialize the backend.  Every time we start up a new SyncBackendHost,
@@ -503,15 +538,6 @@ void ProfileSyncService::StartUp() {
     backend_->UpdateRegisteredInvalidationIds(
         invalidator_registrar_->GetAllRegisteredIds());
   }
-
-  if (!sync_global_error_.get()) {
-#if !defined(OS_ANDROID)
-    sync_global_error_.reset(new SyncGlobalError(this, signin()));
-#endif
-    GlobalErrorServiceFactory::GetForProfile(profile_)->AddGlobalError(
-        sync_global_error_.get());
-    AddObserver(sync_global_error_.get());
-  }
 }
 
 void ProfileSyncService::RegisterInvalidationHandler(
@@ -1240,6 +1266,15 @@ std::string ProfileSyncService::QuerySyncStatusSummary() {
   }
 }
 
+std::string ProfileSyncService::GetBackendInitializationStateString() const {
+  if (sync_initialized())
+    return "Done";
+  else if (!start_up_time_.is_null())
+    return "Deferred";
+  else
+    return "Not started";
+}
+
 bool ProfileSyncService::QueryDetailedSyncStatus(
     SyncBackendHost::Status* result) {
   if (backend_.get() && backend_initialized_) {