Init perfetto really early in WebView [chromium/src : main]
Peter Pakkenberg (Gerrit)
Peter Pakkenberg added 1 comment![]( "Open in Gerrit")
Hi Abhijith, could you take a look at this before I send it to others for review?
Related details
- Abhijith Nair
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abhijith Nair (Gerrit)
Abhijith Nair added 2 comments![]( "Open in Gerrit")
bool enable_system_consumer,This looks different from `enable_system_tracing` in the CC file. Looking at the existing code, they seem to have different semantics.
Can you please clarify which we mean here?
base::RepeatingCallback<bool()> should_allow_system_tracing) {What's the difference between this and `enable_system_tracing`? Are they not the same thing?
Related details
- Peter Pakkenberg
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Pakkenberg (Gerrit)
Peter Pakkenberg added 2 comments![]( "Open in Gerrit")
This looks different from `enable_system_tracing` in the CC file. Looking at the existing code, they seem to have different semantics.
Can you please clarify which we mean here?
Yeah - I realised I didn't have to pipe this extra parameter through anyway, so I removed it, and called `SetSystemTracingEnabled` directly in `aw_browser_process.cc`
base::RepeatingCallback<bool()> should_allow_system_tracing) {What's the difference between this and `enable_system_tracing`? Are they not the same thing?
Not really, no - one overrides a feature flag. The `should_allow_system_tracing` callback is effectively always `return false;` in WebView.
My aim here was to ensure that the feature flag could still be used.
Related details
- Abhijith Nair
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abhijith Nair (Gerrit)
Abhijith Nair added 3 comments![]( "Open in Gerrit")
return g_tracing_initialized_after_featurelist;`after_featurelist` suffix no longer makes sense.
Maybe get rid of the suffix?
traced_process.InitPostFeatureList(enable_consumer);This will crash in Windows, wouldn't it?
https://crsrc.org/c/services/tracing/public/cpp/perfetto/perfetto_traced_process.cc;l=361;drc=50ade2d8d071e10bc5d53234bb2c0b311c515940
I am unsure if there are other places called from `InitTracingInternal` that depends on the feature list.
Maybe we should only enable `InitTracingPreFeatureList` to be called on Android?
We should also prevent a situation where somebody later adds some other feature check in `PerfettoTracedProcess::InitPostFeatureList` and breaks us.
bool g_enable_system_tracing = false;nit: I think it would be useful to add a comment here to say that whether system tracing runs is not completely dependent on the state of this flag.
Related details
- Peter Pakkenberg
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Pakkenberg (Gerrit)
Peter Pakkenberg added 3 comments![]( "Open in Gerrit")
`after_featurelist` suffix no longer makes sense.
Maybe get rid of the suffix?
Acknowledged
This will crash in Windows, wouldn't it?
https://crsrc.org/c/services/tracing/public/cpp/perfetto/perfetto_traced_process.cc;l=361;drc=50ade2d8d071e10bc5d53234bb2c0b311c515940I am unsure if there are other places called from `InitTracingInternal` that depends on the feature list.
Maybe we should only enable `InitTracingPreFeatureList` to be called on Android?
We should also prevent a situation where somebody later adds some other feature check in `PerfettoTracedProcess::InitPostFeatureList` and breaks us.
Good suggestions.
I have added a test for `InitTracingPreFeatureList` that ensures that it doesn't crash if the featurelist hasn't been initialized. That's probably the most we can do here.
I have also gone and removed the `InitPostFeatureList` from `PerfettoTracedProcess` since it was just a direct call through to `SetupClientLibrary`.
nit: I think it would be useful to add a comment here to say that whether system tracing runs is not completely dependent on the state of this flag.
- Abhijith Nair
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abhijith Nair (Gerrit)
Abhijith Nair voted Code-Review+1![]( "Open in Gerrit")
- Peter Pakkenberg
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 5 comments![]( "Open in Gerrit")
bool enable_system_consumer) {Nit: enable_system_backend
Otherwise this gets confusing with `should_allow_system_tracing` argument which refers to consumer, whereas this is for backend.
tracing::SetSystemTracingEnabled(enable_system_consumer);
tracing::InitTracingPreFeatureList(/*enable_consumer=*/true,
/*will_trace_thread_restart=*/false);Could we plumb `enable_system_backend` (as override) directly through InitTracing as optional param:
```
if (ShouldSetupSystemTracing(enable_system_backend_override)) {
```
if (WebViewCachedFlags.get()
.isCachedFeatureEnabled(AwFeatures.WEBVIEW_EARLY_PERFETTO_INIT)) {I'm not sure we need to put this behind a flag.
I don't think there's a lot of value in running an experiment on this: success metric is simply data quality + we might want to monitor crash, although this can be done with the normal rollout.
If this is only to have a kill switch, I think the CL is low risk and easy enough to revert.
base::RepeatingCallback<bool()> should_allow_system_tracing) {Could you rename this to `allow_system_tracing_consumer` here and in .h, that would remove ambiguity with enabling the system backend (see comment in android_webview/browser/aw_browser_process.cc)
void InitTracingPreFeatureList(Nit: Maybe we simply expose `InitTracingInternal` as `InitTracing`,
and have `InitTracingPostFeatureList` call that.
Related details
- Peter Pakkenberg
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Pakkenberg (Gerrit)
Peter Pakkenberg voted and added 5 comments![]( "Open in Gerrit")
Votes added by Peter Pakkenberg
| Commit-Queue | +1 |
5 comments
Nit: enable_system_backend
Otherwise this gets confusing with `should_allow_system_tracing` argument which refers to consumer, whereas this is for backend.
Done
tracing::SetSystemTracingEnabled(enable_system_consumer);
tracing::InitTracingPreFeatureList(/*enable_consumer=*/true,
/*will_trace_thread_restart=*/false);Could we plumb `enable_system_backend` (as override) directly through InitTracing as optional param:
```
if (ShouldSetupSystemTracing(enable_system_backend_override)) {
```
I pulled the feature flag check all the way out and put it in `InitTracingPostFeatureList` instead - that way, there is one less feature checks deep in the init code.
if (WebViewCachedFlags.get()
.isCachedFeatureEnabled(AwFeatures.WEBVIEW_EARLY_PERFETTO_INIT)) {I'm not sure we need to put this behind a flag.
I don't think there's a lot of value in running an experiment on this: success metric is simply data quality + we might want to monitor crash, although this can be done with the normal rollout.If this is only to have a kill switch, I think the CL is low risk and easy enough to revert.
We do want to measure the impact this change in point of initialization has on WebView startup, hence the feature flag.
base::RepeatingCallback<bool()> should_allow_system_tracing) {Could you rename this to `allow_system_tracing_consumer` here and in .h, that would remove ambiguity with enabling the system backend (see comment in android_webview/browser/aw_browser_process.cc)
Done
Nit: Maybe we simply expose `InitTracingInternal` as `InitTracing`,
and have `InitTracingPostFeatureList` call that.
- Abhijith Nair
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 3 comments![]( "Open in Gerrit")
tracing::SetSystemTracingEnabled(enable_system_consumer);
tracing::InitTracingPreFeatureList(/*enable_consumer=*/true,
/*will_trace_thread_restart=*/false);Peter PakkenbergCould we plumb `enable_system_backend` (as override) directly through InitTracing as optional param:
```
if (ShouldSetupSystemTracing(enable_system_backend_override)) {
```
I pulled the feature flag check all the way out and put it in `InitTracingPostFeatureList` instead - that way, there is one less feature checks deep in the init code.
Cool even better!
if (WebViewCachedFlags.get()
.isCachedFeatureEnabled(AwFeatures.WEBVIEW_EARLY_PERFETTO_INIT)) {Peter PakkenbergI'm not sure we need to put this behind a flag.
I don't think there's a lot of value in running an experiment on this: success metric is simply data quality + we might want to monitor crash, although this can be done with the normal rollout.If this is only to have a kill switch, I think the CL is low risk and easy enough to revert.
We do want to measure the impact this change in point of initialization has on WebView startup, hence the feature flag.
Ack.
if (g_tracing_initialized) {
return;
}I tried extra hard to prevent/disallow multiple calls, in order to be able to pinpoint the different initialization paths.
I think the new webview call collides with this one: https://source.chromium.org/chromium/chromium/src/+/main:content/app/content_main_runner_impl.cc;l=1210-1212;drc=e3ad182cacc3a02eb6a13091a867815fd17a129c;bpv=1;bpt=1
Do you think it'd be possible to do one of:
- Make the second call only happen if the kWebViewEarlyPerfettoInit is disabled
- Plumb a flag similar to https://source.chromium.org/chromium/chromium/src/+/main:content/gpu/gpu_main.cc;l=412;drc=e3ad182cacc3a02eb6a13091a867815fd17a129c;bpv=1;bpt=1
- Or at least relax this condition only on webview.
Related details
- Abhijith Nair
- Peter Pakkenberg
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Pakkenberg (Gerrit)
Peter Pakkenberg added 1 comment![]( "Open in Gerrit")
if (g_tracing_initialized) {
return;
}I tried extra hard to prevent/disallow multiple calls, in order to be able to pinpoint the different initialization paths.
I think the new webview call collides with this one: https://source.chromium.org/chromium/chromium/src/+/main:content/app/content_main_runner_impl.cc;l=1210-1212;drc=e3ad182cacc3a02eb6a13091a867815fd17a129c;bpv=1;bpt=1Do you think it'd be possible to do one of:
- Make the second call only happen if the kWebViewEarlyPerfettoInit is disabled
- Plumb a flag similar to https://source.chromium.org/chromium/chromium/src/+/main:content/gpu/gpu_main.cc;l=412;drc=e3ad182cacc3a02eb6a13091a867815fd17a129c;bpv=1;bpt=1
- Or at least relax this condition only on webview.
We sadly don't have any `BUILDFLAG(IS_WEBVIEW)` so this check has to be done dynamically at runtime.
I've reinstated the `DCHECK(!g_tracing_initialized)` and added a check in `ContentMainRunnerImpl` to determine if we should call `InitTracingPostFeatureList` there, since an explicit check seems to be in line with your suggestions.
I also considered simply wrapping the call in `ContentMainRunnerImpl` like this
```
if (!tracing::IsTracingInitialized()) {
tracing::InitTracingPostFeatureList(...);
}
```
Since you hadn't suggested this, it seemed like it wouldn't be in the spirit of what you are trying to achieve, but if you think that approach would also be OK, then I will rewrite to do that instead, since it will be much less plumbing than what I have currently put in.
Related details
- Abhijith Nair
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 4 comments![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Code-Review | +1 |
4 comments
if (!tracing::IsTracingInitialized()) {Can we do without this check? I figured AwBrowserProcess_InitPerfetto would be the earliest call to initialize tracing.
return std::visit(
overloaded{[](const ContentMainDelegate::InvokedInBrowserProcess& arg) {
return !AwBrowserProcess::DidEarlyPerfettoInitialization();
},
[](const ContentMainDelegate::InvokedInChildProcess& arg) {
// We only initialize Perfetto manually in the browser
// process.
return true;
}},
invoked_in);Nit: might be simpler to use conditional
```
const bool is_browser_process =
std::holds_alternative<InvokedInBrowserProcess>(invoked_in);
if (!is_browser_process) {
return true;
}
return !AwBrowserProcess::DidEarlyPerfettoInitialization();
```
if (g_tracing_initialized) {
return;
}Peter PakkenbergI tried extra hard to prevent/disallow multiple calls, in order to be able to pinpoint the different initialization paths.
I think the new webview call collides with this one: https://source.chromium.org/chromium/chromium/src/+/main:content/app/content_main_runner_impl.cc;l=1210-1212;drc=e3ad182cacc3a02eb6a13091a867815fd17a129c;bpv=1;bpt=1Do you think it'd be possible to do one of:
- Make the second call only happen if the kWebViewEarlyPerfettoInit is disabled
- Plumb a flag similar to https://source.chromium.org/chromium/chromium/src/+/main:content/gpu/gpu_main.cc;l=412;drc=e3ad182cacc3a02eb6a13091a867815fd17a129c;bpv=1;bpt=1
- Or at least relax this condition only on webview.
We sadly don't have any `BUILDFLAG(IS_WEBVIEW)` so this check has to be done dynamically at runtime.
I've reinstated the `DCHECK(!g_tracing_initialized)` and added a check in `ContentMainRunnerImpl` to determine if we should call `InitTracingPostFeatureList` there, since an explicit check seems to be in line with your suggestions.
I also considered simply wrapping the call in `ContentMainRunnerImpl` like this
```
if (!tracing::IsTracingInitialized()) {
tracing::InitTracingPostFeatureList(...);
}
```Since you hadn't suggested this, it seemed like it wouldn't be in the spirit of what you are trying to achieve, but if you think that approach would also be OK, then I will rewrite to do that instead, since it will be much less plumbing than what I have currently put in.
- Abhijith Nair
- Peter Pakkenberg
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Pakkenberg (Gerrit)
Peter Pakkenberg added 2 comments![]( "Open in Gerrit")
Can we do without this check? I figured AwBrowserProcess_InitPerfetto would be the earliest call to initialize tracing.
Yes, and now that you point it out, it would be better to DCHECK and crash if it turns out not to be.
return std::visit(
overloaded{[](const ContentMainDelegate::InvokedInBrowserProcess& arg) {
return !AwBrowserProcess::DidEarlyPerfettoInitialization();
},
[](const ContentMainDelegate::InvokedInChildProcess& arg) {
// We only initialize Perfetto manually in the browser
// process.
return true;
}},
invoked_in);Nit: might be simpler to use conditional
```
const bool is_browser_process =
std::holds_alternative<InvokedInBrowserProcess>(invoked_in);
if (!is_browser_process) {
return true;
}
return !AwBrowserProcess::DidEarlyPerfettoInitialization();
```
Yep, that is easier.
My first time having to deal with `std::variant` so I was a bit lost on what the usual approach was.
Related details
- Abhijith Nair
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Pakkenberg (Gerrit)
Peter Pakkenberg added 1 comment![]( "Open in Gerrit")
Hi Peter, can you do an OWNERS review of this CL for //android_webview?
Related details
- Abhijith Nair
- Peter Conn
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Pakkenberg (Gerrit)
Peter Pakkenberg added 1 comment![]( "Open in Gerrit")
Hi Dave, could you do a OWNERS review for //content on this CL?
Related details
- Abhijith Nair
- Dave Tapuska
- Peter Conn
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dave Tapuska
| Code-Review | +1 |
1 comment
// WebView may initialize perfetto early, so check if we should do it here.Can we change this to:
WebView may have already initialized perfetto, so check if we should do it here.
Related details
- Abhijith Nair
- Peter Conn
- Peter Pakkenberg
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Pakkenberg (Gerrit)
Peter Pakkenberg voted and added 1 comment![]( "Open in Gerrit")
Votes added by Peter Pakkenberg
| Commit-Queue | +1 |
1 comment
// WebView may initialize perfetto early, so check if we should do it here.Can we change this to:
WebView may have already initialized perfetto, so check if we should do it here.
Done
Related details
- Abhijith Nair
- Peter Conn
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Conn (Gerrit)
Peter Conn voted and added 2 comments![]( "Open in Gerrit")
Votes added by Peter Conn
| Code-Review | +1 |
2 comments
public static void initPerfetto(boolean enableSystemConsumer) {What is this parameter? (Can you add javadoc?)
Related details
- Abhijith Nair
- Peter Pakkenberg
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Pakkenberg (Gerrit)
Peter Pakkenberg added 1 comment![]( "Open in Gerrit")
public static void initPerfetto(boolean enableSystemConsumer) {What is this parameter? (Can you add javadoc?)
Done
Related details
- Abhijith Nair
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Pakkenberg (Gerrit)
Peter Pakkenberg voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Init perfetto really early in WebView
This CL adds a new cached feature flag that initializes Perfetto as soon
as the native library has loaded.
It also moves the check for `ShouldSetupSystemTracing()` further up the
call stack to avoid feature checks deep in the init code.
- M android_webview/browser/aw_browser_process.cc
- M android_webview/browser/aw_browser_process.h
- M android_webview/common/DEPS
- M android_webview/common/aw_feature_map.cc
- M android_webview/common/aw_features.cc
- M android_webview/common/aw_features.h
- M android_webview/glue/BUILD.gn
- M android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java
- M android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java
- M android_webview/java/src/org/chromium/android_webview/common/ProductionSupportedFlagList.java
- M android_webview/java/src/org/chromium/android_webview/common/WebViewCachedFlags.java
- M android_webview/lib/aw_main_delegate.cc
- M android_webview/lib/aw_main_delegate.h
- M content/app/content_main_runner_impl.cc
- M content/public/app/content_main_delegate.cc
- M content/public/app/content_main_delegate.h
- M services/tracing/public/cpp/perfetto/perfetto_traced_process.cc
- M services/tracing/public/cpp/perfetto/perfetto_traced_process.h
- M services/tracing/public/cpp/trace_startup.cc
- M services/tracing/public/cpp/trace_startup.h
- M services/tracing/public/cpp/tracing_features.h
Code-Review: +1 by Dave Tapuska, +1 by Etienne Pierre-Doray, +1 by Peter Conn
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |