Announcing base::Feature API

[Alexei Svitkine]

Hi chromium-dev,

I'd like to announce a new API in base/ intended to replace the current practice of --enable-foo and --disable-foo style switches for enabling and disabling features. The new base/feature_list.h API simplifies code to test for the state of a feature to:

base::FeatureList::IsEnabled(kMyFeature)

… which can greatly simplify the logic that otherwise may need to have multiple conditionals checking CommandLine and FieldTrial states.

The new API allows expressing the default state of a Feature in a structured way, as part of its definition, while still allowing the state to be overridden through the command-line or with a FieldTrial. This is done by defining a base::Feature struct in the codebase for each feature:

const base::Feature kMyFeature {

  "MyFeature", base::FEATURE_ENABLED_BY_DEFAULT

};

(The above entry can live in a standard location analogous to e.g. chrome_switches.[cc|h] or content_switches.[cc|h] - i.e. chrome_features.cc or content_features.cc, but can live within a component.)

For each feature, you can turn it on or off from the command-line via:

  --enable-features=MyFeature

  --disable-features=MyFeature

So you don't need to add your own --enable-foo and --disable-foo switches when using the new API. The above flags also support passing a comma-separated list of feature names. This is also supported for chrome://flags, where you can use the FEATURE_VALUE_TYPE() macro to define your entry.

Finally, the other benefit of FeatureList is integration with VariationsService, which allows having a server-side FieldTrial override the default state of a feature. Besides experimentation, this is also useful as a kill switch mechanism for your feature, if needed. (This still requires a browser restart to take effect; I'm planning to later expand this support to allow runtime killing of specific features that opt-in to that functionality, but that's not implemented yet.)

So, going forward if you see yourself adding something that requires --enable-foo and --disable-foo flags, considering using base/feature_list.h instead, as eventually we'd like to get to a point where all of this is done through FeatureList. Also, if you'd like to help migrate existing flags to FeatureList API, all help is appreciated!

-Alexei

[Dana Jansens]

On Tue, Nov 24, 2015 at 10:53 AM, Alexei Svitkine <asvi...@chromium.org> wrote:

Hi chromium-dev,

I'd like to announce a new API in base/ intended to replace the current practice of --enable-foo and --disable-foo style switches for enabling and disabling features. The new base/feature_list.h API simplifies code to test for the state of a feature to:

base::FeatureList::IsEnabled(kMyFeature)

… which can greatly simplify the logic that otherwise may need to have multiple conditionals checking CommandLine and FieldTrial states.

The new API allows expressing the default state of a Feature in a structured way, as part of its definition, while still allowing the state to be overridden through the command-line or with a FieldTrial. This is done by defining a base::Feature struct in the codebase for each feature:

const base::Feature kMyFeature {

  "MyFeature", base::FEATURE_ENABLED_BY_DEFAULT

};

(The above entry can live in a standard location analogous to e.g. chrome_switches.[cc|h] or content_switches.[cc|h] - i.e. chrome_features.cc or content_features.cc, but can live within a component.)

For each feature, you can turn it on or off from the command-line via:

  --enable-features=MyFeature

  --disable-features=MyFeature

So you don't need to add your own --enable-foo and --disable-foo switches when using the new API. The above flags also support passing a comma-separated list of feature names. This is also supported for chrome://flags, where you can use the FEATURE_VALUE_TYPE() macro to define your entry.

Finally, the other benefit of FeatureList is integration with VariationsService, which allows having a server-side FieldTrial override the default state of a feature. Besides experimentation, this is also useful as a kill switch mechanism for your feature, if needed. (This still

Is this state surfaced somewhere so users can report what features their browser instance is using when making bug reports?

 

requires a browser restart to take effect; I'm planning to later expand this support to allow runtime killing of specific features that opt-in to that functionality, but that's not implemented yet.)

So, going forward if you see yourself adding something that requires --enable-foo and --disable-foo flags, considering using base/feature_list.h instead, as eventually we'd like to get to a point where all of this is done through FeatureList. Also, if you'd like to help migrate existing flags to FeatureList API, all help is appreciated!

How does this work with ChromeOS guest mode? There's a whitelist of command-line flags, so the enable/disable features flags need to be whitelisted to allow features to be overridden in guest mode: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chromeos/login/chrome_restart_request.cc Otherwise you get different behaviour there which we don't usually want for graphics-related stuff. But maybe there should be a per-feature whitelist for guest mode as well?

 

-Alexei

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Alexei Svitkine]

On Tue, Nov 24, 2015 at 2:10 PM, Dana Jansens <dan...@chromium.org> wrote:

On Tue, Nov 24, 2015 at 10:53 AM, Alexei Svitkine <asvi...@chromium.org> wrote:

Hi chromium-dev,

I'd like to announce a new API in base/ intended to replace the current practice of --enable-foo and --disable-foo style switches for enabling and disabling features. The new base/feature_list.h API simplifies code to test for the state of a feature to:

base::FeatureList::IsEnabled(kMyFeature)

… which can greatly simplify the logic that otherwise may need to have multiple conditionals checking CommandLine and FieldTrial states.

The new API allows expressing the default state of a Feature in a structured way, as part of its definition, while still allowing the state to be overridden through the command-line or with a FieldTrial. This is done by defining a base::Feature struct in the codebase for each feature:

const base::Feature kMyFeature {

  "MyFeature", base::FEATURE_ENABLED_BY_DEFAULT

};

(The above entry can live in a standard location analogous to e.g. chrome_switches.[cc|h] or content_switches.[cc|h] - i.e. chrome_features.cc or content_features.cc, but can live within a component.)

For each feature, you can turn it on or off from the command-line via:

  --enable-features=MyFeature

  --disable-features=MyFeature

So you don't need to add your own --enable-foo and --disable-foo switches when using the new API. The above flags also support passing a comma-separated list of feature names. This is also supported for chrome://flags, where you can use the FEATURE_VALUE_TYPE() macro to define your entry.

Finally, the other benefit of FeatureList is integration with VariationsService, which allows having a server-side FieldTrial override the default state of a feature. Besides experimentation, this is also useful as a kill switch mechanism for your feature, if needed. (This still

Is this state surfaced somewhere so users can report what features their browser instance is using when making bug reports?

Not explicitly, though I welcome proposals. Right now, chrome://version already shows: a) the users' version, b) any command line flags, including feature enable/disable ones, c) variations (i.e. server-side field trials), which may affect feature states. So at the very least, all the info is there to reconstruct this state, although it doesn't currently make it easy. We could add a special URL to dump this information explicitly, if desired.

 

requires a browser restart to take effect; I'm planning to later expand this support to allow runtime killing of specific features that opt-in to that functionality, but that's not implemented yet.)

So, going forward if you see yourself adding something that requires --enable-foo and --disable-foo flags, considering using base/feature_list.h instead, as eventually we'd like to get to a point where all of this is done through FeatureList. Also, if you'd like to help migrate existing flags to FeatureList API, all help is appreciated!

How does this work with ChromeOS guest mode? There's a whitelist of command-line flags, so the enable/disable features flags need to be whitelisted to allow features to be overridden in guest mode: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chromeos/login/chrome_restart_request.cc Otherwise you get different behaviour there which we don't usually want for graphics-related stuff. But maybe there should be a per-feature whitelist for guest mode as well?

That's a good question. Right now, we don't do anything special for ChromeOS. I'm open to either approach you suggest. I guess we should talk with ChromeOS experts about whether it needs to be per-feature or overall.

[Elliott Sprehn]

It would be nice to see how this differs from what we do with RuntimeEnabledFeatures in Blink and if we can change this base/ API to be usable in blink. For starters it seems like this isn't very low overhead, it uses a std::map (why not a hash_map?) so you end up doing a lot of work per feature check. Blink code generates member fields so checking for a feature is an inline boolean read.

Can we do code generation like we do in blink instead? That would make this faster and we could probably use the same infrastructure in blink.

On Tue, Nov 24, 2015 at 10:53 AM, Alexei Svitkine <asvi...@chromium.org> wrote:

--

[Dana Jansens]

On Tue, Nov 24, 2015 at 11:27 AM, Elliott Sprehn <esp...@chromium.org> wrote:

It would be nice to see how this differs from what we do with RuntimeEnabledFeatures in Blink and if we can change this base/ API to be usable in blink. For starters it seems like this isn't very low overhead, it uses a std::map (why not a hash_map?) so you end up doing a lot of work per feature check. Blink code generates member fields so checking for a feature is an inline boolean read.

Can we do code generation like we do in blink instead? That would make this faster and we could probably use the same infrastructure in blink.

+1, using a new API to get rid of passing giant command-lines between processes would be great, rather than wrapping command-line parsing and string-comparisons into a new place within a single switch.

[Peter Kasting]

I'm happy about making this stuff simpler and more consistent.

One concern I have with our existing flags is that people tend to leave them in the codebase too long.  I've tried initiatives in the past to get people to remove old flags (since we have >1000) but it's very difficult to get traction.  Removing flags is really about removing the underlying complexity that the flag requires, which is often a hidden cost but impacts maintenance, testing, etc.

I'm concerned that this system might make it even easier to add long-lived complexity to the codebase, especially where we might look at a feature and not know if there were any ongoing field trials controlling it.

I don't have good solutions to this problem, but perhaps defining a new Feature should require defining an end date after which the feature automatically gets forced to one state or the other?  This would facilitate automated tooling to alert us to "outdated" features that could then hopefully be simplified.  Forcing owners to think about this when they add a Feature, and think about it again at the feature end date, seems like a win to me.

PK

[Marshall Greenblatt]

What about flags that need to be passed to sub-processes (for example [1])? Do you plan to support this via FeatureList?

[1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chrome_content_browser_client.cc&q=ChromeContentBrowserClient::AppendExtraCommandLineSwitches&sq=package:chromium&type=cs&l=1372

 

-Alexei

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Alexei Svitkine]

On Tue, Nov 24, 2015 at 2:27 PM, Elliott Sprehn <esp...@chromium.org> wrote:

It would be nice to see how this differs from what we do with RuntimeEnabledFeatures in Blink and if we can change this base/ API to be usable in blink. For starters it seems like this isn't very low overhead, it uses a std::map (why not a hash_map?) so you end up doing a lot of work per feature check. Blink code generates member fields so checking for a feature is an inline boolean read.

Can we do code generation like we do in blink instead? That would make this faster and we could probably use the same infrastructure in blink.

The new API is meant to replace the use of base/command_line.h and base/metrics/field_trial.h APIs. Both of these have similar performance characteristics as the new API (i.e. both do a look up in a map and FieldTrial actually uses a lock, whereas the new API doesn't need to since overrides are set up while we're still in single process mode).

So, it is not any worse (and in fact slightly better since it's a single lookup vs. possibly multiple) than the previous alternatives for chrome code.

I'm skeptical that we need the complexity of code-generation. If something is really performance critical (e.g. being used in a tight loop), it's probably worth querying the API once and using a cached variable for the state.

Having said that, I don't know all the requirements of RuntimeEnabledFeatures in Blink. Though I support us thinking about unifying the systems - and am open to tweaking the implementation if we need there's a real world performance problem.

[Alexei Svitkine]

On Tue, Nov 24, 2015 at 4:42 PM, Marshall Greenblatt <magree...@gmail.com> wrote:

What about flags that need to be passed to sub-processes (for example [1])? Do you plan to support this via FeatureList?

[1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chrome_content_browser_client.cc&q=ChromeContentBrowserClient::AppendExtraCommandLineSwitches&sq=package:chromium&type=cs&l=1372

Yes, this is supported. The flags are automatically plumbed through to spawned renderer processes. (We can add support for other child processes as well once there's a need.)

[Alexei Svitkine]

I think it's good to think about this problem and I agree that we should try to avoid it. As a start, I hope having the new API will allow us to re-examine the existing flags when we look at whether they should be migrated to it (or in many cases, maybe simply removed). Going forward, it probably makes sense to add a mechanism to keep the list of Features maintainable. I think we can build tools that can e.g. detect features that are off by default (since this is annotated in the source code) and also don't have any active server side experiments and file bugs that maybe they should be deleted if they have been in that state for a while. Conversely, the features that are on by default should have a good reason for them to still be toggleable (e.g. either there's a good reason for them to have a kill switch or something similar) and we can come up with good logic to detect when these might be going stale too.

[Antoine Labour]

On Tue, Nov 24, 2015 at 1:43 PM, Alexei Svitkine <asvi...@chromium.org> wrote:

On Tue, Nov 24, 2015 at 2:27 PM, Elliott Sprehn <esp...@chromium.org> wrote:

It would be nice to see how this differs from what we do with RuntimeEnabledFeatures in Blink and if we can change this base/ API to be usable in blink. For starters it seems like this isn't very low overhead, it uses a std::map (why not a hash_map?) so you end up doing a lot of work per feature check. Blink code generates member fields so checking for a feature is an inline boolean read.

Can we do code generation like we do in blink instead? That would make this faster and we could probably use the same infrastructure in blink.

The new API is meant to replace the use of base/command_line.h and base/metrics/field_trial.h APIs. Both of these have similar performance characteristics as the new API (i.e. both do a look up in a map and FieldTrial actually uses a lock, whereas the new API doesn't need to since overrides are set up while we're still in single process mode).

So, it is not any worse (and in fact slightly better since it's a single lookup vs. possibly multiple) than the previous alternatives for chrome code.

I'm skeptical that we need the complexity of code-generation. If something is really performance critical (e.g. being used in a tight loop), it's probably worth querying the API once and using a cached variable for the state.

I keep reviewing code that either does string compares in perf-sensitive paths, or that caches things in statics that break in --single-process and in tests.

So if we're going to invent a new system, +1 to trying to solve this so that no-one needs to worry about it ever again.

Antoine