Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2382023003:
Optimize ShouldLaunchIncognito() since its on the startup path. (Closed)
|
Created:
4 years, 2 months ago by Alexei Svitkine (slow) Modified:
4 years, 2 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOptimize ShouldLaunchIncognito() since its on the startup path.
Sampling profiler data shows this takes 10ms on the startup path,
mainly due to querying parental controls. Since we only need to
check parental controls if the incognito flag was specified on
the command line, the code is refactored to only query them in
that case.
BUG=651848
Committed: https://crrev.com/b62d7d9eec9d9d49d7d2d50bc3fb08f1a4d1880e
Cr-Commit-Position: refs/heads/master@{#422532}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address comments. #
Total comments: 10
Patch Set 3 : Address gab's comments. #
Messages
Total messages: 34 (18 generated)
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
asvitkine@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
robliao: Please review since you've done some work on this code before. gab: Please review as c/b/prefs owner.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It's surprising to me that this is still showing up on the sampler. IncognitoModePrefs.WindowsParentalControlsInitThread suggests that this computation happens on the blocking thread pretty much all the time. What does your data show? https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:145: return GetAvailabilityInternal(pref_service, true); Nit: true -> enum. Something like CHECK_PARENTAL_CONTROLS and DONT_CHECK_PARENTAL_CONTROLS.
Here's an example view of the UMA data from dev: https://uma.googleplex.com/callstacks?sid=43940bb8d887d5488d27a4695c23ff0d That one shows 8ms - but I've seen another day/version show 10ms. It's not *too* much - but I figured if we can just avoid this work entirely, that's even better. On Fri, Sep 30, 2016 at 1:43 PM, <robliao@chromium.org> wrote: > It's surprising to me that this is still showing up on the sampler. > > IncognitoModePrefs.WindowsParentalControlsInitThread suggests that this > computation happens on the blocking thread pretty much all the time. > > What does your data show? > > > https://codereview.chromium.org/2382023003/diff/40001/ > chrome/browser/prefs/incognito_mode_prefs.cc > File chrome/browser/prefs/incognito_mode_prefs.cc (right): > > https://codereview.chromium.org/2382023003/diff/40001/ > chrome/browser/prefs/incognito_mode_prefs.cc#newcode145 > chrome/browser/prefs/incognito_mode_prefs.cc:145: return > GetAvailabilityInternal(pref_service, true); > Nit: true -> enum. Something like CHECK_PARENTAL_CONTROLS and > DONT_CHECK_PARENTAL_CONTROLS. > > https://codereview.chromium.org/2382023003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> Here's an example view of the UMA data from dev: > https://uma.googleplex.com/callstacks?sid=43940bb8d887d5488d27a4695c23ff0d > That one shows 8ms - but I've seen another day/version show 10ms. It's not *too* much - but I figured if we can just avoid this work entirely, that's even better. Interesting. So we win at starting the race, but we sometimes don't compute quickly enough. The approach looks reasonable. I've added some more comments. https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:165: // Note: The following code structure ensures we don't query I would make this more explicit: We only absolutely need to check for parental controls in the startup path if the user requested to launch in incognito mode since computing parental controls can be slow. Otherwise, we can check later. https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:169: GetAvailability(prefs) != IncognitoModePrefs::DISABLED) { This is also used here: https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_commands.cc?q=... It looks like this uses the CrOS path, so this shouldn't impact it too much. Might be worth commenting in browser_commands for future folks who potentially change the code. Taking a step back, that area looks suspect as it allows Incognito if Chrome is simply in a guest session, even if parental controls are enabled. https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:218: IncognitoModePrefs::Availability IncognitoModePrefs::GetAvailabilityInternal( This can be part of the anonymous namespace, right?
Second comment is most important (likely superseeds other two). https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:169: GetAvailability(prefs) != IncognitoModePrefs::DISABLED) { I think this would be more readable as GetAvailabilityInternal(prefs, true), it others took me some time to match the comment to the code (have to realize that GetAvailability() is implemented to always consider parental controls). https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:172: return GetAvailabilityInternal(prefs, false) == IncognitoModePrefs::FORCED; Won't this launch incognito if the pref is set to FORCED but parental controls disallow it (and there isn't the incognito switch)? I don't this it's possible to launch incognito without checking parental controls. i.e. the way I see to make this work is: decide whether to launch incognito w/o parental controls, if incognito launch is desired (switch or FORCED) then check parental controls. https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:226: if (check_parental_controls && ArePlatformParentalControlsEnabled()) { Also only need to check if |result != IncognitoModePrefs::DISABLED|
https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:172: return GetAvailabilityInternal(prefs, false) == IncognitoModePrefs::FORCED; On 2016/09/30 18:47:20, gab wrote: > Won't this launch incognito if the pref is set to FORCED but parental controls > disallow it (and there isn't the incognito switch)? > > I don't this it's possible to launch incognito without checking parental > controls. > > i.e. the way I see to make this work is: decide whether to launch incognito w/o > parental controls, if incognito launch is desired (switch or FORCED) then check > parental controls. You're right - good catch. Let me re-work the logic as you suggest.
Thanks, comments addressed - PTAL! https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:145: return GetAvailabilityInternal(pref_service, true); On 2016/09/30 17:43:46, robliao wrote: > Nit: true -> enum. Something like CHECK_PARENTAL_CONTROLS and > DONT_CHECK_PARENTAL_CONTROLS. Done. https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:165: // Note: The following code structure ensures we don't query On 2016/09/30 18:26:13, robliao wrote: > I would make this more explicit: > We only absolutely need to check for parental controls in the startup path if > the user requested to launch in incognito mode since computing parental controls > can be slow. Otherwise, we can check later. Done. https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:169: GetAvailability(prefs) != IncognitoModePrefs::DISABLED) { On 2016/09/30 18:47:20, gab wrote: > I think this would be more readable as GetAvailabilityInternal(prefs, true), it > others took me some time to match the comment to the code (have to realize that > GetAvailability() is implemented to always consider parental controls). Done. https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:172: return GetAvailabilityInternal(prefs, false) == IncognitoModePrefs::FORCED; On 2016/09/30 18:55:08, Alexei Svitkine (slow) wrote: > On 2016/09/30 18:47:20, gab wrote: > > Won't this launch incognito if the pref is set to FORCED but parental controls > > disallow it (and there isn't the incognito switch)? > > > > I don't this it's possible to launch incognito without checking parental > > controls. > > > > i.e. the way I see to make this work is: decide whether to launch incognito > w/o > > parental controls, if incognito launch is desired (switch or FORCED) then > check > > parental controls. > > You're right - good catch. Let me re-work the logic as you suggest. Done. https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:218: IncognitoModePrefs::Availability IncognitoModePrefs::GetAvailabilityInternal( On 2016/09/30 18:26:13, robliao wrote: > This can be part of the anonymous namespace, right? I tried it this way, but it was referencing too many things from the class so I had to prefix a bunch pf things like Availability, IntToAvailability, ArePlatformParentalControlsEnabled with the class name - which seemed like it would be better to keep static so it's more concise. Let me know if you prefer I still make it anon. https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:226: if (check_parental_controls && ArePlatformParentalControlsEnabled()) { On 2016/09/30 18:47:20, gab wrote: > Also only need to check if |result != IncognitoModePrefs::DISABLED| Done.
Friendly ping!
lgtm % nits https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/2382023003/diff/40001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:218: IncognitoModePrefs::Availability IncognitoModePrefs::GetAvailabilityInternal( On 2016/09/30 20:16:16, Alexei Svitkine (slow) wrote: > On 2016/09/30 18:26:13, robliao wrote: > > This can be part of the anonymous namespace, right? > > I tried it this way, but it was referencing too many things from the class so I > had to prefix a bunch pf things like Availability, IntToAvailability, > ArePlatformParentalControlsEnabled with the class name - which seemed like it > would be better to keep static so it's more concise. Let me know if you prefer I > still make it anon. Since it's already a class with only static methods that sounds fine to me (i.e. if it was instead a namespace you would put this private method in the .cc in an anon namespace inside that namespace but classes with static methods don't allow this). https://codereview.chromium.org/2382023003/diff/60001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/2382023003/diff/60001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:167: // it was forced via prefs, since computing parental controls can be We need to check parental controls (...) since parentals controls can be slow? Don't think that's what you intended to say here? https://codereview.chromium.org/2382023003/diff/60001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:169: bool should_use_incognito = const https://codereview.chromium.org/2382023003/diff/60001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:173: // Only check parental controls only if |should_use_incognito| is true. double "only" This comment is also probably redundant (i.e. it's merely stating what the simple code below it does word for word). https://codereview.chromium.org/2382023003/diff/60001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.h (right): https://codereview.chromium.org/2382023003/diff/60001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.h:82: enum GetAvailabilityMode { enum class
Thanks! Rob, I'll wait for your lg as well. https://codereview.chromium.org/2382023003/diff/60001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.cc (right): https://codereview.chromium.org/2382023003/diff/60001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:167: // it was forced via prefs, since computing parental controls can be On 2016/10/03 17:08:12, gab wrote: > We need to check parental controls (...) since parentals controls can be slow? > > Don't think that's what you intended to say here? I think the comment wording did work - "we only do X in certain cases since X can be slow" is what it's saying, which is a valid phrasing. However, I agree it was a bit run-on - so I've reworded so it's two sentences now and easier to read. https://codereview.chromium.org/2382023003/diff/60001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:169: bool should_use_incognito = On 2016/10/03 17:08:12, gab wrote: > const Done. https://codereview.chromium.org/2382023003/diff/60001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.cc:173: // Only check parental controls only if |should_use_incognito| is true. On 2016/10/03 17:08:12, gab wrote: > double "only" > > This comment is also probably redundant (i.e. it's merely stating what the > simple code below it does word for word). Done. After re-wording the comment above, I'm OK with not having this extra comment. https://codereview.chromium.org/2382023003/diff/60001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.h (right): https://codereview.chromium.org/2382023003/diff/60001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.h:82: enum GetAvailabilityMode { On 2016/10/03 17:08:12, gab wrote: > enum class This requires prefixing all uses of the constants with the name of the enum, which makes them extremely long - since the constants are already wordy. So I'd rather keep as-is. In other cases, the extra prefixing is nice because it makes sure you don't get conflicts and it's obvious where they're coming from - but since this is a private constant I don't think the benefits outweigh the extra readability hit from prefixing and re-wrapping a lot of the code - such as the one in ShouldLaunchIncognito(). Let me know if you feel strongly about this, though.
https://codereview.chromium.org/2382023003/diff/60001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.h (right): https://codereview.chromium.org/2382023003/diff/60001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.h:82: enum GetAvailabilityMode { On 2016/10/03 18:57:39, Alexei Svitkine (slow) wrote: > On 2016/10/03 17:08:12, gab wrote: > > enum class > > This requires prefixing all uses of the constants with the name of the enum, > which makes them extremely long - since the constants are already wordy. So I'd > rather keep as-is. > > In other cases, the extra prefixing is nice because it makes sure you don't get > conflicts and it's obvious where they're coming from - but since this is a > private constant I don't think the benefits outweigh the extra readability hit > from prefixing and re-wrapping a lot of the code - such as the one in > ShouldLaunchIncognito(). > > Let me know if you feel strongly about this, though. IMO that means that GetAvailabilityMode is too generic of a name. I like a descriptive enum class name with short options, e.g.: enum class ConsiderParentalControls { YES, NO, }; I find enum class should always be used when the numerical values don't mean anything.
https://codereview.chromium.org/2382023003/diff/60001/chrome/browser/prefs/in... File chrome/browser/prefs/incognito_mode_prefs.h (right): https://codereview.chromium.org/2382023003/diff/60001/chrome/browser/prefs/in... chrome/browser/prefs/incognito_mode_prefs.h:82: enum GetAvailabilityMode { On 2016/10/03 19:05:57, gab wrote: > On 2016/10/03 18:57:39, Alexei Svitkine (slow) wrote: > > On 2016/10/03 17:08:12, gab wrote: > > > enum class > > > > This requires prefixing all uses of the constants with the name of the enum, > > which makes them extremely long - since the constants are already wordy. So > I'd > > rather keep as-is. > > > > In other cases, the extra prefixing is nice because it makes sure you don't > get > > conflicts and it's obvious where they're coming from - but since this is a > > private constant I don't think the benefits outweigh the extra readability hit > > from prefixing and re-wrapping a lot of the code - such as the one in > > ShouldLaunchIncognito(). > > > > Let me know if you feel strongly about this, though. > > IMO that means that GetAvailabilityMode is too generic of a name. I like a > descriptive enum class name with short options, e.g.: > > enum class ConsiderParentalControls { > YES, > NO, > }; > > I find enum class should always be used when the numerical values don't mean > anything. At first I really like this suggestion, but then I realized we'd have to rename the param to be something like consider_parental_controls instead of mode, which still makes the code very verbose. :\ Gab and I try to come up with a few different names but couldn't find something we both liked and was not verbose. So I think we've decided to just keep the enums.
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm. Enum looks fine to me. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by asvitkine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2382023003/#ps80001 (title: "Address gab's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Optimize ShouldLaunchIncognito() since its on the startup path. Sampling profiler data shows this takes 10ms on the startup path, mainly due to querying parental controls. Since we only need to check parental controls if the incognito flag was specified on the command line, the code is refactored to only query them in that case. BUG=651848 ========== to ========== Optimize ShouldLaunchIncognito() since its on the startup path. Sampling profiler data shows this takes 10ms on the startup path, mainly due to querying parental controls. Since we only need to check parental controls if the incognito flag was specified on the command line, the code is refactored to only query them in that case. BUG=651848 Committed: https://crrev.com/b62d7d9eec9d9d49d7d2d50bc3fb08f1a4d1880e Cr-Commit-Position: refs/heads/master@{#422532} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b62d7d9eec9d9d49d7d2d50bc3fb08f1a4d1880e Cr-Commit-Position: refs/heads/master@{#422532} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
