From 49d3d11ba7975b2c6df4ecab55b4619da46fcb95 Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Fri, 19 Apr 2019 17:15:04 +0800
Subject: [PATCH] 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: https://github.com/nodejs/node/pull/27306
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
---
 doc/api/cli.md                                |  26 +++-
 doc/node.1                                    |  14 +-
 src/env-inl.h                                 |  17 ++
 src/env.cc                                    |  18 +++
 src/env.h                                     |   6 +
 src/inspector_profiler.cc                     |  16 +-
 src/node.cc                                   |   3 +-
 src/node_options.cc                           |  24 ++-
 src/node_options.h                            |   3 +-
 .../workload/fibonacci-worker-argv.js         |   7 +
 test/fixtures/workload/fibonacci-worker.js    |   4 +-
 test/sequential/test-cpu-prof.js              | 147 +++++++++++++++---
 12 files changed, 231 insertions(+), 54 deletions(-)
 create mode 100644 test/fixtures/workload/fibonacci-worker-argv.js

diff --git a/doc/api/cli.md b/doc/api/cli.md
index d1419e2ccc9ff9..fed5aa90aa3a8f 100644
--- a/doc/api/cli.md
+++ b/doc/api/cli.md
@@ -83,8 +83,13 @@ 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
@@ -92,19 +97,24 @@ $ 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
index 6f4c66053fc504..1fa5ce7f650e99 100644
--- 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
index 3a6caa19f86d6b..ee0503620cd66d 100644
--- 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
index ccb3c404f91ba2..cbb8ab6fe27c02 100644
--- 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
index cfef54dcbd7b3e..def7cba3886ac2 100644
--- 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
index 46d4c4fec8d19e..3507ae4ef2f1e0 100644
--- 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
index 8efc20475df553..061d6bb424431f 100644
--- 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
index 8914acfd60890c..adc6f61586853b 100644
--- 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
index d0d35b162e6ea6..f92261569002e0 100644
--- 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
new file mode 100644
index 00000000000000..9c968bfeec4ae2
--- /dev/null
+++ 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
index 9c968bfeec4ae2..d92fb169317941 100644
--- 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'));
diff --git a/test/sequential/test-cpu-prof.js b/test/sequential/test-cpu-prof.js
index 8b9001ba674e23..ffaa0e9b1b8001 100644
--- a/test/sequential/test-cpu-prof.js
+++ b/test/sequential/test-cpu-prof.js
@@ -1,6 +1,6 @@
 'use strict';
 
-// This tests that --cpu-prof and --cpu-prof-path works.
+// This tests that --cpu-prof, --cpu-prof-dir and --cpu-prof-name works.
 
 const common = require('../common');
 if (process.features.debug &&
@@ -27,17 +27,22 @@ function getCpuProfiles(dir) {
     .map((file) => path.join(dir, file));
 }
 
-function verifyFrames(output, file, suffix) {
+function getFrames(output, file, suffix) {
   const data = fs.readFileSync(file, 'utf8');
   const profile = JSON.parse(data);
   const frames = profile.nodes.filter((i) => {
     const frame = i.callFrame;
     return frame.url.endsWith(suffix);
   });
+  return { frames, nodes: profile.nodes };
+}
+
+function verifyFrames(output, file, suffix) {
+  const { frames, nodes } = getFrames(output, file, suffix);
   if (frames.length === 0) {
     // Show native debug output and the profile for debugging.
     console.log(output.stderr.toString());
-    console.log(profile.nodes);
+    console.log(nodes);
   }
   assert.notDeepStrictEqual(frames, []);
 }
@@ -118,11 +123,11 @@ const env = {
   verifyFrames(output, profiles[0], 'fibonacci-sigint.js');
 }
 
-// Outputs CPU profile from worker.
+// Outputs CPU profile from worker when execArgv is set.
 {
   tmpdir.refresh();
   const output = spawnSync(process.execPath, [
-    fixtures.path('workload', 'fibonacci-worker.js'),
+    fixtures.path('workload', 'fibonacci-worker-argv.js'),
   ], {
     cwd: tmpdir.path,
     env
@@ -136,13 +141,56 @@ const env = {
   verifyFrames(output, profiles[0], 'fibonacci.js');
 }
 
-// Output to specified --cpu-prof-path without --cpu-prof
+// --cpu-prof-name without --cpu-prof
+{
+  tmpdir.refresh();
+  const output = spawnSync(process.execPath, [
+    '--cpu-prof-name',
+    'test.cpuprofile',
+    fixtures.path('workload', 'fibonacci.js'),
+  ], {
+    cwd: tmpdir.path,
+    env
+  });
+  const stderr = output.stderr.toString().trim();
+  if (output.status !== 9) {
+    console.log(stderr);
+  }
+  assert.strictEqual(output.status, 9);
+  assert.strictEqual(
+    stderr,
+    `${process.execPath}: --cpu-prof-name must be used with --cpu-prof`);
+}
+
+// --cpu-prof-dir without --cpu-prof
+{
+  tmpdir.refresh();
+  const output = spawnSync(process.execPath, [
+    '--cpu-prof-dir',
+    'prof',
+    fixtures.path('workload', 'fibonacci.js'),
+  ], {
+    cwd: tmpdir.path,
+    env
+  });
+  const stderr = output.stderr.toString().trim();
+  if (output.status !== 9) {
+    console.log(stderr);
+  }
+  assert.strictEqual(output.status, 9);
+  assert.strictEqual(
+    stderr,
+    `${process.execPath}: --cpu-prof-dir must be used with --cpu-prof`);
+}
+
+// --cpu-prof-name
 {
   tmpdir.refresh();
   const file = path.join(tmpdir.path, 'test.cpuprofile');
   const output = spawnSync(process.execPath, [
-    '--cpu-prof-path',
-    file,
+    '--cpu-prof',
+    '--cpu-prof-name',
+    'test.cpuprofile',
     fixtures.path('workload', 'fibonacci.js'),
   ], {
     cwd: tmpdir.path,
@@ -157,14 +205,13 @@ const env = {
   verifyFrames(output, file, 'fibonacci.js');
 }
 
-// Output to specified --cpu-prof-path with --cpu-prof
+// relative --cpu-prof-dir
 {
   tmpdir.refresh();
-  const file = path.join(tmpdir.path, 'test.cpuprofile');
   const output = spawnSync(process.execPath, [
     '--cpu-prof',
-    '--cpu-prof-path',
-    file,
+    '--cpu-prof-dir',
+    'prof',
     fixtures.path('workload', 'fibonacci.js'),
   ], {
     cwd: tmpdir.path,
@@ -174,19 +221,47 @@ const env = {
     console.log(output.stderr.toString());
   }
   assert.strictEqual(output.status, 0);
-  const profiles = getCpuProfiles(tmpdir.path);
-  assert.deepStrictEqual(profiles, [file]);
-  verifyFrames(output, file, 'fibonacci.js');
+  const dir = path.join(tmpdir.path, 'prof');
+  assert(fs.existsSync(dir));
+  const profiles = getCpuProfiles(dir);
+  assert.strictEqual(profiles.length, 1);
+  verifyFrames(output, profiles[0], 'fibonacci.js');
 }
 
-// Output to specified --cpu-prof-path when it's not absolute
+// absolute --cpu-prof-dir
 {
   tmpdir.refresh();
-  const file = path.join(tmpdir.path, 'test.cpuprofile');
+  const dir = path.join(tmpdir.path, 'prof');
   const output = spawnSync(process.execPath, [
     '--cpu-prof',
-    '--cpu-prof-path',
+    '--cpu-prof-dir',
+    dir,
+    fixtures.path('workload', 'fibonacci.js'),
+  ], {
+    cwd: tmpdir.path,
+    env
+  });
+  if (output.status !== 0) {
+    console.log(output.stderr.toString());
+  }
+  assert.strictEqual(output.status, 0);
+  assert(fs.existsSync(dir));
+  const profiles = getCpuProfiles(dir);
+  assert.strictEqual(profiles.length, 1);
+  verifyFrames(output, profiles[0], 'fibonacci.js');
+}
+
+// --cpu-prof-dir and --cpu-prof-name
+{
+  tmpdir.refresh();
+  const dir = path.join(tmpdir.path, 'prof');
+  const file = path.join(dir, 'test.cpuprofile');
+  const output = spawnSync(process.execPath, [
+    '--cpu-prof',
+    '--cpu-prof-name',
     'test.cpuprofile',
+    '--cpu-prof-dir',
+    dir,
     fixtures.path('workload', 'fibonacci.js'),
   ], {
     cwd: tmpdir.path,
@@ -196,7 +271,41 @@ const env = {
     console.log(output.stderr.toString());
   }
   assert.strictEqual(output.status, 0);
-  const profiles = getCpuProfiles(tmpdir.path);
+  assert(fs.existsSync(dir));
+  const profiles = getCpuProfiles(dir);
   assert.deepStrictEqual(profiles, [file]);
   verifyFrames(output, file, 'fibonacci.js');
 }
+
+// --cpu-prof-dir with worker
+{
+  tmpdir.refresh();
+  const output = spawnSync(process.execPath, [
+    '--cpu-prof-dir',
+    'prof',
+    '--cpu-prof',
+    fixtures.path('workload', 'fibonacci-worker.js'),
+  ], {
+    cwd: tmpdir.path,
+    env
+  });
+  if (output.status !== 0) {
+    console.log(output.stderr.toString());
+  }
+  assert.strictEqual(output.status, 0);
+  const dir = path.join(tmpdir.path, 'prof');
+  assert(fs.existsSync(dir));
+  const profiles = getCpuProfiles(dir);
+  assert.strictEqual(profiles.length, 2);
+  const profile1 = getFrames(output, profiles[0], 'fibonacci.js');
+  const profile2 = getFrames(output, profiles[1], 'fibonacci.js');
+  if (profile1.frames.length === 0 && profile2.frames.length === 0) {
+    // Show native debug output and the profile for debugging.
+    console.log(output.stderr.toString());
+    console.log('CPU path: ', profiles[0]);
+    console.log(profile1.nodes);
+    console.log('CPU path: ', profiles[1]);
+    console.log(profile2.nodes);
+  }
+  assert(profile1.frames.length > 0 || profile2.frames.length > 0);
+}