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

To improve the integration of `--cpu-prof` with workers, this patch splits `--cpu-prof-path` into `--prof-dir` and `--cpu-prof-name`, so when a worker is launched from a thread that enables `--cpu-prof`, if the parent thread sets `--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.
nodejs · joyeecheung · Apr 19, 2019 · Apr 19, 2019 · Apr 19, 2019 · Apr 21, 2019
commit 4212a65498659f92f196569ee20e6ecf5d191fb4
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -83,28 +83,28 @@ 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 `--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-name`
 <!-- 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`.
-
-```console
-$ node --cpu-prof-path /tmp/test.cpuprofile index.js
-```
+Specify the file name of the CPU profile generated by `--cpu-prof`.
 
 ### `--diagnostic-report-directory=directory`
 <!-- YAML
@@ -475,6 +475,19 @@ added: v2.0.0
 
 Generate V8 profiler output.
 
+### `--prof-dir`
+<!-- YAML
+added: REPLACEME
+-->
+
+> Stability: 1 - Experimental
+
+Specify the directory where the CPU profiles generated by `--cpu-prof` will
+be placed.
+
+This does not apply to `--prof` whose output is handled by V8 instead of
+Node.js.
+
 ### `--prof-process`
 <!-- YAML
 added: v5.2.0

diff --git a/doc/node.1 b/doc/node.1
@@ -81,13 +81,12 @@ 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 -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
-.Fl -cpu-prof
-will be written to. When used alone, it implies
+.It Fl -cpu-prof-name
+File name of the V8 CPU profile generated with
 .Fl -cpu-prof
 .
 .It Fl -diagnostic-report-directory
@@ -241,6 +240,13 @@ Instructs the module loader to preserve symbolic links when resolving and cachin
 .It Fl -prof
 Generate V8 profiler output.
 .
+.It Fl -prof-dir
+The directory where the CPU profiles generated by
+.Fl -cpu-prof
+will be placed. Note that this does not apply to
+.Fl -prof
+whose output is handled by V8, not Node.js.
+.
 .It Fl -prof-process
 Process V8 profiler output generated using the V8 option
 .Fl -prof .

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,29 @@ 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_prof_dir(const std::string& path) {
+  prof_dir_ = path;
+}
+
+inline const std::string& Environment::prof_dir() const {
+  return prof_dir_;
+}
+
+inline void Environment::InitializeProfDir(const std::string& dir) {
+  if (!dir.empty()) {
+    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);
+  prof_dir_ = cwd;
+}
+
 #endif  // HAVE_INSPECTOR
 
 inline std::shared_ptr<HostPort> Environment::inspector_host_port() {

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_prof_dir(const std::string& path);
+  inline const std::string& prof_dir() const;
+
+  inline void InitializeProfDir(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 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->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->InitializeProfDir(env->options()->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 && !cpu_prof_name.empty()) {
+    errors->push_back("--cpu-prof-name must be used with --cpu-prof");
+  }
+
+  // We may add support for other profiles in the future.
+  if (!prof_dir.empty() && !cpu_prof) {
+    errors->push_back("--prof-dir must be used with --cpu-prof");
+  }
+
   debug_options_.CheckOptions(errors);
 #endif  // HAVE_INSPECTOR
 }
@@ -338,11 +347,14 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
             "to disk before exit. If --cpu-prof-path 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("--prof-dir",
+            "Directory where the V8 profiles generated by --cpu-prof will be "
+            "placed. Does not affect --prof.",
+            &EnvironmentOptions::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 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'));