For sync exponential backoff, allow one nudge per exponential backoff interva...

For sync exponential backoff, allow one nudge per exponential backoff interval. If the nudge
still leaves the syncer with more work to do, don't accept any further nudges for this interval,
and keep the exponent stage for exponential backoff the same.

As a result of the patch, the unittest can now explicitly determine if exponential backoff
kicked in or not.

We really need to wire this up directly to the error codes to be precise, because the current impl (and
it's the same with my patch) appears to trigger exponential backoff in other cases (ShouldSyncAgain).

TEST=SyncerThreadTest


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

[brg, 2009-10-14 18:05:29]

http://codereview.chromium.org/275015/diff/3001/4002
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/275015/diff/3001/4002#newcode139
Line 139: }
May as well lock.  This read is concurrent with the syncer thread.

http://codereview.chromium.org/275015/diff/3001/4002#newcode380
Line 380: vault_.current_wait_interval_ = CalculatePollingWaitTime(
This write should be locked.

http://codereview.chromium.org/275015/diff/3001/4002#newcode396
Line 396: lock_.AssertAcquired();  // We access 'vault' in here, so we need the
lock.
Chrome has locking classes that release the lock in their dtor.  It is better to
use these auto-locking classes than to manually lock.

And I do no see an unlock here.  But I can not at the moment look through the
calling graph for lock_.

http://codereview.chromium.org/275015/diff/3001/4003
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/275015/diff/3001/4003#newcode79
Line 79: // to occur.
"Encapsulates the parameters that make up an interval on which the syncer thread
is to sleep."?

http://codereview.chromium.org/275015/diff/3001/4003#newcode90
Line 90: EXPONENTIAL_BACKOFF,
Why an enum when a bool would do?  Do you expect future additions?

http://codereview.chromium.org/275015/diff/3001/4003#newcode94
Line 94: bool had_nudge_during_backoff;  // Used to nudge-rate limit the
interval.
The comment here is poorly worded.

http://codereview.chromium.org/275015/diff/3001/4001
File chrome/browser/sync/engine/syncer_thread_unittest.cc (right):

http://codereview.chromium.org/275015/diff/3001/4001#newcode25
Line 25: typedef SyncerThread::WaitInterval WaitInterval;
using?

http://codereview.chromium.org/275015/diff/3001/4001#newcode445
Line 445: ASSERT_TRUE(continue_sync_cycle_param);
Good test.

[tim (not reviewing), 2009-10-14 18:41:00]

http://codereview.chromium.org/275015/diff/3001/4002
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/275015/diff/3001/4002#newcode139
Line 139: }
On 2009/10/14 18:05:29, brg wrote:
> May as well lock.  This read is concurrent with the syncer thread.

It is locked (see line 130).

http://codereview.chromium.org/275015/diff/3001/4002#newcode380
Line 380: vault_.current_wait_interval_ = CalculatePollingWaitTime(
On 2009/10/14 18:05:29, brg wrote:
> This write should be locked.

It is.  This method uses vault_ because it is called with the lock acquired (see
the debug assert at line 309).  The condition variable Wait gives it up
temporarily but re-acquires the lock when signaled.

http://codereview.chromium.org/275015/diff/3001/4002#newcode396
Line 396: lock_.AssertAcquired();  // We access 'vault' in here, so we need the
lock.
On 2009/10/14 18:05:29, brg wrote:
> Chrome has locking classes that release the lock in their dtor.  It is better
to
> use these auto-locking classes than to manually lock.
> 
> And I do no see an unlock here.  But I can not at the moment look through the
> calling graph for lock_.

:) This isn't a call to 'Acquire', it is a debug assert that the lock is held. 
See lock.h AssertAcquired

http://codereview.chromium.org/275015/diff/3001/4003
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/275015/diff/3001/4003#newcode79
Line 79: // to occur.
On 2009/10/14 18:05:29, brg wrote:
> "Encapsulates the parameters that make up an interval on which the syncer
thread
> is to sleep."?

Done.

http://codereview.chromium.org/275015/diff/3001/4003#newcode90
Line 90: EXPONENTIAL_BACKOFF,
On 2009/10/14 18:05:29, brg wrote:
> Why an enum when a bool would do?  Do you expect future additions?

Perhaps (I had 3 to start).  It honestly simplified reasoning about the code
(and makes the tests look nice) to have explicit names for the states, because
they really _are_ states.

http://codereview.chromium.org/275015/diff/3001/4003#newcode94
Line 94: bool had_nudge_during_backoff;  // Used to nudge-rate limit the
interval.
On 2009/10/14 18:05:29, brg wrote:
> The comment here is poorly worded.
Agree, changed.

http://codereview.chromium.org/275015/diff/3001/4001
File chrome/browser/sync/engine/syncer_thread_unittest.cc (right):

http://codereview.chromium.org/275015/diff/3001/4001#newcode25
Line 25: typedef SyncerThread::WaitInterval WaitInterval;
On 2009/10/14 18:05:29, brg wrote:
> using?

hm, didn't work for me (it's a struct, not an enum).

http://codereview.chromium.org/275015/diff/3001/4001#newcode445
Line 445: ASSERT_TRUE(continue_sync_cycle_param);
On 2009/10/14 18:05:29, brg wrote:
> Good test.

Thanks! I hope it works :)

[tim (not reviewing), 2009-10-14 19:59:13]

http://codereview.chromium.org/275015/diff/3001/4003
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/275015/diff/3001/4003#newcode90
Line 90: EXPONENTIAL_BACKOFF,
On 2009/10/14 18:41:00, timsteele wrote:
> On 2009/10/14 18:05:29, brg wrote:
> > Why an enum when a bool would do?  Do you expect future additions?
> 
> Perhaps (I had 3 to start).  It honestly simplified reasoning about the code
> (and makes the tests look nice) to have explicit names for the states, because
> they really _are_ states.
> 

FWIW, I think in a follow up CL it looks like I will need to add a 'THROTTLED'
state.

[brg, 2009-10-14 21:44:50]

LGTM

The two comments are things that should be considered, but do not block the CL.

Please triple check the locks/unlocks.

http://codereview.chromium.org/275015/diff/2002/1005
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/275015/diff/2002/1005#newcode138
Line 138: return false;
This may be out of scope of the current CL, but why is NudgeSyncer returning a
bool?  I do not think it is being used.

http://codereview.chromium.org/275015/diff/2002/1004
File chrome/browser/sync/engine/syncer_thread_unittest.cc (right):

http://codereview.chromium.org/275015/diff/2002/1004#newcode255
Line 255: ASSERT_TRUE(0 <= interval.poll_delta.InSeconds());
These sort of checks against constants would be better served as ASSERT_LE and
ASSERT_GE's, if for no other reason than to silence the lint.

[tim (not reviewing), 2009-10-14 22:42:23]

http://codereview.chromium.org/275015/diff/2002/1005
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/275015/diff/2002/1005#newcode138
Line 138: return false;
On 2009/10/14 21:44:50, brg wrote:
> This may be out of scope of the current CL, but why is NudgeSyncer returning a
> bool?  I do not think it is being used.
You're right, fixed.

http://codereview.chromium.org/275015/diff/2002/1004
File chrome/browser/sync/engine/syncer_thread_unittest.cc (right):

http://codereview.chromium.org/275015/diff/2002/1004#newcode255
Line 255: ASSERT_TRUE(0 <= interval.poll_delta.InSeconds());
On 2009/10/14 21:44:50, brg wrote:
> These sort of checks against constants would be better served as ASSERT_LE and
> ASSERT_GE's, if for no other reason than to silence the lint.

And for meaningful test failure output.  The ones that compare TimeDeltas aren't
compatible with the macros, but I changed the rest that compare integers to use
LE/GE/EQ.

[tim (not reviewing), 2009-10-14 22:44:59]

Oh, and I did another pass on the locking, and overrode instructions in the
debugger to trigger certain code paths.

Will commit shortly!