PasswordStoreMac: Use ScopedVector::weak_erase to improve performance

chrome/browser/password_manager/password_store_mac.cc

 // Copyright (c) 2012 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/password_manager/password_store_mac.h"
 #include "chrome/browser/password_manager/password_store_mac_internal.h"
 
 #include <CoreServices/CoreServices.h>
 #include <set>
 #include <string>
@@ skipping 177 matching lines @@
   return changes;
 }
 
 // Moves the content of |second| to the end of |first|.
 void AppendSecondToFirst(ScopedVector<autofill::PasswordForm>* first,
                          ScopedVector<autofill::PasswordForm>* second) {
   first->insert(first->end(), second->begin(), second->end());
   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 -
 
 // TODO(stuartmorgan): Convert most of this to private helpers in
 // MacKeychainPasswordFormAdapter once it has sufficient higher-level public
 // methods to provide test coverage.
 namespace internal_keychain_helpers {
 
 // Returns a URL built from the given components. To create a URL without a
@@ skipping 187 matching lines @@
   }
   bool equal_realm = form_a.signon_realm == form_b.signon_realm;
   if (strictness == FUZZY_FORM_MATCH) {
     equal_realm |= (!form_a.original_signon_realm.empty()) &&
                    form_a.original_signon_realm == form_b.signon_realm;
   }
   return form_a.scheme == form_b.scheme && equal_realm &&
          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);
 }
 
 // Takes |keychain_forms| and |database_forms| and moves the following 2 types
 // of forms to |merged_forms|: (1) blacklisted |database_forms|, (2)
 // |database_forms| which have a corresponding entry in |keychain_forms|. The
 // database forms of type (2) have their password value updated from the
 // corresponding keychain form, and all the keychain forms corresponding to some
 // database form are removed from |keychain_forms| and deleted.
 void MergePasswordForms(ScopedVector<autofill::PasswordForm>* keychain_forms,
                         ScopedVector<autofill::PasswordForm>* database_forms,
                         ScopedVector<autofill::PasswordForm>* merged_forms) {
   // Pull out the database blacklist items, since they are used as-is rather
   // than being merged with keychain forms.
   ExtractBlacklistForms(database_forms, merged_forms);
 
   // Merge the normal entries.
   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.
   ScopedVector<autofill::PasswordForm> unused_keychain_forms;
   unused_keychain_forms.reserve(keychain_forms->size());
   for (auto& keychain_form : *keychain_forms) {
     if (!ContainsKey(used_keychain_forms, keychain_form)) {
       unused_keychain_forms.push_back(keychain_form);
       keychain_form = nullptr;
     }
@@ skipping 34 matching lines @@
   std::vector<SecKeychainItemRef> keychain_items;
   std::vector<ItemFormPair> item_form_pairs =
       ExtractAllKeychainItemAttributesIntoPasswordForms(&keychain_items,
                                                         keychain);
 
   // Next, compare the attributes of the PasswordForms in |database_forms|
   // against those in |item_form_pairs|, and extract password data for each
   // 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(),
                                        item_form_pairs.end());
   for (SecKeychainItemRef item : keychain_items) {
     keychain.Free(item);
   }
 }
 
 // TODO(stuartmorgan): signon_realm for proxies is not yet supported.
@@ skipping 619 matching lines @@
                                                   &forms_with_keychain_entry);
 
   // Clean up any orphaned database entries.
   RemoveDatabaseForms(database_forms.get());
 
   // Move the orphaned DB forms to the output parameter.
   orphaned_forms->insert(orphaned_forms->end(), database_forms.begin(),
                          database_forms.end());
   database_forms.weak_clear();
 }