sync: Use a single transaction in ProcessUpdates rather than one per entry.

sync: Use a single transaction in ProcessUpdates rather than one per entry. 

BUG=104203
TEST=compare 'Entries' count on about:sync to number of SyncThread>SyncThread Notify tasks in tracker2/profiler.  The entries count should be much smaller.


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

[rlarocque, 2011-11-15 00:52:23]

Hooray!  This should lead to a drastic reduction in the number or lock
acquisitions done by sync.

LGTM after comment.

http://codereview.chromium.org/8533024/diff/1/chrome/browser/sync/engine/proc...
File chrome/browser/sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/8533024/diff/1/chrome/browser/sync/engine/proc...
chrome/browser/sync/engine/process_updates_command.cc:96:
syncable::WriteTransaction* const trans) {
Was there a reason you decided to use a pointer here instead of a reference?

Since the function seems to assume that it's non-NULL, I'd prefer to see a
reference here.

[tim (not reviewing), 2011-11-15 01:14:19]

On 2011/11/15 00:52:23, rlarocque wrote:
> Hooray!  This should lead to a drastic reduction in the number or lock
> acquisitions done by sync.
> 
> LGTM after comment.
> 
>
http://codereview.chromium.org/8533024/diff/1/chrome/browser/sync/engine/proc...
> File chrome/browser/sync/engine/process_updates_command.cc (right):
> 
>
http://codereview.chromium.org/8533024/diff/1/chrome/browser/sync/engine/proc...
> chrome/browser/sync/engine/process_updates_command.cc:96:
> syncable::WriteTransaction* const trans) {
> Was there a reason you decided to use a pointer here instead of a reference?

I agree with your sentiment, but yeah, in this case the reason was that all of
the other functions we call from ProcessUpdate (bunch of stuff in SyncerUtil)
expect a pointer...
> 
> Since the function seems to assume that it's non-NULL, I'd prefer to see a
> reference here.

[commit-bot: I haz the power, 2011-11-15 17:58:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/8533024/1

[commit-bot: I haz the power, 2011-11-15 19:15:04]

Change committed as 110138

[akalin, 2011-11-15 19:33:28]

On 2011/11/15 19:15:04, I haz the power (commit-bot) wrote:
> Change committed as 110138

Just one question.  Would this still be desirable without the
ThreadSafeObserverList problem?  Reducing lock overhead is nice, but the
tradeoff is that we hold the lock for longer periods.  Won't that lead to jank? 
Or is it okay because there's no IO involved?

And shouldn't it be "the entries count should be much larger" in the TEST
description? (although it's too late now)

[rlarocque, 2011-11-15 20:28:06]

On 2011/11/15 19:33:28, akalin wrote:
> On 2011/11/15 19:15:04, I haz the power (commit-bot) wrote:
> > Change committed as 110138
> 
> Just one question.  Would this still be desirable without the
> ThreadSafeObserverList problem?  Reducing lock overhead is nice, but the
> tradeoff is that we hold the lock for longer periods.  Won't that lead to
jank? 
> Or is it okay because there's no IO involved?

That's a question only profiling can answer.  I'm working on instrumenting the
sync code to give us more visibility into the trade-offs involved.  

I did some naive calculations based on traces of the current code.  Your concern
may be justified, but it all depends on how much of the current costs of a
ProcessUpdates transaction is due to overhead and how much is actually spent
processing updates.  I'll get back to you when I've taken a look at the numbers
after Tim's patch.  

There's a lot of low-hanging fruit in the directory (ie. ScopedKernelLock, the
std::set<std::string>-based node lookups, etc.).  If it's actually taking 10s of
microseconds to perform the update, then we should optimize that code.  There's
no reason for it to be so expensive.  

> 
> And shouldn't it be "the entries count should be much larger" in the TEST
> description? (although it's too late now)