This is the second and final patch of changes for implementing the per...

This is the second and final patch of changes for implementing the per datatype throttling feature. Includes the tests.

This consumes the throttled datatypes set in the context and uses that to filter the unsynced handles that we use for committing.

BUG=104819
TEST=unit_tests.exe, sync_unit_tests.exe, sync_integration_tests.exe, manual test cases


Review URL: http://codereview.chromium.org/8631021

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@111464 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/sync/engine/get_commit_ids_command.cc

@@ -43,16 +43,20 @@ void GetCommitIdsCommand::ExecuteImpl(SyncSession* session) {
     encrypted_types_ = cryptographer->GetEncryptedTypes();
     passphrase_missing_ = cryptographer->has_pending_keys();
   };
+
+  const syncable::ModelTypeSet& throttled_types =
+       session->context()->GetThrottledTypes();
   // We filter out all unready entries from the set of unsynced handles to
   // ensure we don't trigger useless sync cycles attempting to retry due to
   // there being work to do. (see ScheduleNextSync in sync_scheduler)
   FilterUnreadyEntries(session->write_transaction(),
+                       throttled_types,
                        &all_unsynced_handles);
 
   StatusController* status = session->mutable_status_controller();
   status->set_unsynced_handles(all_unsynced_handles);
   BuildCommitIds(status->unsynced_handles(), session->write_transaction(),
-                 session->routing_info());
+                 session->routing_info(), throttled_types);
 
   const vector<syncable::Id>& verified_commit_ids =
       ordered_commit_set_->GetAllCommitIds();
@@ -65,13 +69,15 @@ void GetCommitIdsCommand::ExecuteImpl(SyncSession* session) {
 
 namespace {
 
-// An entry ready for commit is defined as one not in conflict (SERVER_VERSION
-// == BASE_VERSION || SERVER_VERSION == 0) and not requiring encryption
-// (any entry containing an encrypted datatype while the cryptographer requires
-// a passphrase is not ready for commit.)
+// An entry ready for commit is defined as:
+// 1. Not in conflict (SERVER_VERSION == BASE_VERSION || SERVER_VERSION == 0)
+// and not requiring encryption (any entry containing an encrypted datatype
+// while the cryptographer requires a passphrase is not ready for commit.)
+// 2. Its type is not currently throttled.
 bool IsEntryReadyForCommit(const syncable::ModelTypeSet& encrypted_types,
                            bool passphrase_missing,
-                           const syncable::Entry& entry) {
+                           const syncable::Entry& entry,
+                           const syncable::ModelTypeSet& throttled_types) {
   if (!entry.Get(syncable::IS_UNSYNCED))
     return false;
 
@@ -87,7 +93,8 @@ bool IsEntryReadyForCommit(const syncable::ModelTypeSet& encrypted_types,
     return false;
   }
 
-  if (encrypted_types.count(entry.GetModelType()) > 0 &&
+  syncable::ModelType type = entry.GetModelType();
+  if (encrypted_types.count(type) > 0 &&
       (passphrase_missing ||
        syncable::EntryNeedsEncryption(encrypted_types, entry))) {
     // This entry requires encryption but is not properly encrypted (possibly
@@ -99,6 +106,10 @@ bool IsEntryReadyForCommit(const syncable::ModelTypeSet& encrypted_types,
     return false;
   }
 
+  // Look at the throttled types.
+  if (throttled_types.count(type) > 0)
+    return false;
+
   return true;
 }
 
@@ -106,6 +117,7 @@ bool IsEntryReadyForCommit(const syncable::ModelTypeSet& encrypted_types,
 
 void GetCommitIdsCommand::FilterUnreadyEntries(
     syncable::BaseTransaction* trans,
+    const syncable::ModelTypeSet& throttled_types,
     syncable::Directory::UnsyncedMetaHandles* unsynced_handles) {
   syncable::Directory::UnsyncedMetaHandles::iterator iter;
   syncable::Directory::UnsyncedMetaHandles new_unsynced_handles;
@@ -114,7 +126,10 @@ void GetCommitIdsCommand::FilterUnreadyEntries(
        iter != unsynced_handles->end();
        ++iter) {
     syncable::Entry entry(trans, syncable::GET_BY_HANDLE, *iter);
-    if (IsEntryReadyForCommit(encrypted_types_, passphrase_missing_, entry))
+    if (IsEntryReadyForCommit(encrypted_types_,
+                              passphrase_missing_,
+                              entry,
+                              throttled_types))
       new_unsynced_handles.push_back(*iter);
   }
   if (new_unsynced_handles.size() != unsynced_handles->size())
@@ -124,7 +139,8 @@ void GetCommitIdsCommand::FilterUnreadyEntries(
 void GetCommitIdsCommand::AddUncommittedParentsAndTheirPredecessors(
     syncable::BaseTransaction* trans,
     syncable::Id parent_id,
-    const ModelSafeRoutingInfo& routes) {
+    const ModelSafeRoutingInfo& routes,
+    const syncable::ModelTypeSet& throttled_types) {
   OrderedCommitSet item_dependencies(routes);
 
   // Climb the tree adding entries leaf -> root.
@@ -136,7 +152,8 @@ void GetCommitIdsCommand::AddUncommittedParentsAndTheirPredecessors(
         item_dependencies.HaveCommitItem(handle)) {
       break;
     }
-    if (!AddItemThenPredecessors(trans, &parent, syncable::IS_UNSYNCED,
+    if (!AddItemThenPredecessors(trans, throttled_types, &parent,
+                                 syncable::IS_UNSYNCED,
                                  &item_dependencies)) {
       break;  // Parent was already present in the set.
     }
@@ -148,8 +165,10 @@ void GetCommitIdsCommand::AddUncommittedParentsAndTheirPredecessors(
 }
 
 bool GetCommitIdsCommand::AddItem(syncable::Entry* item,
-                                  OrderedCommitSet* result) {
-  if (!IsEntryReadyForCommit(encrypted_types_, passphrase_missing_, *item))
+    const syncable::ModelTypeSet& throttled_types,
+    OrderedCommitSet* result) {
+  if (!IsEntryReadyForCommit(encrypted_types_, passphrase_missing_, *item,
+                             throttled_types))
     return false;
   int64 item_handle = item->Get(syncable::META_HANDLE);
   if (result->HaveCommitItem(item_handle) ||
@@ -163,10 +182,11 @@ bool GetCommitIdsCommand::AddItem(syncable::Entry* item,
 
 bool GetCommitIdsCommand::AddItemThenPredecessors(
     syncable::BaseTransaction* trans,
+    const syncable::ModelTypeSet& throttled_types,
     syncable::Entry* item,
     syncable::IndexedBitField inclusion_filter,
     OrderedCommitSet* result) {
-  if (!AddItem(item, result))
+  if (!AddItem(item, throttled_types, result))
     return false;
   if (item->Get(syncable::IS_DEL))
     return true;  // Deleted items have no predecessors.
@@ -177,7 +197,7 @@ bool GetCommitIdsCommand::AddItemThenPredecessors(
     CHECK(prev.good()) << "Bad id when walking predecessors.";
     if (!prev.Get(inclusion_filter))
       break;
-    if (!AddItem(&prev, result))
+    if (!AddItem(&prev, throttled_types, result))
       break;
     prev_id = prev.Get(syncable::PREV_ID);
   }
@@ -186,11 +206,13 @@ bool GetCommitIdsCommand::AddItemThenPredecessors(
 
 void GetCommitIdsCommand::AddPredecessorsThenItem(
     syncable::BaseTransaction* trans,
+    const syncable::ModelTypeSet& throttled_types,
     syncable::Entry* item,
     syncable::IndexedBitField inclusion_filter,
     const ModelSafeRoutingInfo& routes) {
   OrderedCommitSet item_dependencies(routes);
-  AddItemThenPredecessors(trans, item, inclusion_filter, &item_dependencies);
+  AddItemThenPredecessors(trans, throttled_types, item, inclusion_filter,
+                          &item_dependencies);
 
   // Reverse what we added to get the correct order.
   ordered_commit_set_->AppendReverse(item_dependencies);
@@ -203,7 +225,8 @@ bool GetCommitIdsCommand::IsCommitBatchFull() {
 void GetCommitIdsCommand::AddCreatesAndMoves(
     const vector<int64>& unsynced_handles,
     syncable::WriteTransaction* write_transaction,
-    const ModelSafeRoutingInfo& routes) {
+    const ModelSafeRoutingInfo& routes,
+    const syncable::ModelTypeSet& throttled_types) {
   // Add moves and creates, and prepend their uncommitted parents.
   for (CommitMetahandleIterator iterator(unsynced_handles, write_transaction,
                                          ordered_commit_set_.get());
@@ -216,9 +239,9 @@ void GetCommitIdsCommand::AddCreatesAndMoves(
                           metahandle);
     if (!entry.Get(syncable::IS_DEL)) {
       AddUncommittedParentsAndTheirPredecessors(write_transaction,
-          entry.Get(syncable::PARENT_ID), routes);
-      AddPredecessorsThenItem(write_transaction, &entry,
-          syncable::IS_UNSYNCED, routes);
+          entry.Get(syncable::PARENT_ID), routes, throttled_types);
+      AddPredecessorsThenItem(write_transaction, throttled_types, &entry,
+                              syncable::IS_UNSYNCED, routes);
     }
   }
 
@@ -313,7 +336,8 @@ void GetCommitIdsCommand::AddDeletes(const vector<int64>& unsynced_handles,
 
 void GetCommitIdsCommand::BuildCommitIds(const vector<int64>& unsynced_handles,
     syncable::WriteTransaction* write_transaction,
-    const ModelSafeRoutingInfo& routes) {
+    const ModelSafeRoutingInfo& routes,
+    const syncable::ModelTypeSet& throttled_types) {
   ordered_commit_set_.reset(new OrderedCommitSet(routes));
   // Commits follow these rules:
   // 1. Moves or creates are preceded by needed folder creates, from
@@ -325,7 +349,8 @@ void GetCommitIdsCommand::BuildCommitIds(const vector<int64>& unsynced_handles,
   // delete trees.
 
   // Add moves and creates, and prepend their uncommitted parents.
-  AddCreatesAndMoves(unsynced_handles, write_transaction, routes);
+  AddCreatesAndMoves(unsynced_handles, write_transaction, routes,
+                     throttled_types);
 
   // Add all deletes.
   AddDeletes(unsynced_handles, write_transaction);

chrome/browser/sync/engine/get_commit_ids_command.h

@@ -33,7 +33,8 @@ class GetCommitIdsCommand : public SyncerCommand {
   // Builds a vector of IDs that should be committed.
   void BuildCommitIds(const vector<int64>& unsynced_handles,
                       syncable::WriteTransaction* write_transaction,
-                      const ModelSafeRoutingInfo& routes);
+                      const ModelSafeRoutingInfo& routes,
+                      const syncable::ModelTypeSet& throttled_types);
 
   // TODO(chron): Remove writes from this iterator. As a warning, this
   // iterator causes writes to entries and so isn't a pure iterator.
@@ -109,27 +110,35 @@ class GetCommitIdsCommand : public SyncerCommand {
 
  private:
   // Removes all entries not ready for commit from |unsynced_handles|.
-  // An entry is considered unready for commit if it's in conflict or requires
-  // (re)encryption. Any datatype requiring encryption while the cryptographer
-  // is missing a passphrase is considered unready for commit.
+  // An entry is considered unready for commit if:
+  // 1. It's in conflict or requires (re)encryption. Any datatype requiring
+  //     encryption while the cryptographer is missing a passphrase is
+  //     considered unready for commit.
+  // 2. Its type is currently throttled.
   void FilterUnreadyEntries(
       syncable::BaseTransaction* trans,
+      const syncable::ModelTypeSet& throttled_types,
       syncable::Directory::UnsyncedMetaHandles* unsynced_handles);
 
   void AddUncommittedParentsAndTheirPredecessors(
       syncable::BaseTransaction* trans,
       syncable::Id parent_id,
-      const ModelSafeRoutingInfo& routes);
+      const ModelSafeRoutingInfo& routes,
+      const syncable::ModelTypeSet& throttled_types);
 
   // OrderedCommitSet helpers for adding predecessors in order.
   // TODO(ncarter): Refactor these so that the |result| parameter goes away,
   // and AddItem doesn't need to consider two OrderedCommitSets.
-  bool AddItem(syncable::Entry* item, sessions::OrderedCommitSet* result);
+  bool AddItem(syncable::Entry* item,
+               const syncable::ModelTypeSet& throttled_types,
+               sessions::OrderedCommitSet* result);
   bool AddItemThenPredecessors(syncable::BaseTransaction* trans,
+                               const syncable::ModelTypeSet& throttled_types,
                                syncable::Entry* item,
                                syncable::IndexedBitField inclusion_filter,
                                sessions::OrderedCommitSet* result);
   void AddPredecessorsThenItem(syncable::BaseTransaction* trans,
+                               const syncable::ModelTypeSet& throttled_types,
                                syncable::Entry* item,
                                syncable::IndexedBitField inclusion_filter,
                                const ModelSafeRoutingInfo& routes);
@@ -138,7 +147,8 @@ class GetCommitIdsCommand : public SyncerCommand {
 
   void AddCreatesAndMoves(const vector<int64>& unsynced_handles,
                           syncable::WriteTransaction* write_transaction,
-                          const ModelSafeRoutingInfo& routes);
+                          const ModelSafeRoutingInfo& routes,
+                          const syncable::ModelTypeSet& throttled_types);
 
   void AddDeletes(const vector<int64>& unsynced_handles,
                   syncable::WriteTransaction* write_transaction);

chrome/browser/sync/engine/syncer.cc

@@ -145,6 +145,7 @@ void Syncer::SyncShare(sessions::SyncSession* session,
         // for analysis purposes, so Law of Large Numbers FTW.
         session->context()->extensions_monitor()->GetAndClearRecords(
             session->mutable_extensions_activity());
+        session->context()->PruneUnthrottledTypes(base::TimeTicks::Now());
         next_step = CLEANUP_DISABLED_TYPES;
         break;
       case CLEANUP_DISABLED_TYPES: {

chrome/browser/sync/engine/syncer_unittest.cc

@@ -371,7 +371,7 @@ class SyncerTest : public testing::Test,
       GetCommitIdsCommand command(limit);
       command.BuildCommitIds(
           session_->status_controller().unsynced_handles(),
-          session_->write_transaction(), routes);
+          session_->write_transaction(), routes, syncable::ModelTypeSet());
       vector<syncable::Id> output =
           command.ordered_commit_set_->GetAllCommitIds();
       size_t truncated_size = std::min(limit, expected_id_order.size());
@@ -556,6 +556,55 @@ TEST_F(SyncerTest, GetCommitIdsCommandTruncates) {
   DoTruncationTest(dir, unsynced_handle_view, expected_order);
 }
 
+TEST_F(SyncerTest, GetCommitIdsFiltersThrottledEntries) {
+  ScopedDirLookup dir(syncdb_.manager(), syncdb_.name());
+  ASSERT_TRUE(dir.good());
+  syncable::ModelTypeSet throttled_types;
+  throttled_types.insert(syncable::BOOKMARKS);
+  KeyParams key_params = {"localhost", "dummy", "foobar"};
+  sync_pb::EntitySpecifics bookmark_data;
+  AddDefaultExtensionValue(syncable::BOOKMARKS, &bookmark_data);
+
+  mock_server_->AddUpdateDirectory(1, 0, "A", 10, 10);
+  SyncShareAsDelegate();
+
+  {
+    WriteTransaction wtrans(FROM_HERE, UNITTEST, dir);
+    MutableEntry A(&wtrans, GET_BY_ID, ids_.FromNumber(1));
+    ASSERT_TRUE(A.good());
+    A.Put(IS_UNSYNCED, true);
+    A.Put(SPECIFICS, bookmark_data);
+    A.Put(NON_UNIQUE_NAME, "bookmark");
+  }
+
+  // Now set the throttled types.
+  context_->SetUnthrottleTime(
+      throttled_types,
+      base::TimeTicks::Now() + base::TimeDelta::FromSeconds(1200));
+  SyncShareAsDelegate();
+
+  {
+    // Nothing should have been committed as bookmarks is throttled.
+    ReadTransaction rtrans(FROM_HERE, dir);
+    Entry entryA(&rtrans, syncable::GET_BY_ID, ids_.FromNumber(1));
+    ASSERT_TRUE(entryA.good());
+    EXPECT_TRUE(entryA.Get(IS_UNSYNCED));
+  }
+
+  // Now unthrottle.
+  context_->SetUnthrottleTime(
+      throttled_types,
+      base::TimeTicks::Now() - base::TimeDelta::FromSeconds(1200));
+  SyncShareAsDelegate();
+  {
+    // It should have been committed.
+    ReadTransaction rtrans(FROM_HERE, dir);
+    Entry entryA(&rtrans, syncable::GET_BY_ID, ids_.FromNumber(1));
+    ASSERT_TRUE(entryA.good());
+    EXPECT_FALSE(entryA.Get(IS_UNSYNCED));
+  }
+}
+
 TEST_F(SyncerTest, GetCommitIdsFiltersUnreadyEntries) {
   ScopedDirLookup dir(syncdb_.manager(), syncdb_.name());
   ASSERT_TRUE(dir.good());

chrome/browser/sync/sessions/sync_session_context.cc

@@ -58,5 +58,32 @@ void SyncSessionContext::SetUnthrottleTime(const syncable::ModelTypeSet& types,
   }
 }
 
+void SyncSessionContext::PruneUnthrottledTypes(const base::TimeTicks& time) {
+  UnthrottleTimes::iterator it = unthrottle_times_.begin();
+  while (it != unthrottle_times_.end()) {
+    if (it->second <= time) {
+      // Delete and increment the iterator.
+      UnthrottleTimes::iterator iterator_to_delete = it;
+      ++it;
+      unthrottle_times_.erase(iterator_to_delete);
+    } else {
+      // Just increment the iterator.
+      ++it;
+    }
+  }
+}
+
+// TODO(lipalani): Call this function and fill the return values in snapshot
+// so it could be shown in the about:sync page.
+syncable::ModelTypeSet SyncSessionContext::GetThrottledTypes() const {
+  syncable::ModelTypeSet types;
+  for (UnthrottleTimes::const_iterator it = unthrottle_times_.begin();
+       it != unthrottle_times_.end();
+       ++it) {
+    types.insert(it->first);
+  }
+  return types;
+}
+
 }  // namespace sessions
 }  // namespace browser_sync

chrome/browser/sync/sessions/sync_session_context.h

@@ -22,6 +22,7 @@
 #include <map>
 #include <string>
 
+#include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/time.h"
 #include "chrome/browser/sync/engine/model_safe_worker.h"
@@ -109,14 +110,24 @@ class SyncSessionContext {
   }
 
   // This is virtual for unit tests.
-  // TODO(lipalani)  Consult the unthrottle times before committing data to
-  // the server.
   virtual void SetUnthrottleTime(const syncable::ModelTypeSet& types,
                                  const base::TimeTicks& time);
 
+  // This prunes the |unthrottle_time_| map based on the |time| passed in. This
+  // is called by syncer at the SYNCER_BEGIN stage.
+  void PruneUnthrottledTypes(const base::TimeTicks& time);
+
+  // This returns the list of currently throttled types. Unless server returns
+  // new throttled types this will remain constant through out the sync cycle.
+  syncable::ModelTypeSet GetThrottledTypes() const;
+
  private:
   typedef std::map<syncable::ModelType, base::TimeTicks> UnthrottleTimes;
 
+  FRIEND_TEST_ALL_PREFIXES(SyncSessionContextTest, AddUnthrottleTimeTest);
+  FRIEND_TEST_ALL_PREFIXES(SyncSessionContextTest,
+                           GetCurrentlyThrottledTypesTest);
+
   // Rather than force clients to set and null-out various context members, we
   // extend our encapsulation boundary to scoped helpers that take care of this
   // once they are allocated. See definitions of these below.

chrome/browser/sync/sessions/sync_session_context_unittest.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/sessions/sync_session_context.h"
+
+#include "chrome/browser/sync/syncable/model_type.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace browser_sync {
+namespace sessions {
+TEST(SyncSessionContextTest, AddUnthrottleTimeTest) {
+  syncable::ModelTypeSet types;
+  types.insert(syncable::BOOKMARKS);
+  types.insert(syncable::PASSWORDS);
+
+  SyncSessionContext context;
+  base::TimeTicks now = base::TimeTicks::Now();
+  context.SetUnthrottleTime(types, now);
+
+  EXPECT_EQ(context.unthrottle_times_.size(), 2U);
+  EXPECT_EQ(context.unthrottle_times_[syncable::BOOKMARKS], now);
+  EXPECT_EQ(context.unthrottle_times_[syncable::PASSWORDS], now);
+}
+
+TEST(SyncSessionContextTest, GetCurrentlyThrottledTypesTest) {
+  syncable::ModelTypeSet types;
+  types.insert(syncable::BOOKMARKS);
+  types.insert(syncable::PASSWORDS);
+
+  SyncSessionContext context;
+  base::TimeTicks now = base::TimeTicks::Now();
+
+  // Now update the throttled types with time set to 10 seconds earlier from
+  // now.
+  context.SetUnthrottleTime(types, now - base::TimeDelta::FromSeconds(10));
+  context.PruneUnthrottledTypes(base::TimeTicks::Now());
+  EXPECT_EQ(context.GetThrottledTypes(), syncable::ModelTypeSet());
+
+  // Now update the throttled types with time set to 2 hours from now.
+  context.SetUnthrottleTime(types, now + base::TimeDelta::FromSeconds(1200));
+  context.PruneUnthrottledTypes(base::TimeTicks::Now());
+  EXPECT_EQ(context.GetThrottledTypes(), types);
+}
+}  // namespace sessions.
+}  // namespace browser_sync
+

chrome/chrome_tests.gypi

@@ -3305,6 +3305,7 @@
         'browser/sync/sessions/ordered_commit_set_unittest.cc',
         'browser/sync/sessions/session_state_unittest.cc',
         'browser/sync/sessions/status_controller_unittest.cc',
+        'browser/sync/sessions/sync_session_context_unittest.cc',
         'browser/sync/sessions/sync_session_unittest.cc',
         'browser/sync/sessions/test_util.cc',
         'browser/sync/sessions/test_util.h',