From c72381c4ee53fae391901f83258aeb94dd51d7f7 Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Thu, 2 May 2019 20:41:54 +0800
Subject: [PATCH 1/3] src: refactor V8ProfilerConnection::DispatchMessage()

- Auto-generate the message id and return it for future use (we
  can always parse the response to find the message containing
  the profile instead of relying on the inspector connection being
  synchornous).
- Generate the message from method and parameter strings and create
  a `StringView` directly to avoid the unnecessary copy in
  `ToProtocolString()`.
---
 src/inspector_profiler.cc | 74 +++++++++++++++------------------------
 src/inspector_profiler.h  | 10 +++++-
 2 files changed, 38 insertions(+), 46 deletions(-)

diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc
index 530636f4d0927e..a3739d52b21de7 100644
--- a/src/inspector_profiler.cc
+++ b/src/inspector_profiler.cc
@@ -1,4 +1,5 @@
 #include "inspector_profiler.h"
+#include <sstream>
 #include "base_object-inl.h"
 #include "debug_utils.h"
 #include "node_file.h"
@@ -33,12 +34,6 @@ const char* const kPathSeparator = "/";
 #define CWD_BUFSIZE (PATH_MAX)
 #endif
 
-std::unique_ptr<StringBuffer> ToProtocolString(Isolate* isolate,
-                                               Local<Value> value) {
-  TwoByteValue buffer(isolate, value);
-  return StringBuffer::create(StringView(*buffer, buffer.length()));
-}
-
 V8ProfilerConnection::V8ProfilerConnection(Environment* env)
     : session_(env->inspector_agent()->Connect(
           std::make_unique<V8ProfilerConnection::V8ProfilerSessionDelegate>(
@@ -46,8 +41,27 @@ V8ProfilerConnection::V8ProfilerConnection(Environment* env)
           false)),
       env_(env) {}
 
-void V8ProfilerConnection::DispatchMessage(Local<String> message) {
-  session_->Dispatch(ToProtocolString(env()->isolate(), message)->string());
+size_t V8ProfilerConnection::DispatchMessage(const char* method,
+                                             const char* params) {
+  std::stringstream ss;
+  size_t id = next_id();
+  ss << R"({ "id": )" << id;
+  DCHECK(method != nullptr);
+  ss << R"(, "method": ")" << method << '"';
+  if (params != nullptr) {
+    ss << R"(, "params": )" << params;
+  }
+  ss << " }";
+  std::string message = ss.str();
+  const uint8_t* message_data =
+      reinterpret_cast<const uint8_t*>(message.c_str());
+  Debug(env(),
+        DebugCategory::INSPECTOR_PROFILER,
+        "Dispatching message %s\n",
+        message.c_str());
+  session_->Dispatch(StringView(message_data, message.length()));
+  // TODO(joyeecheung): use this to identify the ending message.
+  return id;
 }
 
 static void WriteResult(Environment* env,
@@ -202,34 +216,15 @@ std::string V8CoverageConnection::GetDirectory() const {
 }
 
 void V8CoverageConnection::Start() {
-  Debug(env(),
-        DebugCategory::INSPECTOR_PROFILER,
-        "Sending Profiler.startPreciseCoverage\n");
-  Isolate* isolate = env()->isolate();
-  Local<String> enable = FIXED_ONE_BYTE_STRING(
-      isolate, R"({"id": 1, "method": "Profiler.enable"})");
-  Local<String> start = FIXED_ONE_BYTE_STRING(isolate, R"({
-      "id": 2,
-      "method": "Profiler.startPreciseCoverage",
-      "params": { "callCount": true, "detailed": true }
-  })");
-  DispatchMessage(enable);
-  DispatchMessage(start);
+  DispatchMessage("Profiler.enable");
+  DispatchMessage("Profiler.startPreciseCoverage",
+                  R"({ "callCount": true, "detailed": true })");
 }
 
 void V8CoverageConnection::End() {
   CHECK_EQ(ending_, false);
   ending_ = true;
-  Debug(env(),
-        DebugCategory::INSPECTOR_PROFILER,
-        "Sending Profiler.takePreciseCoverage\n");
-  Isolate* isolate = env()->isolate();
-  HandleScope scope(isolate);
-  Local<String> end = FIXED_ONE_BYTE_STRING(isolate, R"({
-      "id": 3,
-      "method": "Profiler.takePreciseCoverage"
-  })");
-  DispatchMessage(end);
+  DispatchMessage("Profiler.takePreciseCoverage");
 }
 
 std::string V8CpuProfilerConnection::GetDirectory() const {
@@ -257,25 +252,14 @@ MaybeLocal<Object> V8CpuProfilerConnection::GetProfile(Local<Object> result) {
 }
 
 void V8CpuProfilerConnection::Start() {
-  Debug(env(), DebugCategory::INSPECTOR_PROFILER, "Sending Profiler.start\n");
-  Isolate* isolate = env()->isolate();
-  Local<String> enable = FIXED_ONE_BYTE_STRING(
-      isolate, R"({"id": 1, "method": "Profiler.enable"})");
-  Local<String> start = FIXED_ONE_BYTE_STRING(
-      isolate, R"({"id": 2, "method": "Profiler.start"})");
-  DispatchMessage(enable);
-  DispatchMessage(start);
+  DispatchMessage("Profiler.enable");
+  DispatchMessage("Profiler.start");
 }
 
 void V8CpuProfilerConnection::End() {
   CHECK_EQ(ending_, false);
   ending_ = true;
-  Debug(env(), DebugCategory::INSPECTOR_PROFILER, "Sending Profiler.stop\n");
-  Isolate* isolate = env()->isolate();
-  HandleScope scope(isolate);
-  Local<String> end =
-      FIXED_ONE_BYTE_STRING(isolate, R"({"id": 3, "method": "Profiler.stop"})");
-  DispatchMessage(end);
+  DispatchMessage("Profiler.stop");
 }
 
 // For now, we only support coverage profiling, but we may add more
diff --git a/src/inspector_profiler.h b/src/inspector_profiler.h
index cbe053e578cf8c..219405b8c7f3f4 100644
--- a/src/inspector_profiler.h
+++ b/src/inspector_profiler.h
@@ -34,7 +34,13 @@ class V8ProfilerConnection {
   virtual ~V8ProfilerConnection() = default;
 
   Environment* env() const { return env_; }
-  void DispatchMessage(v8::Local<v8::String> message);
+
+  // Dispatch a protocol message, and returns the id of the message.
+  // `method` does not need to be surrounded by quotes.
+  // The optional `params` should be formatted in JSON.
+  // The strings should be in one byte characters - which is enough for
+  // the commands we use here.
+  size_t DispatchMessage(const char* method, const char* params = nullptr);
 
   // Use DispatchMessage() to dispatch necessary inspector messages
   // to start and end the profiling.
@@ -55,9 +61,11 @@ class V8ProfilerConnection {
       v8::Local<v8::Object> result) = 0;
 
  private:
+  size_t next_id() { return id_++; }
   void WriteProfile(v8::Local<v8::String> message);
   std::unique_ptr<inspector::InspectorSession> session_;
   Environment* env_ = nullptr;
+  size_t id_ = 1;
 };
 
 class V8CoverageConnection : public V8ProfilerConnection {

From 41170d9f9a3382c389e3ec7eccd5c9ae5a36d60b Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Thu, 2 May 2019 21:10:10 +0800
Subject: [PATCH 2/3] inspector: implement --cpu-prof-interval

This patch implements --cpu-prof-interval to specify the sampling
interval of the CPU profiler started by --cpu-prof from the command
line. Also adjust the interval to 100 in test-cpu-prof.js to make
the test less flaky - it would fail if the time taken to finish
the workload is smaller than the sampling interval, which was
more likely on powerful machines when the interval was 1000.
---
 doc/api/cli.md                                | 10 +++
 doc/node.1                                    |  6 ++
 src/env-inl.h                                 |  8 +++
 src/env.h                                     |  4 ++
 src/inspector_profiler.cc                     |  6 +-
 src/node_options.cc                           |  9 +++
 src/node_options.h                            |  2 +
 .../workload/fibonacci-worker-argv.js         |  6 +-
 test/sequential/test-cpu-prof.js              | 65 ++++++++++++++++++-
 9 files changed, 113 insertions(+), 3 deletions(-)

diff --git a/doc/api/cli.md b/doc/api/cli.md
index c0682d864c9d0a..4c44fa36827d01 100644
--- a/doc/api/cli.md
+++ b/doc/api/cli.md
@@ -107,6 +107,16 @@ added: v12.0.0
 Specify the directory where the CPU profiles generated by `--cpu-prof` will
 be placed.
 
+### `--cpu-prof-interval`
+<!-- YAML
+added: REPLACEME
+-->
+
+> Stability: 1 - Experimental
+
+Specify the sampling interval in microseconds for the CPU profiles generated
+by `--cpu-prof`. The default is 1000 microseconds.
+
 ### `--cpu-prof-name`
 <!-- YAML
 added: v12.0.0
diff --git a/doc/node.1 b/doc/node.1
index 1fa5ce7f650e99..3ddd4055325a7d 100644
--- a/doc/node.1
+++ b/doc/node.1
@@ -90,6 +90,12 @@ The directory where the CPU profiles generated by
 .Fl -cpu-prof
 will be placed.
 .
+.It Fl -cpu-prof-interval
+The sampling interval in mircoseconds for the CPU profiles generated by
+.Fl -cpu-prof .
+The default is
+.Sy 1000 .
+.
 .It Fl -cpu-prof-name
 File name of the V8 CPU profile generated with
 .Fl -cpu-prof
diff --git a/src/env-inl.h b/src/env-inl.h
index 5761eb71536b08..992e51354e6cf2 100644
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -673,6 +673,14 @@ Environment::cpu_profiler_connection() {
   return cpu_profiler_connection_.get();
 }
 
+inline void Environment::set_cpu_prof_interval(uint64_t interval) {
+  cpu_prof_interval_ = interval;
+}
+
+inline uint64_t Environment::cpu_prof_interval() const {
+  return cpu_prof_interval_;
+}
+
 inline void Environment::set_cpu_prof_name(const std::string& name) {
   cpu_prof_name_ = name;
 }
diff --git a/src/env.h b/src/env.h
index a9fc1d1e59943c..e7af2b1daef150 100644
--- a/src/env.h
+++ b/src/env.h
@@ -1143,6 +1143,9 @@ class Environment : public MemoryRetainer {
   inline void set_cpu_prof_name(const std::string& name);
   inline const std::string& cpu_prof_name() const;
 
+  inline void set_cpu_prof_interval(uint64_t interval);
+  inline uint64_t cpu_prof_interval() const;
+
   inline void set_cpu_prof_dir(const std::string& dir);
   inline const std::string& cpu_prof_dir() const;
 #endif  // HAVE_INSPECTOR
@@ -1183,6 +1186,7 @@ class Environment : public MemoryRetainer {
   std::string coverage_directory_;
   std::string cpu_prof_dir_;
   std::string cpu_prof_name_;
+  uint64_t cpu_prof_interval_;
 #endif  // HAVE_INSPECTOR
 
   std::shared_ptr<EnvironmentOptions> options_;
diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc
index a3739d52b21de7..2d3f061e987d35 100644
--- a/src/inspector_profiler.cc
+++ b/src/inspector_profiler.cc
@@ -21,7 +21,6 @@ using v8::Object;
 using v8::String;
 using v8::Value;
 
-using v8_inspector::StringBuffer;
 using v8_inspector::StringView;
 
 #ifdef _WIN32
@@ -254,6 +253,10 @@ MaybeLocal<Object> V8CpuProfilerConnection::GetProfile(Local<Object> result) {
 void V8CpuProfilerConnection::Start() {
   DispatchMessage("Profiler.enable");
   DispatchMessage("Profiler.start");
+  std::string params = R"({ "interval": )";
+  params += std::to_string(env()->cpu_prof_interval());
+  params += " }";
+  DispatchMessage("Profiler.setSamplingInterval", params.c_str());
 }
 
 void V8CpuProfilerConnection::End() {
@@ -304,6 +307,7 @@ void StartProfilers(Environment* env) {
   }
   if (env->options()->cpu_prof) {
     const std::string& dir = env->options()->cpu_prof_dir;
+    env->set_cpu_prof_interval(env->options()->cpu_prof_interval);
     env->set_cpu_prof_dir(dir.empty() ? GetCwd() : dir);
     if (env->options()->cpu_prof_name.empty()) {
       DiagnosticFilename filename(env, "CPU", "cpuprofile");
diff --git a/src/node_options.cc b/src/node_options.cc
index adc6f61586853b..54f03a308053fe 100644
--- a/src/node_options.cc
+++ b/src/node_options.cc
@@ -156,6 +156,11 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
     if (!cpu_prof_dir.empty()) {
       errors->push_back("--cpu-prof-dir must be used with --cpu-prof");
     }
+    // We can't catch the case where the value passed is the default value,
+    // then the option just becomes a noop which is fine.
+    if (cpu_prof_interval != kDefaultCpuProfInterval) {
+      errors->push_back("--cpu-prof-interval must be used with --cpu-prof");
+    }
   }
 
   debug_options_.CheckOptions(errors);
@@ -351,6 +356,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
             "specified file name of the V8 CPU profile generated with "
             "--cpu-prof",
             &EnvironmentOptions::cpu_prof_name);
+  AddOption("--cpu-prof-interval",
+            "specified sampling interval in microseconds for the V8 CPU "
+            "profile generated with --cpu-prof. (default: 1000)",
+            &EnvironmentOptions::cpu_prof_interval);
   AddOption("--cpu-prof-dir",
             "Directory where the V8 profiles generated by --cpu-prof will be "
             "placed. Does not affect --prof.",
diff --git a/src/node_options.h b/src/node_options.h
index 3b2513a183bc1b..457c3655b3f0e6 100644
--- a/src/node_options.h
+++ b/src/node_options.h
@@ -111,6 +111,8 @@ class EnvironmentOptions : public Options {
   bool prof_process = false;
 #if HAVE_INSPECTOR
   std::string cpu_prof_dir;
+  static const uint64_t kDefaultCpuProfInterval = 1000;
+  uint64_t cpu_prof_interval = kDefaultCpuProfInterval;
   std::string cpu_prof_name;
   bool cpu_prof = false;
 #endif  // HAVE_INSPECTOR
diff --git a/test/fixtures/workload/fibonacci-worker-argv.js b/test/fixtures/workload/fibonacci-worker-argv.js
index 9c968bfeec4ae2..69a78ec4e175ef 100644
--- a/test/fixtures/workload/fibonacci-worker-argv.js
+++ b/test/fixtures/workload/fibonacci-worker-argv.js
@@ -3,5 +3,9 @@
 const { Worker } = require('worker_threads');
 const path = require('path');
 new Worker(path.join(__dirname, 'fibonacci.js'), {
-  execArgv: ['--cpu-prof']
+  execArgv: [
+    '--cpu-prof',
+    '--cpu-prof-interval',
+    process.env.CPU_PROF_INTERVAL || '100'
+  ]
 });
diff --git a/test/sequential/test-cpu-prof.js b/test/sequential/test-cpu-prof.js
index 19c8afd92cd5b9..38be6211e4d23f 100644
--- a/test/sequential/test-cpu-prof.js
+++ b/test/sequential/test-cpu-prof.js
@@ -50,18 +50,43 @@ if (common.isWindows) {
   FIB = 40;
 }
 
+// We need to set --cpu-interval to a smaller value to make sure we can
+// find our workload in the samples. 50us should be a small enough sampling
+// interval for this.
+const kCpuProfInterval = 50;
 const env = {
   ...process.env,
   FIB,
   NODE_DEBUG_NATIVE: 'INSPECTOR_PROFILER'
 };
 
+// Test --cpu-prof without --cpu-prof-interval. Here we just verify that
+// we manage to generate a profile.
+{
+  tmpdir.refresh();
+  const output = spawnSync(process.execPath, [
+    '--cpu-prof',
+    fixtures.path('workload', 'fibonacci.js'),
+  ], {
+    cwd: tmpdir.path,
+    env
+  });
+  if (output.status !== 0) {
+    console.log(output.stderr.toString());
+  }
+  assert.strictEqual(output.status, 0);
+  const profiles = getCpuProfiles(tmpdir.path);
+  assert.strictEqual(profiles.length, 1);
+}
+
 // Outputs CPU profile when event loop is drained.
 // TODO(joyeecheung): share the fixutres with v8 coverage tests
 {
   tmpdir.refresh();
   const output = spawnSync(process.execPath, [
     '--cpu-prof',
+    '--cpu-prof-interval',
+    kCpuProfInterval,
     fixtures.path('workload', 'fibonacci.js'),
   ], {
     cwd: tmpdir.path,
@@ -81,6 +106,8 @@ const env = {
   tmpdir.refresh();
   const output = spawnSync(process.execPath, [
     '--cpu-prof',
+    '--cpu-prof-interval',
+    kCpuProfInterval,
     fixtures.path('workload', 'fibonacci-exit.js'),
   ], {
     cwd: tmpdir.path,
@@ -100,6 +127,8 @@ const env = {
   tmpdir.refresh();
   const output = spawnSync(process.execPath, [
     '--cpu-prof',
+    '--cpu-prof-interval',
+    kCpuProfInterval,
     fixtures.path('workload', 'fibonacci-sigint.js'),
   ], {
     cwd: tmpdir.path,
@@ -123,7 +152,10 @@ const env = {
     fixtures.path('workload', 'fibonacci-worker-argv.js'),
   ], {
     cwd: tmpdir.path,
-    env
+    env: {
+      ...process.env,
+      CPU_PROF_INTERVAL: kCpuProfInterval
+    }
   });
   if (output.status !== 0) {
     console.log(output.stderr.toString());
@@ -176,12 +208,35 @@ const env = {
     `${process.execPath}: --cpu-prof-dir must be used with --cpu-prof`);
 }
 
+// --cpu-prof-interval without --cpu-prof
+{
+  tmpdir.refresh();
+  const output = spawnSync(process.execPath, [
+    '--cpu-prof-interval',
+    kCpuProfInterval,
+    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-interval 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',
+    '--cpu-prof-interval',
+    kCpuProfInterval,
     '--cpu-prof-name',
     'test.cpuprofile',
     fixtures.path('workload', 'fibonacci.js'),
@@ -203,6 +258,8 @@ const env = {
   tmpdir.refresh();
   const output = spawnSync(process.execPath, [
     '--cpu-prof',
+    '--cpu-prof-interval',
+    kCpuProfInterval,
     '--cpu-prof-dir',
     'prof',
     fixtures.path('workload', 'fibonacci.js'),
@@ -227,6 +284,8 @@ const env = {
   const dir = path.join(tmpdir.path, 'prof');
   const output = spawnSync(process.execPath, [
     '--cpu-prof',
+    '--cpu-prof-interval',
+    kCpuProfInterval,
     '--cpu-prof-dir',
     dir,
     fixtures.path('workload', 'fibonacci.js'),
@@ -251,6 +310,8 @@ const env = {
   const file = path.join(dir, 'test.cpuprofile');
   const output = spawnSync(process.execPath, [
     '--cpu-prof',
+    '--cpu-prof-interval',
+    kCpuProfInterval,
     '--cpu-prof-name',
     'test.cpuprofile',
     '--cpu-prof-dir',
@@ -274,6 +335,8 @@ const env = {
 {
   tmpdir.refresh();
   const output = spawnSync(process.execPath, [
+    '--cpu-prof-interval',
+    kCpuProfInterval,
     '--cpu-prof-dir',
     'prof',
     '--cpu-prof',

From 021a1e587beef30063919fa130f56a2e331a0ca2 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <github@addaleax.net>
Date: Sat, 4 May 2019 03:08:51 +0800
Subject: [PATCH 3/3] fixup! inspector: implement --cpu-prof-interval

Co-Authored-By: joyeecheung <joyeec9h3@gmail.com>
---
 doc/node.1 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/node.1 b/doc/node.1
index 3ddd4055325a7d..169416e625757f 100644
--- a/doc/node.1
+++ b/doc/node.1
@@ -91,7 +91,7 @@ The directory where the CPU profiles generated by
 will be placed.
 .
 .It Fl -cpu-prof-interval
-The sampling interval in mircoseconds for the CPU profiles generated by
+The sampling interval in microseconds for the CPU profiles generated by
 .Fl -cpu-prof .
 The default is
 .Sy 1000 .