[Sync] Make syncer commands avoid posting tasks on threads with no work to do

[Sync] Add framework for avoid posting tasks on threads with no work to do

Add abstract GetGroupsToChange() method to ModelChangingSyncerCommand.
Use that to figure out which worker threads to post work on (instead of
posting on all of them).

Implement GetGroupsToChange() for each ModelChangingSyncerCommand.

Add GetEnabledGroups() and GetEnabledGroupsWithConflicts() functions to SyncSession.

Key unapplied updates index by type for ApplyUpdatesCommand.

Make the abstract methods of ModelChangingSyncerCommand protected.

Add HasCustomGroupsToChange() method to ModelChangingSyncerCommand and have all
commands default to returning false to track down the perf regression introduced
by the last couple of times this was landed (112178, 112815).

BUG=97832
TEST=

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

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

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

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

[akalin, 2011-11-22 05:43:34]

+tim for review

I'm not completely done writing unit tests for these, but I wanted to get
started on the review early.

The integration tests should cover most of this, right?

[tim (not reviewing), 2011-11-22 18:40:05]

http://codereview.chromium.org/8637006/diff/2001/chrome/browser/sync/engine/m...
File chrome/browser/sync/engine/model_changing_syncer_command.cc (right):

http://codereview.chromium.org/8637006/diff/2001/chrome/browser/sync/engine/m...
chrome/browser/sync/engine/model_changing_syncer_command.cc:56:
active_groups.insert(it->second);
First, GetActiveGroups belongs on the SyncSession class (it's just operating on
SS data, nothing to do with MCSC, hence the static).

Second, session.routing_info() won't change for the duration of a SyncSession,
so we should probably just cache it in a const member there and return it?

http://codereview.chromium.org/8637006/diff/2001/chrome/browser/sync/engine/m...
File chrome/browser/sync/engine/model_changing_syncer_command.h (right):

http://codereview.chromium.org/8637006/diff/2001/chrome/browser/sync/engine/m...
chrome/browser/sync/engine/model_changing_syncer_command.h:41: //
TODO(lipalani): |ModelChangingExecuteImpl| should return an
nit - remove this

http://codereview.chromium.org/8637006/diff/2001/chrome/browser/sync/engine/m...
chrome/browser/sync/engine/model_changing_syncer_command.h:77: // Returns the
set of groups that have active (i.e., enabled) types.
Can we avoid introducing a new adjective if we can avoid it :)  If it's "i.e.
enabled", lets use enabled?

[akalin, 2011-11-23 03:25:18]

Fixed all comments, PTAL.

Also added a bunch of unit tests.  I think we have good coverage now.

http://codereview.chromium.org/8637006/diff/2001/chrome/browser/sync/engine/m...
File chrome/browser/sync/engine/model_changing_syncer_command.cc (right):

http://codereview.chromium.org/8637006/diff/2001/chrome/browser/sync/engine/m...
chrome/browser/sync/engine/model_changing_syncer_command.cc:56:
active_groups.insert(it->second);
On 2011/11/22 18:40:05, timsteele wrote:
> First, GetActiveGroups belongs on the SyncSession class (it's just operating
on
> SS data, nothing to do with MCSC, hence the static).
> 
> Second, session.routing_info() won't change for the duration of a SyncSession,
> so we should probably just cache it in a const member there and return it?

Done.

http://codereview.chromium.org/8637006/diff/2001/chrome/browser/sync/engine/m...
File chrome/browser/sync/engine/model_changing_syncer_command.h (right):

http://codereview.chromium.org/8637006/diff/2001/chrome/browser/sync/engine/m...
chrome/browser/sync/engine/model_changing_syncer_command.h:41: //
TODO(lipalani): |ModelChangingExecuteImpl| should return an
On 2011/11/22 18:40:05, timsteele wrote:
> nit - remove this

Eh, why?  Doesn't it still hold?

http://codereview.chromium.org/8637006/diff/2001/chrome/browser/sync/engine/m...
chrome/browser/sync/engine/model_changing_syncer_command.h:77: // Returns the
set of groups that have active (i.e., enabled) types.
On 2011/11/22 18:40:05, timsteele wrote:
> Can we avoid introducing a new adjective if we can avoid it :)  If it's "i.e.
> enabled", lets use enabled?

Done.

[commit-bot: I haz the power, 2011-11-23 04:53:44]

No LGTM from valid reviewers yet.

[tim (not reviewing), 2011-11-29 00:01:59]

http://codereview.chromium.org/8637006/diff/2001/chrome/browser/sync/engine/m...
File chrome/browser/sync/engine/model_changing_syncer_command.h (right):

http://codereview.chromium.org/8637006/diff/2001/chrome/browser/sync/engine/m...
chrome/browser/sync/engine/model_changing_syncer_command.h:41: //
TODO(lipalani): |ModelChangingExecuteImpl| should return an
On 2011/11/23 03:25:18, akalin wrote:
> On 2011/11/22 18:40:05, timsteele wrote:
> > nit - remove this
> 
> Eh, why?  Doesn't it still hold?

My bad. Nevermind.

http://codereview.chromium.org/8637006/diff/6001/chrome/browser/sync/engine/a...
File chrome/browser/sync/engine/apply_updates_command.cc (right):

http://codereview.chromium.org/8637006/diff/6001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command.cc:33: for
(syncable::Directory::UnappliedUpdateMetaHandles::const_iterator it =
Might it make sense to keep an index on this in Directory?  Seems like it'd be
pretty cheap / easy.

http://codereview.chromium.org/8637006/diff/6001/chrome/browser/sync/engine/p...
File chrome/browser/sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/8637006/diff/6001/chrome/browser/sync/engine/p...
chrome/browser/sync/engine/process_updates_command.cc:47: return
groups_with_verified_updates;
again, maybe just keep an index on this when we add to VerifiedUpdates?  Seems
like it would be less code, less work, and more isolated...

http://codereview.chromium.org/8637006/diff/6001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/sync_session.cc (right):

http://codereview.chromium.org/8637006/diff/6001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session.cc:31: if (logging::DEBUG_MODE) {
I haven't seen this get used before... is this just a wrapper around ifndef
NDEBUG?

[akalin, 2011-11-30 01:33:03]

Addressed all comments.  PTAL.

http://codereview.chromium.org/8637006/diff/6001/chrome/browser/sync/engine/a...
File chrome/browser/sync/engine/apply_updates_command.cc (right):

http://codereview.chromium.org/8637006/diff/6001/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/apply_updates_command.cc:33: for
(syncable::Directory::UnappliedUpdateMetaHandles::const_iterator it =
On 2011/11/29 00:01:59, timsteele wrote:
> Might it make sense to keep an index on this in Directory?  Seems like it'd be
> pretty cheap / easy.

Done.  Not quite trivial, but doable.  I basically split out the
unapplied_updates index by type and used that.

http://codereview.chromium.org/8637006/diff/6001/chrome/browser/sync/engine/p...
File chrome/browser/sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/8637006/diff/6001/chrome/browser/sync/engine/p...
chrome/browser/sync/engine/process_updates_command.cc:47: return
groups_with_verified_updates;
On 2011/11/29 00:01:59, timsteele wrote:
> again, maybe just keep an index on this when we add to VerifiedUpdates?  Seems
> like it would be less code, less work, and more isolated...

I don't know if I agree with this.  Unlike the unapplied updates case, this is
just looping over a small (max 6 elements) set, and then looking up and
comparing a bunch of pointers.  I don't think it's less code or less work to
keep an index.

As for more isolated, I could move this code into SyncSession if you want. 
Similar code is already in there, with GetEnabledGroupsWithConflicts().  I just
didn't move this one because it's only used once.

http://codereview.chromium.org/8637006/diff/6001/chrome/browser/sync/sessions...
File chrome/browser/sync/sessions/sync_session.cc (right):

http://codereview.chromium.org/8637006/diff/6001/chrome/browser/sync/sessions...
chrome/browser/sync/sessions/sync_session.cc:31: if (logging::DEBUG_MODE) {
On 2011/11/29 00:01:59, timsteele wrote:
> I haven't seen this get used before... is this just a wrapper around ifndef
> NDEBUG?

Yeah pretty much.  Although I should have used DCHECK_IS_ON().  Fixed.

[tim (not reviewing), 2011-11-30 02:21:23]

LGTM

http://codereview.chromium.org/8637006/diff/6001/chrome/browser/sync/engine/p...
File chrome/browser/sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/8637006/diff/6001/chrome/browser/sync/engine/p...
chrome/browser/sync/engine/process_updates_command.cc:47: return
groups_with_verified_updates;
On 2011/11/30 01:33:03, akalin wrote:
> On 2011/11/29 00:01:59, timsteele wrote:
> > again, maybe just keep an index on this when we add to VerifiedUpdates? 
Seems
> > like it would be less code, less work, and more isolated...
> 
> I don't know if I agree with this.  Unlike the unapplied updates case, this is
> just looping over a small (max 6 elements) set, and then looking up and
> comparing a bunch of pointers.  I don't think it's less code or less work to
> keep an index.
> 
> As for more isolated, I could move this code into SyncSession if you want. 
> Similar code is already in there, with GetEnabledGroupsWithConflicts().  I
just
> didn't move this one because it's only used once.

OK, yeah lets do that.

[akalin, 2011-11-30 04:18:44]

> OK, yeah lets do that.

Done.

Checking in as soon as trybots pass.

[commit-bot: I haz the power, 2011-11-30 08:03:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/8637006/27001

[commit-bot: I haz the power, 2011-11-30 09:45:28]

Change committed as 112178

[akalin, 2011-12-02 18:16:43]

I think I figured out what was causing the perf regression, but not why.  I
reverted ProcessUpdatesCommand to use GetEnabledGroups() only for now, and will
see if that fixes the perf regression.  If so, I'll dig into why.

[akalin, 2011-12-03 02:14:40]

http://build.chromium.org/f/chromium/perf/mac-release-10.6/sync/report.html?h...

Hmm, looks like my change doesn't regress autofill anymore, but it still
regresses typed_urls.deletes.  That's weird, because I used the latter to bisect
this speculative fix.

I'll leave it running a bit more to see if autofill stays stable, but I'll
probably revert this again over the weekend.  Sigh.  At this point I might try
splitting up the CL into pieces to get a better bisection.

[akalin, 2011-12-05 19:15:27]

On 2011/12/03 02:14:40, akalin wrote:
>
http://build.chromium.org/f/chromium/perf/mac-release-10.6/sync/report.html?h...
> 
> Hmm, looks like my change doesn't regress autofill anymore, but it still
> regresses typed_urls.deletes.  That's weird, because I used the latter to
bisect
> this speculative fix.
> 
> I'll leave it running a bit more to see if autofill stays stable, but I'll
> probably revert this again over the weekend.  Sigh.  At this point I might try
> splitting up the CL into pieces to get a better bisection.

Reverted it on friday.  New plan: Make all syncer commands use
GetEnabledGroups() (i.e., stick with the old behavior) and send out CLs to
enable them one by one, watching the perf after each one lands.

[akalin, 2011-12-05 19:19:58]

On 2011/12/05 19:15:27, akalin wrote:
> On 2011/12/03 02:14:40, akalin wrote:
> >
>
http://build.chromium.org/f/chromium/perf/mac-release-10.6/sync/report.html?h...
> > 
> > Hmm, looks like my change doesn't regress autofill anymore, but it still
> > regresses typed_urls.deletes.  That's weird, because I used the latter to
> bisect
> > this speculative fix.
> > 
> > I'll leave it running a bit more to see if autofill stays stable, but I'll
> > probably revert this again over the weekend.  Sigh.  At this point I might
try
> > splitting up the CL into pieces to get a better bisection.
> 
> Reverted it on friday.  New plan: Make all syncer commands use
> GetEnabledGroups() (i.e., stick with the old behavior) and send out CLs to
> enable them one by one, watching the perf after each one lands.

Never mind, that seems like a pain.  I will do another bisect and see if
anything else falls out first.

[commit-bot: I haz the power, 2011-12-05 22:28:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/8637006/42002

[commit-bot: I haz the power, 2011-12-05 22:34:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/8637006/41006

[commit-bot: I haz the power, 2011-12-05 22:34:15]

Can't process patch for file
chrome/browser/sync/engine/model_changing_syncer_command.cc.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2011-12-05 22:39:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/8637006/42002

[commit-bot: I haz the power, 2011-12-05 23:48:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/8637006/44007