Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
885033002:
PasswordStoreMac: Use ScopedVector::weak_erase to improve performance (Closed)
|
Created:
5 years, 10 months ago by vabr (Chromium) Modified:
5 years, 10 months ago Reviewers:
vasilii CC:
chromium-reviews, gcasto+watchlist_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPasswordStoreMac: Use ScopedVector::weak_erase to improve performance
Following up on https://codereview.chromium.org/825773003/#msg26, this CL adds weak_erase() calls at some places, to speed-up deletion of ScopedVectors, which are arguably only containing null or dangling pointers.
The benchmark from https://codereview.chromium.org/825773003/#msg24 showed on Mac, unlike on Linux, that weak_erase() + destruction takes (not surprisingly) constant time, whereas ommitting the weak_erase() is linear in the size of the vector.
The CL adds the weak_erase in a slightly complicated way. Instead of just adding the weak_erase() calls, it creates a dedicated method for doing the operation of passing all owned objects out of a scoped vector, to ensure that future changes in the code don't easily add leaks through the weak_ptr.
BUG=451018
Committed: https://crrev.com/c765343eb78c8199ec37aabbd9c0e8f1b79b447c
Cr-Commit-Position: refs/heads/master@{#313952}
Patch Set 1 : #
Total comments: 16
Patch Set 2 : Just rebased #Patch Set 3 : WIP: lambdas #Patch Set 4 : WIP Lambdas (2) #Patch Set 5 : WIP: lambdas (3) #Patch Set 6 : Lambdas, without scoped_ptr #Patch Set 7 : Lambdas + scoped_ptr #
Total comments: 8
Patch Set 8 : Comments addressed #Patch Set 9 : Compily compily comp. #Messages
Total messages: 24 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
vabr@chromium.org changed reviewers: + vasilii@chromium.org
Hi Vasilii, Could you please have a look? Thanks! Vaclav https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:200: PasswordForm* BestKeychainFormForForm( 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. https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:226: void ExtractBlacklistFormsCallback( The three callbacks correspond to the three places in the code below, where the code was cut out. https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:281: void MoveAllFormsOut(ScopedVector<autofill::PasswordForm>* forms, This is the only function which gets to use weak_erase().
https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:200: PasswordForm* BestKeychainFormForForm( On 2015/01/29 15:41:11, vabr (Chromium) wrote: > 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. Acknowledged. https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:202: const std::vector<PasswordForm*>* keychain_forms) { const std::vector<PasswordForm*>& https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:210: base_form, *(*i), internal_keychain_helpers::FUZZY_FORM_MATCH)) { **i https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:222: TakeFormCallback; The typedef should go to the beginning of the file or to the only place where you use it. https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:242: const std::vector<autofill::PasswordForm*>* keychain_forms, const std::vector<autofill::PasswordForm*>& https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:274: unused_db_forms->insert(unused_db_forms->end(), db_form_container.begin(), Do you want to reuse AppendSecondToFirst? https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:283: for (auto* form_ptr : *forms) { autofill::PasswordForm*
Thanks, Vasilii. Please have a look at patch set 6. The main change is the introduction of the lambda-functions in connection with the weak_erase()-using method. Side changes still include moving BestKeychainFormForForm to the anonymous namespace, as well as some minor code improvements. I'll archive the benchmark I did at https://codereview.chromium.org/885293002/. If there are any interesting findings about scoped_ptr's efficiency, I'll at least file a bug. Cheers, Vaclav https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:202: const std::vector<PasswordForm*>* keychain_forms) { On 2015/01/29 16:41:13, vasilii wrote: > const std::vector<PasswordForm*>& 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.) https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:210: base_form, *(*i), internal_keychain_helpers::FUZZY_FORM_MATCH)) { On 2015/01/29 16:41:13, vasilii wrote: > **i Done. https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:222: TakeFormCallback; On 2015/01/29 16:41:13, vasilii wrote: > The typedef should go to the beginning of the file or to the only place where > you use it. Agreed, but I'll remove the typedef anyway. https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:242: const std::vector<autofill::PasswordForm*>* keychain_forms, On 2015/01/29 16:41:13, vasilii wrote: > const std::vector<autofill::PasswordForm*>& Done. https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:274: unused_db_forms->insert(unused_db_forms->end(), db_form_container.begin(), On 2015/01/29 16:41:13, vasilii wrote: > Do you want to reuse AppendSecondToFirst? Indeed! Thanks, it felt like I forgot something. https://codereview.chromium.org/885033002/diff/60001/chrome/browser/password_... chrome/browser/password_manager/password_store_mac.cc:283: for (auto* form_ptr : *forms) { On 2015/01/29 16:41:13, vasilii wrote: > autofill::PasswordForm* Done.
Hi Vasilii, This time I checked it compiles. Also, as discussed offline, the benchmark was skewed probably due to some caching. When I corrected it, using the lambda functions with the scoped_ptr did seem to have an equivalent performance as just inlining weak_clear(). Therefore I went for the lambdas and scoped_ptr, because that is the only option which transfers the ownership fully clearly, and keeps the code readable. Please have a look. Cheers, Vaclav
lgtm https://codereview.chromium.org/885033002/diff/170001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/885033002/diff/170001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:222: // |move_form|, and clears |forms| efficiently. Expand the comment saying something about FormMover (that it is a callable and so on). https://codereview.chromium.org/885033002/diff/170001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:228: mover(form.Pass()); mover(scoped_ptr<autofill::PasswordForm>(form_ptr)); https://codereview.chromium.org/885033002/diff/170001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:233: // more. This was tested on a banchmark, and seemed to make a difference on bEnchmark
Thanks, Vasilii. I addressed your comments and will push patch set 7 to the CQ. Cheers, Vaclav https://codereview.chromium.org/885033002/diff/170001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/885033002/diff/170001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:222: // |move_form|, and clears |forms| efficiently. On 2015/01/30 14:43:34, vasilii wrote: > Expand the comment saying something about FormMover (that it is a callable and > so on). Done. https://codereview.chromium.org/885033002/diff/170001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:228: mover(form.Pass()); On 2015/01/30 14:43:34, vasilii wrote: > mover(scoped_ptr<autofill::PasswordForm>(form_ptr)); Done. https://codereview.chromium.org/885033002/diff/170001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:233: // more. This was tested on a banchmark, and seemed to make a difference on On 2015/01/30 14:43:34, vasilii wrote: > bEnchmark Done.
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/885033002/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/885033002/diff/170001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/885033002/diff/170001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:228: mover(form.Pass()); On 2015/01/30 14:49:51, vabr (Chromium) wrote: > On 2015/01/30 14:43:34, vasilii wrote: > > mover(scoped_ptr<autofill::PasswordForm>(form_ptr)); > > Done. Just copy-paste my comment :-)
https://codereview.chromium.org/885033002/diff/170001/chrome/browser/password... File chrome/browser/password_manager/password_store_mac.cc (right): https://codereview.chromium.org/885033002/diff/170001/chrome/browser/password... chrome/browser/password_manager/password_store_mac.cc:228: mover(form.Pass()); On 2015/01/30 15:29:08, vasilii wrote: > On 2015/01/30 14:49:51, vabr (Chromium) wrote: > > On 2015/01/30 14:43:34, vasilii wrote: > > > mover(scoped_ptr<autofill::PasswordForm>(form_ptr)); > > > > Done. > > Just copy-paste my comment :-) Hopefully done this time. :)
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/885033002/210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/885033002/210001
Message was sent while issue was closed.
Committed patchset #9 (id:210001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/c765343eb78c8199ec37aabbd9c0e8f1b79b447c Cr-Commit-Position: refs/heads/master@{#313952} |
