[iOS tracing] Enable in-process Perfetto tracing for developer profiling [chromium/src : main]
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
TEST_F(IOSStartupTracingControllerTest, CreatesValidDeveloperConfig) {Justin CohenYou need an additional test that covers the recording of an actual trace by calling Initialize and OnAppEnterBackground. Make sure to reset perfetto global state to prevent crosstalk between tests. Ideally, the test should record a synthetic trace event, then ensure it is present in the recording.
Added a startup and a manual one. ptal!
Related details
- Etienne Pierre-Doray
- Justin Novosad
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Novosad (Gerrit)
Justin Novosad voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Novosad
| Code-Review | +1 |
1 comment
TEST_F(IOSStartupTracingControllerTest, CreatesValidDeveloperConfig) {Justin CohenYou need an additional test that covers the recording of an actual trace by calling Initialize and OnAppEnterBackground. Make sure to reset perfetto global state to prevent crosstalk between tests. Ideally, the test should record a synthetic trace event, then ensure it is present in the recording.
Added a startup and a manual one. ptal!
- Etienne Pierre-Doray
- Justin Cohen
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Novosad (Gerrit)
Justin Novosad voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Novosad
| Code-Review | +1 |
1 comment
IOSStartupTracingController::GetInstance().Initialize();Nit: Is it weird that a class with "Startup" in its name is also used for manual trace recording?
Justin Cohen (Gerrit)
Justin Cohen added 2 comments![]( "Open in Gerrit")
static base::NoDestructor<EmergencyTraceFinalisationCoordinator> instance;I don't love this change but I need something to make this more repeatedly testable. Or I can drop the Startup test.
static base::NoDestructor<TraceStartupConfig> g_instance;Same here
Related details
- Etienne Pierre-Doray
- Justin Novosad
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 3 comments![]( "Open in Gerrit")
IOSStartupTracingController::GetInstance().Initialize();Nit: Is it weird that a class with "Startup" in its name is also used for manual trace recording?
I'm open to suggestions. WDYT?
[ChromeEarlGrey waitForWebStateContainingText:"Stop and Share"];Justin CohenIsn't this condition tautological? I mean the button with this label remains visible regardless of whether or not the button is disabled. This test should instead observe UI state transitions by monitoring the statusText element. In this case, I think it should be waiting for the text to change to "Recording...".
Justin Cohenmoved to https://chromium-review.googlesource.com/c/chromium/src/+/7751575
Done
[ChromeEarlGrey waitForWebStateContainingText:"Start Recording"];Justin CohenSame issue here. The test should be waiting for the statusText to change back to "Ready" IMHO.
Justin Cohenmoved to https://chromium-review.googlesource.com/c/chromium/src/+/7751575
Done
Justin Novosad (Gerrit)
Justin Novosad added 2 comments![]( "Open in Gerrit")
static base::NoDestructor<EmergencyTraceFinalisationCoordinator> instance;I don't love this change but I need something to make this more repeatedly testable. Or I can drop the Startup test.
What if ResetForTesting did a reset of the instance's state instead of destroying it?
IOSStartupTracingController::GetInstance().Initialize();Justin CohenNit: Is it weird that a class with "Startup" in its name is also used for manual trace recording?
I'm open to suggestions. WDYT?
- Etienne Pierre-Doray
- Justin Cohen
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 3 comments![]( "Open in Gerrit")
static base::NoDestructor<EmergencyTraceFinalisationCoordinator> instance;Justin NovosadI don't love this change but I need something to make this more repeatedly testable. Or I can drop the Startup test.
What if ResetForTesting did a reset of the instance's state instead of destroying it?
Done
IOSStartupTracingController::GetInstance().Initialize();Justin CohenNit: Is it weird that a class with "Startup" in its name is also used for manual trace recording?
Justin NovosadI'm open to suggestions. WDYT?
IOSTracingController?
- Etienne Pierre-Doray
- Justin Novosad
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
static base::NoDestructor<TraceStartupConfig> g_instance;Justin CohenSame here
Done
Related details
- Etienne Pierre-Doray
- Justin Novosad
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Novosad (Gerrit)
Justin Novosad voted Code-Review+1![]( "Open in Gerrit")
- Etienne Pierre-Doray
- Justin Cohen
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| 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")
perfetto::TracingInitArgs init_args;I'm wondering if we want to use base::PerfettoPlatform for init_args.platform.
This uses a custom task runner. Although the main reason for it was FD watcher to support mojo, another benefit is the ability to (optionally) run in the thread pool, which is convenient in tests to flush tasks with TaskEnvironment.
config.mutable_buffers()->front().set_size_kb(1024 * 50);It's probably cleaner to specify the buffer size on trace_config upfront.
startup_tracing_controller_->Stop(std::move(callback));Up until now, all callers would do WaitUntilStopped() to ensure a trace is flushed before moving forward.
Is it intentional to simply Stop() here (which approaches detach semantic)? (AFAIU on_tracing_stopped_ is only used in test to run_loop.Run()).
The declaration comment makes it sounds like we want to run_loop.Run() still.
TRACE_EVENT0("startup", "StartupTestEvent");Nit: Let's use TRACE_EVENT(...); newer and functionally equivalent.
"perfetto/trace_packet_tokenizer.cc",
"perfetto/trace_packet_tokenizer.h",This and probably a few other things are specific to JSON export, which you could probably do without (guarded behind iOS) if you wanted to save binary size.
Related details
- Justin Cohen
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted and added 7 comments![]( "Open in Gerrit")
Votes added by Justin Cohen
| Commit-Queue | +1 |
7 comments
class IOSBackgroundTracingManager {Justin CohenLooking at what this does, it fills a role closer to StartupTracingController than BackgroundTracingManager (counterpart in content) + some initialization logic.
In fact, I'm wondering how much of StartupTracingController we could simply more to services and reuse here, which would support the same set of command line arguments.
Justin CohenHow about something like this, moving StartupTracingController to components/tracing/common?
https://chromium-review.googlesource.com/c/chromium/src/+/7750890
Now IOSStartupTracingController is much smaller.
Done
void IOSBackgroundTracingManager::Initialize() {Justin CohenEven though you might not need all of PerfettoTracedProcess, I think we can improve code sharing (by extracting and reusing parts of it)
- I think we want to use base::tracing::PerfettoPlatform on iOS
- We should reuse the same code for DataSource registrations
Justin CohenChanged to do:
perfetto::Tracing::Initialize(init_args);
tracing::RegisterCommonPerfettoDataSources(/*enable_consumer=*/true);from this parent refactor CL:
https://chromium-review.googlesource.com/c/chromium/src/+/7751295wdyt?
Done
I'm wondering if we want to use base::PerfettoPlatform for init_args.platform.
This uses a custom task runner. Although the main reason for it was FD watcher to support mojo, another benefit is the ability to (optionally) run in the thread pool, which is convenient in tests to flush tasks with TaskEnvironment.
Done
config.mutable_buffers()->front().set_size_kb(1024 * 50);It's probably cleaner to specify the buffer size on trace_config upfront.
Done
startup_tracing_controller_->Stop(std::move(callback));Up until now, all callers would do WaitUntilStopped() to ensure a trace is flushed before moving forward.
Is it intentional to simply Stop() here (which approaches detach semantic)? (AFAIU on_tracing_stopped_ is only used in test to run_loop.Run()).The declaration comment makes it sounds like we want to run_loop.Run() still.
removed this entirely
Nit: Let's use TRACE_EVENT(...); newer and functionally equivalent.
- Etienne Pierre-Doray
- Justin Novosad
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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")
void IOSTracingController::Initialize() {I don't love that we split initialization and instantiating the class in production (but it is somewhat needed for tests). In the past, this had lead to very tangled initialization sequences with PerfettoTracedProcess.
There's an attempt to improve that for PerfettoTracedProcess, but it's still half baked. WDYT of doing a similar pattern:
- GetInstance returns a g_instance.
- IOSTracingController sets the g_instance pointer.
- For production: CreateInstance() creates a static NoDestructor and initializes it.
- For test: Ideally we instantiate the class directly (or we do MaybeCreateInstanceForTesting()) + SetupForTesting, and we plumb a custom task runner.
// Start the shared startup tracing controller. It will check if
// TraceStartupConfig is enabled via command line internally.
startup_tracing_controller_->StartIfNeeded();FYI: Not sure about initialization order during startup on iOS, but this posts a task to the ThreadPool, which means you don't get any tracing data before the ThreadPool is started.
There are ways to work around that if you want earlier tracing.
// Ensure background tasks (like tracer destruction) are finished before
// resetting Perfetto.
base::ThreadPoolInstance::Get()->FlushForTesting();Now that the platform runs on a threadPool thread, it should be possible to follow the pattern in PerfettoTracedProcess::ResetForTesting, and do everything there.
base::trace_event::TraceLog::ResetForTesting();This shouldn't be needed (TraceLog is now only useful as a helper to create a tracing session)
TraceStartupConfig& TraceStartupConfig::GetInstance() {
static base::NoDestructor<TraceStartupConfig> instance;
return *instance;
}
// static
void TraceStartupConfig::ResetForTesting() {
GetInstance().Clear();
GetInstance().Initialize();
}What do you think about replacing this by a scoped TraceStartupConfig + setting g_instance pointer to access it in GetInstance().
TraceStartupConfig can live in PerfettoTracedProcess, and in IOSTracingController on iOS
Related details
- Justin Cohen
- Justin Novosad
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 6 comments![]( "Open in Gerrit")
ptal!
void IOSTracingController::Initialize() {I don't love that we split initialization and instantiating the class in production (but it is somewhat needed for tests). In the past, this had lead to very tangled initialization sequences with PerfettoTracedProcess.
There's an attempt to improve that for PerfettoTracedProcess, but it's still half baked. WDYT of doing a similar pattern:
- GetInstance returns a g_instance.
- IOSTracingController sets the g_instance pointer.
- For production: CreateInstance() creates a static NoDestructor and initializes it.
- For test: Ideally we instantiate the class directly (or we do MaybeCreateInstanceForTesting()) + SetupForTesting, and we plumb a custom task runner.
Done, although perfetto::internal::TracingMuxerImpl::InitializeInstance's globals makes this tricky. I need to keep the base::tracing::PerfettoPlatform around between tests, leading to an owned_platform_ + platform_ member.
// Start the shared startup tracing controller. It will check if
// TraceStartupConfig is enabled via command line internally.
startup_tracing_controller_->StartIfNeeded();FYI: Not sure about initialization order during startup on iOS, but this posts a task to the ThreadPool, which means you don't get any tracing data before the ThreadPool is started.
There are ways to work around that if you want earlier tracing.
On iOS ThreadPoolInstance::Create is called before IOSChromeMainParts::PreCreateThreads (where IOSTracingController::CreateInstance(); is called)
// Ensure background tasks (like tracer destruction) are finished before
// resetting Perfetto.
base::ThreadPoolInstance::Get()->FlushForTesting();Now that the platform runs on a threadPool thread, it should be possible to follow the pattern in PerfettoTracedProcess::ResetForTesting, and do everything there.
Done
base::trace_event::TraceLog::ResetForTesting();This shouldn't be needed (TraceLog is now only useful as a helper to create a tracing session)
Done
TraceStartupConfig& TraceStartupConfig::GetInstance() {
static base::NoDestructor<TraceStartupConfig> instance;
return *instance;
}
// static
void TraceStartupConfig::ResetForTesting() {
GetInstance().Clear();
GetInstance().Initialize();
}What do you think about replacing this by a scoped TraceStartupConfig + setting g_instance pointer to access it in GetInstance().
TraceStartupConfig can live in PerfettoTracedProcess, and in IOSTracingController on iOS
I tried, but a bunch of places access TraceStartupConfig::GetInstance() before PerfettoTracedProcess is created, so it'll crash.
See 71..72 for the reversal of my attempt.
Related details
- Etienne Pierre-Doray
- Justin Novosad
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 7 comments![]( "Open in Gerrit")
void IOSTracingController::Initialize() {Justin CohenI don't love that we split initialization and instantiating the class in production (but it is somewhat needed for tests). In the past, this had lead to very tangled initialization sequences with PerfettoTracedProcess.
There's an attempt to improve that for PerfettoTracedProcess, but it's still half baked. WDYT of doing a similar pattern:
- GetInstance returns a g_instance.
- IOSTracingController sets the g_instance pointer.
- For production: CreateInstance() creates a static NoDestructor and initializes it.
- For test: Ideally we instantiate the class directly (or we do MaybeCreateInstanceForTesting()) + SetupForTesting, and we plumb a custom task runner.
Done, although perfetto::internal::TracingMuxerImpl::InitializeInstance's globals makes this tricky. I need to keep the base::tracing::PerfettoPlatform around between tests, leading to an owned_platform_ + platform_ member.
Right, considering platform_ that needs to survive through tests, I think I prefer MaybeCreateInstanceForTesting() (with static NoDestructor) + SetupForTesting() (or InitializeForTest()?).
But either way, I think ResetForTesting() should be the only method needed to reset the state (and makes it so that we can initialize it again)
if (!platform_) {
owned_platform_ = std::make_unique<base::tracing::PerfettoPlatform>(
base::ThreadPool::CreateSequencedTaskRunner({base::MayBlock()}));
platform_ = owned_platform_.get();
}If going with MaybeCreateInstanceForTesting(), I think this can happen in constructor unconditionally.
if (!perfetto::Tracing::IsInitialized()) {Can this be a DCHECK, now that Initialize is only called once (in test it's called multiple time, but ResetForTesting() should put it back to uninitialized).
startup_tracing_controller_ =
std::make_unique<tracing::StartupTracingController>(
base::NullCallback(),
base::SingleThreadTaskRunner::GetCurrentDefault());I think this could happen in constructor, and if so we can do it unconditionally.
if (!g_test_platform) {
g_test_platform = new base::tracing::PerfettoPlatform(
base::ThreadPool::CreateSequencedTaskRunner({base::MayBlock()}));
}Not really an issue yet, but this seems somewhat incompatible with having multiple tests files in the same test targets using IOSTracingController.
I think this is solved by making IOSTracingController in charge of creating the PerfettoPlatform.
tracing::StartupTracingController::ResetForTesting();
tracing::TraceStartupConfig::ResetForTesting();
perfetto::Tracing::ResetForTesting();These should all be part of StartupTracingController::ResetForTesting()
TraceStartupConfig& TraceStartupConfig::GetInstance() {
static base::NoDestructor<TraceStartupConfig> instance;
return *instance;
}
// static
void TraceStartupConfig::ResetForTesting() {
GetInstance().Clear();
GetInstance().Initialize();
}Justin CohenWhat do you think about replacing this by a scoped TraceStartupConfig + setting g_instance pointer to access it in GetInstance().
TraceStartupConfig can live in PerfettoTracedProcess, and in IOSTracingController on iOS
I tried, but a bunch of places access TraceStartupConfig::GetInstance() before PerfettoTracedProcess is created, so it'll crash.
See 71..72 for the reversal of my attempt.
- Justin Cohen
- Justin Novosad
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 7 comments![]( "Open in Gerrit")
void IOSTracingController::Initialize() {Justin CohenI don't love that we split initialization and instantiating the class in production (but it is somewhat needed for tests). In the past, this had lead to very tangled initialization sequences with PerfettoTracedProcess.
There's an attempt to improve that for PerfettoTracedProcess, but it's still half baked. WDYT of doing a similar pattern:
- GetInstance returns a g_instance.
- IOSTracingController sets the g_instance pointer.
- For production: CreateInstance() creates a static NoDestructor and initializes it.
- For test: Ideally we instantiate the class directly (or we do MaybeCreateInstanceForTesting()) + SetupForTesting, and we plumb a custom task runner.
Etienne Pierre-DorayDone, although perfetto::internal::TracingMuxerImpl::InitializeInstance's globals makes this tricky. I need to keep the base::tracing::PerfettoPlatform around between tests, leading to an owned_platform_ + platform_ member.
Right, considering platform_ that needs to survive through tests, I think I prefer MaybeCreateInstanceForTesting() (with static NoDestructor) + SetupForTesting() (or InitializeForTest()?).
But either way, I think ResetForTesting() should be the only method needed to reset the state (and makes it so that we can initialize it again)
Done
if (!platform_) {
owned_platform_ = std::make_unique<base::tracing::PerfettoPlatform>(
base::ThreadPool::CreateSequencedTaskRunner({base::MayBlock()}));
platform_ = owned_platform_.get();
}If going with MaybeCreateInstanceForTesting(), I think this can happen in constructor unconditionally.
Done
Can this be a DCHECK, now that Initialize is only called once (in test it's called multiple time, but ResetForTesting() should put it back to uninitialized).
Done
startup_tracing_controller_ =
std::make_unique<tracing::StartupTracingController>(
base::NullCallback(),
base::SingleThreadTaskRunner::GetCurrentDefault());I think this could happen in constructor, and if so we can do it unconditionally.
Done
if (!g_test_platform) {
g_test_platform = new base::tracing::PerfettoPlatform(
base::ThreadPool::CreateSequencedTaskRunner({base::MayBlock()}));
}Not really an issue yet, but this seems somewhat incompatible with having multiple tests files in the same test targets using IOSTracingController.
I think this is solved by making IOSTracingController in charge of creating the PerfettoPlatform.
Done
tracing::StartupTracingController::ResetForTesting();
tracing::TraceStartupConfig::ResetForTesting();
perfetto::Tracing::ResetForTesting();These should all be part of StartupTracingController::ResetForTesting()
- Etienne Pierre-Doray
- Justin Novosad
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 5 comments![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Code-Review | +1 |
5 comments
g_instance = instance.get();Nit: Not needed considering the constructor does it?
if (startup_tracing_controller_) {Nit: is this ever nullptr?
void IOSTracingController::UpdateTaskRunnerForTesting() {Nit: How about
```
void InitializeForTesting() {
platform_->ResetTaskRunner(...);
Initialize();
}
```
And we can make Initialize() private.
tracing::StartupTracingController::ResetForTesting();
tracing::TraceStartupConfig::ResetForTesting();
perfetto::Tracing::ResetForTesting();Justin CohenThese should all be part of StartupTracingController::ResetForTesting()
Done
Looks like perfetto::Tracing::ResetForTesting is gone now; which I'd expect cause failures when trying re Initialize() again.
Related details
- Justin Cohen
- Justin Novosad
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
tracing::StartupTracingController::ResetForTesting();
tracing::TraceStartupConfig::ResetForTesting();
perfetto::Tracing::ResetForTesting();Justin CohenThese should all be part of StartupTracingController::ResetForTesting()
Etienne Pierre-DorayDone
Looks like perfetto::Tracing::ResetForTesting is gone now; which I'd expect cause failures when trying re Initialize() again.
Oh sorry, I see you put it in StartupTracingController::ResetForTesting(), but I meant it should be in IOSTracingController::ResetForTesting() 😊
Justin Cohen (Gerrit)
Justin Cohen added 4 comments![]( "Open in Gerrit")
g_instance = instance.get();Nit: Not needed considering the constructor does it?
Done
if (startup_tracing_controller_) {Nit: is this ever nullptr?
Done
void IOSTracingController::UpdateTaskRunnerForTesting() {Nit: How about
```
void InitializeForTesting() {
platform_->ResetTaskRunner(...);
Initialize();
}
```And we can make Initialize() private.
Done
tracing::StartupTracingController::ResetForTesting();
tracing::TraceStartupConfig::ResetForTesting();
perfetto::Tracing::ResetForTesting();Justin CohenThese should all be part of StartupTracingController::ResetForTesting()
Etienne Pierre-DorayDone
Etienne Pierre-DorayLooks like perfetto::Tracing::ResetForTesting is gone now; which I'd expect cause failures when trying re Initialize() again.
Oh sorry, I see you put it in StartupTracingController::ResetForTesting(), but I meant it should be in IOSTracingController::ResetForTesting() 😊
Got it, moved TraceStartupConfig::ResetForTesting(); and perfetto::Tracing::ResetForTesting() to IOSTracingController::ResetForTesting()
Related details
- Justin Novosad
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
Over to junov@ for iOS, and then rohitrao for iOS OWNERS
Related details
- Justin Novosad
- Justin Novosad
- Rohit Rao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Novosad (Gerrit)
Justin Novosad added 5 comments![]( "Open in Gerrit")
LGTM with nits.
DCHECK(!perfetto::Tracing::IsInitialized());This should be a CHECK rather than a DCHECK. See: https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/checks.md#invariant_verification-mechanisms
tracing::TraceStartupConfig::ResetForTesting();Is this really needed? The test fixture already takes care of this in the TearDown
IOSTracingController::GetInstance().InitializeForTesting();Should this be move to the fixture's setup method?
IOSTracingController::GetInstance().InitializeForTesting();same here.
Related details
- Justin Cohen
- Justin Novosad
- Rohit Rao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rohit Rao (Gerrit)
Rohit Rao voted and added 3 comments![]( "Open in Gerrit")
Votes added by Rohit Rao
| Code-Review | +1 |
3 comments
lgtm but please wait for junov as well
static base::NoDestructor<IOSTracingController> instance;How does this work?
tracing_lib_type = "component"Can we remove this variable and just define line 33 as `component("cpp")` instead?
Related details
- Justin Cohen
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted and added 6 comments![]( "Open in Gerrit")
Votes added by Justin Cohen
| Auto-Submit | +1 |
6 comments
static base::NoDestructor<IOSTracingController> instance;Justin CohenHow does this work?
We use a separate static NoDestructor in MaybeCreateInstanceForTesting() to ensure that tests have their own dedicated singleton instance of IOSTracingController. This ensures that the platform_ (which Perfetto requires to survive) is not destroyed between tests, avoiding crashes related to Perfetto's global state.
CreateInstance() calls Initialize() immediately because production code needs the controller to be active as soon as it is created in PreCreateThreads(). In contrast, MaybeCreateInstanceForTesting() does not call Initialize() because tests often need to set up specific conditions (like command-line switches in base::test::ScopedCommandLine) before tracing is initialized. Tests can then call InitializeForTesting() when they are ready.
This should be a CHECK rather than a DCHECK. See: https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/checks.md#invariant_verification-mechanisms
Done
Is this really needed? The test fixture already takes care of this in the TearDown
It's needed to pick up the ScopedCommandLine
IOSTracingController::GetInstance().InitializeForTesting();Should this be move to the fixture's setup method?
tl;dr yes and no. I moved it to setup, but we also need this to pick up the ScopedCommandLine, e.g.
```
// Reset the config to pick up the new command line switches.
tracing::TraceStartupConfig::ResetForTesting();
// Reset and re-initialize to restart startup tracing.
IOSTracingController::GetInstance().ResetForTesting();
IOSTracingController::GetInstance().InitializeForTesting();
```
IOSTracingController::GetInstance().InitializeForTesting();Justin Cohensame here.
Done
Can we remove this variable and just define line 33 as `component("cpp")` instead?
- Justin Novosad
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Novosad (Gerrit)
Justin Novosad voted![]( "Open in Gerrit")
- Justin Cohen
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[iOS tracing] Enable in-process Perfetto tracing for developer profiling
This CL brings up the native Perfetto tracing infrastructure on iOS by
introducing `IOSTracingController`, a lightweight tracing manager.
Because iOS cannot use the standard multi-process tracing stack, this
controller initializes Perfetto locally using `kInProcessBackend` and
delegates to `tracing::StartupTracingController` to handle
`--trace-startup` flags.
Specifically, this CL:
- Initializes the tracing controller in
`IOSChromeMainParts::PreCreateThreads()` so background threads can
be captured and named.
- Configures a standard developer trace config that supports native
flamegraphs (stack sampling), system metrics, and track events.
- Compiles `services/tracing/public/cpp` components for iOS (previously
excluded), guarding Blink-specific shared memory logic in
`trace_startup_config.cc` behind `USE_BLINK`.
- Removes incompatible content-layer dependencies from
`components/tracing` build targets on iOS.
- M components/tracing/BUILD.gn
- A ios/chrome/browser/tracing/BUILD.gn
- A ios/chrome/browser/tracing/DEPS
- A ios/chrome/browser/tracing/ios_tracing_controller.h
- A ios/chrome/browser/tracing/ios_tracing_controller.mm
- A ios/chrome/browser/tracing/ios_tracing_controller_unittest.mm
- M ios/chrome/browser/web/model/BUILD.gn
- M ios/chrome/browser/web/model/DEPS
- M ios/chrome/browser/web/model/chrome_main_parts.mm
- M ios/chrome/test/BUILD.gn
- M services/tracing/public/cpp/BUILD.gn
- M services/tracing/public/cpp/startup_tracing_controller.cc
- M services/tracing/public/cpp/startup_tracing_controller.h
- M services/tracing/public/cpp/trace_startup_config.cc
- M services/tracing/public/cpp/trace_startup_config.h
Code-Review: +1 by Justin Novosad, +1 by Etienne Pierre-Doray, +1 by Rohit Rao
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |