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: https://github.com/nodejs/node/pull/21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

2 files changed, 77 insertions, 44 deletions

diff --git a/src/tracing/agent.cc b/src/tracing/agent.cc
index a3ddfb61a9..9cc21863e3 100644
--- a/src/tracing/agent.cc
+++ b/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,36 +104,27 @@ 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(
@@ -138,24 +132,24 @@ void Agent::Enable(const std::set<std::string>& categories) {
     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;
diff --git a/src/tracing/agent.h b/src/tracing/agent.h
index fd79842759..022e86f11a 100644
--- a/src/tracing/agent.h
+++ b/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