Revert of Sync: MutableEntry shouldn't track changes that have no actual impact on the stored data (patchset #1 id:20001 of https://codereview.chromium.org/1237493002/)

Reason for revert:
Causes crash [19596:1287:0716/172955:FATAL:mutable_entry.cc(260)] Check failed: kernel_->is_dirty().

in Builder Mac10.9 Tests (dbg)

https://sheriff-o-matic.appspot.com/chromium/failure/sync_integration_tests%20on%20Mac-10.9%3A%3AMac10.9%20Tests%20(dbg)

Original issue's description:
> Sync: MutableEntry shouldn't track changes that have no actual impact on the stored data
>
> This change optimizes MutableEntry and ModelNeutralMutableEntry
> methods that update sync data. Most of these methods compare
> new and old values and skip the update when the values match.
> However most of these methods still unconditionally register EntryKernel
> to be tracked in EntryKernelMutation list by making a TrackChangesTo
> call, which immediately makes a deep copy of EntryKernel.
> Then there is one more deep copy at the end of the
> transaction scope.
>
> This fix avoids the two unnecessary deep copies by calling
> TrackChangesTo only when necessary e.g. when the new value
> is actually different.
>
> In addition I used this as an opportunity to do a small code cleanup around
> marking MutableEntry instances as dirty.
>
> BUG=468144
>
> Committed: https://crrev.com/193873c7e382f11bf09b6739d07ac82e9dd14fd7
> Cr-Commit-Position: refs/heads/master@{#339162}

TBR=pavely@chromium.org,stanisc@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=468144

Review URL: https://codereview.chromium.org/1237843003

Cr-Commit-Position: refs/heads/master@{#339192}

3 files changed, 66 insertions, 64 deletions

diff --git a/sync/syncable/model_neutral_mutable_entry.cc b/sync/syncable/model_neutral_mutable_entry.cc
index 498ba9b..bdf5299 100644
--- a/sync/syncable/model_neutral_mutable_entry.cc
+++ b/sync/syncable/model_neutral_mutable_entry.cc
@@ -105,65 +105,66 @@ ModelNeutralMutableEntry::ModelNeutralMutableEntry(
 
 void ModelNeutralMutableEntry::PutBaseVersion(int64 value) {
   DCHECK(kernel_);
+  base_write_transaction_->TrackChangesTo(kernel_);
   if (kernel_->ref(BASE_VERSION) != value) {
-    base_write_transaction_->TrackChangesTo(kernel_);
     kernel_->put(BASE_VERSION, value);
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
 void ModelNeutralMutableEntry::PutServerVersion(int64 value) {
   DCHECK(kernel_);
+  base_write_transaction_->TrackChangesTo(kernel_);
   if (kernel_->ref(SERVER_VERSION) != value) {
-    base_write_transaction_->TrackChangesTo(kernel_);
     ScopedKernelLock lock(dir());
     kernel_->put(SERVER_VERSION, value);
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
 void ModelNeutralMutableEntry::PutServerMtime(base::Time value) {
   DCHECK(kernel_);
+  base_write_transaction_->TrackChangesTo(kernel_);
   if (kernel_->ref(SERVER_MTIME) != value) {
-    base_write_transaction_->TrackChangesTo(kernel_);
     kernel_->put(SERVER_MTIME, value);
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
 void ModelNeutralMutableEntry::PutServerCtime(base::Time value) {
   DCHECK(kernel_);
+  base_write_transaction_->TrackChangesTo(kernel_);
   if (kernel_->ref(SERVER_CTIME) != value) {
-    base_write_transaction_->TrackChangesTo(kernel_);
     kernel_->put(SERVER_CTIME, value);
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
 bool ModelNeutralMutableEntry::PutId(const Id& value) {
   DCHECK(kernel_);
+  base_write_transaction_->TrackChangesTo(kernel_);
   if (kernel_->ref(ID) != value) {
-    base_write_transaction_->TrackChangesTo(kernel_);
     if (!dir()->ReindexId(base_write_transaction(), kernel_, value))
       return false;
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
   return true;
 }
 
 void ModelNeutralMutableEntry::PutServerParentId(const Id& value) {
   DCHECK(kernel_);
+  base_write_transaction_->TrackChangesTo(kernel_);
+
   if (kernel_->ref(SERVER_PARENT_ID) != value) {
-    base_write_transaction_->TrackChangesTo(kernel_);
     kernel_->put(SERVER_PARENT_ID, value);
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
 bool ModelNeutralMutableEntry::PutIsUnsynced(bool value) {
   DCHECK(kernel_);
-  if (kernel_->ref(IS_UNSYNCED) != value) {
   base_write_transaction_->TrackChangesTo(kernel_);
+  if (kernel_->ref(IS_UNSYNCED) != value) {
     MetahandleSet* index = &dir()->kernel()->unsynced_metahandles;
 
     ScopedKernelLock lock(dir());
@@ -183,15 +184,15 @@ bool ModelNeutralMutableEntry::PutIsUnsynced(bool value) {
       }
     }
     kernel_->put(IS_UNSYNCED, value);
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
   return true;
 }
 
 bool ModelNeutralMutableEntry::PutIsUnappliedUpdate(bool value) {
   DCHECK(kernel_);
+  base_write_transaction_->TrackChangesTo(kernel_);
   if (kernel_->ref(IS_UNAPPLIED_UPDATE) != value) {
-    base_write_transaction_->TrackChangesTo(kernel_);
     // Use kernel_->GetServerModelType() instead of
     // GetServerModelType() as we may trigger some DCHECKs in the
     // latter.
@@ -215,27 +216,28 @@ bool ModelNeutralMutableEntry::PutIsUnappliedUpdate(bool value) {
       }
     }
     kernel_->put(IS_UNAPPLIED_UPDATE, value);
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
   return true;
 }
 
 void ModelNeutralMutableEntry::PutServerIsDir(bool value) {
   DCHECK(kernel_);
-  if (kernel_->ref(SERVER_IS_DIR) != value) {
-    base_write_transaction_->TrackChangesTo(kernel_);
+  base_write_transaction_->TrackChangesTo(kernel_);
+  bool old_value = kernel_->ref(SERVER_IS_DIR);
+  if (old_value != value) {
     kernel_->put(SERVER_IS_DIR, value);
-    MarkDirty();
+    kernel_->mark_dirty(GetDirtyIndexHelper());
   }
 }
 
 void ModelNeutralMutableEntry::PutServerIsDel(bool value) {
   DCHECK(kernel_);
+  base_write_transaction_->TrackChangesTo(kernel_);
   bool old_value = kernel_->ref(SERVER_IS_DEL);
   if (old_value != value) {
-    base_write_transaction_->TrackChangesTo(kernel_);
     kernel_->put(SERVER_IS_DEL, value);
-    MarkDirty();
+    kernel_->mark_dirty(GetDirtyIndexHelper());
   }
 
   if (!value || kernel_->ref(IS_UNAPPLIED_UPDATE)) {
@@ -257,10 +259,11 @@ void ModelNeutralMutableEntry::PutServerIsDel(bool value) {
 void ModelNeutralMutableEntry::PutServerNonUniqueName(
     const std::string& value) {
   DCHECK(kernel_);
+  base_write_transaction_->TrackChangesTo(kernel_);
+
   if (kernel_->ref(SERVER_NON_UNIQUE_NAME) != value) {
-    base_write_transaction_->TrackChangesTo(kernel_);
     kernel_->put(SERVER_NON_UNIQUE_NAME, value);
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
@@ -280,7 +283,7 @@ bool ModelNeutralMutableEntry::PutUniqueServerTag(const string& new_tag) {
   dir()->kernel()->server_tags_map.erase(
       kernel_->ref(UNIQUE_SERVER_TAG));
   kernel_->put(UNIQUE_SERVER_TAG, new_tag);
-  MarkDirty();
+  kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   if (!new_tag.empty()) {
     dir()->kernel()->server_tags_map[new_tag] = kernel_;
   }
@@ -304,7 +307,7 @@ bool ModelNeutralMutableEntry::PutUniqueClientTag(const string& new_tag) {
   dir()->kernel()->client_tags_map.erase(
       kernel_->ref(UNIQUE_CLIENT_TAG));
   kernel_->put(UNIQUE_CLIENT_TAG, new_tag);
-  MarkDirty();
+  kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   if (!new_tag.empty()) {
     dir()->kernel()->client_tags_map[new_tag] = kernel_;
   }
@@ -320,8 +323,6 @@ void ModelNeutralMutableEntry::PutUniqueBookmarkTag(const std::string& tag) {
     return;
   }
 
-  //TODO(stanisc): Does this need a call to TrackChangesTo?
-
   if (!kernel_->ref(UNIQUE_BOOKMARK_TAG).empty() &&
       tag != kernel_->ref(UNIQUE_BOOKMARK_TAG)) {
     // There is only one scenario where our tag is expected to change.  That
@@ -334,18 +335,18 @@ void ModelNeutralMutableEntry::PutUniqueBookmarkTag(const std::string& tag) {
   }
 
   kernel_->put(UNIQUE_BOOKMARK_TAG, tag);
-  MarkDirty();
+  kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
 }
 
 void ModelNeutralMutableEntry::PutServerSpecifics(
     const sync_pb::EntitySpecifics& value) {
   DCHECK(kernel_);
   CHECK(!value.password().has_client_only_encrypted_data());
+  base_write_transaction_->TrackChangesTo(kernel_);
   // TODO(ncarter): This is unfortunately heavyweight.  Can we do
   // better?
   const std::string& serialized_value = value.SerializeAsString();
   if (serialized_value != kernel_->ref(SERVER_SPECIFICS).SerializeAsString()) {
-    base_write_transaction_->TrackChangesTo(kernel_);
     if (kernel_->ref(IS_UNAPPLIED_UPDATE)) {
       // Remove ourselves from unapplied_update_metahandles with our
       // old server type.
@@ -364,7 +365,7 @@ void ModelNeutralMutableEntry::PutServerSpecifics(
     } else {
       kernel_->put(SERVER_SPECIFICS, value);
     }
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
 
     if (kernel_->ref(IS_UNAPPLIED_UPDATE)) {
       // Add ourselves back into unapplied_update_metahandles with our
@@ -381,12 +382,12 @@ void ModelNeutralMutableEntry::PutBaseServerSpecifics(
     const sync_pb::EntitySpecifics& value) {
   DCHECK(kernel_);
   CHECK(!value.password().has_client_only_encrypted_data());
+  base_write_transaction_->TrackChangesTo(kernel_);
   // TODO(ncarter): This is unfortunately heavyweight.  Can we do
   // better?
   const std::string& serialized_value = value.SerializeAsString();
   if (serialized_value !=
       kernel_->ref(BASE_SERVER_SPECIFICS).SerializeAsString()) {
-    base_write_transaction_->TrackChangesTo(kernel_);
     // Check for potential sharing - BASE_SERVER_SPECIFICS is often
     // copied from SERVER_SPECIFICS.
     if (serialized_value ==
@@ -395,30 +396,30 @@ void ModelNeutralMutableEntry::PutBaseServerSpecifics(
     } else {
       kernel_->put(BASE_SERVER_SPECIFICS, value);
     }
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
 void ModelNeutralMutableEntry::PutServerUniquePosition(
     const UniquePosition& value) {
   DCHECK(kernel_);
+  base_write_transaction_->TrackChangesTo(kernel_);
   if(!kernel_->ref(SERVER_UNIQUE_POSITION).Equals(value)) {
-    base_write_transaction_->TrackChangesTo(kernel_);
     // We should never overwrite a valid position with an invalid one.
     DCHECK(value.IsValid());
     ScopedKernelLock lock(dir());
     kernel_->put(SERVER_UNIQUE_POSITION, value);
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
 void ModelNeutralMutableEntry::PutServerAttachmentMetadata(
     const sync_pb::AttachmentMetadata& value) {
   DCHECK(kernel_);
+  base_write_transaction_->TrackChangesTo(kernel_);
   const std::string& serialized_value = value.SerializeAsString();
   if (serialized_value !=
       kernel_->ref(SERVER_ATTACHMENT_METADATA).SerializeAsString()) {
-    base_write_transaction_->TrackChangesTo(kernel_);
     // Check for potential sharing - SERVER_ATTACHMENT_METADATA is often
     // copied from ATTACHMENT_METADATA.
     if (serialized_value ==
@@ -427,7 +428,7 @@ void ModelNeutralMutableEntry::PutServerAttachmentMetadata(
     } else {
       kernel_->put(SERVER_ATTACHMENT_METADATA, value);
     }
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
@@ -443,19 +444,20 @@ void ModelNeutralMutableEntry::PutDirtySync(bool value) {
 void ModelNeutralMutableEntry::PutParentIdPropertyOnly(const Id& parent_id) {
   base_write_transaction_->TrackChangesTo(kernel_);
   dir()->ReindexParentId(base_write_transaction(), kernel_, parent_id);
-  MarkDirty();
+  kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
 }
 
 void ModelNeutralMutableEntry::UpdateTransactionVersion(int64 value) {
+  ScopedKernelLock lock(dir());
   kernel_->put(TRANSACTION_VERSION, value);
-  MarkDirty();
+  kernel_->mark_dirty(&(dir()->kernel()->dirty_metahandles));
 }
 
 ModelNeutralMutableEntry::ModelNeutralMutableEntry(BaseWriteTransaction* trans)
   : Entry(trans), base_write_transaction_(trans) {}
 
-void ModelNeutralMutableEntry::MarkDirty() {
-  kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+MetahandleSet* ModelNeutralMutableEntry::GetDirtyIndexHelper() {
+  return &dir()->kernel()->dirty_metahandles;
 }
 
 }  // namespace syncable
diff --git a/sync/syncable/model_neutral_mutable_entry.h b/sync/syncable/model_neutral_mutable_entry.h
index 76f5c63..5ff71ad 100644
--- a/sync/syncable/model_neutral_mutable_entry.h
+++ b/sync/syncable/model_neutral_mutable_entry.h
@@ -100,7 +100,7 @@ class SYNC_EXPORT_PRIVATE ModelNeutralMutableEntry : public Entry {
  protected:
   explicit ModelNeutralMutableEntry(BaseWriteTransaction* trans);
 
-  void MarkDirty();
+  syncable::MetahandleSet* GetDirtyIndexHelper();
 
  private:
   friend class syncer::WriteNode;
diff --git a/sync/syncable/mutable_entry.cc b/sync/syncable/mutable_entry.cc
index 6c13261..8e33a67 100644
--- a/sync/syncable/mutable_entry.cc
+++ b/sync/syncable/mutable_entry.cc
@@ -122,36 +122,36 @@ MutableEntry::MutableEntry(WriteTransaction* trans, GetTypeRoot, ModelType type)
 
 void MutableEntry::PutLocalExternalId(int64 value) {
   DCHECK(kernel_);
+  write_transaction()->TrackChangesTo(kernel_);
   if (kernel_->ref(LOCAL_EXTERNAL_ID) != value) {
-    write_transaction()->TrackChangesTo(kernel_);
     ScopedKernelLock lock(dir());
     kernel_->put(LOCAL_EXTERNAL_ID, value);
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
 void MutableEntry::PutMtime(base::Time value) {
   DCHECK(kernel_);
+  write_transaction()->TrackChangesTo(kernel_);
   if (kernel_->ref(MTIME) != value) {
-    write_transaction()->TrackChangesTo(kernel_);
     kernel_->put(MTIME, value);
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
 void MutableEntry::PutCtime(base::Time value) {
   DCHECK(kernel_);
+  write_transaction()->TrackChangesTo(kernel_);
   if (kernel_->ref(CTIME) != value) {
-    write_transaction()->TrackChangesTo(kernel_);
     kernel_->put(CTIME, value);
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
 void MutableEntry::PutParentId(const Id& value) {
   DCHECK(kernel_);
+  write_transaction()->TrackChangesTo(kernel_);
   if (kernel_->ref(PARENT_ID) != value) {
-    write_transaction()->TrackChangesTo(kernel_);
     PutParentIdPropertyOnly(value);
     if (!GetIsDel()) {
       if (!PutPredecessor(Id())) {
@@ -164,20 +164,20 @@ void MutableEntry::PutParentId(const Id& value) {
 
 void MutableEntry::PutIsDir(bool value) {
   DCHECK(kernel_);
-  if (kernel_->ref(IS_DIR) != value) {
-    write_transaction()->TrackChangesTo(kernel_);
+  write_transaction()->TrackChangesTo(kernel_);
+  bool old_value = kernel_->ref(IS_DIR);
+  if (old_value != value) {
     kernel_->put(IS_DIR, value);
-    MarkDirty();
+    kernel_->mark_dirty(GetDirtyIndexHelper());
   }
 }
 
 void MutableEntry::PutIsDel(bool value) {
   DCHECK(kernel_);
+  write_transaction()->TrackChangesTo(kernel_);
   if (value == kernel_->ref(IS_DEL)) {
     return;
   }
-
-  write_transaction()->TrackChangesTo(kernel_);
   if (value) {
     // If the server never knew about this item and it's deleted then we don't
     // need to keep it around.  Unsetting IS_UNSYNCED will:
@@ -202,27 +202,28 @@ void MutableEntry::PutIsDel(bool value) {
         &dir()->kernel()->parent_child_index);
 
     kernel_->put(IS_DEL, value);
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
 void MutableEntry::PutNonUniqueName(const std::string& value) {
   DCHECK(kernel_);
+  write_transaction()->TrackChangesTo(kernel_);
+
   if (kernel_->ref(NON_UNIQUE_NAME) != value) {
-    write_transaction()->TrackChangesTo(kernel_);
     kernel_->put(NON_UNIQUE_NAME, value);
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
 void MutableEntry::PutSpecifics(const sync_pb::EntitySpecifics& value) {
   DCHECK(kernel_);
   CHECK(!value.password().has_client_only_encrypted_data());
+  write_transaction()->TrackChangesTo(kernel_);
   // TODO(ncarter): This is unfortunately heavyweight.  Can we do
   // better?
   const std::string& serialized_value = value.SerializeAsString();
   if (serialized_value != kernel_->ref(SPECIFICS).SerializeAsString()) {
-    write_transaction()->TrackChangesTo(kernel_);
     // Check for potential sharing - SPECIFICS is often
     // copied from SERVER_SPECIFICS.
     if (serialized_value ==
@@ -231,21 +232,21 @@ void MutableEntry::PutSpecifics(const sync_pb::EntitySpecifics& value) {
     } else {
       kernel_->put(SPECIFICS, value);
     }
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
 void MutableEntry::PutUniquePosition(const UniquePosition& value) {
   DCHECK(kernel_);
+  write_transaction()->TrackChangesTo(kernel_);
   if(!kernel_->ref(UNIQUE_POSITION).Equals(value)) {
-    write_transaction()->TrackChangesTo(kernel_);
     // We should never overwrite a valid position with an invalid one.
     DCHECK(value.IsValid());
     ScopedKernelLock lock(dir());
     ScopedParentChildIndexUpdater updater(
         lock, kernel_, &dir()->kernel()->parent_child_index);
     kernel_->put(UNIQUE_POSITION, value);
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
@@ -257,7 +258,6 @@ bool MutableEntry::PutPredecessor(const Id& predecessor_id) {
     if (!predecessor.good())
       return false;
     dir()->PutPredecessor(kernel_, predecessor.kernel_);
-    DCHECK(kernel_->is_dirty());
   }
   return true;
 }
@@ -265,10 +265,10 @@ bool MutableEntry::PutPredecessor(const Id& predecessor_id) {
 void MutableEntry::PutAttachmentMetadata(
     const sync_pb::AttachmentMetadata& value) {
   DCHECK(kernel_);
+  write_transaction()->TrackChangesTo(kernel_);
   const std::string& serialized_value = value.SerializeAsString();
   if (serialized_value !=
       kernel_->ref(ATTACHMENT_METADATA).SerializeAsString()) {
-    write_transaction()->TrackChangesTo(kernel_);
     dir()->UpdateAttachmentIndex(GetMetahandle(),
                                  kernel_->ref(ATTACHMENT_METADATA), value);
     // Check for potential sharing - ATTACHMENT_METADATA is often
@@ -279,7 +279,7 @@ void MutableEntry::PutAttachmentMetadata(
     } else {
       kernel_->put(ATTACHMENT_METADATA, value);
     }
-    MarkDirty();
+    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   }
 }
 
@@ -298,7 +298,7 @@ void MutableEntry::MarkAttachmentAsOnServer(
     record->set_is_on_server(true);
   }
   kernel_->put(ATTACHMENT_METADATA, attachment_metadata);
-  MarkDirty();
+  kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
   MarkForSyncing(this);
 }