88ms startup performance regression in autofill::AddressNormalizerFactory::GetInstance() |
||||||||||
Issue descriptionData from the UMA Sampling Profiler shows that calls to autofill::AddressNormalizerFactory::GetInstance() during pre-message loop start regressed browser process UI thread startup on 64-bit Chrome on Windows by 88ms. This occurred in the 64.0.3279.0 canary release. The responsible CL appears to be https://chromium.googlesource.com/chromium/src.git/+/1b492842fcc2c05087efbc8fa9be477ebc758002. Flame graph difference of canary 64.0.3278.0 vs. 64.0.3279.0 for the function: https://uma.googleplex.com/p/chrome/callstacks?sid=bf637233243457e2cdba7d761d2924cb
,
Nov 30 2017
It looks like its the creation of the ICU Collator, specifically the load of the collation data in the Collation root which is taking this time. AFAICT, this change has triggered a syncronous file open + read from ICU common data.
,
Nov 30 2017
Thanks. I was actually wondering why autofill::AddressNormalizerFactory::GetInstance() would be called on Chrome start. I'll investigate this piece.
,
Nov 30 2017
It's created in the context of the tab. There's an autofill driver attached to the webcontents, which interacts with the AutofillManager, which is initialized with a normalizer.
,
Nov 30 2017
I'll do two things here: * Remove AutofillManager/AddressNormalizer from the startup path. * Inside the AddressNormalizer, move the initialization of the expensive member to a background task on another thread.
,
Nov 30 2017
Focusing on the second bullet for now, and filed crbug.com/790542 for the first.
,
Dec 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/22663635c786e62d5958d7cc6eb56ec39d2f2b6d commit 22663635c786e62d5958d7cc6eb56ec39d2f2b6d Author: Mathieu Perreault <mathp@chromium.org> Date: Fri Dec 01 20:27:44 2017 [Autofill] Do not create the AddressNormalizer in constructor Bug: 789711 Change-Id: Iae6bc67ba67f8f9e492bd065a8d75903b7cf92a9 Reviewed-on: https://chromium-review.googlesource.com/801457 Commit-Queue: Mathieu Perreault <mathp@chromium.org> Reviewed-by: Roger McFarlane <rogerm@chromium.org> Cr-Commit-Position: refs/heads/master@{#521038} [modify] https://crrev.com/22663635c786e62d5958d7cc6eb56ec39d2f2b6d/chrome/browser/autofill/android/personal_data_manager_android.cc [modify] https://crrev.com/22663635c786e62d5958d7cc6eb56ec39d2f2b6d/chrome/browser/autofill/android/personal_data_manager_android.h
,
Dec 4 2017
,
Dec 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dedd6128557ac421342b4103c60f32240e58a191 commit dedd6128557ac421342b4103c60f32240e58a191 Author: Mathieu Perreault <mathp@chromium.org> Date: Fri Dec 08 21:38:31 2017 [Autofill] Create AddressValidator in a background task. Will start a background task to create an AddressValidator in the constructor. When the AddressValidator is loaded, it will load rules for any pending normalization request. Bug: 789711 Change-Id: I59542f6a1bbaba1a58805ab3d51fa7c7b1ceeff3 Reviewed-on: https://chromium-review.googlesource.com/806715 Reviewed-by: Roger McFarlane <rogerm@chromium.org> Reviewed-by: François Doray <fdoray@chromium.org> Commit-Queue: Mathieu Perreault <mathp@chromium.org> Cr-Commit-Position: refs/heads/master@{#522875} [modify] https://crrev.com/dedd6128557ac421342b4103c60f32240e58a191/components/autofill/core/browser/address_normalizer_impl.cc [modify] https://crrev.com/dedd6128557ac421342b4103c60f32240e58a191/components/autofill/core/browser/address_normalizer_impl.h [modify] https://crrev.com/dedd6128557ac421342b4103c60f32240e58a191/components/autofill/core/browser/address_normalizer_impl_unittest.cc [modify] https://crrev.com/dedd6128557ac421342b4103c60f32240e58a191/components/autofill/core/browser/field_filler_unittest.cc
,
Dec 9 2017
,
Dec 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c3c8e19beafb7cae637ce0ac181699477cb8cdbc commit c3c8e19beafb7cae637ce0ac181699477cb8cdbc Author: Kenneth Russell <kbr@chromium.org> Date: Sat Dec 09 16:11:06 2017 Revert "[Autofill] Create AddressValidator in a background task." This reverts commit dedd6128557ac421342b4103c60f32240e58a191. Reason for revert: Likely cause of http://crbug.com/793520 . Original change's description: > [Autofill] Create AddressValidator in a background task. > > Will start a background task to create an AddressValidator in the > constructor. > > When the AddressValidator is loaded, it will load rules for any > pending normalization request. > > Bug: 789711 > Change-Id: I59542f6a1bbaba1a58805ab3d51fa7c7b1ceeff3 > Reviewed-on: https://chromium-review.googlesource.com/806715 > Reviewed-by: Roger McFarlane <rogerm@chromium.org> > Reviewed-by: François Doray <fdoray@chromium.org> > Commit-Queue: Mathieu Perreault <mathp@chromium.org> > Cr-Commit-Position: refs/heads/master@{#522875} TBR=rogerm@chromium.org,fdoray@chromium.org,mathp@chromium.org NOTRY=true Change-Id: Ifec15561b6e7a5466dfccb47922e8173e3666552 No-Tree-Checks: true Bug: 789711 , 793520 Reviewed-on: https://chromium-review.googlesource.com/817766 Commit-Queue: Kenneth Russell <kbr@chromium.org> Reviewed-by: Kenneth Russell <kbr@chromium.org> Cr-Commit-Position: refs/heads/master@{#522999} [modify] https://crrev.com/c3c8e19beafb7cae637ce0ac181699477cb8cdbc/components/autofill/core/browser/address_normalizer_impl.cc [modify] https://crrev.com/c3c8e19beafb7cae637ce0ac181699477cb8cdbc/components/autofill/core/browser/address_normalizer_impl.h [modify] https://crrev.com/c3c8e19beafb7cae637ce0ac181699477cb8cdbc/components/autofill/core/browser/address_normalizer_impl_unittest.cc [modify] https://crrev.com/c3c8e19beafb7cae637ce0ac181699477cb8cdbc/components/autofill/core/browser/field_filler_unittest.cc
,
Dec 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/80b8b184a7a469eed3399d79c8bed9380f789942 commit 80b8b184a7a469eed3399d79c8bed9380f789942 Author: Mathieu Perreault <mathp@chromium.org> Date: Mon Dec 11 20:05:50 2017 [Reland] [Autofill] Create AddressValidator in a background task. Will start a background task to create an AddressValidator in the constructor. When the AddressValidator is loaded, it will load rules for any pending normalization request. Reland with ValidationRulesStorageFactory destruction fix. Bug: 789711 Test: components_unittests Change-Id: Ib08581c5afd48ddfe016d3024f28b228e33f646f Reviewed-on: https://chromium-review.googlesource.com/819594 Reviewed-by: Roger McFarlane <rogerm@chromium.org> Commit-Queue: Mathieu Perreault <mathp@chromium.org> Cr-Commit-Position: refs/heads/master@{#523182} [modify] https://crrev.com/80b8b184a7a469eed3399d79c8bed9380f789942/chrome/browser/autofill/validation_rules_storage_factory.cc [modify] https://crrev.com/80b8b184a7a469eed3399d79c8bed9380f789942/components/autofill/core/browser/address_normalizer_impl.cc [modify] https://crrev.com/80b8b184a7a469eed3399d79c8bed9380f789942/components/autofill/core/browser/address_normalizer_impl.h [modify] https://crrev.com/80b8b184a7a469eed3399d79c8bed9380f789942/components/autofill/core/browser/address_normalizer_impl_unittest.cc [modify] https://crrev.com/80b8b184a7a469eed3399d79c8bed9380f789942/components/autofill/core/browser/field_filler_unittest.cc
,
Dec 12 2017
,
Dec 12 2017
,
Dec 12 2017
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 12 2017
Thanks for the fix mathp@ - is it well tested and verified in canary? The change looks to be quite large and I'd like to ensure this won't introduce any new regressions.
,
Dec 14 2017
Yes, and I don't see crashes in Canary versions.
,
Dec 15 2017
Approving merge to M64. Branch:3282
,
Dec 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f1881b23769ee6f0e9316e1baf299660c4699e8d commit f1881b23769ee6f0e9316e1baf299660c4699e8d Author: Mathieu Perreault <mathp@chromium.org> Date: Fri Dec 15 18:25:59 2017 [Autofill] Do not create the AddressNormalizer in constructor TBR=mathp@chromium.org (cherry picked from commit 22663635c786e62d5958d7cc6eb56ec39d2f2b6d) Bug: 789711 Change-Id: Iae6bc67ba67f8f9e492bd065a8d75903b7cf92a9 Reviewed-on: https://chromium-review.googlesource.com/801457 Commit-Queue: Mathieu Perreault <mathp@chromium.org> Reviewed-by: Roger McFarlane <rogerm@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#521038} Reviewed-on: https://chromium-review.googlesource.com/830214 Reviewed-by: Mathieu Perreault <mathp@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#239} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [modify] https://crrev.com/f1881b23769ee6f0e9316e1baf299660c4699e8d/chrome/browser/autofill/android/personal_data_manager_android.cc [modify] https://crrev.com/f1881b23769ee6f0e9316e1baf299660c4699e8d/chrome/browser/autofill/android/personal_data_manager_android.h
,
Dec 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4f3e920edf0a77863dfa70fc97ef02d0b6ff999d commit 4f3e920edf0a77863dfa70fc97ef02d0b6ff999d Author: Mathieu Perreault <mathp@chromium.org> Date: Fri Dec 15 18:35:58 2017 [Autofill] Create AddressValidator in a background task. Will start a background task to create an AddressValidator in the constructor. When the AddressValidator is loaded, it will load rules for any pending normalization request. TBR=mathp@chromium.org (cherry picked from commit dedd6128557ac421342b4103c60f32240e58a191) Bug: 789711 Change-Id: I59542f6a1bbaba1a58805ab3d51fa7c7b1ceeff3 Reviewed-on: https://chromium-review.googlesource.com/806715 Reviewed-by: Roger McFarlane <rogerm@chromium.org> Reviewed-by: François Doray <fdoray@chromium.org> Commit-Queue: Mathieu Perreault <mathp@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#522875} Reviewed-on: https://chromium-review.googlesource.com/830194 Reviewed-by: Mathieu Perreault <mathp@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#240} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [modify] https://crrev.com/4f3e920edf0a77863dfa70fc97ef02d0b6ff999d/components/autofill/core/browser/address_normalizer_impl.cc [modify] https://crrev.com/4f3e920edf0a77863dfa70fc97ef02d0b6ff999d/components/autofill/core/browser/address_normalizer_impl.h [modify] https://crrev.com/4f3e920edf0a77863dfa70fc97ef02d0b6ff999d/components/autofill/core/browser/address_normalizer_impl_unittest.cc [modify] https://crrev.com/4f3e920edf0a77863dfa70fc97ef02d0b6ff999d/components/autofill/core/browser/field_filler_unittest.cc
,
Jan 5 2018
Bulk adding Performance-Browser label for bugs identified with the UMA Sampling Profiler. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by ma...@chromium.org
, Nov 30 2017Labels: M-64