PasswordStoreMac: Use ScopedVector::weak_erase to improve performance

chrome/browser/password_manager/password_store_mac.cc

Index: chrome/browser/password_manager/password_store_mac.cc
diff --git a/chrome/browser/password_manager/password_store_mac.cc b/chrome/browser/password_manager/password_store_mac.cc
index 96518ffb46c3524bb441264a80799338b5a8df07..d53806d3bda3d02f217867d2fa938f3e95d0fc85 100644
--- a/chrome/browser/password_manager/password_store_mac.cc
+++ b/chrome/browser/password_manager/password_store_mac.cc
@@ -195,6 +195,103 @@ void AppendSecondToFirst(ScopedVector<autofill::PasswordForm>* first,
   second->weak_clear();
 }
 
+// Returns an the best match for |base_form| from |keychain_forms|, or NULL if
+// there is no suitable match.
+PasswordForm* BestKeychainFormForForm(

[vabr (Chromium), 2015/01/29 15:41:11]

This is a plain move out of the internal_keychain_helpers namespace, because the
function is not declared in the corresponding header and only used here. The
only change I made in the body was adding internal_keychain_helpers:: where
necessary.

[vasilii, 2015/01/29 16:41:12]

Acknowledged.

+    const PasswordForm& base_form,
+    const std::vector<PasswordForm*>* keychain_forms) {

[vasilii, 2015/01/29 16:41:13]

const std::vector<PasswordForm*>&

[vabr (Chromium), 2015/01/30 12:57:09]

Done.
(Originally I did not intend to modify this function at all, just move it. But I
guess it's short enough that it makes sense to do some improvements.)

+  PasswordForm* partial_match = NULL;
+  for (std::vector<PasswordForm*>::const_iterator i = keychain_forms->begin();
+       i != keychain_forms->end(); ++i) {
+    // TODO(stuartmorgan): We should really be scoring path matches and picking
+    // the best, rather than just checking exact-or-not (although in practice
+    // keychain items with paths probably came from us).
+    if (internal_keychain_helpers::FormsMatchForMerge(
+            base_form, *(*i), internal_keychain_helpers::FUZZY_FORM_MATCH)) {

[vasilii, 2015/01/29 16:41:13]

**i

[vabr (Chromium), 2015/01/30 12:57:09]

Done.

+      if (base_form.origin == (*i)->origin) {
+        return *i;
+      } else if (!partial_match) {
+        partial_match = *i;
+      }
+    }
+  }
+  return partial_match;
+}
+
+typedef base::Callback<void(scoped_ptr<autofill::PasswordForm>)>
+    TakeFormCallback;

[vasilii, 2015/01/29 16:41:13]

The typedef should go to the beginning of the file or to the only place where
you use it.

[vabr (Chromium), 2015/01/30 12:57:09]

Agreed, but I'll remove the typedef anyway.

+
+// Stores |form| in either |non_blacklist| or |blacklist|, depending on whether
+// |form| is blacklisted by the user.
+void ExtractBlacklistFormsCallback(

[vabr (Chromium), 2015/01/29 15:41:11]

The three callbacks correspond to the three places in the code below, where the
code was cut out.

+    ScopedVector<autofill::PasswordForm>* non_blacklist,
+    ScopedVector<autofill::PasswordForm>* blacklist,
+    scoped_ptr<autofill::PasswordForm> form) {
+  if (form->blacklisted_by_user)
+    blacklist->push_back(form.release());
+  else
+    non_blacklist->push_back(form.release());
+}
+
+// If |db_form|, coming from the login DB, has a match in |keychain_forms|, the
+// password of the match is copied into |db_form|, which is moved to
+// |merged_forms|. The used keychain form is also marked in
+// |used_keychain_forms|. Otherwise, |db_form| is moved to
+// |unused_database_forms|.
+void MergePasswordFormsCallback(
+    const std::vector<autofill::PasswordForm*>* keychain_forms,

[vasilii, 2015/01/29 16:41:13]

const std::vector<autofill::PasswordForm*>&

[vabr (Chromium), 2015/01/30 12:57:09]

Done.

+    std::set<autofill::PasswordForm*>* used_keychain_forms,
+    ScopedVector<autofill::PasswordForm>* unused_database_forms,
+    ScopedVector<autofill::PasswordForm>* merged_forms,
+    scoped_ptr<autofill::PasswordForm> db_form) {
+  PasswordForm* best_match = BestKeychainFormForForm(*db_form, keychain_forms);
+  if (best_match) {
+    used_keychain_forms->insert(best_match);
+    db_form->password_value = best_match->password_value;
+    merged_forms->push_back(db_form.release());
+  } else {
+    unused_database_forms->push_back(db_form.release());
+  }
+}
+
+// If there is a password stored in |keychain| for |db_form|, adds it to
+// |db_form| and moves |db_form| to |passwords|. Otherwise moves |db_form| to
+// |unused_db_forms|.
+void GetPasswordsForFormsCallback(
+    const AppleKeychain* keychain,
+    const std::vector<internal_keychain_helpers::ItemFormPair>& item_form_pairs,
+    ScopedVector<autofill::PasswordForm>* passwords,
+    ScopedVector<autofill::PasswordForm>* unused_db_forms,
+    scoped_ptr<autofill::PasswordForm> db_form) {
+  ScopedVector<autofill::PasswordForm> keychain_matches =
+      internal_keychain_helpers::ExtractPasswordsMergeableWithForm(
+          *keychain, item_form_pairs, *db_form);
+
+  ScopedVector<autofill::PasswordForm> db_form_container;
+  db_form_container.push_back(db_form.release());
+  internal_keychain_helpers::MergePasswordForms(&keychain_matches,
+                                                &db_form_container, passwords);
+  unused_db_forms->insert(unused_db_forms->end(), db_form_container.begin(),

[vasilii, 2015/01/29 16:41:13]

Do you want to reuse AppendSecondToFirst?

[vabr (Chromium), 2015/01/30 12:57:09]

Indeed! Thanks, it felt like I forgot something.

+                          db_form_container.end());
+  db_form_container.weak_clear();
+}
+
+// Iterates over all elements in |forms|, passes the pointed to objects to
+// |move_form|, and clears |forms| efficiently.
+void MoveAllFormsOut(ScopedVector<autofill::PasswordForm>* forms,

[vabr (Chromium), 2015/01/29 15:41:11]

This is the only function which gets to use weak_erase().

+                     TakeFormCallback move_form) {
+  for (auto* form_ptr : *forms) {

[vasilii, 2015/01/29 16:41:13]

autofill::PasswordForm*

[vabr (Chromium), 2015/01/30 12:57:09]

Done.

+    scoped_ptr<autofill::PasswordForm> form(form_ptr);
+    move_form.Run(form.Pass());
+  }
+  // We moved the ownership of every form out of |forms|. For performance
+  // reasons, we can just weak_clear it, instead of nullptr-ing the respective
+  // elements and letting the vector's destructor to go through the list once
+  // more. This was tested on a banchmark, and seemed to make a difference on
+  // Mac.
+  forms->weak_clear();
+}
+
 }  // namespace
 
 #pragma mark -
@@ -402,40 +499,15 @@ bool FormsMatchForMerge(const PasswordForm& form_a,
          form_a.username_value == form_b.username_value;
 }
 
-// Returns an the best match for |base_form| from |keychain_forms|, or NULL if
-// there is no suitable match.
-PasswordForm* BestKeychainFormForForm(
-    const PasswordForm& base_form,
-    const std::vector<PasswordForm*>* keychain_forms) {
-  PasswordForm* partial_match = NULL;
-  for (std::vector<PasswordForm*>::const_iterator i = keychain_forms->begin();
-       i != keychain_forms->end(); ++i) {
-    // TODO(stuartmorgan): We should really be scoring path matches and picking
-    // the best, rather than just checking exact-or-not (although in practice
-    // keychain items with paths probably came from us).
-    if (FormsMatchForMerge(base_form, *(*i), FUZZY_FORM_MATCH)) {
-      if (base_form.origin == (*i)->origin) {
-        return *i;
-      } else if (!partial_match) {
-        partial_match = *i;
-      }
-    }
-  }
-  return partial_match;
-}
-
 // Moves entries from |forms| that are blacklist entries into |blacklist|.
 void ExtractBlacklistForms(ScopedVector<autofill::PasswordForm>* forms,
                            ScopedVector<autofill::PasswordForm>* blacklist) {
   blacklist->reserve(blacklist->size() + forms->size());
   ScopedVector<autofill::PasswordForm> non_blacklist;
-  for (auto& form : *forms) {
-    if (form->blacklisted_by_user)
-      blacklist->push_back(form);
-    else
-      non_blacklist.push_back(form);
-    form = nullptr;
-  }
+  // Move forms in either |non_blacklist| or |blacklist|, depending on whether
+  // they are blacklisted by the user.
+  MoveAllFormsOut(forms, base::Bind(&ExtractBlacklistFormsCallback,
+                                    &non_blacklist, blacklist));
   forms->swap(non_blacklist);
 }
 
@@ -456,18 +528,14 @@ void MergePasswordForms(ScopedVector<autofill::PasswordForm>* keychain_forms,
   ScopedVector<autofill::PasswordForm> unused_database_forms;
   unused_database_forms.reserve(database_forms->size());
   std::set<autofill::PasswordForm*> used_keychain_forms;
-  for (auto& db_form : *database_forms) {
-    PasswordForm* best_match =
-        BestKeychainFormForForm(*db_form, &keychain_forms->get());
-    if (best_match) {
-      used_keychain_forms.insert(best_match);
-      db_form->password_value = best_match->password_value;
-      merged_forms->push_back(db_form);
-    } else {
-      unused_database_forms.push_back(db_form);
-    }
-    db_form = nullptr;
-  }
+  // Move all database forms to either |merged_forms| or
+  // |unused_database_forms|, based on whether they have a match in the keychain
+  // forms or not. If there is a match, add its password to the DB form and
+  // mark the keychain form as used.
+  MoveAllFormsOut(
+      database_forms,
+      base::Bind(&MergePasswordFormsCallback, &keychain_forms->get(),
+                 &used_keychain_forms, &unused_database_forms, merged_forms));
   database_forms->swap(unused_database_forms);
 
   // Clear out all the Keychain entries we used.
@@ -522,18 +590,11 @@ void GetPasswordsForForms(const AppleKeychain& keychain,
   // matching PasswordForm using its corresponding SecKeychainItemRef.
   ScopedVector<autofill::PasswordForm> unused_db_forms;
   unused_db_forms.reserve(database_forms->size());
-  for (auto& db_form : *database_forms) {
-    ScopedVector<autofill::PasswordForm> keychain_matches =
-        ExtractPasswordsMergeableWithForm(keychain, item_form_pairs, *db_form);
-
-    ScopedVector<autofill::PasswordForm> db_form_container;
-    db_form_container.push_back(db_form);
-    db_form = nullptr;
-    MergePasswordForms(&keychain_matches, &db_form_container, passwords);
-    unused_db_forms.insert(unused_db_forms.end(), db_form_container.begin(),
-                           db_form_container.end());
-    db_form_container.weak_clear();
-  }
+  // Move database forms with a password stored in |keychain| to |passwords|,
+  // including the password. The rest is moved to |unused_db_forms|.
+  MoveAllFormsOut(database_forms,
+                  base::Bind(&GetPasswordsForFormsCallback, &keychain,
+                             item_form_pairs, passwords, &unused_db_forms));
   database_forms->swap(unused_db_forms);
 
   STLDeleteContainerPairSecondPointers(item_form_pairs.begin(),