[Startup Tracing] Add --trace-config-file flag

[Startup Tracing] Add --trace-config-file flag

This CL adds --trace-config-file flag. Please see trace_config_file.h for details.

Design doc:
https://docs.google.com/document/d/1PgdXUOJF3WtEmYWUyGRbC2Fz2ICCZKO9jPvpLPRSHH8/edit?usp=sharing

BUG=317481, 482098
Committed: https://crrev.com/de52fb95509666be1062b50a78bea36adfbac183
Cr-Commit-Position: refs/heads/master@{#346062}

[Zhen Wang, 2015-08-24 04:53:07]

zhenw@chromium.org changed reviewers:
+ dsinclair@chromium.org

[Zhen Wang, 2015-08-24 04:53:08]

ptal

[dsinclair, 2015-08-24 15:04:50]

lgtm w/ nits.

https://codereview.chromium.org/1315463002/diff/1/components/tracing/trace_co...
File components/tracing/trace_config_file.cc (right):

https://codereview.chromium.org/1315463002/diff/1/components/tracing/trace_co...
components/tracing/trace_config_file.cc:18: 
nit: remove blank line

https://codereview.chromium.org/1315463002/diff/1/components/tracing/trace_co...
components/tracing/trace_config_file.cc:74: } else {
Don't need the else because the if() returns.

https://codereview.chromium.org/1315463002/diff/1/components/tracing/trace_co...
components/tracing/trace_config_file.cc:77: }
nit: don't need {}'s here.

https://codereview.chromium.org/1315463002/diff/1/components/tracing/trace_co...
components/tracing/trace_config_file.cc:95: }
nit: {}'s

https://codereview.chromium.org/1315463002/diff/1/components/tracing/trace_co...
components/tracing/trace_config_file.cc:100: if
(dict->GetDictionary(kTraceConfigParam, &trace_config_dict)) {
if (!dict->...)
  return false;

https://codereview.chromium.org/1315463002/diff/1/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1315463002/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:394: // when kTraceStartup or
KTraceConfigFile is in the command line.
nit: kTraceConfigFile?

[Zhen Wang, 2015-08-24 16:39:24]

zhenw@chromium.org changed reviewers:
+ blundell@chromium.org, sievers@chromium.org, sky@chromium.org

[Zhen Wang, 2015-08-24 16:39:25]

+blundell for components/BUILD.gn and components/components_tests.gyp
+sievers for content/
+sky for mandoline/ and mojo/

cc primiano for memory infra startup/shutdown tracing

https://codereview.chromium.org/1315463002/diff/1/components/tracing/trace_co...
File components/tracing/trace_config_file.cc (right):

https://codereview.chromium.org/1315463002/diff/1/components/tracing/trace_co...
components/tracing/trace_config_file.cc:18: 
On 2015/08/24 15:04:49, dsinclair wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/1315463002/diff/1/components/tracing/trace_co...
components/tracing/trace_config_file.cc:74: } else {
On 2015/08/24 15:04:49, dsinclair wrote:
> Don't need the else because the if() returns.

Done.

https://codereview.chromium.org/1315463002/diff/1/components/tracing/trace_co...
components/tracing/trace_config_file.cc:77: }
On 2015/08/24 15:04:50, dsinclair wrote:
> nit: don't need {}'s here.

Done.

https://codereview.chromium.org/1315463002/diff/1/components/tracing/trace_co...
components/tracing/trace_config_file.cc:95: }
On 2015/08/24 15:04:49, dsinclair wrote:
> nit: {}'s

Done.

https://codereview.chromium.org/1315463002/diff/1/components/tracing/trace_co...
components/tracing/trace_config_file.cc:100: if
(dict->GetDictionary(kTraceConfigParam, &trace_config_dict)) {
On 2015/08/24 15:04:50, dsinclair wrote:
> if (!dict->...)
>   return false;

Done.

https://codereview.chromium.org/1315463002/diff/1/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1315463002/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:394: // when kTraceStartup or
KTraceConfigFile is in the command line.
On 2015/08/24 15:04:50, dsinclair wrote:
> nit: kTraceConfigFile?

Done.

[Primiano Tucci (use gerrit), 2015-08-24 16:49:58]

On 2015/08/24 16:39:25, Zhen Wang wrote:
> cc primiano for memory infra startup/shutdown tracing


Thanks for the CC. Don't worry about memory-infra right now. Turns out that is
already *gracefully* not working for startup tracing. Details in
crbug.com/524057
We'll look into that after this has landed.

[no sievers, 2015-08-24 22:50:16]

https://codereview.chromium.org/1315463002/diff/20001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/content/app/content_mai...
content/app/content_main_runner.cc:635:
base::trace_event::TraceLog::RECORDING_MODE);
You're not really changing it, but do you know why we enable it twice on Android
(onLibraryLoaded and here)?

https://codereview.chromium.org/1315463002/diff/20001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/content/browser/browser...
content/browser/browser_main_loop.cc:393: // ContentMainRunner should have
enabled tracing of the browser process
If so, can we simply do:
DCHECK_IMPLIES(parameters.command_line.HasSwitch(switches::kTraceStartup),
tracing::TraceLog::IsEnabled());

or should it be TracingController::GetInstance()->IsRecording()?

https://codereview.chromium.org/1315463002/diff/20001/content/browser/browser...
content/browser/browser_main_loop.cc:398:
tracing::TraceConfigFile::GetInstance()->GetStartupDuration() > 0)) {
Are there other ways than the two command lines (config or kTraceStartup) that
would lead to tracing being enabled here? If not can we just have a single check
whether it's enabled?

https://codereview.chromium.org/1315463002/diff/20001/content/browser/browser...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/content/browser/browser...
content/browser/browser_main_runner.cc:248: base::FilePath result_file;
Just wondering: Is this a fix unrelated to the cl description?
Does it make sure trace files are written out when quitting, and when we are
*not* using startup tracing (incl. trace config based)?
Mind putting a comment?

https://codereview.chromium.org/1315463002/diff/20001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:116: switches::kTraceConfigFile,
You are passing this to all child processes, but where do you actually pass the
config to the tracing controller in that process?

[blundell, 2015-08-25 08:23:50]

Is this component really supposed to be built for iOS, which can't use //content
or //ipc?

[sky, 2015-08-25 16:11:56]

sky@chromium.org changed reviewers:
+ msw@chromium.org

[sky, 2015-08-25 16:11:56]

sky->msw

[Zhen Wang, 2015-08-25 17:38:14]

On 2015/08/25 08:23:50, blundell wrote:
> Is this component really supposed to be built for iOS, which can't use
//content
> or //ipc?

dsinclair@, tracing is supported on iOS in general, right? How about
--trace-startup? Does it support iOS before?

https://codereview.chromium.org/1315463002/diff/20001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/content/app/content_mai...
content/app/content_main_runner.cc:635:
base::trace_event::TraceLog::RECORDING_MODE);
On 2015/08/24 22:50:16, sievers wrote:
> You're not really changing it, but do you know why we enable it twice on
Android
> (onLibraryLoaded and here)?

I thought this is only executed for desktop Chrome, right?

https://codereview.chromium.org/1315463002/diff/20001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/content/browser/browser...
content/browser/browser_main_loop.cc:393: // ContentMainRunner should have
enabled tracing of the browser process
On 2015/08/24 22:50:16, sievers wrote:
> If so, can we simply do:
> DCHECK_IMPLIES(parameters.command_line.HasSwitch(switches::kTraceStartup),
> tracing::TraceLog::IsEnabled());
> 
> or should it be TracingController::GetInstance()->IsRecording()?

I am not aware of any other ways lead to tracing being enabled here. But I think
it is safe to check the flags, because we other ways may be added in the future.
And |_is_tracing_startup_for_duration_| is specific to those two flags, but not
to tracing controller.

https://codereview.chromium.org/1315463002/diff/20001/content/browser/browser...
content/browser/browser_main_loop.cc:398:
tracing::TraceConfigFile::GetInstance()->GetStartupDuration() > 0)) {
On 2015/08/24 22:50:16, sievers wrote:
> Are there other ways than the two command lines (config or kTraceStartup) that
> would lead to tracing being enabled here? If not can we just have a single
check
> whether it's enabled?

ditto

https://codereview.chromium.org/1315463002/diff/20001/content/browser/browser...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/content/browser/browser...
content/browser/browser_main_runner.cc:248: base::FilePath result_file;
On 2015/08/24 22:50:16, sievers wrote:
> Just wondering: Is this a fix unrelated to the cl description?
> Does it make sure trace files are written out when quitting, and when we are
> *not* using startup tracing (incl. trace config based)?
> Mind putting a comment?

This is required for --trace-config-flag when duration is not specified.

Comments added above.

https://codereview.chromium.org/1315463002/diff/20001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:116: switches::kTraceConfigFile,
On 2015/08/24 22:50:16, sievers wrote:
> You are passing this to all child processes, but where do you actually pass
the
> config to the tracing controller in that process?

When TraceConfigFile is initialized in the child process, it will also read the
specified config file from the file system and create the configuration based on
that.

[dsinclair, 2015-08-25 17:43:08]

On 2015/08/25 17:38:14, Zhen Wang wrote:
> On 2015/08/25 08:23:50, blundell wrote:
> > Is this component really supposed to be built for iOS, which can't use
> //content
> > or //ipc?
> 
> dsinclair@, tracing is supported on iOS in general, right? How about
> --trace-startup? Does it support iOS before?
> 

I don't know. I haven't heard of anyone using tracing on iOS but, I guess if you
can access the devtools protocol then you can enable tracing.

[Zhen Wang, 2015-08-25 17:46:11]

On 2015/08/25 17:43:08, dsinclair wrote:
> On 2015/08/25 17:38:14, Zhen Wang wrote:
> > On 2015/08/25 08:23:50, blundell wrote:
> > > Is this component really supposed to be built for iOS, which can't use
> > //content
> > > or //ipc?
> > 
> > dsinclair@, tracing is supported on iOS in general, right? How about
> > --trace-startup? Does it support iOS before?
> > 
> 
> I don't know. I haven't heard of anyone using tracing on iOS but, I guess if
you
> can access the devtools protocol then you can enable tracing.

I guess it is safe to exclude startup tracing on iOS for now? The old
--trace-startup implementation are in //content anyway, which iOS does not use.
And Telemetry does not support iOS, either.

[msw, 2015-08-25 18:06:49]

https://codereview.chromium.org/1315463002/diff/20001/components/tracing/trac...
File components/tracing/tracing_switches.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/components/tracing/trac...
components/tracing/tracing_switches.cc:11: const char kTraceConfigFile[]       
= "trace-config-file";
nit: match the spacing of other lines.

https://codereview.chromium.org/1315463002/diff/20001/content/app/android/lib...
File content/app/android/library_loader_hooks.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/content/app/android/lib...
content/app/android/library_loader_hooks.cc:97: if
(tracing::TraceConfigFile::GetInstance()->IsEnabled()) {
Maybe keep a helper like tracing::EnableStartupTracingIfConfigFileExists()?

https://codereview.chromium.org/1315463002/diff/20001/mandoline/app/desktop/l...
File mandoline/app/desktop/launcher_process.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/mandoline/app/desktop/l...
mandoline/app/desktop/launcher_process.cc:94: g_tracing = true;
Shouldn't this only be set true if the next condition is true? ie:
"else if (...TraceConfigFile...IsEnabled()) { g_tracing = true; ..."

https://codereview.chromium.org/1315463002/diff/20001/mojo/runner/desktop/lau...
File mojo/runner/desktop/launcher_process.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/mojo/runner/desktop/lau...
mojo/runner/desktop/launcher_process.cc:93: g_tracing = true;
Ditto from mandoline.

[Zhen Wang, 2015-08-25 18:18:37]

https://codereview.chromium.org/1315463002/diff/20001/components/tracing/trac...
File components/tracing/tracing_switches.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/components/tracing/trac...
components/tracing/tracing_switches.cc:11: const char kTraceConfigFile[]       
= "trace-config-file";
On 2015/08/25 18:06:49, msw wrote:
> nit: match the spacing of other lines.

Done.

https://codereview.chromium.org/1315463002/diff/20001/content/app/android/lib...
File content/app/android/library_loader_hooks.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/content/app/android/lib...
content/app/android/library_loader_hooks.cc:97: if
(tracing::TraceConfigFile::GetInstance()->IsEnabled()) {
On 2015/08/25 18:06:49, msw wrote:
> Maybe keep a helper like tracing::EnableStartupTracingIfConfigFileExists()?

TraceConfigFile is now more than just startup tracing. It also supports shutdown
tracing or stopping tracing via devtools (without startup duration). I think
using this format does not give the impression that this is only tied to startup
tracing.

https://codereview.chromium.org/1315463002/diff/20001/mandoline/app/desktop/l...
File mandoline/app/desktop/launcher_process.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/mandoline/app/desktop/l...
mandoline/app/desktop/launcher_process.cc:94: g_tracing = true;
On 2015/08/25 18:06:49, msw wrote:
> Shouldn't this only be set true if the next condition is true? ie:
> "else if (...TraceConfigFile...IsEnabled()) { g_tracing = true; ..."

Oh, right. Fixed. Thanks!

https://codereview.chromium.org/1315463002/diff/20001/mojo/runner/desktop/lau...
File mojo/runner/desktop/launcher_process.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/mojo/runner/desktop/lau...
mojo/runner/desktop/launcher_process.cc:93: g_tracing = true;
On 2015/08/25 18:06:49, msw wrote:
> Ditto from mandoline.

fixed

[msw, 2015-08-25 18:22:47]

mojo/* and mandoline/* lgtm with a nit.

https://codereview.chromium.org/1315463002/diff/20001/content/app/android/lib...
File content/app/android/library_loader_hooks.cc (right):

https://codereview.chromium.org/1315463002/diff/20001/content/app/android/lib...
content/app/android/library_loader_hooks.cc:97: if
(tracing::TraceConfigFile::GetInstance()->IsEnabled()) {
On 2015/08/25 18:18:37, Zhen Wang wrote:
> On 2015/08/25 18:06:49, msw wrote:
> > Maybe keep a helper like tracing::EnableStartupTracingIfConfigFileExists()?
> 
> TraceConfigFile is now more than just startup tracing. It also supports
shutdown
> tracing or stopping tracing via devtools (without startup duration). I think
> using this format does not give the impression that this is only tied to
startup
> tracing.

Fair enough.

https://codereview.chromium.org/1315463002/diff/60001/mandoline/app/desktop/l...
File mandoline/app/desktop/launcher_process.cc (right):

https://codereview.chromium.org/1315463002/diff/60001/mandoline/app/desktop/l...
mandoline/app/desktop/launcher_process.cc:94: if
(tracing::TraceConfigFile::GetInstance()->IsEnabled()) {
nit: combine with the line above for an "else if". (ditto in mojo)

[Zhen Wang, 2015-08-25 18:54:33]

blundell@, the startup tracing code (tracing:startup_tracing) in this CL only
depends on //base. The changes in content/ support desktop and android use
cases. If one wants to support iOS, he/she can adds the support accordingly.

I think whether tracing:tracing (depends on //ipc) should be built for iOS is a
separate issue.

https://codereview.chromium.org/1315463002/diff/60001/mandoline/app/desktop/l...
File mandoline/app/desktop/launcher_process.cc (right):

https://codereview.chromium.org/1315463002/diff/60001/mandoline/app/desktop/l...
mandoline/app/desktop/launcher_process.cc:94: if
(tracing::TraceConfigFile::GetInstance()->IsEnabled()) {
On 2015/08/25 18:22:47, msw wrote:
> nit: combine with the line above for an "else if". (ditto in mojo)

Done.

[blundell, 2015-08-26 16:58:14]

On 2015/08/25 18:54:33, Zhen Wang wrote:
> blundell@, the startup tracing code (tracing:startup_tracing) in this CL only
> depends on //base. The changes in content/ support desktop and android use
> cases. If one wants to support iOS, he/she can adds the support accordingly.
> 
> I think whether tracing:tracing (depends on //ipc) should be built for iOS is
a
> separate issue.

The code I'm reviewing in this CL is adding a dependency on
//components/tracing:tracing on iOS.

> 
>
https://codereview.chromium.org/1315463002/diff/60001/mandoline/app/desktop/l...
> File mandoline/app/desktop/launcher_process.cc (right):
> 
>
https://codereview.chromium.org/1315463002/diff/60001/mandoline/app/desktop/l...
> mandoline/app/desktop/launcher_process.cc:94: if
> (tracing::TraceConfigFile::GetInstance()->IsEnabled()) {
> On 2015/08/25 18:22:47, msw wrote:
> > nit: combine with the line above for an "else if". (ditto in mojo)
> 
> Done.

[Zhen Wang, 2015-08-26 18:28:52]

On 2015/08/26 16:58:14, blundell wrote:
> On 2015/08/25 18:54:33, Zhen Wang wrote:
> > blundell@, the startup tracing code (tracing:startup_tracing) in this CL
only
> > depends on //base. The changes in content/ support desktop and android use
> > cases. If one wants to support iOS, he/she can adds the support accordingly.
> > 
> > I think whether tracing:tracing (depends on //ipc) should be built for iOS
is
> a
> > separate issue.
> 
> The code I'm reviewing in this CL is adding a dependency on
> //components/tracing:tracing on iOS.

Oh, you are right. My bad. Fixed now.

[blundell, 2015-08-26 19:38:00]

//components top-level lgtm

[Zhen Wang, 2015-08-27 16:48:28]

ping sievers. :-)

[no sievers, 2015-08-27 20:34:18]

lgtm

https://codereview.chromium.org/1315463002/diff/100001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1315463002/diff/100001/content/browser/browse...
content/browser/browser_main_loop.cc:1342:
TracingControllerAndroid::GenerateTracingFilePath(&trace_file);
Hmm so the filename from the trace config has no effect on Android?

[Zhen Wang, 2015-08-27 20:53:12]

sky@, do you mind stamping this CL for mojo/ since msw@ has lg2m it?

https://codereview.chromium.org/1315463002/diff/100001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1315463002/diff/100001/content/browser/browse...
content/browser/browser_main_loop.cc:1342:
TracingControllerAndroid::GenerateTracingFilePath(&trace_file);
On 2015/08/27 20:34:18, sievers wrote:
> Hmm so the filename from the trace config has no effect on Android?

That is correct. TraceConfigFile.GetResultFile() is only declared on non-Android
platforms.

[sky, 2015-08-27 23:17:41]

LGTM

[Zhen Wang, 2015-08-28 00:40:54]

The CQ bit was checked by zhenw@chromium.org

[Zhen Wang, 2015-08-28 00:40:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1315463002/#ps100001
(title: "review fix - blundell")

[commit-bot: I haz the power, 2015-08-28 00:41:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1315463002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1315463002/100001

[commit-bot: I haz the power, 2015-08-28 03:07:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-08-28 03:08:08]

Patchset 6 (id:??) landed as
https://crrev.com/de52fb95509666be1062b50a78bea36adfbac183
Cr-Commit-Position: refs/heads/master@{#346062}

[sergeyv, 2015-08-28 03:32:45]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1319093002/ by sergeyv@chromium.org.

The reason for reverting is: Reason for the revert: broken compilation at
Win8/Win x64.

Failure example:
http://build.chromium.org/p/chromium.win/buildstatus?builder=Win%20x64%20GN&n....