Issue metadata
Sign in to add a comment
|
Fix Windows Omnibox performance regression |
||||||||||||||||||||||
Issue descriptionOmnibox.CharTypedToRepaintLatency regressed and we tracked it down to this CL: https://chromium-review.googlesource.com/c/chromium/src/+/824363 More details here: b/71008393 Now that we've identified the culprit, we understand the cause of the regression. This bug is to track implementing a fix for the performance regression (while keeping the functionality change of the original CL). Assigning to CL author for follow-up. Thanks! This unfortunately regressed in M64 (due to a merge to beta), so it has already made it to stable. Putting an optimistic target of M65 for this, if we have enough time to fix and merge to that milestone. (If that doesn't work out, we should at least ensure it makes it to M66 - which might require a merge too given it branches soon.)
,
Feb 26 2018
I think you meant https://chromium-review.googlesource.com/c/chromium/src/+/824363 ?
,
Feb 26 2018
Er, yes - I linked to the wrong one! Thanks for catching, let me edit description. :)
,
Feb 26 2018
,
Feb 26 2018
,
Feb 27 2018
Issue 800838 is related -- feel free to merge (or not). FWIW, there are some thoughts on how the implementation could be improved in 800838#c5 and 800838#c8.
,
Feb 27 2018
By the way, if there's still ongoing discussion on the best long term solution, perhaps a good short term way to recover the perf loss is to just disable the caching on CrOS (e.g. with an ifdef)? Assuming only CrOS was affected by the behavior change.
,
Feb 27 2018
Also see issue 806130 which covers the underlying performance issue: the brand code is read from the Windows registry every time GetBrand() is invoked.
,
Feb 28 2018
Making the bug available. I don't think I'm a good owner for this as it's not a privacy issue.
,
Feb 28 2018
tnagel: Given your CL introduced the regression, I think it's reasonable to hold you to landing a fix for it. Per my comment 7, it might be pretty simple if we just ifdef the non-caching codepath for CrOS?
,
Feb 28 2018
Correction from the original report: It turns out the problematic change was merged into Stable M-63 (during a re-spin!), not making it to Stable as part of M-64 as the original report says.
,
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 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 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
Requesting merge of CL from comment 15 to M66. This recovers an Omnibox performance regression and is very safe. (The revert/reland activity on this bug was just to verify it was having the expected perf effect since some other CL moved metrics at the same time as the original landing.)
,
Mar 22 2018
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), josafat@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 23 2018
Approving merge to M66. Branch:3359
,
Mar 26 2018
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
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
,
Mar 26 2018
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by asvitk...@chromium.org
, Feb 26 2018