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>

Author: addaleax

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