src: refactor tracing agent code

Avoid unnecessary operations, add a bit of documentation,
and turn the `ClientHandle` smart pointer alias into a real
class.

This should allow de-coupling the unnecessary combination of
a single specific `file_writer` from `Agent`.

PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

src/inspector/tracing_agent.cc

@@ -46,9 +46,7 @@ class InspectorTraceWriter : public node::tracing::AsyncTraceWriter {
 }  // namespace
 
 TracingAgent::TracingAgent(Environment* env)
-                           : env_(env),
-                             trace_writer_(
-                                  tracing::Agent::EmptyClientHandle()) {
+                           : env_(env) {
 }
 
 TracingAgent::~TracingAgent() {
@@ -62,7 +60,7 @@ void TracingAgent::Wire(UberDispatcher* dispatcher) {
 
 DispatchResponse TracingAgent::start(
     std::unique_ptr<protocol::NodeTracing::TraceConfig> traceConfig) {
-  if (trace_writer_ != nullptr) {
+  if (!trace_writer_.empty()) {
     return DispatchResponse::Error(
         "Call NodeTracing::end to stop tracing before updating the config");
   }

src/inspector/tracing_agent.h

@@ -2,16 +2,13 @@
 #define SRC_INSPECTOR_TRACING_AGENT_H_
 
 #include "node/inspector/protocol/NodeTracing.h"
+#include "tracing/agent.h"
 #include "v8.h"
 
 
 namespace node {
 class Environment;
 
-namespace tracing {
-class Agent;
-}  // namespace tracing
-
 namespace inspector {
 namespace protocol {
 
@@ -32,8 +29,7 @@ class TracingAgent : public NodeTracing::Backend {
   void DisconnectTraceClient();
 
   Environment* env_;
-  std::unique_ptr<std::pair<tracing::Agent*, int>,
-                  void (*)(std::pair<tracing::Agent*, int>*)> trace_writer_;
+  tracing::AgentWriterHandle trace_writer_;
   std::unique_ptr<NodeTracing::Frontend> frontend_;
 };
 

src/node.cc

@@ -387,7 +387,7 @@ class NodeTraceStateObserver :
 static struct {
 #if NODE_USE_V8_PLATFORM
   void Initialize(int thread_pool_size) {
-    tracing_agent_.reset(new tracing::Agent(trace_file_pattern));
+    tracing_agent_.reset(new tracing::Agent());
     auto controller = tracing_agent_->GetTracingController();
     controller->AddTraceStateObserver(new NodeTraceStateObserver(controller));
     tracing::TraceEventHelper::SetTracingController(controller);
@@ -427,12 +427,13 @@ static struct {
 #endif  // HAVE_INSPECTOR
 
   void StartTracingAgent() {
-    tracing_agent_->Enable(trace_enabled_categories);
+    tracing_file_writer_ = tracing_agent_->AddClient(
+        trace_enabled_categories,
+        new tracing::NodeTraceWriter(trace_file_pattern));
   }
 
   void StopTracingAgent() {
-    if (tracing_agent_)
-      tracing_agent_->Stop();
+    tracing_file_writer_.reset();
   }
 
   tracing::Agent* GetTracingAgent() const {
@@ -444,6 +445,7 @@ static struct {
   }
 
   std::unique_ptr<tracing::Agent> tracing_agent_;
+  tracing::AgentWriterHandle tracing_file_writer_;
   NodePlatform* platform_;
 #else  // !NODE_USE_V8_PLATFORM
   void Initialize(int thread_pool_size) {}

src/tracing/agent.cc

@@ -44,7 +44,7 @@ using v8::platform::tracing::TraceWriter;
 using std::string;
 
 Agent::Agent(const std::string& log_file_pattern)
-    : log_file_pattern_(log_file_pattern), file_writer_(EmptyClientHandle()) {
+    : log_file_pattern_(log_file_pattern) {
   tracing_controller_ = new TracingController();
   tracing_controller_->Initialize(nullptr);
 }
@@ -62,20 +62,23 @@ void Agent::Start() {
   // This thread should be created *after* async handles are created
   // (within NodeTraceWriter and NodeTraceBuffer constructors).
   // Otherwise the thread could shut down prematurely.
-  CHECK_EQ(0, uv_thread_create(&thread_, ThreadCb, this));
+  CHECK_EQ(0, uv_thread_create(&thread_, [](void* arg) {
+    Agent* agent = static_cast<Agent*>(arg);
+    uv_run(&agent->tracing_loop_, UV_RUN_DEFAULT);
+  }, this));
   started_ = true;
 }
 
-Agent::ClientHandle Agent::AddClient(const std::set<std::string>& categories,
-                                     std::unique_ptr<AsyncTraceWriter> writer) {
+AgentWriterHandle Agent::AddClient(
+    const std::set<std::string>& categories,
+    std::unique_ptr<AsyncTraceWriter> writer) {
   Start();
   ScopedSuspendTracing suspend(tracing_controller_, this);
   int id = next_writer_id_++;
   writers_[id] = std::move(writer);
   categories_[id] = categories;
 
-  auto client_id = new std::pair<Agent*, int>(this, id);
-  return ClientHandle(client_id, &DisconnectClient);
+  return AgentWriterHandle(this, id);
 }
 
 void Agent::Stop() {
@@ -101,61 +104,52 @@ void Agent::Disconnect(int client) {
   categories_.erase(client);
 }
 
-// static
-void Agent::ThreadCb(void* arg) {
-  Agent* agent = static_cast<Agent*>(arg);
-  uv_run(&agent->tracing_loop_, UV_RUN_DEFAULT);
-}
-
 void Agent::Enable(const std::string& categories) {
   if (categories.empty())
     return;
   std::set<std::string> categories_set;
-  std::stringstream category_list(categories);
+  std::istringstream category_list(categories);
   while (category_list.good()) {
     std::string category;
     getline(category_list, category, ',');
-    categories_set.insert(category);
+    categories_set.emplace(std::move(category));
   }
   Enable(categories_set);
 }
 
 void Agent::Enable(const std::set<std::string>& categories) {
-  std::string cats;
-  for (const std::string cat : categories)
-    cats += cat + ", ";
   if (categories.empty())
     return;
 
   file_writer_categories_.insert(categories.begin(), categories.end());
   std::set<std::string> full_list(file_writer_categories_.begin(),
                                   file_writer_categories_.end());
-  if (!file_writer_) {
+  if (file_writer_.empty()) {
     // Ensure background thread is running
     Start();
     std::unique_ptr<NodeTraceWriter> writer(
         new NodeTraceWriter(log_file_pattern_, &tracing_loop_));
     file_writer_ = AddClient(full_list, std::move(writer));
   } else {
     ScopedSuspendTracing suspend(tracing_controller_, this);
-    categories_[file_writer_->second] = full_list;
+    categories_[file_writer_.id_] = full_list;
   }
 }
 
 void Agent::Disable(const std::set<std::string>& categories) {
-  for (auto category : categories) {
+  for (const std::string& category : categories) {
     auto it = file_writer_categories_.find(category);
     if (it != file_writer_categories_.end())
       file_writer_categories_.erase(it);
   }
-  if (!file_writer_)
+  if (file_writer_.empty())
     return;
   ScopedSuspendTracing suspend(tracing_controller_, this);
-  categories_[file_writer_->second] = { file_writer_categories_.begin(),
-                                        file_writer_categories_.end() };
+  categories_[file_writer_.id_] = { file_writer_categories_.begin(),
+                                    file_writer_categories_.end() };
 }
 
-TraceConfig* Agent::CreateTraceConfig() {
+TraceConfig* Agent::CreateTraceConfig() const {
   if (categories_.empty())
     return nullptr;
   TraceConfig* trace_config = new TraceConfig();
@@ -165,9 +159,9 @@ TraceConfig* Agent::CreateTraceConfig() {
   return trace_config;
 }
 
-std::string Agent::GetEnabledCategories() {
+std::string Agent::GetEnabledCategories() const {
   std::string categories;
-  for (const auto& category : flatten(categories_)) {
+  for (const std::string& category : flatten(categories_)) {
     if (!categories.empty())
       categories += ',';
     categories += category;

src/tracing/agent.h

@@ -4,6 +4,7 @@
 #include "libplatform/v8-tracing.h"
 #include "uv.h"
 #include "v8.h"
+#include "util.h"
 
 #include <set>
 #include <string>
@@ -15,6 +16,8 @@ namespace tracing {
 using v8::platform::tracing::TraceConfig;
 using v8::platform::tracing::TraceObject;
 
+class Agent;
+
 class AsyncTraceWriter {
  public:
   virtual ~AsyncTraceWriter() {}
@@ -31,42 +34,58 @@ class TracingController : public v8::platform::tracing::TracingController {
   }
 };
 
+class AgentWriterHandle {
+ public:
+  inline AgentWriterHandle() {}
+  inline ~AgentWriterHandle() { reset(); }
+
+  inline AgentWriterHandle(AgentWriterHandle&& other);
+  inline AgentWriterHandle& operator=(AgentWriterHandle&& other);
+  inline bool empty() const { return agent_ == nullptr; }
+  inline void reset();
+
+ private:
+  inline AgentWriterHandle(Agent* agent, int id) : agent_(agent), id_(id) {}
+
+  AgentWriterHandle(const AgentWriterHandle& other) = delete;
+  AgentWriterHandle& operator=(const AgentWriterHandle& other) = delete;
+
+  Agent* agent_ = nullptr;
+  int id_;
+
+  friend class Agent;
+};
 
 class Agent {
  public:
-  // Resetting the pointer disconnects client
-  using ClientHandle = std::unique_ptr<std::pair<Agent*, int>,
-                                       void (*)(std::pair<Agent*, int>*)>;
-
-  static ClientHandle EmptyClientHandle() {
-    return ClientHandle(nullptr, DisconnectClient);
-  }
   explicit Agent(const std::string& log_file_pattern);
   void Stop();
 
   TracingController* GetTracingController() { return tracing_controller_; }
 
   // Destroying the handle disconnects the client
-  ClientHandle AddClient(const std::set<std::string>& categories,
-                         std::unique_ptr<AsyncTraceWriter> writer);
+  AgentWriterHandle AddClient(const std::set<std::string>& categories,
+                              std::unique_ptr<AsyncTraceWriter> writer);
 
   // These 3 methods operate on a "default" client, e.g. the file writer
   void Enable(const std::string& categories);
   void Enable(const std::set<std::string>& categories);
   void Disable(const std::set<std::string>& categories);
-  std::string GetEnabledCategories();
 
+  // Returns a comma-separated list of enabled categories.
+  std::string GetEnabledCategories() const;
+
+  // Writes to all writers registered through AddClient().
   void AppendTraceEvent(TraceObject* trace_event);
+  // Flushes all writers registered through AddClient().
   void Flush(bool blocking);
 
-  TraceConfig* CreateTraceConfig();
+  TraceConfig* CreateTraceConfig() const;
 
  private:
+  friend class AgentWriterHandle;
+
   static void ThreadCb(void* arg);
-  static void DisconnectClient(std::pair<Agent*, int>* id_agent) {
-    id_agent->first->Disconnect(id_agent->second);
-    delete id_agent;
-  }
 
   void Start();
   void StopTracing();
@@ -77,14 +96,34 @@ class Agent {
   uv_loop_t tracing_loop_;
   bool started_ = false;
 
-  std::unordered_map<int, std::set<std::string>> categories_;
-  TracingController* tracing_controller_ = nullptr;
-  ClientHandle file_writer_;
+  // Each individual Writer has one id.
   int next_writer_id_ = 1;
+  // These maps store the original arguments to AddClient(), by id.
+  std::unordered_map<int, std::set<std::string>> categories_;
   std::unordered_map<int, std::unique_ptr<AsyncTraceWriter>> writers_;
+  TracingController* tracing_controller_ = nullptr;
+  AgentWriterHandle file_writer_;
   std::multiset<std::string> file_writer_categories_;
 };
 
+void AgentWriterHandle::reset() {
+  if (agent_ != nullptr)
+    agent_->Disconnect(id_);
+  agent_ = nullptr;
+}
+
+AgentWriterHandle& AgentWriterHandle::operator=(AgentWriterHandle&& other) {
+  reset();
+  agent_ = other.agent_;
+  id_ = other.id_;
+  other.agent_ = nullptr;
+  return *this;
+}
+
+AgentWriterHandle::AgentWriterHandle(AgentWriterHandle&& other) {
+  *this = std::move(other);
+}
+
 }  // namespace tracing
 }  // namespace node