sync: Refactor update application

sync: Refactor update application

This is a refactoring patch.  It removes an unnecessary ConflictResolver
parameter from a few helper functions and rewrites the update
application logic.

This patch is part of a series.  I expect that future patches will be
easier to implement if an applicator can be reused, and the iterator
members make it difficult to do so with the current design.

BUG=147681
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159186

[rlarocque, 2012-09-25 17:37:45]

Fred, the git logs tell me you were the last one to make meaningful changes to
apply_updates_command.cc, so I picked you for the review.  Feel free to reassign
if you think someone else would know this code better.

=====

I generally dislike patches that refactor working code, but I think it makes
sense in this case.  I want to get this function straightened out before I head
in the general direction of http://codereview.chromium.org/10933075/.

Please review.

http://codereview.chromium.org/10964057/diff/7001/sync/engine/update_applicat...
File sync/engine/update_applicator.cc (right):

http://codereview.chromium.org/10964057/diff/7001/sync/engine/update_applicat...
sync/engine/update_applicator.cc:87: break;
I don't like this, but I think it's cleaner than a flag variable.  Suggestions
welcome.

[akalin, 2012-09-25 22:00:55]

this looks good, although I don't think I'm super-familiar with this logic.  may
be worth getting tim to take a look also.

http://codereview.chromium.org/10964057/diff/7001/sync/engine/update_applicat...
File sync/engine/update_applicator.cc (right):

http://codereview.chromium.org/10964057/diff/7001/sync/engine/update_applicat...
sync/engine/update_applicator.cc:92: to_apply = to_reapply;
not sure what this line does

http://codereview.chromium.org/10964057/diff/7001/sync/engine/update_applicat...
File sync/engine/update_applicator.h (right):

http://codereview.chromium.org/10964057/diff/7001/sync/engine/update_applicat...
sync/engine/update_applicator.h:47: std::vector<int64> to_apply);
const ref vector?

[rlarocque, 2012-09-25 22:16:46]

http://codereview.chromium.org/10964057/diff/7001/sync/engine/update_applicat...
File sync/engine/update_applicator.cc (right):

http://codereview.chromium.org/10964057/diff/7001/sync/engine/update_applicat...
sync/engine/update_applicator.cc:92: to_apply = to_reapply;
On 2012/09/25 22:00:55, akalin wrote:
> not sure what this line does

I'm using the parameter as if it were a function-scoped variable here. 
Technically, it is.  However, one could argue this is clever and not very
C++-like.  

Maybe it would be more clear if I made the parameter a const-ref (as you
suggested earlier) and explicitly declared the local variable that I need.

[akalin, 2012-09-25 22:22:24]

http://codereview.chromium.org/10964057/diff/7001/sync/engine/update_applicat...
File sync/engine/update_applicator.cc (right):

http://codereview.chromium.org/10964057/diff/7001/sync/engine/update_applicat...
sync/engine/update_applicator.cc:92: to_apply = to_reapply;
On 2012/09/25 22:16:46, rlarocque wrote:
> On 2012/09/25 22:00:55, akalin wrote:
> > not sure what this line does
> 
> I'm using the parameter as if it were a function-scoped variable here. 
> Technically, it is.  However, one could argue this is clever and not very
> C++-like.  
> 
> Maybe it would be more clear if I made the parameter a const-ref (as you
> suggested earlier) and explicitly declared the local variable that I need.

oh, i missed that it was inside the while loop.

Yeah, I'd suggest an explicit local variable.  Then you can clear and swap.

[rlarocque, 2012-09-26 18:45:58]

On 2012/09/25 22:22:24, akalin wrote:
>
http://codereview.chromium.org/10964057/diff/7001/sync/engine/update_applicat...
> File sync/engine/update_applicator.cc (right):
> 
>
http://codereview.chromium.org/10964057/diff/7001/sync/engine/update_applicat...
> sync/engine/update_applicator.cc:92: to_apply = to_reapply;
> On 2012/09/25 22:16:46, rlarocque wrote:
> > On 2012/09/25 22:00:55, akalin wrote:
> > > not sure what this line does
> > 
> > I'm using the parameter as if it were a function-scoped variable here. 
> > Technically, it is.  However, one could argue this is clever and not very
> > C++-like.  
> > 
> > Maybe it would be more clear if I made the parameter a const-ref (as you
> > suggested earlier) and explicitly declared the local variable that I need.
> 
> oh, i missed that it was inside the while loop.
> 
> Yeah, I'd suggest an explicit local variable.  Then you can clear and swap.

Good idea.  Done.

[rlarocque, 2012-09-26 18:46:37]

+Tim as an expert on how the code currently works.  Please review.

[tim (not reviewing), 2012-09-26 21:13:14]

http://codereview.chromium.org/10964057/diff/7001/sync/engine/update_applicat...
File sync/engine/update_applicator.cc (right):

http://codereview.chromium.org/10964057/diff/7001/sync/engine/update_applicat...
sync/engine/update_applicator.cc:87: break;
On 2012/09/25 17:37:45, rlarocque wrote:
> I don't like this, but I think it's cleaner than a flag variable.  Suggestions
> welcome.

If we have unapplied updates for type T in the Directory, and then we try to run
a config job for a subset of (AllEnabledTypes - T), I believe the code in
ApplyUpdatesCommand will result in entries of type T being placed in
GROUP_PASSIVE and included as part of the sync cycle. In that case,
to_reapply.size() couldn't possibly == to_apply.size(), because we'll have
skipped some entries, but (I think) logically we'd be in the same case you're
targeting here with the 'break'?

[rlarocque, 2012-09-26 21:32:29]

http://codereview.chromium.org/10964057/diff/7001/sync/engine/update_applicat...
File sync/engine/update_applicator.cc (right):

http://codereview.chromium.org/10964057/diff/7001/sync/engine/update_applicat...
sync/engine/update_applicator.cc:87: break;
On 2012/09/26 21:13:14, timsteele wrote:
> On 2012/09/25 17:37:45, rlarocque wrote:
> > I don't like this, but I think it's cleaner than a flag variable. 
Suggestions
> > welcome.
> 
> If we have unapplied updates for type T in the Directory, and then we try to
run
> a config job for a subset of (AllEnabledTypes - T), I believe the code in
> ApplyUpdatesCommand will result in entries of type T being placed in
> GROUP_PASSIVE and included as part of the sync cycle. In that case,
> to_reapply.size() couldn't possibly == to_apply.size(), because we'll have
> skipped some entries, but (I think) logically we'd be in the same case you're
> targeting here with the 'break'?  

I don't think so, though I admit I had not anticipated the situation you
describe.

It's different in the next patch upload, but in this one it's not really obvious
that I'm modifying the value of to_apply with each loop iteration.  See the
comments on line 92.

If we skip items with SkipUpdate(), then they won't be in to_apply on the next
iteration.  Only the items we classified as hierarchy conflicts will attempt a
re-application on the following iteration.  Does that make sense?

[tim (not reviewing), 2012-09-26 21:56:25]

Ah, I missed that.  Makes sense now.  I wish we didn't have that SkipUpdate
logic :( Whatever uses UpdateApplicator should just naturally get it right. It'd
require changing how GetGroupForModelType works at the moment, hopefully we can
clear that up when we get around to removing other group_restriction_ abuses.

http://codereview.chromium.org/10964057/diff/16003/sync/engine/update_applica...
File sync/engine/update_applicator.cc (right):

http://codereview.chromium.org/10964057/diff/16003/sync/engine/update_applica...
sync/engine/update_applicator.cc:78: to_reapply.push_back(*i);
Add a comment somewhere explaining what constitutes a re_apply-able item and
what doesn't.

http://codereview.chromium.org/10964057/diff/16003/sync/engine/update_applica...
sync/engine/update_applicator.cc:87: // We made no progress.  Must be stubborn
hierarchy conflicts.
Would it be clearer to add a size-getter on application_results_'s hierarchy
conflicts and check that against to_apply?  Or as a dcheck?

[tim (not reviewing), 2012-09-26 21:58:26]

http://codereview.chromium.org/10964057/diff/16003/sync/engine/update_applica...
File sync/engine/update_applicator.cc (right):

http://codereview.chromium.org/10964057/diff/16003/sync/engine/update_applica...
sync/engine/update_applicator.cc:78: to_reapply.push_back(*i);
On 2012/09/26 21:56:25, timsteele wrote:
> Add a comment somewhere explaining what constitutes a re_apply-able item and
> what doesn't. 

Oh, I see you did this above.  I guess I expected to see it next to / near
to_reapply.  Well, food for thought.  No big deal I guess.

[rlarocque, 2012-09-26 22:24:32]

http://codereview.chromium.org/10964057/diff/16003/sync/engine/update_applica...
File sync/engine/update_applicator.cc (right):

http://codereview.chromium.org/10964057/diff/16003/sync/engine/update_applica...
sync/engine/update_applicator.cc:87: // We made no progress.  Must be stubborn
hierarchy conflicts.
On 2012/09/26 21:56:25, timsteele wrote:
> Would it be clearer to add a size-getter on application_results_'s hierarchy
> conflicts and check that against to_apply?  Or as a dcheck?

I was thinking of the application_results_'s contents as an output of this
process.  I'd rather not feed the results back in as an input if it can be
avoided.

On a more practical note, I hope to replace application_result_'s set of
hierarchy conflicts with a counter.  The only consumer of that data is the
about:sync page, and it only cares about the size of the set.  That's another
(probably better) reason to keep a local to_reapply_ set.

[tim (not reviewing), 2012-09-27 15:04:30]

LGTM (though see my earlier nit)

http://codereview.chromium.org/10964057/diff/16003/sync/engine/update_applica...
File sync/engine/update_applicator.cc (right):

http://codereview.chromium.org/10964057/diff/16003/sync/engine/update_applica...
sync/engine/update_applicator.cc:54: for (UpdateIterator i = to_apply.begin(); i
!= to_apply.end(); ++i) {
As far as I can tell, the only logical differences between this and the previous
code are 1) the previous code wound up processing items in reverse order, which
I don't think was optimizing for anything but instead making it easier to 2)
avoid re-considering successfully applied items *in-place* (rather than using
extra storage as you do here).  Assuming I have that right, this new code is way
easier to understand.  Nice job.

The old code was old -- dates back to very early days.  I suspect at that time,
space may have been more of a concern (perhaps there were no batch size
limitations), I'm not entirely sure.  Another hint to that effect is it used to
take care to check early_exit_requested_ between each call to
AttemptOneApplication (that's why it was singled-out rather than "do all" as
you've done here).  I think this is also fine given todays system.

For archaeological purposes (if you're interested), using critique, look up

11682892 (see update_applicator.cc)
4493866-p10 (see syncer.cc)
2841323-p10 (see syncer.cc) This is the initial checkin of the update
application code, which remained mostly intact (although we stopped 'swapping'
iterator values at some point).

[rlarocque, 2012-09-27 18:48:03]

I made some minor adjustments to improve readability.  Functionality remains
unchanged.  

Unless there are any objections, I'll hit the commit button later today.

[commit-bot: I haz the power, 2012-09-27 22:40:45]

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

[commit-bot: I haz the power, 2012-09-28 03:07:34]

Change committed as 159186