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 <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

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()){
 returnDispatchResponse::Error(
 "Call NodeTracing::end to stop tracing before updating the config");
  }

src/inspector/tracing_agent.h

@@ -2,16 +2,13 @@
 #defineSRC_INSPECTOR_TRACING_AGENT_H_
 
 #include"node/inspector/protocol/NodeTracing.h"
+#include"tracing/agent.h"
 #include"v8.h"
 
 
 namespacenode{
 classEnvironment;
 
-namespacetracing{
-classAgent;
-} // namespace tracing
-
 namespaceinspector{
 namespaceprotocol{
 
@@ -32,8 +29,7 @@ class TracingAgent : public NodeTracing::Backend{
 voidDisconnectTraceClient();
 
  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 :
 staticstruct{
 #if NODE_USE_V8_PLATFORM
 voidInitialize(int thread_pool_size){
- tracing_agent_.reset(newtracing::Agent(trace_file_pattern));
+ tracing_agent_.reset(newtracing::Agent());
 auto controller = tracing_agent_->GetTracingController();
  controller->AddTraceStateObserver(newNodeTraceStateObserver(controller));
 tracing::TraceEventHelper::SetTracingController(controller);
@@ -427,12 +427,13 @@ static struct{
 #endif// HAVE_INSPECTOR
 
 voidStartTracingAgent(){
- tracing_agent_->Enable(trace_enabled_categories);
+ tracing_file_writer_ = tracing_agent_->AddClient(
+ trace_enabled_categories,
+newtracing::NodeTraceWriter(trace_file_pattern));
  }
 
 voidStopTracingAgent(){
-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
 voidInitialize(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_ = newTracingController();
  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);
-returnClientHandle(client_id, &DisconnectClient);
+returnAgentWriterHandle(this, id);
 }
 
 voidAgent::Stop(){
@@ -101,61 +104,52 @@ void Agent::Disconnect(int client){
  categories_.erase(client);
 }
 
-// static
-voidAgent::ThreadCb(void* arg){
- Agent* agent = static_cast<Agent*>(arg);
-uv_run(&agent->tracing_loop_, UV_RUN_DEFAULT);
-}
-
 voidAgent::Enable(const std::string& categories){
 if (categories.empty())
 return;
  std::set<std::string> categories_set;
- std::stringstreamcategory_list(categories);
+ std::istringstreamcategory_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);
 }
 
 voidAgent::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(
 newNodeTraceWriter(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;
  }
 }
 
 voidAgent::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())
 returnnullptr;
  TraceConfig* trace_config = newTraceConfig();
@@ -165,9 +159,9 @@ TraceConfig* Agent::CreateTraceConfig(){
 return trace_config;
 }
 
-std::string Agent::GetEnabledCategories(){
+std::string Agent::GetEnabledCategories() const{
  std::string categories;
-for (constauto& category : flatten(categories_)){
+for (conststd::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;
 
+classAgent;
+
 classAsyncTraceWriter{
 public:
 virtual~AsyncTraceWriter(){}
@@ -31,42 +34,58 @@ class TracingController : public v8::platform::tracing::TracingController{
  }
 };
 
+classAgentWriterHandle{
+public:
+inlineAgentWriterHandle(){}
+inline~AgentWriterHandle(){reset()}
+
+inlineAgentWriterHandle(AgentWriterHandle&& other);
+inline AgentWriterHandle& operator=(AgentWriterHandle&& other);
+inlineboolempty() const{return agent_ == nullptr}
+inlinevoidreset();
+
+private:
+inlineAgentWriterHandle(Agent* agent, int id) : agent_(agent), id_(id){}
+
+AgentWriterHandle(const AgentWriterHandle& other) = delete;
+ AgentWriterHandle& operator=(const AgentWriterHandle& other) = delete;
+
+ Agent* agent_ = nullptr;
+int id_;
+
+friendclassAgent;
+};
 
 classAgent{
 public:
-// Resetting the pointer disconnects client
-using ClientHandle = std::unique_ptr<std::pair<Agent*, int>,
-void (*)(std::pair<Agent*, int>*)>
-
-static ClientHandle EmptyClientHandle(){
-returnClientHandle(nullptr, DisconnectClient);
- }
 explicitAgent(const std::string& log_file_pattern);
 voidStop();
 
  TracingController* GetTracingController(){return tracing_controller_}
 
 // Destroying the handle disconnects the client
-ClientHandleAddClient(const std::set<std::string>& categories,
- std::unique_ptr<AsyncTraceWriter> writer);
+AgentWriterHandleAddClient(const std::set<std::string>& categories,
+std::unique_ptr<AsyncTraceWriter> writer);
 
 // These 3 methods operate on a "default" client, e.g. the file writer
 voidEnable(const std::string& categories);
 voidEnable(const std::set<std::string>& categories);
 voidDisable(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().
 voidAppendTraceEvent(TraceObject* trace_event);
+// Flushes all writers registered through AddClient().
 voidFlush(bool blocking);
 
- TraceConfig* CreateTraceConfig();
+ TraceConfig* CreateTraceConfig()const;
 
 private:
+friendclassAgentWriterHandle;
+
 staticvoidThreadCb(void* arg);
-staticvoidDisconnectClient(std::pair<Agent*, int>* id_agent){
- id_agent->first->Disconnect(id_agent->second);
-delete id_agent;
- }
 
 voidStart();
 voidStopTracing();
@@ -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_;
 };
 
+voidAgentWriterHandle::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