sync: Refactor job ownership in SyncScheduler

sync: Refactor job ownership in SyncScheduler

This change separates the tracking of what work needs to be done from
the decision of when to do it.  Prior to this change, SyncSessionJobs
were owned either by Closures posted to the sync thread's message loop,
or held temporarily in unscheduled_nudge_storage_, a member of the
SyncScheduler.  Following this change, there can be only two jobs in
existence, and they will be referenced only by the scoped_ptr members of
SyncScheduler named pending_nudge_job_ and pending_configure_job_.

This change, along with some previous changes to the way we schedule
tasks, makes it possible to simplify the job "saving" logic.  Jobs with
purpose == NUDGE are saved by assigning them to pending_nudge_job_ or
coalescing them with the existing pending_nudge_job_.  Jobs with purpose
== CONFIGURE are never coalesced, and can be saved in the
pending_configure_job_ member.  These changes allow us to make
SyncSessionJob::Clone() obsolete.

The logic in ScheduleNudgeImpl() has been updated to take advantage of
the fact that the pending job is much easier to find now.  It should now
be much better at coalescing its sources.  In other words, there will be
less scenarios where it can drop notification hints.  However, there are
still some cases in DecideOnJob() that may induce it to drop hints
unnecessarily.

The scheduling logic has been modified, too.  We've removed support for
the nudge while in an exponential backoff interval.  This makes it
possible to track the next wakeup time using a single timer, since the
wakeup event will be one of:
- The end of a throttled interval
- An end-of-backoff-interval retry
- A scheduled nudge
and these scenarios are now mutually exclusive.

BUG=175024
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192666

[rlarocque, 2013-04-02 23:01:12]

Please review.

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:608: ResumeWaiting();
Here's a paragraph that I had originally intended to write in the commit
message:

> Because canary jobs are invoked synchronously (since r191458), there are
> now almost no cases where we can interrupt a WaitInterval.   This makes
> the logic to resume an interrupted WaitInterval obsolete.  As far as I
> know, the one case where we do interrupt a WaitInterval is the single
> nudge while in backoff, and we should never decide to SAVE such a job in
> DoSyncSessionJob.  Therefore, we'll never need to resume a WaitInterval
> that gets interrupted by this nudge while in backoff.

It turns out I was wrong.  We can decide to SAVE a nudge-while-in-backoff.  That
happens when we switch into configure mode between ScheduleNudgeImpl() and
DoSyncSessionJob().  If we received a CONFIGURE in the interim, and we decided
to SAVE it (because we are in an EXPONENTIAL_BACKOFF), we might be tempted to
assume that it's save to just set the pending_configure_job_ and wait for the
canary at the end of the wait interval.  That would be wrong, because the
nudge-while-in-backoff has preempted the job at the end of the WaitInterval. 
The result is that neither the nudge nor the configure will be run.

ResumeWaiting() solves this problem by making sure that the current pending job
is the canary at the end of the wait interval.

I have some ideas of safer/better ways to fix this problem.  My preference would
be to remove the nudge-while-in-backoff altogether, but there are other options.
 We should talk in person about this.

[rlarocque, 2013-04-02 23:02:44]

I forgot to specify a reviewer in my first message.  Let's try this again.

[tim (not reviewing), 2013-04-04 00:22:11]

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:653: bool
SyncSchedulerImpl::DoNudgeSyncSessionJob(JobPriority priority) {
Digging around, it looks like these Do*SSJ methods exist solely because you
don't want to hand ownership over to the posted task (which makes sense). Is
that right?  

Couldn't we have a single function taking a Purpose be responsible for Pass()ing
the right job to DoSyncSessionJobImpl? Or DoSyncSessionJobImpl itself? The code
as written made me think there were significant differences between the two and
the fact that it was all about ownership wasn't very clear.

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:907: pending_wakeup_.Cancel();
Can we rename pending_wakeup_ to pending_wakeup_event_? I was confused for a
moment that we owned the job twice, once with pending_[nudge,configure]_job_ and
once with pending_wakeup_.  'event' would make it clearer that it is not a job.

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.h (right):

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.h:311: scoped_ptr<SyncSessionJob>
pending_configure_job_;
Why does this belong here? Unless I'm mistaken this introduces a state that
should not logically exist. Can't we avoid that entropy? We can only have a
pending config job if it is part of the WaitInterval, and it'd be clearer if the
ownership model reflected it in some way.  Are you planning to address that
somehow?

[rlarocque, 2013-04-04 00:59:48]

PTAL.

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:653: bool
SyncSchedulerImpl::DoNudgeSyncSessionJob(JobPriority priority) {
On 2013/04/04 00:22:11, timsteele wrote:
> Digging around, it looks like these Do*SSJ methods exist solely because you
> don't want to hand ownership over to the posted task (which makes sense). Is
> that right?  
> 
> Couldn't we have a single function taking a Purpose be responsible for
Pass()ing
> the right job to DoSyncSessionJobImpl? Or DoSyncSessionJobImpl itself? The
code
> as written made me think there were significant differences between the two
and
> the fact that it was all about ownership wasn't very clear.

Your suggestion would work for now, but it wouldn't work once we remove
SyncSessionJob.  These functions will diverge significantly in the near future.

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:907: pending_wakeup_.Cancel();
On 2013/04/04 00:22:11, timsteele wrote:
> Can we rename pending_wakeup_ to pending_wakeup_event_? I was confused for a
> moment that we owned the job twice, once with pending_[nudge,configure]_job_
and
> once with pending_wakeup_.  'event' would make it clearer that it is not a
job.

Done.

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.h (right):

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.h:311: scoped_ptr<SyncSessionJob>
pending_configure_job_;
On 2013/04/04 00:22:11, timsteele wrote:
> Why does this belong here? Unless I'm mistaken this introduces a state that
> should not logically exist. Can't we avoid that entropy? We can only have a
> pending config job if it is part of the WaitInterval, and it'd be clearer if
the
> ownership model reflected it in some way.  Are you planning to address that
> somehow?

I'm not that worried about it.  We have plenty of DCHECKs when heading into our
out of CONFIGURATION_MODE to catch misuse of this variable.  It's not that hard
to enforce our assumptions.  It will be even easier to enforce them when this
pending_configure_job_ is replaced with an instance of ConfigurationParams
(which is the only one of this SyncSessionJob's members that we really care
about).

I could make the same argument that WaitInterval shouldn't have a
pending_configure_job_ member when we're not in CONFIGURATION_MODE.  (That
member was removed in this patch, btw.)

If we really wanted to fix it, we'd need to use something like this:
http://en.wikipedia.org/wiki/State_pattern.  I'd rather not go down that road,
though.  That pattern makes it difficult to implement transitions between
states, and requires a lot of boilerplate code.  I think a few DCHECKs is the
least bad solution here.

[tim (not reviewing), 2013-04-04 18:07:30]

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:349: else // We are here because timer ran
out. So retry.
Just noticed this clause is redundant and somewhat confusing -- the return on
line 352 handles it and I think it is clearer than this comment.  Maybe we
should remove it?

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:608: ResumeWaiting();
On 2013/04/02 23:01:12, rlarocque wrote:
> Here's a paragraph that I had originally intended to write in the commit
> message:
> 
> > Because canary jobs are invoked synchronously (since r191458), there are
> > now almost no cases where we can interrupt a WaitInterval.   This makes
> > the logic to resume an interrupted WaitInterval obsolete.  As far as I
> > know, the one case where we do interrupt a WaitInterval is the single
> > nudge while in backoff, and we should never decide to SAVE such a job in
> > DoSyncSessionJob.  Therefore, we'll never need to resume a WaitInterval
> > that gets interrupted by this nudge while in backoff.
> 
> It turns out I was wrong.  We can decide to SAVE a nudge-while-in-backoff. 
That
> happens when we switch into configure mode between ScheduleNudgeImpl() and
> DoSyncSessionJob().  If we received a CONFIGURE in the interim, and we decided
> to SAVE it (because we are in an EXPONENTIAL_BACKOFF), we might be tempted to
> assume that it's save to just set the pending_configure_job_ and wait for the
> canary at the end of the wait interval.  That would be wrong, because the
> nudge-while-in-backoff has preempted the job at the end of the WaitInterval. 
> The result is that neither the nudge nor the configure will be run.
> 
> ResumeWaiting() solves this problem by making sure that the current pending
job
> is the canary at the end of the wait interval.
> 
> I have some ideas of safer/better ways to fix this problem.  My preference
would
> be to remove the nudge-while-in-backoff altogether, but there are other
options.
>  We should talk in person about this.  

It looks like we actually keep the WaitInterval timer set even though we're
resetting the pending_wakeup_, so it's not as clean of a "preemptive" cut as one
may think.  

I didn't fully realize PostDelayedTask is now a destructive operation. e.g we
need to be really careful not to ever call it (except the one careful case it is
called today) because it replaces anything that is posted. I think
'PostDelayedTask' is a dangerous name because it is *not* the same as posting to
MessageLoop::PostDelayedTask at all. 

We should fix that, at least by renaming. As for WaitIntervals, should
PostDelayedTask really let this half-state happen? Can we avoid having the
responsibility of a wait interval callback split between WaitInterval and
sync_loop_?  We can talk in person this afternoon.

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:922: if (mode_ == CONFIGURATION_MODE &&
pending_configure_job_) {
There's an implicit point here that canary jobs have priorities beyond the
JobPriority. If a nudge sends us to backoff and we then switch to config mode,
we have to run the config job first, even though there was a nudge canary. I
think this is covered by tests, but it's not that obvious looking at the code
unless you consider this function along with the fact that once we go to config
mode we can never leave until the job is run.  A comment would be a good idea.

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.h (right):

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.h:311: scoped_ptr<SyncSessionJob>
pending_configure_job_;
On 2013/04/04 00:59:48, rlarocque wrote:
> On 2013/04/04 00:22:11, timsteele wrote:
> > Why does this belong here? Unless I'm mistaken this introduces a state that
> > should not logically exist. Can't we avoid that entropy? We can only have a
> > pending config job if it is part of the WaitInterval, and it'd be clearer if
> the
> > ownership model reflected it in some way.  Are you planning to address that
> > somehow?
> 
> I'm not that worried about it.  We have plenty of DCHECKs when heading into
our
> out of CONFIGURATION_MODE to catch misuse of this variable.  It's not that
hard
> to enforce our assumptions.  It will be even easier to enforce them when this
> pending_configure_job_ is replaced with an instance of ConfigurationParams
> (which is the only one of this SyncSessionJob's members that we really care
> about).
> 
> I could make the same argument that WaitInterval shouldn't have a
> pending_configure_job_ member when we're not in CONFIGURATION_MODE.  (That
> member was removed in this patch, btw.)
> 
> If we really wanted to fix it, we'd need to use something like this:
> http://en.wikipedia.org/wiki/State_pattern.  I'd rather not go down that road,
> though.  That pattern makes it difficult to implement transitions between
> states, and requires a lot of boilerplate code.  I think a few DCHECKs is the
> least bad solution here.

Ok. A small concern I have is as the need to add nudge or config or new-job
specific state arises it will become increasingly hard to reason about what
members are valid when; hopefully "the next guy" will carefully consider and
replicate the DCHECKs you've added for whatever members happen to be the only
config state today. A comment near member declaration, similar to what you've
said here pointing out what applies to what mode/scheduler state may help (maybe
when you get around to removing SyncSchedulerJob).

[rlarocque, 2013-04-05 00:48:50]

PTAL.

Here's the patch with the nudge in backoff removed.  I decided to remove
PostDelayedTask rather than rename it, since the OneShotTimer now manages all
the pending work, and it doesn't need a helper function.  

I've moved the timer out of the WaitInterval and renamed it to
'pending_wakeup_timer_'.  I don't like that name, but haven't thought of
anything better.

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/13422003/diff/8001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:349: else // We are here because timer ran
out. So retry.
On 2013/04/04 18:07:30, timsteele wrote:
> Just noticed this clause is redundant and somewhat confusing -- the return on
> line 352 handles it and I think it is clearer than this comment.  Maybe we
> should remove it?

I was planning to leave it alone for now.  There's a lot about DecideOnJob()
that is redundant and confusing.  Fixing it is a job for another CL.

[tim (not reviewing), 2013-04-05 16:56:44]

https://codereview.chromium.org/13422003/diff/17001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/13422003/diff/17001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.cc:319: DCHECK_EQ(MessageLoop::current(),
sync_loop_);
It looks like all non-DCHECK use of sync_loop_ was removed with this CL. In that
case, we should remove the member and replace these DCHECKs with
NonThreadSafe::CalledOnValidThread.

https://codereview.chromium.org/13422003/diff/17001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.cc:347: else // We are here because timer ran
out. So retry.
In fact this comment isn't true, so we should fix it.  We can wind up here if we
get a network change notification during EXPONENTIAL_BACKOFF, for example.  In
that case, we should attempt to sync before the WaitInterval expires.

https://codereview.chromium.org/13422003/diff/17001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_impl.h (right):

https://codereview.chromium.org/13422003/diff/17001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.h:134: // EXPONENTIAL_BACKOFF intervals are
nudge-rate limited to 1 per interval.
Amend comment.

https://codereview.chromium.org/13422003/diff/17001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.h:148: // This bool is set to true if we have
observed a nudge during this
Remove comment.

[rlarocque, 2013-04-05 19:13:29]

PTAL. The latest patch includes a rebase, addresses your comments, and fixes a
few other oversights in the previous version.  I've also updated the commit
message.

https://codereview.chromium.org/13422003/diff/17001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/13422003/diff/17001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.cc:319: DCHECK_EQ(MessageLoop::current(),
sync_loop_);
On 2013/04/05 16:56:44, timsteele wrote:
> It looks like all non-DCHECK use of sync_loop_ was removed with this CL. In
that
> case, we should remove the member and replace these DCHECKs with
> NonThreadSafe::CalledOnValidThread.

Done.

https://codereview.chromium.org/13422003/diff/17001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.cc:347: else // We are here because timer ran
out. So retry.
On 2013/04/05 16:56:44, timsteele wrote:
> In fact this comment isn't true, so we should fix it.  We can wind up here if
we
> get a network change notification during EXPONENTIAL_BACKOFF, for example.  In
> that case, we should attempt to sync before the WaitInterval expires.  

Done.

https://codereview.chromium.org/13422003/diff/17001/sync/engine/sync_schedule...
File sync/engine/sync_scheduler_impl.h (right):

https://codereview.chromium.org/13422003/diff/17001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.h:134: // EXPONENTIAL_BACKOFF intervals are
nudge-rate limited to 1 per interval.
On 2013/04/05 16:56:44, timsteele wrote:
> Amend comment.

Done.

https://codereview.chromium.org/13422003/diff/17001/sync/engine/sync_schedule...
sync/engine/sync_scheduler_impl.h:148: // This bool is set to true if we have
observed a nudge during this
On 2013/04/05 16:56:44, timsteele wrote:
> Remove comment.

Done.

[tim (not reviewing), 2013-04-05 19:30:36]

LGTM with nit!

https://codereview.chromium.org/13422003/diff/24002/sync/engine/sync_session_...
File sync/engine/sync_session_job.cc (left):

https://codereview.chromium.org/13422003/diff/24002/sync/engine/sync_session_...
sync/engine/sync_session_job.cc:40: // Did we run through all SyncerSteps from
start_step() to end_step()
This comment still applies, I think, it was just in the wrong place.

[commit-bot: I haz the power, 2013-04-05 20:27:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/13422003/31001

[commit-bot: I haz the power, 2013-04-06 00:22:32]

Change committed as 192666