New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 806130 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

search::GetNewTabPageURL() reads the RLZ brand registry key every time it's called

Project Member Reported by wittman@chromium.org, Jan 26 2018

Issue description

This 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.
 
Is the brand something we could cache for the lifetime of the process? If so, do you mind taking this, Greg?

Comment 2 by treib@chromium.org, Jan 26 2018

Components: UI>Browser>NewTabPage
Labels: OS-Chrome OS-Linux OS-Mac
Somewhat related: Bug 786035

Comment 3 by grt@chromium.org, 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.
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?

Comment 5 by grt@chromium.org, 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.
Cc: tnagel@chromium.org borisv@chromium.org
 Issue 723072  has been merged into this issue.
Labels: -OS-Linux -OS-Chrome -OS-Mac

Comment 8 by tnagel@chromium.org, Jan 30 2018

If you're touching this code, please also see issue 800838.

Comment 9 by tnagel@chromium.org, Jan 30 2018

Labels: OS-Mac
Note that  issue 723072  mentions plist and thus Mac seems to be affected (unless this has changed in the meantime).
Labels: -OS-Mac
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

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. 
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.

Comment 13 by zea@chromium.org, Jan 30 2018

Cc: ma...@chromium.org
Labels: zine-triaged
Owner: asvitk...@chromium.org
Status: Started (was: Available)
https://chromium-review.googlesource.com/c/chromium/src/+/947522
Project Member

Comment 15 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
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.)
Thanks for the fix, this is a great improvement. 
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.

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.
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.

Comment 21 Deleted

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.


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?
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?
I checked other versions including most recent Mac Dev release - which had data in general, but nothing for the brand function.
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
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.
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.
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
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.
Cc: mark@chromium.org
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.
Project Member

Comment 32 by bugdroid1@chromium.org, 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

Comment 33 by mark@chromium.org, 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.

Comment 34 by mark@chromium.org, 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.
Project Member

Comment 35 by bugdroid1@chromium.org, 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

Project Member

Comment 36 by bugdroid1@chromium.org, 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

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.
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
Project Member

Comment 39 by bugdroid1@chromium.org, Mar 26 2018

Labels: merge-merged-3359
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