sync: preliminary support for deferred SyncBackendHost initialization

sync: preliminary support for deferred SyncBackendHost initialization
BUG=80194
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186521

[tim (not reviewing), 2013-02-15 21:44:26]

No functional change here unless you pass the --sync-enable-deferred-startup,
but sending for review to make sure everyone's cool with the direction.

I'll update the bug, but my plan from here is to trigger
StartUpSlowBackendComponents when a data type has reported a user-initiated
change, with a fallback to timer.  SyncableService implementations today all
have their own variant of 'if (!sync_processor_) return;' when a change occurs,
and my plan is to have them send a flare over to the UI thread that sync should
start ASAP. I think we can add an optional "best effort start sync soon" closure
that DTs could use if they wanted without incurring too much complexity. 

I think this deals with the concerns raised in the bug about waiting for
browser-window-opened, and it also jitters things better than strictly using a
timer.

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

https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:313: StartUp(STARTUP_IMMEDIATE);
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.

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

Mostly LG

https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:499: if (!sync_global_error_.get())
{
why is this moved up here?

https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:511: return;
I'd like to see a timestamp set here, so we can record the average time spent
deferring startup in a histogram

https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.h:384: std::string
GetBackendInitializationStateString() const;
Newline above this and add comment about what this describes.

https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.h:720: enum StartUpDeferred {
nit: StartUpPolicy seems like a better name for this

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

https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:313: StartUp(STARTUP_IMMEDIATE);
On 2013/02/18 10:08:15, Andrew T Wilson wrote:
> I have two concerns:
> 
> 1) this is going to delay the initial sync on ChromeOS.

(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?


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

https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:499: if (!sync_global_error_.get())
{
On 2013/02/19 19:01:06, Nicolas Zea wrote:
> why is this moved up here?

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

https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:511: return;
On 2013/02/19 19:01:06, Nicolas Zea wrote:
> I'd like to see a timestamp set here, so we can record the average time spent
> deferring startup in a histogram

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?

https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.h:384: std::string
GetBackendInitializationStateString() const;
On 2013/02/19 19:01:06, Nicolas Zea wrote:
> Newline above this and add comment about what this describes.

Done.

https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.h:720: enum StartUpDeferred {
On 2013/02/19 19:01:06, Nicolas Zea wrote:
> nit: StartUpPolicy seems like a better name for this

Went with StartUpDeferredOption, because 'start up policy' sounds pretty
ambiguous (i.e I could see us adding a param to StartUp down the road and
'policy' making sense for that too).  Let me know if this is OK with you.

[tim (not reviewing), 2013-02-25 21:03:18]

On 2013/02/21 03:50:54, timsteele wrote:
>
https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
> File chrome/browser/sync/profile_sync_service.cc (right):
> 
>
https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
> chrome/browser/sync/profile_sync_service.cc:313: StartUp(STARTUP_IMMEDIATE);
> On 2013/02/18 10:08:15, Andrew T Wilson wrote:
> > I have two concerns:
> > 
> > 1) this is going to delay the initial sync on ChromeOS.
> 
> (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?
> 
> 
> > 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.
> 
>
https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
> chrome/browser/sync/profile_sync_service.cc:499: if
(!sync_global_error_.get())
> {
> On 2013/02/19 19:01:06, Nicolas Zea wrote:
> > why is this moved up here?
> 
> 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).
> 
>
https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
> chrome/browser/sync/profile_sync_service.cc:511: return;
> On 2013/02/19 19:01:06, Nicolas Zea wrote:
> > I'd like to see a timestamp set here, so we can record the average time
spent
> > deferring startup in a histogram
> 
> 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?
> 
>
https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
> File chrome/browser/sync/profile_sync_service.h (right):
> 
>
https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
> chrome/browser/sync/profile_sync_service.h:384: std::string
> GetBackendInitializationStateString() const;
> On 2013/02/19 19:01:06, Nicolas Zea wrote:
> > Newline above this and add comment about what this describes.
> 
> Done.
> 
>
https://codereview.chromium.org/12288010/diff/1/chrome/browser/sync/profile_s...
> chrome/browser/sync/profile_sync_service.h:720: enum StartUpDeferred {
> On 2013/02/19 19:01:06, Nicolas Zea wrote:
> > nit: StartUpPolicy seems like a better name for this
> 
> Went with StartUpDeferredOption, because 'start up policy' sounds pretty
> ambiguous (i.e I could see us adding a param to StartUp down the road and
> 'policy' making sense for that too).  Let me know if this is OK with you.

ping

[Nicolas Zea, 2013-02-25 21:12:04]

LGTM

https://codereview.chromium.org/12288010/diff/7001/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/12288010/diff/7001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:490: start_up_time_ =
base::Time::Now();
I'm a bit concerned about the possibility of TryStart being called multiple
times, and therefore start_up_time_ being set more than once, messing up the
histogram. Perhaps condition this on start_up_time_.is_null(). Feel free to
leave it if you're confident this can't be hit more than once.

[tim (not reviewing), 2013-03-06 19:30:30]

On 2013/02/25 21:12:04, Nicolas Zea wrote:
> LGTM
> 
>
https://codereview.chromium.org/12288010/diff/7001/chrome/browser/sync/profil...
> File chrome/browser/sync/profile_sync_service.cc (right):
> 
>
https://codereview.chromium.org/12288010/diff/7001/chrome/browser/sync/profil...
> chrome/browser/sync/profile_sync_service.cc:490: start_up_time_ =
> base::Time::Now();
> I'm a bit concerned about the possibility of TryStart being called multiple
> times, and therefore start_up_time_ being set more than once, messing up the
> histogram. Perhaps condition this on start_up_time_.is_null(). Feel free to
> leave it if you're confident this can't be hit more than once.

We should not call StartUp more than once.  I looked up where we added the code
in SyncBackendHost that checks if the start_up_time_ is null, and as best I can
tell it was not necessary at the time it was added either.  I added a few
DCHECKs.

[commit-bot: I haz the power, 2013-03-06 19:31:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/12288010/11001

[commit-bot: I haz the power, 2013-03-06 22:32:51]

Change committed as 186521