Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2227343002:
Add Browser Task Scheduler Flags (Closed)
|
Created:
4 years, 4 months ago by robliao Modified:
4 years, 4 months ago CC:
chromium-reviews, gab+watch_chromium.org, robliao+watch_chromium.org, fdoray+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Browser Task Scheduler Flags
Adds the browser scheduler command line flags as well as exposing it
via chrome://flags.
Enabling the feature will place the browser in a forced variations
group, allowing it to use task scheduler parameters from there. If the
variations group is unavailable, then enabling this will still not
enable the browser task scheduler
BUG=553459, 633389
Committed: https://crrev.com/2ce242f8b6f3485e962363f6e05873576c313719
Cr-Commit-Position: refs/heads/master@{#411199}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add DCHECK #Patch Set 3 : Adjust DCHECK message #
Messages
Total messages: 40 (21 generated)
The CQ bit was checked by robliao@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: This issue passed the CQ dry run.
Description was changed from ========== Add Browser Task Scheduler Flags Adds the browser scheduler command line flags as well as exposing it via chrome://flags. Enabling the feature will place the browser in a forced variations group, allowing it to use task scheduler parameters from there. If the variations group is unavailable, then enabling this will still not enable the browser task scheduler BUG=553459 ========== to ========== Add Browser Task Scheduler Flags Adds the browser scheduler command line flags as well as exposing it via chrome://flags. Enabling the feature will place the browser in a forced variations group, allowing it to use task scheduler parameters from there. If the variations group is unavailable, then enabling this will still not enable the browser task scheduler BUG=553459 ==========
robliao@chromium.org changed reviewers: + fdoray@chromium.org, gab@chromium.org
"Enabling the feature will place the browser in a forced variations group, allowing it to use task scheduler parameters from there." Hmmm, I only see the flag being added but nothing is done about its state?
On 2016/08/10 16:34:51, gab wrote: > "Enabling the feature will place the browser in a forced variations group, > allowing it to use task scheduler parameters from there." > > Hmmm, I only see the flag being added but nothing is done about its state? Variations takes care of that. Just adding the command line flag will allow us to use forcing flag on the variations server to specify parameters. This means when a flag is specified, we will have variation parameters. When a flag is disabled or unspecified, the variation parameters won't be mapped in. The same code in MaybeInitializeTaskScheduler() just works as a result.
On 2016/08/10 16:38:23, robliao wrote: > On 2016/08/10 16:34:51, gab wrote: > > "Enabling the feature will place the browser in a forced variations group, > > allowing it to use task scheduler parameters from there." > > > > Hmmm, I only see the flag being added but nothing is done about its state? > > Variations takes care of that. Just adding the command line flag will allow us > to use forcing flag on the variations server to specify parameters. This means > when a flag is specified, we will have variation parameters. When a flag is > disabled or unspecified, the variation parameters won't be mapped in. The same > code in MaybeInitializeTaskScheduler() just works as a result. Oh cool :-), didn't know the mapping was done server side, that's awesome! (maybe make that clear in CL desc) lgtm then!
https://codereview.chromium.org/2227343002/diff/1/base/task_scheduler/switche... File base/task_scheduler/switches.cc (right): https://codereview.chromium.org/2227343002/diff/1/base/task_scheduler/switche... base/task_scheduler/switches.cc:10: const char kEnableBrowserTaskScheduler[] = "enable-browser-task-scheduler"; Can we have a DCHECK in chrome if --enable-browser-task-scheduler but we don't have valid variations for it? Then a developer using this without variations (i.e. dev build for which Finch hasn't been synced yet) will not be left debugging for hours before realizing he's missing variations. (that'd also make a nice place to add a code comment on how the mapping is done server-side :-))
On 2016/08/10 16:48:57, gab wrote: > https://codereview.chromium.org/2227343002/diff/1/base/task_scheduler/switche... > File base/task_scheduler/switches.cc (right): > > https://codereview.chromium.org/2227343002/diff/1/base/task_scheduler/switche... > base/task_scheduler/switches.cc:10: const char kEnableBrowserTaskScheduler[] = > "enable-browser-task-scheduler"; > Can we have a DCHECK in chrome if --enable-browser-task-scheduler but we don't > have valid variations for it? Then a developer using this without variations > (i.e. dev build for which Finch hasn't been synced yet) will not be left > debugging for hours before realizing he's missing variations. > > (that'd also make a nice place to add a code comment on how the mapping is done > server-side :-)) Should probably also associate this (and I guess the last one but too late) with issue 633389.
Description was changed from ========== Add Browser Task Scheduler Flags Adds the browser scheduler command line flags as well as exposing it via chrome://flags. Enabling the feature will place the browser in a forced variations group, allowing it to use task scheduler parameters from there. If the variations group is unavailable, then enabling this will still not enable the browser task scheduler BUG=553459 ========== to ========== Add Browser Task Scheduler Flags Adds the browser scheduler command line flags as well as exposing it via chrome://flags. Enabling the feature will place the browser in a forced variations group, allowing it to use task scheduler parameters from there. If the variations group is unavailable, then enabling this will still not enable the browser task scheduler BUG=553459,633389 ==========
Patchset #2 (id:20001) has been deleted
robliao@chromium.org changed reviewers: + thestig@chromium.org
robliao@chromium.org changed reviewers: + isherman@chromium.org
Reviewers, please review the filepaths by your username. Thanks! thestig: chrome/browser/* isherman: *metrics* https://codereview.chromium.org/2227343002/diff/1/base/task_scheduler/switche... File base/task_scheduler/switches.cc (right): https://codereview.chromium.org/2227343002/diff/1/base/task_scheduler/switche... base/task_scheduler/switches.cc:10: const char kEnableBrowserTaskScheduler[] = "enable-browser-task-scheduler"; On 2016/08/10 16:48:57, gab wrote: > Can we have a DCHECK in chrome if --enable-browser-task-scheduler but we don't > have valid variations for it? Then a developer using this without variations > (i.e. dev build for which Finch hasn't been synced yet) will not be left > debugging for hours before realizing he's missing variations. > > (that'd also make a nice place to add a code comment on how the mapping is done > server-side :-)) Added a DCHECK in the variations check.
histograms.xml lgtm
The CQ bit was checked by robliao@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
Adding danakj@ as FYI on flags in base.
The CQ bit was checked by robliao@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...
danakj@chromium.org changed reviewers: + danakj@chromium.org
Do you need both flags? Normally I'd expect 1 flag (the non-default), and when the default changes, the available flag would also change.
On 2016/08/10 22:17:54, danakj wrote: > Do you need both flags? Normally I'd expect 1 flag (the non-default), and when > the default changes, the available flag would also change. The use of variations means we'll probably need both flags. If folks get opted in, then they can use the disable flag to opt-out and disable the feature. If folks want to opt in early, then they can use the enable flag to enable the feature.
On 2016/08/10 22:30:41, robliao wrote: > On 2016/08/10 22:17:54, danakj wrote: > > Do you need both flags? Normally I'd expect 1 flag (the non-default), and when > > the default changes, the available flag would also change. > > The use of variations means we'll probably need both flags. > If folks get opted in, then they can use the disable flag to opt-out and disable > the feature. > If folks want to opt in early, then they can use the enable flag to enable the > feature. So speaking from experience this "getting opted in" sounds like you're going to become sad. Slimming paint did something like this and then it was really hard to tell what mode anyone was in when bugs happened or etc. So at least make sure you're considering how you're going to know a given crash or bug report is in what mode, and maybe how a user (or chrome developer) will know what mode they are using. LGTM to having flag(s) in general.
On 2016/08/10 22:36:01, danakj wrote: > On 2016/08/10 22:30:41, robliao wrote: > > On 2016/08/10 22:17:54, danakj wrote: > > > Do you need both flags? Normally I'd expect 1 flag (the non-default), and > when > > > the default changes, the available flag would also change. > > > > The use of variations means we'll probably need both flags. > > If folks get opted in, then they can use the disable flag to opt-out and > disable > > the feature. > > If folks want to opt in early, then they can use the enable flag to enable the > > feature. > > So speaking from experience this "getting opted in" sounds like you're going to > become sad. Slimming paint did something like this and then it was really hard > to tell what mode anyone was in when bugs happened or etc. So at least make sure > you're considering how you're going to know a given crash or bug report is in > what mode, and maybe how a user (or chrome developer) will know what mode they > are using. > > LGTM to having flag(s) in general. Thanks for the heads up! We're going to have a chrome:// page for the internals, so that will allow users to tell if the task scheduler is enabled. I'm not sure if we can correlate specific crash reports to variations settings.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/10 22:40:11, robliao wrote: > On 2016/08/10 22:36:01, danakj wrote: > > On 2016/08/10 22:30:41, robliao wrote: > > > On 2016/08/10 22:17:54, danakj wrote: > > > > Do you need both flags? Normally I'd expect 1 flag (the non-default), and > > when > > > > the default changes, the available flag would also change. > > > > > > The use of variations means we'll probably need both flags. > > > If folks get opted in, then they can use the disable flag to opt-out and > > disable > > > the feature. > > > If folks want to opt in early, then they can use the enable flag to enable > the > > > feature. > > > > So speaking from experience this "getting opted in" sounds like you're going > to > > become sad. Slimming paint did something like this and then it was really hard > > to tell what mode anyone was in when bugs happened or etc. So at least make > sure > > you're considering how you're going to know a given crash or bug report is in > > what mode, and maybe how a user (or chrome developer) will know what mode they > > are using. > > > > LGTM to having flag(s) in general. > > Thanks for the heads up! We're going to have a chrome:// page for the internals, > so that will allow users to tell if the task scheduler is enabled. I'm not sure > if we can correlate specific crash reports to variations settings. Yes, crash reports are tagged with the variations state.
Nice. I just pulled a random report and didn't see it. I was probably looking in the wrong place. On Wed, Aug 10, 2016 at 4:45 PM, <isherman@chromium.org> wrote: > On 2016/08/10 22:40:11, robliao wrote: > > On 2016/08/10 22:36:01, danakj wrote: > > > On 2016/08/10 22:30:41, robliao wrote: > > > > On 2016/08/10 22:17:54, danakj wrote: > > > > > Do you need both flags? Normally I'd expect 1 flag (the > non-default), > and > > > when > > > > > the default changes, the available flag would also change. > > > > > > > > The use of variations means we'll probably need both flags. > > > > If folks get opted in, then they can use the disable flag to opt-out > and > > > disable > > > > the feature. > > > > If folks want to opt in early, then they can use the enable flag to > enable > > the > > > > feature. > > > > > > So speaking from experience this "getting opted in" sounds like you're > going > > to > > > become sad. Slimming paint did something like this and then it was > really > hard > > > to tell what mode anyone was in when bugs happened or etc. So at least > make > > sure > > > you're considering how you're going to know a given crash or bug > report is > in > > > what mode, and maybe how a user (or chrome developer) will know what > mode > they > > > are using. > > > > > > LGTM to having flag(s) in general. > > > > Thanks for the heads up! We're going to have a chrome:// page for the > internals, > > so that will allow users to tell if the task scheduler is enabled. I'm > not > sure > > if we can correlate specific crash reports to variations settings. > > Yes, crash reports are tagged with the variations state. > > https://codereview.chromium.org/2227343002/ > -- 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.
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, thestig@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2227343002/#ps60001 (title: "Adjust DCHECK message")
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.
Description was changed from ========== Add Browser Task Scheduler Flags Adds the browser scheduler command line flags as well as exposing it via chrome://flags. Enabling the feature will place the browser in a forced variations group, allowing it to use task scheduler parameters from there. If the variations group is unavailable, then enabling this will still not enable the browser task scheduler BUG=553459,633389 ========== to ========== Add Browser Task Scheduler Flags Adds the browser scheduler command line flags as well as exposing it via chrome://flags. Enabling the feature will place the browser in a forced variations group, allowing it to use task scheduler parameters from there. If the variations group is unavailable, then enabling this will still not enable the browser task scheduler BUG=553459,633389 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add Browser Task Scheduler Flags Adds the browser scheduler command line flags as well as exposing it via chrome://flags. Enabling the feature will place the browser in a forced variations group, allowing it to use task scheduler parameters from there. If the variations group is unavailable, then enabling this will still not enable the browser task scheduler BUG=553459,633389 ========== to ========== Add Browser Task Scheduler Flags Adds the browser scheduler command line flags as well as exposing it via chrome://flags. Enabling the feature will place the browser in a forced variations group, allowing it to use task scheduler parameters from there. If the variations group is unavailable, then enabling this will still not enable the browser task scheduler BUG=553459,633389 Committed: https://crrev.com/2ce242f8b6f3485e962363f6e05873576c313719 Cr-Commit-Position: refs/heads/master@{#411199} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2ce242f8b6f3485e962363f6e05873576c313719 Cr-Commit-Position: refs/heads/master@{#411199} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
