sync: Expose sync functionality as functions

sync: Expose sync functionality as functions

This change is a refactor to clean up ugliness introduced in previous
commits and prepare for future features.

The most notable change is the removal of "state machine" logic from
syncer.cc.  This allows us to remove the SyncerSteps enum and related
code.  The SyncShare function + enum parameters have been replaced with
the functions NormalSyncShare(), ConfigureSyncShare() and
PollSyncShare().  These changes should make it possible to address
crbug.com/109422, and to re-enable commits during poll-triggered sync
cycles (if desrired, see r206475).

The logic for fetching and applying updates has been modified, too.
Since the behaviour of GetUpdates varies depending on the type of cycle
(Configure, GetUpdates, or Poll) the logic to build and execute these
GetUpdate requests has been split up.  This enables us to remove the
NudgeTracker from the SyncSession (an ugly hack introduced in
r199136).  It should make it easier to implement crbug.com/147685.

In the interest of keeping this change as small and simple as possible
some obvious refactorings have not been intentionally excluded from this
CL.  For example, the logic around when to send SYNC_CYCLE_ENDED events
or when to return true or false frome the SyncShare functions remains
very complicated.  Untangling that mess would require some non-trivial
changes to the SyncScheduler, so they've been deferred until later.

BUG=147685, 109422
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209867

[rlarocque, 2013-06-18 00:25:20]

Here's an attempt to functionize syncer.cc and download_updates_command.cc. 
Please review.

I realize there's still some ugly or redundant code in this CL.  The
SyncSourceInfo class could be deleted, or at least removed from the SyncSession.
 The logic to handle EarlyExit() and return values in the SyncShare functions
could be cleaned up.  I decided not to fix these issues in this CL because the
diff is big enough as is.

https://codereview.chromium.org/17052007/diff/1/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/17052007/diff/1/sync/engine/download.cc#newco...
sync/engine/download.cc:174: bool need_encryption_key =
ShouldRequestEncryptionKey(session->context());
I'm not sure if this is necessary outside of configure mode, so I decided to
leave it in here just in case.

[tim (not reviewing), 2013-06-21 17:24:00]

syncer.cc comments in follow up to offline discussion, overall nothing too
major. Will review rest of the CL now.

https://chromiumcodereview.appspot.com/17052007/diff/2001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://chromiumcodereview.appspot.com/17052007/diff/2001/sync/engine/syncer....
sync/engine/syncer.cc:77: if (download_result != SYNCER_OK)
If ExitRequested is true, we can / should note that we aborted by returning
'false' to the caller.

https://chromiumcodereview.appspot.com/17052007/diff/2001/sync/engine/syncer....
sync/engine/syncer.cc:144: return SYNCER_OK;
I don't think you meant to leave this here.  I also think all three SyncShare
variants start and end the exact same way, and if we're making the syncer aware
of 'normal' 'config' anyway, I wonder why we wouldn't keep that in common so
it's only written / modified once

SyncShare(..., variant) {
  SendEventNotification(BEGIN);
  switch (variant) 
    NORMAL:
      NormalSyncShare(..)
    CONFIGURE:
    ...
      
  if (ExitRequested())
    false
  else 
    SendEventNotification(ENDED)
}

https://chromiumcodereview.appspot.com/17052007/diff/2001/sync/engine/syncer....
sync/engine/syncer.cc:149: 
What do you think about adding DCHECK(!ExitRequested()) here? I see no reason to
force any extra work at shutdown, it doesn't look like we were doing it
previously.

[tim (not reviewing), 2013-06-21 17:41:38]

https://chromiumcodereview.appspot.com/17052007/diff/2001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://chromiumcodereview.appspot.com/17052007/diff/2001/sync/engine/syncer....
sync/engine/syncer.cc:68: while
(!session->status_controller().ServerSaysNothingMoreToDownload()) {
I think the general paradigm of "check ExitRequested before every major syncer
step" was useful at the very least to defend against new steps being added in
the future that block shutdown and may not be as performant as the ones we have
today. Previously, you added a SyncerCommand for a new step and that bail-out
was handled for you with code that has worked and has never changed since the
inception of syncer.cc. 

It also gave a tighter level of isolation than we have now from bugs being
introduced in a particular step (or, for example, in the SCM::Abort code, which
has happened before) from creating shutdown hangs (e.g on stable).

There's a small complexity benefit to be had in the boilerplate code today by
removing it. But it makes the framework slightly less defensive against
inadvertently slow code or bugs in the future. 

Do you see other pros/cons?

[rlarocque, 2013-06-21 18:30:36]

https://chromiumcodereview.appspot.com/17052007/diff/2001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://chromiumcodereview.appspot.com/17052007/diff/2001/sync/engine/syncer....
sync/engine/syncer.cc:68: while
(!session->status_controller().ServerSaysNothingMoreToDownload()) {
On 2013/06/21 17:41:38, timsteele wrote:
> I think the general paradigm of "check ExitRequested before every major syncer
> step" was useful at the very least to defend against new steps being added in
> the future that block shutdown and may not be as performant as the ones we
have
> today. Previously, you added a SyncerCommand for a new step and that bail-out
> was handled for you with code that has worked and has never changed since the
> inception of syncer.cc. 
> 
> It also gave a tighter level of isolation than we have now from bugs being
> introduced in a particular step (or, for example, in the SCM::Abort code,
which
> has happened before) from creating shutdown hangs (e.g on stable).
> 
> There's a small complexity benefit to be had in the boilerplate code today by
> removing it. But it makes the framework slightly less defensive against
> inadvertently slow code or bugs in the future. 
> 
> Do you see other pros/cons?

IMO, we never should have defaulted to checking ExitRequested at every step.

Every early exit code path is a potential source of bugs.  Each one must be
carefully considered to make sure that it's safe to stop processing at that
point.  The rarely hit paths are especially dangerous because they could slip
through testing and cause problems only after we deploy the code to millions of
users.  

As for the "pro" side, I'd argue that most of these checks have negligible
benefits.  The early exit during blocking network I/O makes sense.  However, the
other checks are almost useless.

Let's say our goal is to reduce worst case shutdown blocking time.  Today, the
current worst case is probably when we have lots of updates to apply, and the
shutdown request arrives just after the pre-APPLY_UPDATES ExitRequested().  In
this very rare case, we'll spend tens of milliseconds applying updates (~90ms in
a test on my dev machine).  

My changes will affect this metric.  The new worst case would be if the exit
request arrives just after the last download updates.  In that case, we would
have to pay the cost of PROCESS_UPDATES + STORE_TIMESTAMPS + APPLY_UPDATES. 
However, the time for PROCESS_UPDATES + STORE_TIMESTAMPS is around 4ms.  That's
not a big enough change for anyone to notice.

If we think tens of milliseconds is too long to block during shutdown, then we
should fix it.  However, I don't see how one could argue that 90ms of blocking
is OK, but 95ms is not.

https://chromiumcodereview.appspot.com/17052007/diff/2001/sync/engine/syncer....
sync/engine/syncer.cc:77: if (download_result != SYNCER_OK)
On 2013/06/21 17:24:00, timsteele wrote:
> If ExitRequested is true, we can / should note that we aborted by returning
> 'false' to the caller.

Good point.  I was trying to follow the same convention as the old code, but
forgot that the loop condition is (!ExitRequested()).  It would be more
backwards compatible to 'return !ExitRequested()'.

https://chromiumcodereview.appspot.com/17052007/diff/2001/sync/engine/syncer....
sync/engine/syncer.cc:144: return SYNCER_OK;
On 2013/06/21 17:24:00, timsteele wrote:
> I don't think you meant to leave this here.  I also think all three SyncShare
> variants start and end the exact same way, and if we're making the syncer
aware
> of 'normal' 'config' anyway, I wonder why we wouldn't keep that in common so
> it's only written / modified once
> 
> SyncShare(..., variant) {
>   SendEventNotification(BEGIN);
>   switch (variant) 
>     NORMAL:
>       NormalSyncShare(..)
>     CONFIGURE:
>     ...
>       
>   if (ExitRequested())
>     false
>   else 
>     SendEventNotification(ENDED)
> }

The SyncShare variants have different parameters.  It would be difficult to
share code in that way while still passing through the right parameters for each
type of SyncShare.

https://chromiumcodereview.appspot.com/17052007/diff/2001/sync/engine/syncer....
sync/engine/syncer.cc:149: 
On 2013/06/21 17:24:00, timsteele wrote:
> What do you think about adding DCHECK(!ExitRequested()) here? I see no reason
to
> force any extra work at shutdown, it doesn't look like we were doing it
> previously.

I don't think a DCHECK() is would be safe...  I'll assume you meant "if
(ExitRequested()) { return; }".

If we get here, that means that an early exit was not requested at the time that
our last DownloadUpdates() finished.  That was (at worst) ~5ms ago.  I don't
think it's worth checking again.

[rlarocque, 2013-06-21 18:33:20]

+Nicolas

Nicolas, will this change conflict with your work to put types into a state
where they process updates but don't apply them?

[Nicolas Zea, 2013-06-21 19:46:56]

On 2013/06/21 18:33:20, rlarocque wrote:
> +Nicolas
> 
> Nicolas, will this change conflict with your work to put types into a state
> where they process updates but don't apply them?

I don't think so? Keep in mind, I'm not changing anything in the syncer layer,
the logic to process + not apply was pre-existing, it's just a matter of the
type being in passive state. As far as I can tell you haven't modified the use
of the routing info in applying updates?

[rlarocque, 2013-06-21 20:00:32]

On 2013/06/21 19:46:56, Nicolas Zea wrote:
> On 2013/06/21 18:33:20, rlarocque wrote:
> > +Nicolas
> > 
> > Nicolas, will this change conflict with your work to put types into a state
> > where they process updates but don't apply them?
> 
> I don't think so? Keep in mind, I'm not changing anything in the syncer layer,
> the logic to process + not apply was pre-existing, it's just a matter of the
> type being in passive state. As far as I can tell you haven't modified the use
> of the routing info in applying updates?

Yep, that's what I thought, too.  Thanks for looking.

[tim (not reviewing), 2013-06-21 21:24:03]

I feel like the ExitRequested semantics change is significant enough to warrant
a second set of eyes. If my case does nothing to sway you or anyone else towards
wanting to keep it, though, then maybe removing it is the right thing?

https://chromiumcodereview.appspot.com/17052007/diff/2001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://chromiumcodereview.appspot.com/17052007/diff/2001/sync/engine/syncer....
sync/engine/syncer.cc:149: 
On 2013/06/21 18:30:36, rlarocque wrote:
> On 2013/06/21 17:24:00, timsteele wrote:
> > What do you think about adding DCHECK(!ExitRequested()) here? I see no
reason
> to
> > force any extra work at shutdown, it doesn't look like we were doing it
> > previously.
> 
> I don't think a DCHECK() is would be safe...  I'll assume you meant "if
> (ExitRequested()) { return; }".
> 
> If we get here, that means that an early exit was not requested at the time
that
> our last DownloadUpdates() finished.  That was (at worst) ~5ms ago.  I don't
> think it's worth checking again.

Keep in mind that 90 milliseconds (as quoted above) on your state-of-the-art
powerhouse dev machine could grow considerably on slower hardware, crappy
phones, etc.  And few things can be as aggravating as a browser that won't
disappear despite frantic X-clicking. I can't make a case that sync is worthy of
holding shutdown for even 90ms longer.

Another data point: Chrome OS sends a SIGTERM if the browser hasn't shut down in
3 seconds (used to be 3, may have changed). Deciding to never abort operations
that we know may iterate over the entire set of synced items even when we know
it's time to (and safe to) quit only encroaches on that ceiling. Should the
timeout become more aggressive, and should we ever add more work to a sync cycle
(both of which I would argue *will happen* in time), having a well defined
boundary of steps where it is safe to "stop processing" is a good feature. We
have a reasonable de-coupling now with download + process + apply, and I think
it would be far more work to tease that apart later.

Lastly, aside from that, in my experience over the years we have seen
significant slowdowns from time to time in the various sync steps. Especially
with ApplyUpdates, and frankly, at shutdown, IMO the earlier we can stop 
needing all the various threads around to dispatch to, the better.

Does anyone else have any feelings on this?  I'd prefer keeping the bail-out at
the very least in the ApplyUpdates case.

https://chromiumcodereview.appspot.com/17052007/diff/14001/sync/engine/get_co...
File sync/engine/get_commit_ids_command.h (right):

https://chromiumcodereview.appspot.com/17052007/diff/14001/sync/engine/get_co...
sync/engine/get_commit_ids_command.h:127: ModelTypeSet requested_types_;
const ?

[rlarocque, 2013-06-24 20:03:08]

Addressed comments and rebased.

I guess we're now waiting for Nicolas and/or Haitao's comments on that other
discussion?

https://codereview.chromium.org/17052007/diff/14001/sync/engine/get_commit_id...
File sync/engine/get_commit_ids_command.h (right):

https://codereview.chromium.org/17052007/diff/14001/sync/engine/get_commit_id...
sync/engine/get_commit_ids_command.h:127: ModelTypeSet requested_types_;
On 2013/06/21 21:24:03, timsteele wrote:
> const ?

Done.

[Nicolas Zea, 2013-06-24 21:18:16]

The suggestion is basically to just check ExitRequested before ApplyUpdates
right? That seems like a simple enough and harmless change with a reasonable
opportunity for upside, so I'd prefer doing it I think. Particularly if we do
something like in my comment below

https://codereview.chromium.org/17052007/diff/23001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://codereview.chromium.org/17052007/diff/23001/sync/engine/syncer.cc#new...
sync/engine/syncer.cc:89: if (ExitRequested()) {
this pattern of checking ExitRequested seems to be repeated a lot, perhaps
encapsulate it in a function? Maybe pass a closure to the function, and have all
ApplyUpdates and SendEventNotifications calls be bound before being passed to
that function?

[rlarocque, 2013-06-24 21:59:30]

I've re-added the check, but I reserve the right to remove it again if it gets
in the way.  I see no evidence that it has any noticeable benefit.

https://codereview.chromium.org/17052007/diff/23001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://codereview.chromium.org/17052007/diff/23001/sync/engine/syncer.cc#new...
sync/engine/syncer.cc:89: if (ExitRequested()) {
On 2013/06/24 21:18:16, Nicolas Zea wrote:
> this pattern of checking ExitRequested seems to be repeated a lot, perhaps
> encapsulate it in a function? Maybe pass a closure to the function, and have
all
> ApplyUpdates and SendEventNotifications calls be bound before being passed to
> that function?

I'd prefer to not use closures unless they're absolutely necessary.  C++ was not
meant to be used that way.  

Besides, I intend to clean up this logic in a follow-up patch.

[rlarocque, 2013-06-25 18:45:46]

I've fixed some integration test issues and re-enabled that early exit.  PTAL.

[Nicolas Zea, 2013-06-25 23:47:34]

style-wise I think I'd still prefer to have the
DownloadUpdates/ApplyUpdates/etc. functions handle the Exit checking themselves
(or via a wrapper), but if you're planning to clean this up anyways it probably
doesn't matter.

otherwise LGTM

https://codereview.chromium.org/17052007/diff/48001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://codereview.chromium.org/17052007/diff/48001/sync/engine/syncer.cc#new...
sync/engine/syncer.cc:62: bool Syncer::NormalSyncShare(SyncSession* session,
nit: session should be the last param (inputs before outputs), here and below

https://codereview.chromium.org/17052007/diff/48001/sync/engine/syncer.cc#new...
sync/engine/syncer.cc:79: if (!ExitRequested()) {
Given that this logic is done 6 times in this file, it could probably be it's
own method (e.g. return HandleCycleEnd()) which checks for exit and sends the
event notification as necessary.

[rlarocque, 2013-06-26 00:56:07]

Patch updated.  Tim, PTAL.

https://codereview.chromium.org/17052007/diff/48001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://codereview.chromium.org/17052007/diff/48001/sync/engine/syncer.cc#new...
sync/engine/syncer.cc:62: bool Syncer::NormalSyncShare(SyncSession* session,
On 2013/06/25 23:47:34, Nicolas Zea wrote:
> nit: session should be the last param (inputs before outputs), here and below

Done.

https://codereview.chromium.org/17052007/diff/48001/sync/engine/syncer.cc#new...
sync/engine/syncer.cc:79: if (!ExitRequested()) {
On 2013/06/25 23:47:34, Nicolas Zea wrote:
> Given that this logic is done 6 times in this file, it could probably be it's
> own method (e.g. return HandleCycleEnd()) which checks for exit and sends the
> event notification as necessary.

Done.

[tim (not reviewing), 2013-06-26 20:26:30]

https://codereview.chromium.org/17052007/diff/57001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/17052007/diff/57001/sync/engine/download.cc#n...
sync/engine/download.cc:161: DCHECK(!request_types.Empty());
There's just too much duplicated code here. I think everything up to this point
in this function is copied in the Configure (and Poll?) variant.  Worse, I keep
having to scroll up and down to convince myself they're the same or different
and I'm not even sure if I'm right; the readability isn't great.  Previously all
boilerplate was handled by common code; you should figure out how to make that
work  in this world as well before checking in.

https://codereview.chromium.org/17052007/diff/57001/sync/engine/download.cc#n...
sync/engine/download.cc:165: nudge_tracker.updates_source());
The GetUpdatesCallerInfo setting seems to be the same as well except in one case
it comes from a nudge tracker.  Depending how you deal with the above code
duplication you might want to parameterize this as well.

https://codereview.chromium.org/17052007/diff/57001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://codereview.chromium.org/17052007/diff/57001/sync/engine/syncer.cc#new...
sync/engine/syncer.cc:103: SyncerError download_result =
ConfigureDownloadUpdates(
"ConfigureDownloadUpdates" makes it sound like we're configuring a download
updates.  DownloadUpdatesForConfigure makes more sense.

[rlarocque, 2013-06-26 22:00:51]

PTAL

https://codereview.chromium.org/17052007/diff/57001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/17052007/diff/57001/sync/engine/download.cc#n...
sync/engine/download.cc:161: DCHECK(!request_types.Empty());
On 2013/06/26 20:26:30, timsteele wrote:
> There's just too much duplicated code here. I think everything up to this
point
> in this function is copied in the Configure (and Poll?) variant.  Worse, I
keep
> having to scroll up and down to convince myself they're the same or different
> and I'm not even sure if I'm right; the readability isn't great.  Previously
all
> boilerplate was handled by common code; you should figure out how to make that
> work  in this world as well before checking in.

I've expanded InitDownloadUpdates to include more common code.  

I was reluctant to do this earlier, since it results in a function that doesn't
make a lot of sense.  It performs a handful of trivial tasks that aren't related
to each other, except in that they're common to all three download update
functions.  However, you make a good point about the benefits of making it clear
that these functions do have a few things in common.

https://codereview.chromium.org/17052007/diff/57001/sync/engine/download.cc#n...
sync/engine/download.cc:165: nudge_tracker.updates_source());
On 2013/06/26 20:26:30, timsteele wrote:
> The GetUpdatesCallerInfo setting seems to be the same as well except in one
case
> it comes from a nudge tracker.  Depending how you deal with the above code
> duplication you might want to parameterize this as well.

That will change.  The SyncSourceInfo becomes redundant once this patch lands. 
I haven't removed it yet because a lot of unit tests depend on it.

https://codereview.chromium.org/17052007/diff/57001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://codereview.chromium.org/17052007/diff/57001/sync/engine/syncer.cc#new...
sync/engine/syncer.cc:103: SyncerError download_result =
ConfigureDownloadUpdates(
On 2013/06/26 20:26:30, timsteele wrote:
> "ConfigureDownloadUpdates" makes it sound like we're configuring a download
> updates.  DownloadUpdatesForConfigure makes more sense.
> 

Done.

[tim (not reviewing), 2013-06-27 17:42:02]

https://codereview.chromium.org/17052007/diff/68001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/17052007/diff/68001/sync/engine/download.cc#n...
sync/engine/download.cc:246: SyncerError PollDownloadUpdates(
DownloadUpdatesForPoll

https://codereview.chromium.org/17052007/diff/68001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://codereview.chromium.org/17052007/diff/68001/sync/engine/syncer.cc#new...
sync/engine/syncer.cc:143: return HandleCycleEnd(session);
As best I can tell, just *one* of the ~22 lines in each of PollSyncShare and
ConfigureSyncShare differ; one calls DownloadUpdatesForPoll and one calls
DownloadUpdatesforConfigure. I think the rest is identical?  It would be
preferable from a readability and maintainability standpoint to share more code,
which you could achieve (I think) pretty easily with a switch statement or a
download_closure / function pointer passed to a DownloadAndApply helper. It
would more clearly inform the reader that the difference lies in the download.h
routine chosen, and reduce the number of places we have to update if we want to
change any of the identical lines in each place.

It looks like the same logic applies to "Normal", except after the identical
section (which could be factored out) it goes on to do more work.

[rlarocque, 2013-06-27 18:08:44]

https://codereview.chromium.org/17052007/diff/68001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://codereview.chromium.org/17052007/diff/68001/sync/engine/syncer.cc#new...
sync/engine/syncer.cc:143: return HandleCycleEnd(session);
On 2013/06/27 17:42:02, timsteele wrote:
> As best I can tell, just *one* of the ~22 lines in each of PollSyncShare and
> ConfigureSyncShare differ; one calls DownloadUpdatesForPoll and one calls
> DownloadUpdatesforConfigure. I think the rest is identical?  It would be
> preferable from a readability and maintainability standpoint to share more
code,
> which you could achieve (I think) pretty easily with a switch statement or a
> download_closure / function pointer passed to a DownloadAndApply helper. It
> would more clearly inform the reader that the difference lies in the
download.h
> routine chosen, and reduce the number of places we have to update if we want
to
> change any of the identical lines in each place.
> 
> It looks like the same logic applies to "Normal", except after the identical
> section (which could be factored out) it goes on to do more work.

Yes, they share a lot of code.  I'm hoping to reduce this to a trivial amount of
code, but I was trying to keep this CL easy to review.  

I don't think it's appropriate to use a Closure, function pointer, or vptr just
to share a while statement.

[tim (not reviewing), 2013-06-27 18:22:15]

https://codereview.chromium.org/17052007/diff/68001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://codereview.chromium.org/17052007/diff/68001/sync/engine/syncer.cc#new...
sync/engine/syncer.cc:143: return HandleCycleEnd(session);
On 2013/06/27 18:08:45, rlarocque wrote:
> On 2013/06/27 17:42:02, timsteele wrote:
> > As best I can tell, just *one* of the ~22 lines in each of PollSyncShare and
> > ConfigureSyncShare differ; one calls DownloadUpdatesForPoll and one calls
> > DownloadUpdatesforConfigure. I think the rest is identical?  It would be
> > preferable from a readability and maintainability standpoint to share more
> code,
> > which you could achieve (I think) pretty easily with a switch statement or a
> > download_closure / function pointer passed to a DownloadAndApply helper. It
> > would more clearly inform the reader that the difference lies in the
> download.h
> > routine chosen, and reduce the number of places we have to update if we want
> to
> > change any of the identical lines in each place.
> > 
> > It looks like the same logic applies to "Normal", except after the identical
> > section (which could be factored out) it goes on to do more work.
> 
> Yes, they share a lot of code.  I'm hoping to reduce this to a trivial amount
of
> code, but I was trying to keep this CL easy to review.  
> 
> I don't think it's appropriate to use a Closure, function pointer, or vptr
just
> to share a while statement.

But it's not just a while statement.  There's also UpdateStartTime, sending
SYNC_CYCLE_BEGIN, setting the last updates result, handling the !SYNCER_OK case,
checking ExitRequested before ApplyUpdates, and calling ApplyUpdates.  If any of
those change or we add anything the problem only gets worse.  I propose
something like this, which I honestly find much more readable and enlightening,
from a "what does sync share do and how do the variants differ" perspective:

SyncShareNoCommitImpl(ModelTypeSet, SyncSession, Closure download_routine) {
  DownloadAndApply(session, request_types, download_routine);
  return HandleCycleEnd(session);
}

PollSyncShare(...) {
  return SyncShareNoCommit(...,..., DownloadUpdatesForPoll);
}

ConfigureSyncShare(...) {
  return SyncShareNoCommit(...,..., DownloadUpdatesForConfigure);
}

NormalSyncShare(...) {
  DownloadAndApply(...);
  if (ExitRequested())
    return false;
  BuildAndPostCommits();
  set_commit_result(...)
  return HandleCycleEnd();
}

[tim (not reviewing), 2013-06-27 18:24:17]

On 2013/06/27 18:22:15, timsteele wrote:
> https://codereview.chromium.org/17052007/diff/68001/sync/engine/syncer.cc
> File sync/engine/syncer.cc (right):
> 
>
https://codereview.chromium.org/17052007/diff/68001/sync/engine/syncer.cc#new...
> sync/engine/syncer.cc:143: return HandleCycleEnd(session);
> On 2013/06/27 18:08:45, rlarocque wrote:
> > On 2013/06/27 17:42:02, timsteele wrote:
> > > As best I can tell, just *one* of the ~22 lines in each of PollSyncShare
and
> > > ConfigureSyncShare differ; one calls DownloadUpdatesForPoll and one calls
> > > DownloadUpdatesforConfigure. I think the rest is identical?  It would be
> > > preferable from a readability and maintainability standpoint to share more
> > code,
> > > which you could achieve (I think) pretty easily with a switch statement or
a
> > > download_closure / function pointer passed to a DownloadAndApply helper.
It
> > > would more clearly inform the reader that the difference lies in the
> > download.h
> > > routine chosen, and reduce the number of places we have to update if we
want
> > to
> > > change any of the identical lines in each place.
> > > 
> > > It looks like the same logic applies to "Normal", except after the
identical
> > > section (which could be factored out) it goes on to do more work.
> > 
> > Yes, they share a lot of code.  I'm hoping to reduce this to a trivial
amount
> of
> > code, but I was trying to keep this CL easy to review.  
> > 
> > I don't think it's appropriate to use a Closure, function pointer, or vptr
> just
> > to share a while statement.
> 
> But it's not just a while statement.  There's also UpdateStartTime, sending
> SYNC_CYCLE_BEGIN, setting the last updates result, handling the !SYNCER_OK
case,
> checking ExitRequested before ApplyUpdates, and calling ApplyUpdates.  If any
of
> those change or we add anything the problem only gets worse.  I propose
> something like this, which I honestly find much more readable and
enlightening,
> from a "what does sync share do and how do the variants differ" perspective:
> 
> SyncShareNoCommitImpl(ModelTypeSet, SyncSession, Closure download_routine) {
>   DownloadAndApply(session, request_types, download_routine);
>   return HandleCycleEnd(session);
> }
> 
> PollSyncShare(...) {
>   return SyncShareNoCommit(...,..., DownloadUpdatesForPoll);
> }
> 
> ConfigureSyncShare(...) {
>   return SyncShareNoCommit(...,..., DownloadUpdatesForConfigure);
> }
> 
> NormalSyncShare(...) {
>   DownloadAndApply(...);
>   if (ExitRequested())
>     return false;
>   BuildAndPostCommits();
>   set_commit_result(...)
>   return HandleCycleEnd();
> }

Actually the helper routine may not be necessary, sorry, was just
pseudocoding...

[rlarocque, 2013-06-27 18:46:07]

https://codereview.chromium.org/17052007/diff/68001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

https://codereview.chromium.org/17052007/diff/68001/sync/engine/syncer.cc#new...
sync/engine/syncer.cc:143: return HandleCycleEnd(session);
On 2013/06/27 18:22:15, timsteele wrote:
> On 2013/06/27 18:08:45, rlarocque wrote:
> > On 2013/06/27 17:42:02, timsteele wrote:
> > > As best I can tell, just *one* of the ~22 lines in each of PollSyncShare
and
> > > ConfigureSyncShare differ; one calls DownloadUpdatesForPoll and one calls
> > > DownloadUpdatesforConfigure. I think the rest is identical?  It would be
> > > preferable from a readability and maintainability standpoint to share more
> > code,
> > > which you could achieve (I think) pretty easily with a switch statement or
a
> > > download_closure / function pointer passed to a DownloadAndApply helper.
It
> > > would more clearly inform the reader that the difference lies in the
> > download.h
> > > routine chosen, and reduce the number of places we have to update if we
want
> > to
> > > change any of the identical lines in each place.
> > > 
> > > It looks like the same logic applies to "Normal", except after the
identical
> > > section (which could be factored out) it goes on to do more work.
> > 
> > Yes, they share a lot of code.  I'm hoping to reduce this to a trivial
amount
> of
> > code, but I was trying to keep this CL easy to review.  
> > 
> > I don't think it's appropriate to use a Closure, function pointer, or vptr
> just
> > to share a while statement.
> 
> But it's not just a while statement.  There's also UpdateStartTime, sending
> SYNC_CYCLE_BEGIN, setting the last updates result, handling the !SYNCER_OK
case,
> checking ExitRequested before ApplyUpdates, and calling ApplyUpdates.  If any
of
> those change or we add anything the problem only gets worse.  I propose
> something like this, which I honestly find much more readable and
enlightening,
> from a "what does sync share do and how do the variants differ" perspective:
> 
> SyncShareNoCommitImpl(ModelTypeSet, SyncSession, Closure download_routine) {
>   DownloadAndApply(session, request_types, download_routine);
>   return HandleCycleEnd(session);
> }
> 
> PollSyncShare(...) {
>   return SyncShareNoCommit(...,..., DownloadUpdatesForPoll);
> }
> 
> ConfigureSyncShare(...) {
>   return SyncShareNoCommit(...,..., DownloadUpdatesForConfigure);
> }
> 
> NormalSyncShare(...) {
>   DownloadAndApply(...);
>   if (ExitRequested())
>     return false;
>   BuildAndPostCommits();
>   set_commit_result(...)
>   return HandleCycleEnd();
> }

If we're worried about adding or modifying common tasks that must be done at the
beginning of a sync cycle, I could merge session->UpdateStartTime() and
session->SendEventNotification into a session->DoCycleInitTasks().  

Though, really, I'd prefer to move away from the notion of a "sync cycle".  Both
of these functions are of questionable use and could probably be removed.  

The while loop and return value handling is complicated because I'm trying to
maintain compatibility with the old interfaces.  I'm hoping the code could
eventually look like this:

do {
  SyncerResult result = PollDownloadUpdates(...)
} while (result == MORE_TO_DOWNLOAD);

if (!IsError(result) && !ExitRequested())
  ApplyUpdates(session);
return result;

...give or take the sending of a cycle ended notification.  If we're worried
about common tasks, we can add a session->DoCycleEndTasks() call to it.

Yes, there will be some duplication.  But I don't think it's unreasonable for
three functions to share:
- A call to common pre-sync cycle functions.
- A loop
- A call to ApplyUpdates()
- A call to common post-sync cycle functions.

[tim (not reviewing), 2013-07-01 17:35:46]

On 2013/06/27 18:46:07, rlarocque wrote:
> https://codereview.chromium.org/17052007/diff/68001/sync/engine/syncer.cc
> File sync/engine/syncer.cc (right):
> 
>
https://codereview.chromium.org/17052007/diff/68001/sync/engine/syncer.cc#new...
> sync/engine/syncer.cc:143: return HandleCycleEnd(session);
> On 2013/06/27 18:22:15, timsteele wrote:
> > On 2013/06/27 18:08:45, rlarocque wrote:
> > > On 2013/06/27 17:42:02, timsteele wrote:
> > > > As best I can tell, just *one* of the ~22 lines in each of PollSyncShare
> and
> > > > ConfigureSyncShare differ; one calls DownloadUpdatesForPoll and one
calls
> > > > DownloadUpdatesforConfigure. I think the rest is identical?  It would be
> > > > preferable from a readability and maintainability standpoint to share
more
> > > code,
> > > > which you could achieve (I think) pretty easily with a switch statement
or
> a
> > > > download_closure / function pointer passed to a DownloadAndApply helper.
> It
> > > > would more clearly inform the reader that the difference lies in the
> > > download.h
> > > > routine chosen, and reduce the number of places we have to update if we
> want
> > > to
> > > > change any of the identical lines in each place.
> > > > 
> > > > It looks like the same logic applies to "Normal", except after the
> identical
> > > > section (which could be factored out) it goes on to do more work.
> > > 
> > > Yes, they share a lot of code.  I'm hoping to reduce this to a trivial
> amount
> > of
> > > code, but I was trying to keep this CL easy to review.  
> > > 
> > > I don't think it's appropriate to use a Closure, function pointer, or vptr
> > just
> > > to share a while statement.
> > 
> > But it's not just a while statement.  There's also UpdateStartTime, sending
> > SYNC_CYCLE_BEGIN, setting the last updates result, handling the !SYNCER_OK
> case,
> > checking ExitRequested before ApplyUpdates, and calling ApplyUpdates.  If
any
> of
> > those change or we add anything the problem only gets worse.  I propose
> > something like this, which I honestly find much more readable and
> enlightening,
> > from a "what does sync share do and how do the variants differ" perspective:
> > 
> > SyncShareNoCommitImpl(ModelTypeSet, SyncSession, Closure download_routine) {
> >   DownloadAndApply(session, request_types, download_routine);
> >   return HandleCycleEnd(session);
> > }
> > 
> > PollSyncShare(...) {
> >   return SyncShareNoCommit(...,..., DownloadUpdatesForPoll);
> > }
> > 
> > ConfigureSyncShare(...) {
> >   return SyncShareNoCommit(...,..., DownloadUpdatesForConfigure);
> > }
> > 
> > NormalSyncShare(...) {
> >   DownloadAndApply(...);
> >   if (ExitRequested())
> >     return false;
> >   BuildAndPostCommits();
> >   set_commit_result(...)
> >   return HandleCycleEnd();
> > }
> 
> If we're worried about adding or modifying common tasks that must be done at
the
> beginning of a sync cycle, I could merge session->UpdateStartTime() and
> session->SendEventNotification into a session->DoCycleInitTasks().  
> 
> Though, really, I'd prefer to move away from the notion of a "sync cycle". 
Both
> of these functions are of questionable use and could probably be removed.  
> 
> The while loop and return value handling is complicated because I'm trying to
> maintain compatibility with the old interfaces.  I'm hoping the code could
> eventually look like this:
> 
> do {
>   SyncerResult result = PollDownloadUpdates(...)
> } while (result == MORE_TO_DOWNLOAD);
> 
> if (!IsError(result) && !ExitRequested())
>   ApplyUpdates(session);
> return result;
> 
> ...give or take the sending of a cycle ended notification.  If we're worried
> about common tasks, we can add a session->DoCycleEndTasks() call to it.
> 
> Yes, there will be some duplication.  But I don't think it's unreasonable for
> three functions to share:
> - A call to common pre-sync cycle functions.
> - A loop
> - A call to ApplyUpdates()
> - A call to common post-sync cycle functions.

Your proposed end state looks ok, and if I was reviewing that I might sign off,
but this CL doesn't get us there. As for closures, two people have already
suggested use of closures to reduce duplication in this patch; I don't think
"c++ wasn't meant to be used that way" is a good enough argument against that
feedback, given that it's the way many things work in Chromium, and lots of
effort went in to making a useable callback system.  

So in short, there is still too much duplication here wrapped around one
different line of code.

[rlarocque, 2013-07-02 20:05:23]

After consulting with chromium-dev, I've decided to take your advice.  I've
refactored the syncer code to use more closures.

PTAL.

[tim (not reviewing), 2013-07-02 21:01:52]

Cool.  syncer.cc looks a lot better now!

A few more similarly themed code-duplication-reduction comments on other files.

https://codereview.chromium.org/17052007/diff/92001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/17052007/diff/92001/sync/engine/download.cc#n...
sync/engine/download.cc:198: status->set_updates_request_types(request_types);
Can these two lines (get &status and set_request_types) go in to
InitDownloadUpdatesRequest?

https://codereview.chromium.org/17052007/diff/92001/sync/engine/download.cc#n...
sync/engine/download.cc:270: dir->GetDownloadProgress(it.Get(),
progress_marker);
All Download* variants do the same thing for GetDownloadProgress.  Could these
~10 lines be pulled in to InitDownloadUpdatesRequest?  It's worth making it
clear that all three cases are actually identical (and only have to change one
place if we modify filtering down the road, like when we added ProxyTypes).

The 'Normal' case additionally sets notification hints, but that could easily be
done separately / factored out for the 'Normal' case in that function with an
extra loop over the < 30 data types.  Or have a simple helper routine that
optionally sets notification hints, although I don't think that's necessary in
the name of perf here.

https://codereview.chromium.org/17052007/diff/92001/sync/engine/download.cc#n...
sync/engine/download.cc:279:
get_updates->mutable_caller_info()->set_notifications_enabled(
Can this (set_notifications_enabled) go in to InitDownloadUpdatesRequest?

[rlarocque, 2013-07-02 22:20:24]

Addressed comments.  PTAL.

https://codereview.chromium.org/17052007/diff/92001/sync/engine/download.cc
File sync/engine/download.cc (right):

https://codereview.chromium.org/17052007/diff/92001/sync/engine/download.cc#n...
sync/engine/download.cc:198: status->set_updates_request_types(request_types);
On 2013/07/02 21:01:53, timsteele wrote:
> Can these two lines (get &status and set_request_types) go in to
> InitDownloadUpdatesRequest?

Done.

https://codereview.chromium.org/17052007/diff/92001/sync/engine/download.cc#n...
sync/engine/download.cc:270: dir->GetDownloadProgress(it.Get(),
progress_marker);
On 2013/07/02 21:01:53, timsteele wrote:
> All Download* variants do the same thing for GetDownloadProgress.  Could these
> ~10 lines be pulled in to InitDownloadUpdatesRequest?  It's worth making it
> clear that all three cases are actually identical (and only have to change one
> place if we modify filtering down the road, like when we added ProxyTypes).
> 
> The 'Normal' case additionally sets notification hints, but that could easily
be
> done separately / factored out for the 'Normal' case in that function with an
> extra loop over the < 30 data types.  Or have a simple helper routine that
> optionally sets notification hints, although I don't think that's necessary in
> the name of perf here.

Done.

https://codereview.chromium.org/17052007/diff/92001/sync/engine/download.cc#n...
sync/engine/download.cc:279:
get_updates->mutable_caller_info()->set_notifications_enabled(
On 2013/07/02 21:01:53, timsteele wrote:
> Can this (set_notifications_enabled) go in to InitDownloadUpdatesRequest?

Done.

[tim (not reviewing), 2013-07-02 22:38:54]

LGTM!

[commit-bot: I haz the power, 2013-07-02 22:48:51]

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

[commit-bot: I haz the power, 2013-07-03 03:44:21]

Change committed as 209867