Make TemplateURLService batch up keyword table changes.

This came about because a trace showed the database thread spending a long
period of time executing a series of many individual keyword updates, each of
which was committed separately and took ~90 ms.

With this change, pieces of TemplateURLService that may make more than one
change to the keyword table at a time batch those changes together so there is a
single call to the database thread and a single SQL transaction wrapping all the
changes.  In manual testing of TwoClientSearchEnginesSyncTest.AddMultiple (which
adds a bunch of search engines to one client and then syncs those changes to
another client), with the number of search engines synced elevated to 50 and
also 100, these changes saved approximately 6 seconds per test run.

By making WebDataService wrap even individual keyword table changes in a batch,
this also allows eliminating some implementation methods on WebDataService and
making similar methods on KeywordTable private.

BUG=308371
TEST=none
R=shess@chromium.org, stevet@chromium.org

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

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

5 files changed, 194 insertions, 73 deletions

diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc
index 3a222fb..7138828 100644
--- a/chrome/browser/search_engines/template_url_service.cc
+++ b/chrome/browser/search_engines/template_url_service.cc
@@ -490,6 +490,7 @@ TemplateURL* TemplateURLService::GetTemplateURLForHost(
 }
 
 void TemplateURLService::Add(TemplateURL* template_url) {
+  WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
   if (AddNoNotify(template_url, true))
     NotifyObservers();
 }
@@ -524,6 +525,7 @@ void TemplateURLService::AddExtensionControlledTURL(
       template_url->GetExtensionId(),
       TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION));
 
+  WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
   if (AddNoNotify(template_url, true)) {
     // Note that we can't call CanMakeDefault() here, since it would return
     // false when another extension is already controlling the default search
@@ -558,6 +560,7 @@ void TemplateURLService::RemoveExtensionControlledTURL(
     DCHECK(!is_default_search_managed());
     default_search_provider_ = NULL;
   }
+  WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
   RemoveNoNotify(url);
   if (restore_dse)
     SetDefaultSearchProviderAfterRemovingDefaultExtension();
@@ -579,6 +582,7 @@ void TemplateURLService::RemoveAutoGeneratedForOriginBetween(
     base::Time created_before) {
   GURL o(origin.GetOrigin());
   bool should_notify = false;
+  WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
   for (size_t i = 0; i < template_urls_.size();) {
     if (template_urls_[i]->date_created() >= created_after &&
         (created_before.is_null() ||
@@ -748,6 +752,9 @@ void TemplateURLService::RepairPrepopulatedSearchEngines() {
   TemplateURL* current_dse = default_search_provider_;
   ActionsFromPrepopulateData actions(CreateActionsFromCurrentPrepopulateData(
       &prepopulated_urls, template_urls_, current_dse));
+
+  WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
+
   // Remove items.
   for (std::vector<TemplateURL*>::iterator i = actions.removed_engines.begin();
        i < actions.removed_engines.end(); ++i)
@@ -837,6 +844,8 @@ void TemplateURLService::OnWebDataServiceRequestDone(
       &template_urls, &default_search_provider, &new_resource_keyword_version,
       &pre_sync_deletes_);
 
+  WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
+
   AddTemplateURLsAndSetupDefaultEngine(&template_urls, default_search_provider);
 
   // This initializes provider_map_ which should be done before
@@ -968,6 +977,8 @@ syncer::SyncError TemplateURLService::ProcessSyncChanges(
   base::AutoReset<DefaultSearchChangeOrigin> change_origin(&dsp_change_origin_,
       DSP_CHANGE_SYNC_UNINTENTIONAL);
 
+  WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
+
   syncer::SyncChangeList new_changes;
   syncer::SyncError error;
   for (syncer::SyncChangeList::const_iterator iter = change_list.begin();
@@ -1126,6 +1137,8 @@ syncer::SyncMergeResult TemplateURLService::MergeDataAndStartSyncing(
       GetAllSyncData(syncer::SEARCH_ENGINES));
   SyncDataMap sync_data_map = CreateGUIDToSyncDataMap(initial_sync_data);
 
+  WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
+
   merge_result.set_num_items_before_association(local_data_map.size());
   for (SyncDataMap::const_iterator iter = sync_data_map.begin();
       iter != sync_data_map.end(); ++iter) {
@@ -1455,6 +1468,7 @@ void TemplateURLService::Init(const Initializer* initializers,
     ChangeToLoadedState();
 
     // Add specific initializers, if any.
+    WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
     for (int i(0); i < num_initializers; ++i) {
       DCHECK(initializers[i].keyword);
       DCHECK(initializers[i].url);
@@ -1970,6 +1984,7 @@ void TemplateURLService::AddTabToSearchVisit(const TemplateURL& t_url) {
 }
 
 void TemplateURLService::GoogleBaseURLChanged(const GURL& old_base_url) {
+  WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
   bool something_changed = false;
   for (TemplateURLVector::iterator i(template_urls_.begin());
        i != template_urls_.end(); ++i) {
@@ -2034,6 +2049,7 @@ void TemplateURLService::UpdateDefaultSearch() {
     // unmanaged.  In that case, preferences have no impact on the default.
     return;
   }
+  WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
   if (is_default_search_managed_ && new_is_default_managed) {
     // The default was managed and remains managed.  Update the default only
     // if it has changed; we don't want to respond to changes triggered by
diff --git a/chrome/browser/webdata/keyword_table.cc b/chrome/browser/webdata/keyword_table.cc
index 0c22338..a481e76 100644
--- a/chrome/browser/webdata/keyword_table.cc
+++ b/chrome/browser/webdata/keyword_table.cc
@@ -241,24 +241,32 @@ bool KeywordTable::MigrateToVersion(int version,
   return true;
 }
 
-bool KeywordTable::AddKeyword(const TemplateURLData& data) {
-  DCHECK(data.id);
-  std::string query("INSERT INTO keywords (" + GetKeywordColumns() + ") "
-                    "VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,"
-                    "        ?)");
-  sql::Statement s(db_->GetCachedStatement(SQL_FROM_HERE, query.c_str()));
-  BindURLToStatement(data, &s, 0, 1);
+bool KeywordTable::PerformOperations(const Operations& operations) {
+  sql::Transaction transaction(db_);
+  if (!transaction.Begin())
+    return false;
 
-  return s.Run();
-}
+  for (Operations::const_iterator i(operations.begin()); i != operations.end();
+       ++i) {
+    switch (i->first) {
+      case ADD:
+        if (!AddKeyword(i->second))
+          return false;
+        break;
 
-bool KeywordTable::RemoveKeyword(TemplateURLID id) {
-  DCHECK(id);
-  sql::Statement s(db_->GetCachedStatement(
-      SQL_FROM_HERE, "DELETE FROM keywords WHERE id = ?"));
-  s.BindInt64(0, id);
+      case REMOVE:
+        if (!RemoveKeyword(i->second.id))
+          return false;
+        break;
 
-  return s.Run();
+      case UPDATE:
+        if (!UpdateKeyword(i->second))
+          return false;
+        break;
+    }
+  }
+
+  return transaction.Commit();
 }
 
 bool KeywordTable::GetKeywords(Keywords* keywords) {
@@ -281,23 +289,6 @@ bool KeywordTable::GetKeywords(Keywords* keywords) {
   return succeeded;
 }
 
-bool KeywordTable::UpdateKeyword(const TemplateURLData& data) {
-  DCHECK(data.id);
-  sql::Statement s(db_->GetCachedStatement(
-      SQL_FROM_HERE,
-      "UPDATE keywords SET short_name=?, keyword=?, favicon_url=?, url=?, "
-      "safe_for_autoreplace=?, originating_url=?, date_created=?, "
-      "usage_count=?, input_encodings=?, show_in_default_list=?, "
-      "suggest_url=?, prepopulate_id=?, created_by_policy=?, instant_url=?, "
-      "last_modified=?, sync_guid=?, alternate_urls=?, "
-      "search_terms_replacement_key=?, image_url=?, search_url_post_params=?, "
-      "suggest_url_post_params=?, instant_url_post_params=?, "
-      "image_url_post_params=?, new_tab_url=? WHERE id=?"));
-  BindURLToStatement(data, &s, 24, 0);  // "24" binds id() as the last item.
-
-  return s.Run();
-}
-
 bool KeywordTable::SetDefaultSearchProviderID(int64 id) {
   return meta_table_->SetValue(kDefaultSearchProviderKey, id);
 }
@@ -513,6 +504,43 @@ bool KeywordTable::GetKeywordDataFromStatement(const sql::Statement& s,
   return true;
 }
 
+bool KeywordTable::AddKeyword(const TemplateURLData& data) {
+  DCHECK(data.id);
+  std::string query("INSERT INTO keywords (" + GetKeywordColumns() + ") "
+                    "VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,"
+                    "        ?)");
+  sql::Statement s(db_->GetCachedStatement(SQL_FROM_HERE, query.c_str()));
+  BindURLToStatement(data, &s, 0, 1);
+
+  return s.Run();
+}
+
+bool KeywordTable::RemoveKeyword(TemplateURLID id) {
+  DCHECK(id);
+  sql::Statement s(db_->GetCachedStatement(
+      SQL_FROM_HERE, "DELETE FROM keywords WHERE id = ?"));
+  s.BindInt64(0, id);
+
+  return s.Run();
+}
+
+bool KeywordTable::UpdateKeyword(const TemplateURLData& data) {
+  DCHECK(data.id);
+  sql::Statement s(db_->GetCachedStatement(
+      SQL_FROM_HERE,
+      "UPDATE keywords SET short_name=?, keyword=?, favicon_url=?, url=?, "
+      "safe_for_autoreplace=?, originating_url=?, date_created=?, "
+      "usage_count=?, input_encodings=?, show_in_default_list=?, "
+      "suggest_url=?, prepopulate_id=?, created_by_policy=?, instant_url=?, "
+      "last_modified=?, sync_guid=?, alternate_urls=?, "
+      "search_terms_replacement_key=?, image_url=?, search_url_post_params=?, "
+      "suggest_url_post_params=?, instant_url_post_params=?, "
+      "image_url_post_params=?, new_tab_url=? WHERE id=?"));
+  BindURLToStatement(data, &s, 24, 0);  // "24" binds id() as the last item.
+
+  return s.Run();
+}
+
 bool KeywordTable::GetKeywordAsString(TemplateURLID id,
                                       const std::string& table_name,
                                       std::string* result) {
diff --git a/chrome/browser/webdata/keyword_table.h b/chrome/browser/webdata/keyword_table.h
index 3756626..70c1ac5 100644
--- a/chrome/browser/webdata/keyword_table.h
+++ b/chrome/browser/webdata/keyword_table.h
@@ -77,6 +77,14 @@ class Statement;
 //
 class KeywordTable : public WebDatabaseTable {
  public:
+  enum OperationType {
+    ADD,
+    REMOVE,
+    UPDATE,
+  };
+
+  typedef std::pair<OperationType, TemplateURLData> Operation;
+  typedef std::vector<Operation> Operations;
   typedef std::vector<TemplateURLData> Keywords;
 
   // Constants exposed for the benefit of test code:
@@ -95,23 +103,17 @@ class KeywordTable : public WebDatabaseTable {
   virtual bool MigrateToVersion(int version,
                                 bool* update_compatible_version) OVERRIDE;
 
-  // Adds a new keyword, updating the id field on success.
-  // Returns true if successful.
-  bool AddKeyword(const TemplateURLData& data);
-
-  // Removes the specified keyword.
-  // Returns true if successful.
-  bool RemoveKeyword(TemplateURLID id);
+  // Performs an arbitrary number of Add/Remove/Update operations as a single
+  // transaction.  This is provided for efficiency reasons: if the caller needs
+  // to perform a large number of operations, doing them in a single transaction
+  // instead of one-per-transaction can be dramatically more efficient.
+  bool PerformOperations(const Operations& operations);
 
   // Loads the keywords into the specified vector. It's up to the caller to
   // delete the returned objects.
   // Returns true on success.
   bool GetKeywords(Keywords* keywords);
 
-  // Updates the database values for the specified url.
-  // Returns true on success.
-  bool UpdateKeyword(const TemplateURLData& data);
-
   // ID (TemplateURLData->id) of the default search provider.
   bool SetDefaultSearchProviderID(int64 id);
   int64 GetDefaultSearchProviderID();
@@ -154,6 +156,18 @@ class KeywordTable : public WebDatabaseTable {
   static bool GetKeywordDataFromStatement(const sql::Statement& s,
                                           TemplateURLData* data);
 
+  // Adds a new keyword, updating the id field on success.
+  // Returns true if successful.
+  bool AddKeyword(const TemplateURLData& data);
+
+  // Removes the specified keyword.
+  // Returns true if successful.
+  bool RemoveKeyword(TemplateURLID id);
+
+  // Updates the database values for the specified url.
+  // Returns true on success.
+  bool UpdateKeyword(const TemplateURLData& data);
+
   // Gets a string representation for keyword with id specified.
   // Used to store its result in |meta| table or to compare with another
   // keyword. Returns true on success, false otherwise.
diff --git a/chrome/browser/webdata/web_data_service.cc b/chrome/browser/webdata/web_data_service.cc
index 030383e..2e829f1 100644
--- a/chrome/browser/webdata/web_data_service.cc
+++ b/chrome/browser/webdata/web_data_service.cc
@@ -41,11 +41,24 @@ WDKeywordsResult::WDKeywordsResult()
 
 WDKeywordsResult::~WDKeywordsResult() {}
 
+WebDataService::KeywordBatchModeScoper::KeywordBatchModeScoper(
+    WebDataService* service)
+    : service_(service) {
+  if (service_)
+    service_->AdjustKeywordBatchModeLevel(true);
+}
+
+WebDataService::KeywordBatchModeScoper::~KeywordBatchModeScoper() {
+  if (service_)
+    service_->AdjustKeywordBatchModeLevel(false);
+}
+
 WebDataService::WebDataService(scoped_refptr<WebDatabaseService> wdbs,
                                const ProfileErrorCallback& callback)
     : WebDataServiceBase(
           wdbs, callback,
-          BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI)) {
+          BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI)),
+      keyword_batch_mode_level_(0) {
 }
 
 //////////////////////////////////////////////////////////////////////////////
@@ -55,18 +68,38 @@ WebDataService::WebDataService(scoped_refptr<WebDatabaseService> wdbs,
 //////////////////////////////////////////////////////////////////////////////
 
 void WebDataService::AddKeyword(const TemplateURLData& data) {
-  wdbs_->ScheduleDBTask(
-      FROM_HERE, Bind(&WebDataService::AddKeywordImpl, this, data));
+  if (keyword_batch_mode_level_) {
+    queued_keyword_operations_.push_back(
+        KeywordTable::Operation(KeywordTable::ADD, data));
+  } else {
+    AdjustKeywordBatchModeLevel(true);
+    AddKeyword(data);
+    AdjustKeywordBatchModeLevel(false);
+  }
 }
 
 void WebDataService::RemoveKeyword(TemplateURLID id) {
-  wdbs_->ScheduleDBTask(
-      FROM_HERE, Bind(&WebDataService::RemoveKeywordImpl, this, id));
+  if (keyword_batch_mode_level_) {
+    TemplateURLData data;
+    data.id = id;
+    queued_keyword_operations_.push_back(
+        KeywordTable::Operation(KeywordTable::REMOVE, data));
+  } else {
+    AdjustKeywordBatchModeLevel(true);
+    RemoveKeyword(id);
+    AdjustKeywordBatchModeLevel(false);
+  }
 }
 
 void WebDataService::UpdateKeyword(const TemplateURLData& data) {
-  wdbs_->ScheduleDBTask(
-      FROM_HERE, Bind(&WebDataService::UpdateKeywordImpl, this, data));
+  if (keyword_batch_mode_level_) {
+    queued_keyword_operations_.push_back(
+        KeywordTable::Operation(KeywordTable::UPDATE, data));
+  } else {
+    AdjustKeywordBatchModeLevel(true);
+    UpdateKeyword(data);
+    AdjustKeywordBatchModeLevel(false);
+  }
 }
 
 WebDataServiceBase::Handle WebDataService::GetKeywords(
@@ -122,10 +155,28 @@ WebDataServiceBase::Handle WebDataService::GetWebAppImages(
 WebDataService::WebDataService()
     : WebDataServiceBase(
           NULL, ProfileErrorCallback(),
-          BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI)) {
+          BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI)),
+      keyword_batch_mode_level_(0) {
 }
 
 WebDataService::~WebDataService() {
+  DCHECK(!keyword_batch_mode_level_);
+}
+
+void WebDataService::AdjustKeywordBatchModeLevel(bool entering_batch_mode) {
+  if (entering_batch_mode) {
+    ++keyword_batch_mode_level_;
+  } else {
+    DCHECK(keyword_batch_mode_level_);
+    --keyword_batch_mode_level_;
+    if (!keyword_batch_mode_level_ && !queued_keyword_operations_.empty()) {
+      wdbs_->ScheduleDBTask(
+          FROM_HERE,
+          Bind(&WebDataService::PerformKeywordOperationsImpl, this,
+               queued_keyword_operations_));
+      queued_keyword_operations_.clear();
+    }
+  }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -134,22 +185,10 @@ WebDataService::~WebDataService() {
 //
 ////////////////////////////////////////////////////////////////////////////////
 
-WebDatabase::State WebDataService::AddKeywordImpl(
-    const TemplateURLData& data, WebDatabase* db) {
-  return KeywordTable::FromWebDatabase(db)->AddKeyword(data) ?
-      WebDatabase::COMMIT_NEEDED : WebDatabase::COMMIT_NOT_NEEDED;
-}
-
-WebDatabase::State WebDataService::RemoveKeywordImpl(
-    TemplateURLID id, WebDatabase* db) {
-  DCHECK(id);
-  return KeywordTable::FromWebDatabase(db)->RemoveKeyword(id) ?
-      WebDatabase::COMMIT_NEEDED : WebDatabase::COMMIT_NOT_NEEDED;
-}
-
-WebDatabase::State WebDataService::UpdateKeywordImpl(
-    const TemplateURLData& data, WebDatabase* db) {
-  return KeywordTable::FromWebDatabase(db)->UpdateKeyword(data) ?
+WebDatabase::State WebDataService::PerformKeywordOperationsImpl(
+    const KeywordTable::Operations& operations,
+    WebDatabase* db) {
+  return KeywordTable::FromWebDatabase(db)->PerformOperations(operations) ?
       WebDatabase::COMMIT_NEEDED : WebDatabase::COMMIT_NOT_NEEDED;
 }
 
diff --git a/chrome/browser/webdata/web_data_service.h b/chrome/browser/webdata/web_data_service.h
index 0b54ded..c19e22e 100644
--- a/chrome/browser/webdata/web_data_service.h
+++ b/chrome/browser/webdata/web_data_service.h
@@ -93,6 +93,27 @@ class WebDataServiceConsumer;
 
 class WebDataService : public WebDataServiceBase {
  public:
+  // Instantiate this to turn on keyword batch mode on the provided |service|
+  // until the scoper is destroyed.  When batch mode is on, calls to any of the
+  // three keyword table modification functions below will result in locally
+  // queueing the operation; on setting this back to false, all the
+  // modifications will be performed at once.  This is a performance
+  // optimization; see comments on KeywordTable::PerformOperations().
+  //
+  // If multiple scopers are in-scope simultaneously, batch mode will only be
+  // exited when all are destroyed.  If |service| is NULL, the object will do
+  // nothing.
+  class KeywordBatchModeScoper {
+   public:
+    explicit KeywordBatchModeScoper(WebDataService* service);
+    ~KeywordBatchModeScoper();
+
+   private:
+    WebDataService* service_;
+
+    DISALLOW_COPY_AND_ASSIGN(KeywordBatchModeScoper);
+  };
+
   // Retrieve a WebDataService for the given context.
   static scoped_refptr<WebDataService> FromBrowserContext(
       content::BrowserContext* context);
@@ -176,6 +197,9 @@ class WebDataService : public WebDataServiceBase {
   virtual ~WebDataService();
 
  private:
+  // Called by the KeywordBatchModeScoper (see comments there).
+  void AdjustKeywordBatchModeLevel(bool entering_batch_mode);
+
   //////////////////////////////////////////////////////////////////////////////
   //
   // The following methods are only invoked on the DB thread.
@@ -187,12 +211,9 @@ class WebDataService : public WebDataServiceBase {
   // Keywords.
   //
   //////////////////////////////////////////////////////////////////////////////
-  WebDatabase::State AddKeywordImpl(const TemplateURLData& data,
-                                    WebDatabase* db);
-  WebDatabase::State RemoveKeywordImpl(TemplateURLID id,
-                                       WebDatabase* db);
-  WebDatabase::State UpdateKeywordImpl(const TemplateURLData& data,
-                                       WebDatabase* db);
+  WebDatabase::State PerformKeywordOperationsImpl(
+      const KeywordTable::Operations& operations,
+      WebDatabase* db);
   scoped_ptr<WDTypedResult> GetKeywordsImpl(WebDatabase* db);
   WebDatabase::State SetDefaultSearchProviderIDImpl(TemplateURLID id,
                                                     WebDatabase* db);
@@ -252,6 +273,9 @@ class WebDataService : public WebDataServiceBase {
       const IE7PasswordInfo& info, WebDatabase* db);
 #endif  // defined(OS_WIN)
 
+  size_t keyword_batch_mode_level_;
+  KeywordTable::Operations queued_keyword_operations_;
+
   DISALLOW_COPY_AND_ASSIGN(WebDataService);
 };