search::GetNewTabPageURL() reads the RLZ brand registry key every time it's called |
|||||||||
Issue descriptionThis is taking 0.38% of non-idle runtime on average. Seems like we could and should avoid this. Stack profile of all the search::GetNewTabPageURL() calls in a recent canary: https://uma.googleplex.com/p/chrome/callstacks?sid=60e7460232e75e2d4faa5da455c0cece Looks like this is happening through the google_brand::GetBrand() call during the search URL terms replacement.
,
Jan 26 2018
,
Jan 26 2018
On Windows, I could see adding the brand code to install_static::InstallDetails::Payload so that it is read once during process startup (in product_install_details.cc:MakeProductDetails and setup_install_details.cc:MakeInstallDetails) and henceforth available to any caller for free. I'd be happy to review such a contribution from someone else, though I don't have cycles to work on this now. Perhaps later in the quarter if no one picks it up before then.
,
Jan 26 2018
Can the brand code change dynamically? If not, why not just add a simple static that caches the value once we've retrieved it once?
,
Jan 26 2018
The approach I described provides a test seam (ScopedInstallDetails) so that code using the brand code can be tested without registry redirection hijinx. You could certainly add ForTesting functions to diddle with the cached value with your approach.
,
Jan 29 2018
,
Jan 29 2018
,
Jan 30 2018
If you're touching this code, please also see issue 800838.
,
Jan 30 2018
Note that issue 723072 mentions plist and thus Mac seems to be affected (unless this has changed in the meantime).
,
Jan 30 2018
It looks like the Mac side has been fixed (or if not the performance impact is so low as to be negligible). In the Mac startup execution profile (first 30 seconds of startup) search::GetNewTabPageURL() executes for just 323μs[1], and the function is too small to visible in the steady-state execution profile. [1] https://uma.googleplex.com/p/chrome/callstacks?sid=6f288a9f12efbeae4046c66545d69601
,
Jan 30 2018
The perf measurements of this bug are tricky, as this is a repeated read of the same file over and over resulting in most likely hitting the file cache of the OS, unless the OS is under memory pressure. For Windows the best way to verify the bug is via the Process explorer to filter out everything else, but registry read of Chrome of the brand code. On Mac, the problem would be trickier. There are DTrace scripts that allow tracking of each file access, but they require disabling System Integrity Protection. Honestly, for verifying on Mac, it would be sufficient if someone with Mac build simply drops a temporary Debug logging at the point of reading the brand code and runs Chrome, opening a few tabs, observing the live logs.
,
Jan 30 2018
Perf measurements here are actually fairly trivial using the profiles generated by the UMA sampling profiler (the ones I've linked in this bug). The profiles represent aggregated execution time across the call graph for all canary users, so if the function doesn't show up as a significant in these profiles it can't be having a significant effect on execution time over the population. I don't know if the Mac implementation is regularly hitting the file cache, but if it is it's not having any significant effect on execution performance so probably not worth spending much effort trying to address.
,
Jan 30 2018
,
Mar 3 2018
https://chromium-review.googlesource.com/c/chromium/src/+/947522
,
Mar 6 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9dd1ef42a8a33c5ba09ec645150f580724979b36 commit 9dd1ef42a8a33c5ba09ec645150f580724979b36 Author: Alexei Svitkine <asvitkine@chromium.org> Date: Tue Mar 06 00:55:47 2018 Cache brand code on Windows. Querying the registry is expensive and contributes to start up time, omnibox query time and idle. Since the brand code is not expected to change on Windows during the course of a session, this CL caches the value, so that future queries are free. This should restore the Omnibox perf that regressed by https://chromium-review.googlesource.com/c/chromium/src/+/824363 and also improve perf elsewhere since the caching is now done at a lower level than that change. Since this is expensive only on Windows, the caching is done in the Windows codepath only. Bug: 816698 , 806130 Change-Id: I458fad29f18b1b1ff2d55b9334ae733fa448eec9 Reviewed-on: https://chromium-review.googlesource.com/947522 Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Reviewed-by: Greg Thompson <grt@chromium.org> Cr-Commit-Position: refs/heads/master@{#541009} [modify] https://crrev.com/9dd1ef42a8a33c5ba09ec645150f580724979b36/chrome/browser/google/google_brand.cc [modify] https://crrev.com/9dd1ef42a8a33c5ba09ec645150f580724979b36/chrome/browser/google/google_brand.h
,
Mar 6 2018
Marking as Fixed per CL above. Can verify with sampling profiler data once we have it in UMA (tomorrow, I guess assuming the change went out with today's canary.)
,
Mar 6 2018
Thanks for the fix, this is a great improvement.
,
Mar 6 2018
I wouldn't call it an improvement, more like fixing the regression from the reverted fix, see (https://bugs.chromium.org/p/chromium/issues/detail?id=723072), fixed close to 1 year ago. That older bug should have been reopened with this one (806130) merged towards it, as the original issue still lingers for Mac. asvitkine@ - thanks for fixing this for Windows and applying the fix at the proper code place. I will take a look at fixing it for Mac too, based on the code that you put in. Unless folks are against it, I will re-link the bugs accordingly, so that we can track the root cause from the start.
,
Mar 6 2018
borisv: The CL is doing the caching at a different level that would benefit other callers like NTP or e.g. UMA reporting. The other CL was doing it only for RLZ checks.
,
Mar 6 2018
Although looking at your linked bug, it is talking about the more general problem even if the original CL that landed for it was more specific. So yeah, we could re-open that one and dupe this one into it instead. I didn't realise there was also a Mac perf issue - since we didn't see an effect on Omnibox performance on Mac when the original CL was reverted. I guess we could look more at sampling profiler data to see how much this affects Mac? Though given fix is pretty simple and if we know Mac impl reads plist, seems fine to just fix there as well.
,
Mar 6 2018
I see. Good clarification. That said, the other bug is still valid then, as we have not addressed the Mac problem, so we should reopen it. Mac will end up loading a large plist file and searching through it tons of times. See https://cs.chromium.org/chromium/src/chrome/browser/mac/keystone_glue.mm?rcl=34b3f727653506b39c608decc378695b0c865480&l=1168. For some reason, though I don't get any UMA data for Mac, but I am sure there is regression there too. Luckily, the Windows version of the GetBrand in google_brand.cc can be made less platform specific to call either GoogleUpdateSettings::GetBrand() or KeystoneGlue::BrandCode() based on the OS, caching both results. I will take a look at that later this week when I switch to Chromium development. I am not working actively in Chromium.
,
Mar 6 2018
It's indeed strange that we don't see any Mac data about GetBrand in sampling profiler. Do we simply not have enough samples due to lower population size? Or are Macs almost always on SSDs and we don't see the impact?
,
Mar 6 2018
Good question. I don't see any Mac data, not only GetBrand related. See here: https://uma.googleplex.com/p/chrome/callstacks?sid=047e0d261891f03a0d4e7f9a8a7d4212. Maybe we didn't ship this canary release to Mac at all?
,
Mar 6 2018
I checked other versions including most recent Mac Dev release - which had data in general, but nothing for the brand function.
,
Mar 6 2018
Looking at the underlying data we do see calls to GetBrand on Mac, but they're so rare that they don't contribute any appreciable execution time and thus they're not visible in the sampling profiler dashboard. In 67.0.3361.0 for example, there were a total of 8 samples recorded in google_brand::GetBrand(). Here's an example stack: google_brand::GetBrand(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*) UIThreadSearchTermsData::GetRlzParameterValue(bool) const TemplateURLRef::HandleReplacements(TemplateURLRef::SearchTermsArgs const&, SearchTermsData const&, std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >*) const TemplateURLRef::ReplaceSearchTerms(TemplateURLRef::SearchTermsArgs const&, SearchTermsData const&, std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >*) const search::(anonymous namespace)::NewTabURLDetails::ForProfile(Profile*) search::IsInstantNTPURL(GURL const&, Profile*) OmniboxEditModel::ClassifyPage() const OmniboxEditModel::StartAutocomplete(bool, bool) OmniboxEditModel::UpdateInput(bool, bool) OmniboxEditModel::OnAfterPossibleChange(OmniboxView::StateChanges const&, bool) OmniboxViewMac::OnAfterPossibleChange(bool) -[AutocompleteTextFieldEditor interpretKeyEvents:] <unsymbolized function> <unsymbolized function> <unsymbolized function> -[ChromeEventProcessingWindow sendEvent:] <unsymbolized function> __34-[BrowserCrApplication sendEvent:]_block_invoke base::mac::CallWithEHFrame(void () block_pointer) -[BrowserCrApplication sendEvent:] <unsymbolized function> base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) <name omitted> ChromeBrowserMainParts::MainMessageLoopRun(int*) content::BrowserMainLoop::RunMainMessageLoopParts() content::BrowserMainRunnerImpl::Run() content::BrowserMain(content::MainFunctionParams const&) content::ContentMainRunnerImpl::Run() service_manager::Main(service_manager::MainParams const&) content::ContentMain(content::ContentMainParams const&) ChromeMain main
,
Mar 6 2018
It's possible that code called by this function is disproportionately likely to be in the 4% of stacks that we're unable to unwind on Mac and so we're missing samples, but this seems unlikely. Let me take a look at the functions being invoked by google_brand::GetBrand() and see if there's anything obviously missing.
,
Mar 6 2018
Good point. I am wondering if we have a bigger bug with Windows code doing more work than it needs. We made this work resource cheap by caching stuff, but the work still does not have to be done at all.
,
Mar 6 2018
Ah, it looks like we don't actually load the brand file outside of the stable channel: https://cs.chromium.org/chromium/src/chrome/browser/mac/keystone_glue.mm?rcl=9350d9dc037bfed22f896f8b324e589f7df0f895&l=331-359
,
Mar 7 2018
wittman: ah, good find. That would explain why we don't see it. borisv: I'm wondering whether the logic could be updated so that Stable and other channels work the same - as they do on Windows? If channels work differently, then we lose visibility into performance (and functionality) issues on earlier channels - such as in this case, where had Mike not found the logic difference, we would have concluded that the brand code is very cheap to get on Mac based on canary/dev data.
,
Mar 7 2018
Yep, indeed a good find! I was looking at that file and still missed it. Adding mark@, who I believe wrote the original code. This was probably an optimization, given that we do not ship different brand codes for the canaries. That said, this optimization masked the performance issue on stable, so yes, I am all for updating it. I will put an update later this week. Ideally, we should see a perf regression on the next canary that should confirm the perf problem with stable. Then I will update the brand caching code to fix it for Mac too.
,
Mar 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b93f6ee55444118d4dcb237591a5786dbe374e87 commit b93f6ee55444118d4dcb237591a5786dbe374e87 Author: Alexei Svitkine <asvitkine@chromium.org> Date: Mon Mar 12 16:34:40 2018 Revert "Cache brand code on Windows." This reverts commit 9dd1ef42a8a33c5ba09ec645150f580724979b36. Reason for revert: Latest canary data did not have expected recovery on Omnibox.CharTypedToRepaintLatency and had a regression on Omnibox.CharTypedToRepaintLatency.ToPaint. Reverting to see if that movement is related to this CL or whether there were other changes in the meanwhile. Original change's description: > Cache brand code on Windows. > > Querying the registry is expensive and contributes to start up time, > omnibox query time and idle. Since the brand code is not expected to > change on Windows during the course of a session, this CL caches > the value, so that future queries are free. > > This should restore the Omnibox perf that regressed by > https://chromium-review.googlesource.com/c/chromium/src/+/824363 > and also improve perf elsewhere since the caching is now done at > a lower level than that change. Since this is expensive only on > Windows, the caching is done in the Windows codepath only. > > Bug: 816698 , 806130 > Change-Id: I458fad29f18b1b1ff2d55b9334ae733fa448eec9 > Reviewed-on: https://chromium-review.googlesource.com/947522 > Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> > Reviewed-by: Peter Kasting <pkasting@chromium.org> > Reviewed-by: Greg Thompson <grt@chromium.org> > Cr-Commit-Position: refs/heads/master@{#541009} TBR=pkasting@chromium.org,asvitkine@chromium.org,grt@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 816698 , 806130 Change-Id: I09e12c17011f72f25ecb369ca3075e2067013f8a Reviewed-on: https://chromium-review.googlesource.com/958012 Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> Cr-Commit-Position: refs/heads/master@{#542506} [modify] https://crrev.com/b93f6ee55444118d4dcb237591a5786dbe374e87/chrome/browser/google/google_brand.cc [modify] https://crrev.com/b93f6ee55444118d4dcb237591a5786dbe374e87/chrome/browser/google/google_brand.h
,
Mar 12 2018
Responding to comment 31: I didn’t write it, TVL did, but he’s long gone from Chrome, and I did review it and I do remember it. I don’t have much new to contribute, but I can confirm: - The brand code is somewhat expensive to fetch on macOS. - That this varies by channel is (unfortunately) intentional. We had a big ugly multidimensional matrix of channels, Keystone install modes (system/user), Keystone ticket types (system/user), co-install forms (between channels), and channel migrations. What we have now is the delicate result of intense deliberation intended to address all of the cells of that matrix. It was indeed very intentional.
,
Mar 12 2018
Oh, I forgot one more very important dimension: the ticket upgrade. We really needed to treat the ticket type dimension as a tri-state: system, user, and was-user-but-is-system-and-nothing’s-compensated-yet. That was sadly a distinct case.
,
Mar 21 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cfd298773e9c7e294dea2c643796544e4fcc228e commit cfd298773e9c7e294dea2c643796544e4fcc228e Author: Alexei Svitkine <asvitkine@chromium.org> Date: Wed Mar 21 16:14:32 2018 Reland "Cache brand code on Windows." This reverts commit b93f6ee55444118d4dcb237591a5786dbe374e87. Reason for revert: Reverting showed the relevant metrics regressed, so this change did have the expected improvement. Original change's description: > Revert "Cache brand code on Windows." > > This reverts commit 9dd1ef42a8a33c5ba09ec645150f580724979b36. > > Reason for revert: Latest canary data did not have expected recovery on Omnibox.CharTypedToRepaintLatency and had a regression on Omnibox.CharTypedToRepaintLatency.ToPaint. Reverting to see if that movement is related to this CL or whether there were other changes in the meanwhile. > > Original change's description: > > Cache brand code on Windows. > > > > Querying the registry is expensive and contributes to start up time, > > omnibox query time and idle. Since the brand code is not expected to > > change on Windows during the course of a session, this CL caches > > the value, so that future queries are free. > > > > This should restore the Omnibox perf that regressed by > > https://chromium-review.googlesource.com/c/chromium/src/+/824363 > > and also improve perf elsewhere since the caching is now done at > > a lower level than that change. Since this is expensive only on > > Windows, the caching is done in the Windows codepath only. > > > > Bug: 816698 , 806130 > > Change-Id: I458fad29f18b1b1ff2d55b9334ae733fa448eec9 > > Reviewed-on: https://chromium-review.googlesource.com/947522 > > Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> > > Reviewed-by: Peter Kasting <pkasting@chromium.org> > > Reviewed-by: Greg Thompson <grt@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#541009} > > TBR=pkasting@chromium.org,asvitkine@chromium.org,grt@chromium.org > > # Not skipping CQ checks because original CL landed > 1 day ago. > > Bug: 816698 , 806130 > Change-Id: I09e12c17011f72f25ecb369ca3075e2067013f8a > Reviewed-on: https://chromium-review.googlesource.com/958012 > Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> > Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> > Cr-Commit-Position: refs/heads/master@{#542506} TBR=pkasting@chromium.org,asvitkine@chromium.org,grt@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 816698 , 806130 Change-Id: Id70bcaa6be1e91240010e74990dd2bd81d673258 Reviewed-on: https://chromium-review.googlesource.com/973581 Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> Cr-Commit-Position: refs/heads/master@{#544723} [modify] https://crrev.com/cfd298773e9c7e294dea2c643796544e4fcc228e/chrome/browser/google/google_brand.cc [modify] https://crrev.com/cfd298773e9c7e294dea2c643796544e4fcc228e/chrome/browser/google/google_brand.h
,
Mar 21 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/86578012097568b4e2cc60510c17d7a13ff4eaa4 commit 86578012097568b4e2cc60510c17d7a13ff4eaa4 Author: Boris Vidolov <borisv@chromium.org> Date: Wed Mar 21 16:55:25 2018 Unify branch code extraction between Stable and Canary. Minimize the I/O operations that extract the channel. Bug: 806130 Change-Id: I9d9379153e80abad7eb190b4940c595f76b46b2c Reviewed-on: https://chromium-review.googlesource.com/967512 Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Mark Mentovai <mark@chromium.org> Commit-Queue: Boris Vidolov <borisv@chromium.org> Cr-Commit-Position: refs/heads/master@{#544738} [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/app/chrome_main_delegate.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/browser_process_impl.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/chrome_browser_main.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/component_updater/chrome_component_updater_configurator.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/diagnostics/recon_diagnostics.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/extensions/updater/chrome_update_client_config.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/mac/keystone_glue.h [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/mac/keystone_glue.mm [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/media/webrtc/webrtc_text_log_handler.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/net/default_network_context_params.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/profile_resetter/resettable_settings_snapshot.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/search_engines/ui_thread_search_terms_data.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/ui/aura/accessibility/ax_root_obj_wrapper.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/ui/webui/flash_ui.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/ui/webui/nacl_ui.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/ui/webui/net_export_ui.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/ui/webui/net_internals/net_internals_ui.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/ui/webui/net_internals/net_internals_ui_browsertest.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/ui/webui/settings/about_handler.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/ui/webui/version_ui.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/update_client/chrome_update_query_params_delegate.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/browser/update_client/chrome_update_query_params_delegate_unittest.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/common/channel_info.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/common/channel_info.h [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/common/channel_info_android.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/common/channel_info_chromeos.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/common/channel_info_mac.mm [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/common/channel_info_posix.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/chrome/common/channel_info_win.cc [modify] https://crrev.com/86578012097568b4e2cc60510c17d7a13ff4eaa4/components/sync/driver/about_sync_util.cc
,
Mar 21 2018
Note that the CL at #36 will regress the performance for the Mac Canary build. This is intentional and the goal is to bring the Canary and Stable builds to the same level.
,
Mar 21 2018
We have been following the wrong path for Mac. Mac brand code has always been cached in KeystoneGlue::BrandCode via a static variable. This is why we did not see impact on Mac! static std::string* s_brand_code = new std::string(BrandCodeInternal()); return *s_brand_code; https://cs.chromium.org/chromium/src/chrome/browser/mac/keystone_glue.mm?rcl=d496b8f5f7b44d31923054ee27ad523c1b071257&l=1195
,
Mar 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c233179b37d27d473f24a5f2386cf55083a014a6 commit c233179b37d27d473f24a5f2386cf55083a014a6 Author: Alexei Svitkine <asvitkine@chromium.org> Date: Mon Mar 26 16:35:16 2018 Reland "Cache brand code on Windows." This reverts commit b93f6ee55444118d4dcb237591a5786dbe374e87. Reason for revert: Reverting showed the relevant metrics regressed, so this change did have the expected improvement. Original change's description: > Revert "Cache brand code on Windows." > > This reverts commit 9dd1ef42a8a33c5ba09ec645150f580724979b36. > > Reason for revert: Latest canary data did not have expected recovery on Omnibox.CharTypedToRepaintLatency and had a regression on Omnibox.CharTypedToRepaintLatency.ToPaint. Reverting to see if that movement is related to this CL or whether there were other changes in the meanwhile. > > Original change's description: > > Cache brand code on Windows. > > > > Querying the registry is expensive and contributes to start up time, > > omnibox query time and idle. Since the brand code is not expected to > > change on Windows during the course of a session, this CL caches > > the value, so that future queries are free. > > > > This should restore the Omnibox perf that regressed by > > https://chromium-review.googlesource.com/c/chromium/src/+/824363 > > and also improve perf elsewhere since the caching is now done at > > a lower level than that change. Since this is expensive only on > > Windows, the caching is done in the Windows codepath only. > > > > Bug: 816698 , 806130 > > Change-Id: I458fad29f18b1b1ff2d55b9334ae733fa448eec9 > > Reviewed-on: https://chromium-review.googlesource.com/947522 > > Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> > > Reviewed-by: Peter Kasting <pkasting@chromium.org> > > Reviewed-by: Greg Thompson <grt@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#541009} > > TBR=pkasting@chromium.org,asvitkine@chromium.org,grt@chromium.org > > # Not skipping CQ checks because original CL landed > 1 day ago. > > Bug: 816698 , 806130 > Change-Id: I09e12c17011f72f25ecb369ca3075e2067013f8a > Reviewed-on: https://chromium-review.googlesource.com/958012 > Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> > Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> > Cr-Commit-Position: refs/heads/master@{#542506} TBR=asvitkine@chromium.org, grt@chromium.org, pkasting@chromium.org (cherry picked from commit cfd298773e9c7e294dea2c643796544e4fcc228e) Bug: 816698 , 806130 Change-Id: Id70bcaa6be1e91240010e74990dd2bd81d673258 Reviewed-on: https://chromium-review.googlesource.com/973581 Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#544723} Reviewed-on: https://chromium-review.googlesource.com/980820 Cr-Commit-Position: refs/branch-heads/3359@{#430} Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276} [modify] https://crrev.com/c233179b37d27d473f24a5f2386cf55083a014a6/chrome/browser/google/google_brand.cc [modify] https://crrev.com/c233179b37d27d473f24a5f2386cf55083a014a6/chrome/browser/google/google_brand.h |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by wittman@chromium.org
, Jan 26 2018