inspector: split --cpu-prof-path to --cpu-prof-dir and --cpu-prof-name

To improve the integration of `--cpu-prof` with workers, this patch splits `--cpu-prof-path` into `--cpu-prof-dir` and `--cpu-prof-name`, so when a worker is launched from a thread that enables `--cpu-prof`, if the parent thread sets `--cpu-prof-dir`, then the profile of both thread would be generated to the specified directory. If they end up specifying the same `--cpu-prof-name` the behavior is undefined the last profile will overwritten the first one. PR-URL: #27306 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
nodejs · Apr 22, 2019 · 49d3d11 · 49d3d11
1 parent a3d1922
commit 49d3d11
Show file tree

Hide file tree

Showing 12 changed files with 231 additions and 54 deletions.
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -83,28 +83,38 @@ added: REPLACEME
 > Stability: 1 - Experimental
 
 Starts the V8 CPU profiler on start up, and writes the CPU profile to disk
-before exit. If `--cpu-prof-path` is not specified, the profile will be
-written to `${cwd}/CPU.${yyyymmdd}.${hhmmss}.${pid}.${tid}.${seq}.cpuprofile`.
+before exit.
+
+If `--cpu-prof-dir` is not specified, the generated profile will be placed
+in the current working directory.
+
+If `--cpu-prof-name` is not specified, the generated profile will be
+named `CPU.${yyyymmdd}.${hhmmss}.${pid}.${tid}.${seq}.cpuprofile`.
 
 ```console
 $ node --cpu-prof index.js
 $ ls *.cpuprofile
 CPU.20190409.202950.15293.0.0.cpuprofile
 ```
 
-### `--cpu-prof-path`
+### `--cpu-prof-dir`
 <!-- YAML
 added: REPLACEME
 -->
 
 > Stability: 1 - Experimental
 
-Location where the CPU profile generated by `--cpu-prof`
-should be written to. When used alone, it implies `--cpu-prof`.
+Specify the directory where the CPU profiles generated by `--cpu-prof` will
+be placed.
 
-```console
-$ node --cpu-prof-path /tmp/test.cpuprofile index.js
-```
+### `--cpu-prof-name`
+<!-- YAML
+added: REPLACEME
+-->
+
+> Stability: 1 - Experimental
+
+Specify the file name of the CPU profile generated by `--cpu-prof`.
 
 ### `--enable-fips`
 <!-- YAML

diff --git a/doc/node.1 b/doc/node.1
@@ -81,13 +81,17 @@ Print source-able bash completion script for Node.js.
 .It Fl -cpu-prof
 Start the V8 CPU profiler on start up, and write the CPU profile to disk
 before exit. If
-.Fl -cpu-prof-path
-is not specified, the profile will be written to the current working directory.
+.Fl -cpu-prof-dir
+is not specified, the profile will be written to the current working directory
+with a generated file name.
 .
-.It Fl -cpu-prof-path
-Path the V8 CPU profile generated with
+.It Fl -cpu-prof-dir
+The directory where the CPU profiles generated by
 .Fl -cpu-prof
-will be written to. When used alone, it implies
+will be placed.
+.
+.It Fl -cpu-prof-name
+File name of the V8 CPU profile generated with
 .Fl -cpu-prof
 .
 .It Fl -enable-fips

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -38,6 +38,14 @@
 
 #include <utility>
 
+#ifdef _WIN32
+/* MAX_PATH is in characters, not bytes. Make sure we have enough headroom. */
+#define CWD_BUFSIZE (MAX_PATH * 4)
+#else
+#include <climits>  // PATH_MAX on Solaris.
+#define CWD_BUFSIZE (PATH_MAX)
+#endif
+
 namespace node {
 
 inline v8::Isolate* IsolateData::isolate() const {
@@ -678,6 +686,15 @@ inline void Environment::set_cpu_profile_path(const std::string& path) {
 inline const std::string& Environment::cpu_profile_path() const {
   return cpu_profile_path_;
 }
+
+inline void Environment::set_cpu_prof_dir(const std::string& path) {
+  cpu_prof_dir_ = path;
+}
+
+inline const std::string& Environment::cpu_prof_dir() const {
+  return cpu_prof_dir_;
+}
+
 #endif  // HAVE_INSPECTOR
 
 inline std::shared_ptr<HostPort> Environment::inspector_host_port() {

diff --git a/src/env.cc b/src/env.cc
@@ -845,6 +845,24 @@ void Environment::stop_sub_worker_contexts() {
   }
 }
 
+#if HAVE_INSPECTOR
+
+void Environment::InitializeCPUProfDir(const std::string& dir) {
+  if (!dir.empty()) {
+    cpu_prof_dir_ = dir;
+    return;
+  }
+  char cwd[CWD_BUFSIZE];
+  size_t size = CWD_BUFSIZE;
+  int err = uv_cwd(cwd, &size);
+  // TODO(joyeecheung): fallback to exec path / argv[0]
+  CHECK_EQ(err, 0);
+  CHECK_GT(size, 0);
+  cpu_prof_dir_ = cwd;
+}
+
+#endif  // HAVE_INSPECTOR
+
 void MemoryTracker::TrackField(const char* edge_name,
                                const CleanupHookCallback& value,
                                const char* node_name) {

diff --git a/src/env.h b/src/env.h
@@ -1137,6 +1137,11 @@ class Environment : public MemoryRetainer {
 
   inline void set_cpu_profile_path(const std::string& path);
   inline const std::string& cpu_profile_path() const;
+
+  inline void set_cpu_prof_dir(const std::string& path);
+  inline const std::string& cpu_prof_dir() const;
+
+  void InitializeCPUProfDir(const std::string& dir);
 #endif  // HAVE_INSPECTOR
 
  private:
@@ -1173,6 +1178,7 @@ class Environment : public MemoryRetainer {
   std::unique_ptr<profiler::V8CoverageConnection> coverage_connection_;
   std::unique_ptr<profiler::V8CpuProfilerConnection> cpu_profiler_connection_;
   std::string coverage_directory_;
+  std::string cpu_prof_dir_;
   std::string cpu_profile_path_;
 #endif  // HAVE_INSPECTOR
 

diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc
@@ -318,19 +318,13 @@ void StartCoverageCollection(Environment* env) {
   env->coverage_connection()->Start();
 }
 
-void StartCpuProfiling(Environment* env, const std::string& profile_path) {
-  std::string path;
-  if (profile_path.empty()) {
-    char cwd[CWD_BUFSIZE];
-    size_t size = CWD_BUFSIZE;
-    int err = uv_cwd(cwd, &size);
-    // TODO(joyeecheung): fallback to exec path / argv[0]
-    CHECK_EQ(err, 0);
-    CHECK_GT(size, 0);
+void StartCpuProfiling(Environment* env, const std::string& profile_name) {
+  std::string path = env->cpu_prof_dir() + std::string(kPathSeparator);
+  if (profile_name.empty()) {
     DiagnosticFilename filename(env, "CPU", "cpuprofile");
-    path = cwd + std::string(kPathSeparator) + (*filename);
+    path += *filename;
   } else {
-    path = profile_path;
+    path += profile_name;
   }
   env->set_cpu_profile_path(std::move(path));
   env->set_cpu_profiler_connection(

diff --git a/src/node.cc b/src/node.cc
@@ -239,7 +239,8 @@ MaybeLocal<Value> RunBootstrapping(Environment* env) {
 
 #if HAVE_INSPECTOR
   if (env->options()->cpu_prof) {
-    profiler::StartCpuProfiling(env, env->options()->cpu_prof_path);
+    env->InitializeCPUProfDir(env->options()->cpu_prof_dir);
+    profiler::StartCpuProfiling(env, env->options()->cpu_prof_name);
   }
 #endif  // HAVE_INSPECTOR
 

diff --git a/src/node_options.cc b/src/node_options.cc
@@ -149,6 +149,15 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
   }
 
 #if HAVE_INSPECTOR
+  if (!cpu_prof) {
+    if (!cpu_prof_name.empty()) {
+      errors->push_back("--cpu-prof-name must be used with --cpu-prof");
+    }
+    if (!cpu_prof_dir.empty()) {
+      errors->push_back("--cpu-prof-dir must be used with --cpu-prof");
+    }
+  }
+
   debug_options_.CheckOptions(errors);
 #endif  // HAVE_INSPECTOR
 }
@@ -335,14 +344,17 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
 #if HAVE_INSPECTOR
   AddOption("--cpu-prof",
             "Start the V8 CPU profiler on start up, and write the CPU profile "
-            "to disk before exit. If --cpu-prof-path is not specified, write "
+            "to disk before exit. If --cpu-prof-dir is not specified, write "
             "the profile to the current working directory.",
             &EnvironmentOptions::cpu_prof);
-  AddOption("--cpu-prof-path",
-            "Path the V8 CPU profile generated with --cpu-prof will be "
-            "written to.",
-            &EnvironmentOptions::cpu_prof_path);
-  Implies("--cpu-prof-path", "--cpu-prof");
+  AddOption("--cpu-prof-name",
+            "specified file name of the V8 CPU profile generated with "
+            "--cpu-prof",
+            &EnvironmentOptions::cpu_prof_name);
+  AddOption("--cpu-prof-dir",
+            "Directory where the V8 profiles generated by --cpu-prof will be "
+            "placed. Does not affect --prof.",
+            &EnvironmentOptions::cpu_prof_dir);
 #endif  // HAVE_INSPECTOR
   AddOption("--redirect-warnings",
             "write warnings to file instead of stderr",

diff --git a/src/node_options.h b/src/node_options.h
@@ -110,7 +110,8 @@ class EnvironmentOptions : public Options {
   bool preserve_symlinks_main = false;
   bool prof_process = false;
 #if HAVE_INSPECTOR
-  std::string cpu_prof_path;
+  std::string cpu_prof_dir;
+  std::string cpu_prof_name;
   bool cpu_prof = false;
 #endif  // HAVE_INSPECTOR
   std::string redirect_warnings;

diff --git a/test/fixtures/workload/fibonacci-worker-argv.js b/test/fixtures/workload/fibonacci-worker-argv.js
@@ -0,0 +1,7 @@
+'use strict';
+
+const { Worker } = require('worker_threads');
+const path = require('path');
+new Worker(path.join(__dirname, 'fibonacci.js'), {
+  execArgv: ['--cpu-prof']
+});
diff --git a/test/fixtures/workload/fibonacci-worker.js b/test/fixtures/workload/fibonacci-worker.js
@@ -2,6 +2,4 @@
 
 const { Worker } = require('worker_threads');
 const path = require('path');
-new Worker(path.join(__dirname, 'fibonacci.js'), {
-  execArgv: ['--cpu-prof']
-});
+new Worker(path.join(__dirname, 'fibonacci.js'));