Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2683913002:
Enable startup tracing from command line flags in components/tracing (Closed)
|
Created:
3 years, 10 months ago by ssid Modified:
3 years, 10 months ago CC:
chromium-reviews, darin-cc_chromium.org, devtools-reviews_chromium.org, jam, pfeldman, tracing+reviews_chromium.org, vmpstr+watch_chromium.org, wfh+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable startup tracing from command line flags in components/tracing
This CL does:
1. Move the code that enables TraceLog early into
components/tracing/common. This code is executed in all processes and
duplicated in multiple locations.
2. Move TraceConfigFile into components/tracing/common since this code
is used in all processes except renderer and zygote.
3. Enable heap profiling in trace_startup.
BUG=609912
Review-Url: https://codereview.chromium.org/2683913002
Cr-Commit-Position: refs/heads/master@{#450956}
Committed: https://chromium.googlesource.com/chromium/src/+/69e8419f8d98ee886c1f7b34413ab97d6237cba9
Patch Set 1 : . #
Total comments: 2
Patch Set 2 : Rename. #
Total comments: 4
Patch Set 3 : Remove command line arg. #
Total comments: 2
Patch Set 4 : Comment about utility process #Patch Set 5 : fix comment. #Patch Set 6 : Rebase. #Messages
Total messages: 68 (41 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
ssid@chromium.org changed reviewers: + primiano@chromium.org, zhenw@chromium.org
ptal thanks.
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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
@Zhen ping.
https://codereview.chromium.org/2683913002/diff/20001/components/tracing/comm... File components/tracing/common/startup_command_parser.cc (right): https://codereview.chromium.org/2683913002/diff/20001/components/tracing/comm... components/tracing/common/startup_command_parser.cc:16: void ParseStartupTraceCommands(const base::CommandLine& command_line, This function name does not capture what it actually does. Maybe rename it to EnableStartupTracingIfNeeded or something similar? And the file name may be renamed accordingly.
https://codereview.chromium.org/2683913002/diff/20001/components/tracing/comm... File components/tracing/common/startup_command_parser.cc (right): https://codereview.chromium.org/2683913002/diff/20001/components/tracing/comm... components/tracing/common/startup_command_parser.cc:16: void ParseStartupTraceCommands(const base::CommandLine& command_line, On 2017/02/10 19:56:01, Zhen Wang wrote: > This function name does not capture what it actually does. Maybe rename it to > EnableStartupTracingIfNeeded or something similar? And the file name may be > renamed accordingly. Renamed as suggested. I'd be happy to rename the file if you have better suggestion. Thanks
ssid@chromium.org changed reviewers: + oysteine@chromium.org
+Oystein for tracing/ changes.
lgtm
nit: you need to update the CL description for the file name change.
Description was changed from ========== Parse startup tracing flags in components/tracing This CL does: 1. Move the code that enables TraceLog early into components/tracing/common. This code is executed in all processes and duplicated in multiple locations. 2. Move TraceConfigFile into components/tracing/common since this code is used in all processes except renderer and zygote. 3. Enable heap profiling in startup_trace_parser. BUG=609912 ========== to ========== Enable startup tracing from command line flags in components/tracing This CL does: 1. Move the code that enables TraceLog early into components/tracing/common. This code is executed in all processes and duplicated in multiple locations. 2. Move TraceConfigFile into components/tracing/common since this code is used in all processes except renderer and zygote. 3. Enable heap profiling in trace_startup. BUG=609912 ==========
@Oystein ping
https://codereview.chromium.org/2683913002/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2683913002/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:121: void EnableHeapProfilingIfNeeded(); Does this still need to be called from the MemoryDumpManager() constructor, in addition to from the startup tracing? If not, it'd be nice to simplify it a bit to just pass in the profiling_mode string.
On 2017/02/14 at 19:42:10, oystein wrote: > https://codereview.chromium.org/2683913002/diff/40001/base/trace_event/memory... > File base/trace_event/memory_dump_manager.h (right): > > https://codereview.chromium.org/2683913002/diff/40001/base/trace_event/memory... > base/trace_event/memory_dump_manager.h:121: void EnableHeapProfilingIfNeeded(); > Does this still need to be called from the MemoryDumpManager() constructor, in addition to from the startup tracing? If not, it'd be nice to simplify it a bit to just pass in the profiling_mode string. Also FYI https://codereview.chromium.org/2695013005/, should ping that CL if this one lands first.
> Also FYI https://codereview.chromium.org/2695013005/, should ping that CL if > this one lands first. Will do once we decide on the pattern here. https://codereview.chromium.org/2683913002/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2683913002/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:121: void EnableHeapProfilingIfNeeded(); On 2017/02/14 19:42:10, oystein wrote: > Does this still need to be called from the MemoryDumpManager() constructor, in > addition to from the startup tracing? If not, it'd be nice to simplify it a bit > to just pass in the profiling_mode string. Some processes may not enable startup tracing from content main runner. For example, Renderer processes in Android do not use enable tracing in library loader. So, for these cases we need to call this both from constructor and from Initialize in case the constructor does not have command line initialized. If the api becomes EnableHeapProfiling(std::string flag_args); Then all the call sites should read the flags :(
On 2017/02/14 at 19:58:54, ssid wrote: > > Also FYI https://codereview.chromium.org/2695013005/, should ping that CL if > > this one lands first. > > Will do once we decide on the pattern here. > > https://codereview.chromium.org/2683913002/diff/40001/base/trace_event/memory... > File base/trace_event/memory_dump_manager.h (right): > > https://codereview.chromium.org/2683913002/diff/40001/base/trace_event/memory... > base/trace_event/memory_dump_manager.h:121: void EnableHeapProfilingIfNeeded(); > On 2017/02/14 19:42:10, oystein wrote: > > Does this still need to be called from the MemoryDumpManager() constructor, in > > addition to from the startup tracing? If not, it'd be nice to simplify it a bit > > to just pass in the profiling_mode string. > > Some processes may not enable startup tracing from content main runner. For example, Renderer processes in Android do not use enable tracing in library loader. So, for these cases we need to call this both from constructor and from Initialize in case the constructor does not have command line initialized. > > If the api becomes > EnableHeapProfiling(std::string flag_args); > Then all the call sites should read the flags :( Okay, yep that makes sense to me. lgtm but it would be awesome if we could have a browsertest for this switch, I can't see any right now?
On 2017/02/14 22:00:46, oystein wrote: > On 2017/02/14 at 19:58:54, ssid wrote: > > > Also FYI https://codereview.chromium.org/2695013005/, should ping that CL if > > > this one lands first. > > > > Will do once we decide on the pattern here. > > > > > https://codereview.chromium.org/2683913002/diff/40001/base/trace_event/memory... > > File base/trace_event/memory_dump_manager.h (right): > > > > > https://codereview.chromium.org/2683913002/diff/40001/base/trace_event/memory... > > base/trace_event/memory_dump_manager.h:121: void > EnableHeapProfilingIfNeeded(); > > On 2017/02/14 19:42:10, oystein wrote: > > > Does this still need to be called from the MemoryDumpManager() constructor, > in > > > addition to from the startup tracing? If not, it'd be nice to simplify it a > bit > > > to just pass in the profiling_mode string. > > > > Some processes may not enable startup tracing from content main runner. For > example, Renderer processes in Android do not use enable tracing in library > loader. So, for these cases we need to call this both from constructor and from > Initialize in case the constructor does not have command line initialized. > > > > If the api becomes > > EnableHeapProfiling(std::string flag_args); > > Then all the call sites should read the flags :( > > Okay, yep that makes sense to me. > > lgtm but it would be awesome if we could have a browsertest for this switch, I > can't see any right now? Kraynov is working on the browser test. https://bugs.chromium.org/p/chromium/issues/detail?id=670828
ssid@chromium.org changed reviewers: + avi@chromium.org
+avi: Ptal at the changes in content/. Thanks
LGTM with a comment. Agree with oysteine that having a browsertest here, if there is none, would be nice. https://codereview.chromium.org/2683913002/diff/40001/components/tracing/comm... File components/tracing/common/trace_startup.h (right): https://codereview.chromium.org/2683913002/diff/40001/components/tracing/comm... components/tracing/common/trace_startup.h:17: EnableStartupTracingIfNeeded(const base::CommandLine& command_line, There doesn't seem to be an actual need of passing the command_line. It seems you can just do CommandLine::ForCurrentProcess in the .cc file. there doesn't seem to be any precedent for passing the cmdline around.
ssid@chromium.org changed reviewers: + nasko@chromium.org
+nasco since Avi is OOO. Ptal at content/ changes. Only functional change is called EnableHeapProfiling in trace_startup. Other changes are moving around code.
https://codereview.chromium.org/2683913002/diff/40001/components/tracing/comm... File components/tracing/common/trace_startup.h (right): https://codereview.chromium.org/2683913002/diff/40001/components/tracing/comm... components/tracing/common/trace_startup.h:17: EnableStartupTracingIfNeeded(const base::CommandLine& command_line, On 2017/02/15 15:33:34, Primiano Tucci wrote: > There doesn't seem to be an actual need of passing the command_line. It seems > you can just do CommandLine::ForCurrentProcess in the .cc file. there doesn't > seem to be any precedent for passing the cmdline around. Done.
content/ LGTM with a question. https://codereview.chromium.org/2683913002/diff/80001/content/app/content_mai... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/2683913002/diff/80001/content/app/content_mai... content/app/content_main_runner.cc:612: // cannot access the file system. Is that the real reason? Utility processes also don't have filesystem access.
Thanks. Zhen, do you have any idea about Utility process? https://codereview.chromium.org/2683913002/diff/80001/content/app/content_mai... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/2683913002/diff/80001/content/app/content_mai... content/app/content_main_runner.cc:612: // cannot access the file system. On 2017/02/15 19:18:58, nasko wrote: > Is that the real reason? Utility processes also don't have filesystem access. Um Then I would guess startup tracing does not work well in Utility process. This was the reason given for adding this condition in the CL here: https://codereview.chromium.org/1212893009#msg6 I do not want to make functional changes for this case in this CL since I haven't tested. I am planning to add a browsertest for startup tracing this week. I will fix this case along with the test.
ssid@chromium.org changed reviewers: - avi@chromium.org
-Avi
The CQ bit was checked by ssid@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...
On 2017/02/15 19:41:34, ssid wrote: > Thanks. Zhen, do you have any idea about Utility process? > > https://codereview.chromium.org/2683913002/diff/80001/content/app/content_mai... > File content/app/content_main_runner.cc (right): > > https://codereview.chromium.org/2683913002/diff/80001/content/app/content_mai... > content/app/content_main_runner.cc:612: // cannot access the file system. > On 2017/02/15 19:18:58, nasko wrote: > > Is that the real reason? Utility processes also don't have filesystem access. > > Um Then I would guess startup tracing does not work well in Utility process. > This was the reason given for adding this condition in the CL here: > https://codereview.chromium.org/1212893009#msg6 > > I do not want to make functional changes for this case in this CL since I > haven't tested. I am planning to add a browsertest for startup tracing this > week. I will fix this case along with the test. I agree not making functional changes in this CL, but adding a comment that isn't entirely accurate might not be helpful either.
On 2017/02/15 19:51:25, nasko wrote: > On 2017/02/15 19:41:34, ssid wrote: > > Thanks. Zhen, do you have any idea about Utility process? > > > > > https://codereview.chromium.org/2683913002/diff/80001/content/app/content_mai... > > File content/app/content_main_runner.cc (right): > > > > > https://codereview.chromium.org/2683913002/diff/80001/content/app/content_mai... > > content/app/content_main_runner.cc:612: // cannot access the file system. > > On 2017/02/15 19:18:58, nasko wrote: > > > Is that the real reason? Utility processes also don't have filesystem > access. > > > > Um Then I would guess startup tracing does not work well in Utility process. > > This was the reason given for adding this condition in the CL here: > > https://codereview.chromium.org/1212893009#msg6 > > > > I do not want to make functional changes for this case in this CL since I > > haven't tested. I am planning to add a browsertest for startup tracing this > > week. I will fix this case along with the test. > > I agree not making functional changes in this CL, but adding a comment that > isn't entirely accurate might not be helpful either. Sure. I have added a TODO after the comment. Does that sound better?
The CQ bit was checked by ssid@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...
On 2017/02/15 20:04:55, ssid wrote: > On 2017/02/15 19:51:25, nasko wrote: > > On 2017/02/15 19:41:34, ssid wrote: > > > Thanks. Zhen, do you have any idea about Utility process? > > > > > > > > > https://codereview.chromium.org/2683913002/diff/80001/content/app/content_mai... > > > File content/app/content_main_runner.cc (right): > > > > > > > > > https://codereview.chromium.org/2683913002/diff/80001/content/app/content_mai... > > > content/app/content_main_runner.cc:612: // cannot access the file system. > > > On 2017/02/15 19:18:58, nasko wrote: > > > > Is that the real reason? Utility processes also don't have filesystem > > access. > > > > > > Um Then I would guess startup tracing does not work well in Utility process. > > > This was the reason given for adding this condition in the CL here: > > > https://codereview.chromium.org/1212893009#msg6 > > > > > > I do not want to make functional changes for this case in this CL since I > > > haven't tested. I am planning to add a browsertest for startup tracing this > > > week. I will fix this case along with the test. > > > > I agree not making functional changes in this CL, but adding a comment that > > isn't entirely accurate might not be helpful either. > > Sure. I have added a TODO after the comment. Does that sound better? Yeah, thanks! We should do a better audit to include all types of processes, utility was just an example.
Patchset #5 (id:100001) has been deleted
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by ssid@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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by ssid@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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, oysteine@chromium.org, zhenw@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2683913002/#ps140001 (title: "fix comment.")
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
Failed to apply patch for content/app/content_main_runner.cc:
While running git apply --index -p1;
error: patch failed: content/app/content_main_runner.cc:41
error: content/app/content_main_runner.cc: patch does not apply
Patch: content/app/content_main_runner.cc
Index: content/app/content_main_runner.cc
diff --git a/content/app/content_main_runner.cc
b/content/app/content_main_runner.cc
index
ee6ab17a0dc24ac400302dd074a0b2cdae168332..f796361651fb6cb14c4026f82e02e78b7c87bb4d
100644
--- a/content/app/content_main_runner.cc
+++ b/content/app/content_main_runner.cc
@@ -41,9 +41,7 @@
#include "base/strings/stringprintf.h"
#include "base/trace_event/trace_event.h"
#include "build/build_config.h"
-#include "components/tracing/browser/trace_config_file.h"
-#include "components/tracing/common/trace_to_console.h"
-#include "components/tracing/common/tracing_switches.h"
+#include "components/tracing/common/trace_startup.h"
#include "content/app/mojo/mojo_init.h"
#include "content/browser/browser_main.h"
#include "content/browser/renderer_host/render_process_host_impl.h"
@@ -610,31 +608,12 @@ class ContentMainRunnerImpl : public ContentMainRunner {
// Enable startup tracing asap to avoid early TRACE_EVENT calls being
// ignored. For Android, startup tracing is enabled in an even earlier
place
// content/app/android/library_loader_hooks.cc.
- if (command_line.HasSwitch(switches::kTraceStartup)) {
- base::trace_event::TraceConfig trace_config(
- command_line.GetSwitchValueASCII(switches::kTraceStartup),
- base::trace_event::RECORD_UNTIL_FULL);
- base::trace_event::TraceLog::GetInstance()->SetEnabled(
- trace_config,
- base::trace_event::TraceLog::RECORDING_MODE);
- } else if (command_line.HasSwitch(switches::kTraceToConsole)) {
- base::trace_event::TraceConfig trace_config =
- tracing::GetConfigForTraceToConsole();
- LOG(ERROR) << "Start " << switches::kTraceToConsole
- << " with CategoryFilter '"
- << trace_config.ToCategoryFilterString() << "'.";
- base::trace_event::TraceLog::GetInstance()->SetEnabled(
- trace_config,
- base::trace_event::TraceLog::RECORDING_MODE);
- } else if (process_type != switches::kZygoteProcess &&
- process_type != switches::kRendererProcess) {
- if (tracing::TraceConfigFile::GetInstance()->IsEnabled()) {
- // This checks kTraceConfigFile switch.
- base::trace_event::TraceLog::GetInstance()->SetEnabled(
- tracing::TraceConfigFile::GetInstance()->GetTraceConfig(),
- base::trace_event::TraceLog::RECORDING_MODE);
- }
- }
+ // Zygote process does not have file thread and renderer process on Win10
+ // cannot access the file system.
+ // TODO(ssid): Check if other processes can enable startup tracing here.
+ bool can_access_file_system = (process_type != switches::kZygoteProcess &&
+ process_type != switches::kRendererProcess);
+ tracing::EnableStartupTracingIfNeeded(can_access_file_system);
#endif // !OS_ANDROID
#if defined(OS_WIN)
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
The CQ bit was unchecked by ssid@chromium.org
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
Patchset #6 (id:200001) has been deleted
The CQ bit was checked by ssid@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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, oysteine@chromium.org, zhenw@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2683913002/#ps220001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1487255283442260,
"parent_rev": "5e263dacaf3676b151b75f1fc481c88796081dc0", "commit_rev":
"69e8419f8d98ee886c1f7b34413ab97d6237cba9"}
Message was sent while issue was closed.
Description was changed from ========== Enable startup tracing from command line flags in components/tracing This CL does: 1. Move the code that enables TraceLog early into components/tracing/common. This code is executed in all processes and duplicated in multiple locations. 2. Move TraceConfigFile into components/tracing/common since this code is used in all processes except renderer and zygote. 3. Enable heap profiling in trace_startup. BUG=609912 ========== to ========== Enable startup tracing from command line flags in components/tracing This CL does: 1. Move the code that enables TraceLog early into components/tracing/common. This code is executed in all processes and duplicated in multiple locations. 2. Move TraceConfigFile into components/tracing/common since this code is used in all processes except renderer and zygote. 3. Enable heap profiling in trace_startup. BUG=609912 Review-Url: https://codereview.chromium.org/2683913002 Cr-Commit-Position: refs/heads/master@{#450956} Committed: https://chromium.googlesource.com/chromium/src/+/69e8419f8d98ee886c1f7b34413a... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:220001) as https://chromium.googlesource.com/chromium/src/+/69e8419f8d98ee886c1f7b34413a... |
