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

chrome/browser/sync/engine/process_updates_command.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/engine/process_updates_command.h"
 
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/location.h"
@@ skipping 27 matching lines @@
 void ProcessUpdatesCommand::ModelChangingExecuteImpl(SyncSession* session) {
   syncable::ScopedDirLookup dir(session->context()->directory_manager(),
                                 session->context()->account_name());
   if (!dir.good()) {
     LOG(ERROR) << "Scoped dir lookup failed!";
     return;
   }
 
   StatusController* status = session->status_controller();
 
+  syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER, dir);
   const sessions::UpdateProgress& progress(status->update_progress());
   vector<sessions::VerifiedUpdate>::const_iterator it;
   for (it = progress.VerifiedUpdatesBegin();
        it != progress.VerifiedUpdatesEnd();
        ++it) {
     const sync_pb::SyncEntity& update = it->second;
 
     if (it->first != VERIFY_SUCCESS && it->first != VERIFY_UNDELETE)
       continue;
-    switch (ProcessUpdate(dir, update)) {
+    switch (ProcessUpdate(dir, update, &trans)) {
       case SUCCESS_PROCESSED:
       case SUCCESS_STORED:
         break;
       default:
         NOTREACHED();
         break;
     }
   }
 
   status->set_num_consecutive_errors(0);
@@ skipping 16 matching lines @@
                                                                same_id,
                                                                deleted,
                                                                is_directory,
                                                                model_type);
 }
 }  // namespace
 
 // Process a single update. Will avoid touching global state.
 ServerUpdateProcessingResult ProcessUpdatesCommand::ProcessUpdate(
     const syncable::ScopedDirLookup& dir,
-    const sync_pb::SyncEntity& proto_update) {
+    const sync_pb::SyncEntity& proto_update,
+    syncable::WriteTransaction* const trans) {

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

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.

 
   const SyncEntity& update = *static_cast<const SyncEntity*>(&proto_update);
   syncable::Id server_id = update.id();
   const std::string name = SyncerProtoUtil::NameFromSyncEntity(update);
 
-  syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER, dir);
-
   // Look to see if there's a local item that should recieve this update,
   // maybe due to a duplicate client tag or a lost commit response.
-  syncable::Id local_id = SyncerUtil::FindLocalIdToUpdate(&trans, update);
+  syncable::Id local_id = SyncerUtil::FindLocalIdToUpdate(trans, update);
 
   // FindLocalEntryToUpdate has veto power.
   if (local_id.IsNull()) {
     return SUCCESS_PROCESSED;  // The entry has become irrelevant.
   }
 
-  SyncerUtil::CreateNewEntry(&trans, local_id);
+  SyncerUtil::CreateNewEntry(trans, local_id);
 
   // We take a two step approach. First we store the entries data in the
   // server fields of a local entry and then move the data to the local fields
-  syncable::MutableEntry target_entry(&trans, syncable::GET_BY_ID, local_id);
+  syncable::MutableEntry target_entry(trans, syncable::GET_BY_ID, local_id);
 
   // We need to run the Verify checks again; the world could have changed
   // since VerifyUpdatesCommand.
-  if (!ReverifyEntry(&trans, update, &target_entry)) {
+  if (!ReverifyEntry(trans, update, &target_entry)) {
     return SUCCESS_PROCESSED;  // The entry has become irrelevant.
   }
 
   // If we're repurposing an existing local entry with a new server ID,
   // change the ID now, after we're sure that the update can succeed.
   if (local_id != server_id) {
     DCHECK(!update.deleted());
-    SyncerUtil::ChangeEntryIDAndUpdateChildren(&trans, &target_entry,
+    SyncerUtil::ChangeEntryIDAndUpdateChildren(trans, &target_entry,
         server_id);
     // When IDs change, versions become irrelevant.  Forcing BASE_VERSION
     // to zero would ensure that this update gets applied, but would indicate
     // creation or undeletion if it were committed that way.  Instead, prefer
     // forcing BASE_VERSION to entry.version() while also forcing
     // IS_UNAPPLIED_UPDATE to true.  If the item is UNSYNCED, it's committable
     // from the new state; it may commit before the conflict resolver gets
     // a crack at it.
     if (target_entry.Get(syncable::IS_UNSYNCED) ||
         target_entry.Get(syncable::BASE_VERSION) > 0) {
@@ skipping 17 matching lines @@
     // If these don't match, it means that we have a different view of the
     // truth from other clients.  That's a sync bug, though we may be able
     // to recover the next time this item commits.
     LOG_IF(ERROR, !SyncerUtil::ServerAndLocalEntriesMatch(&target_entry))
         << target_entry;
   }
   return SUCCESS_PROCESSED;
 }
 
 }  // namespace browser_sync