From daafe6c1953cc50432a20491edeb23d949dd95e2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 16 Jul 2018 23:34:55 +0200 Subject: [PATCH] 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 Reviewed-By: Eugene Ostroukhov Reviewed-By: Ali Ijaz Sheikh --- src/inspector/tracing_agent.cc | 6 +-- src/inspector/tracing_agent.h | 8 +--- src/node.cc | 10 +++-- src/tracing/agent.cc | 46 +++++++++------------ src/tracing/agent.h | 75 ++++++++++++++++++++++++++-------- 5 files changed, 87 insertions(+), 58 deletions(-) diff --git a/src/inspector/tracing_agent.cc b/src/inspector/tracing_agent.cc index c0425aab29d4a0..92c3597590dabf 100644 --- a/src/inspector/tracing_agent.cc +++ b/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 traceConfig) { - if (trace_writer_ != nullptr) { + if (!trace_writer_.empty()) { return DispatchResponse::Error( "Call NodeTracing::end to stop tracing before updating the config"); } diff --git a/src/inspector/tracing_agent.h b/src/inspector/tracing_agent.h index 478107c5acdd64..029fce7c191b42 100644 --- a/src/inspector/tracing_agent.h +++ b/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, - void (*)(std::pair*)> trace_writer_; + tracing::AgentWriterHandle trace_writer_; std::unique_ptr frontend_; }; diff --git a/src/node.cc b/src/node.cc index 138595ba86834c..f5be3f7954d53d 100644 --- a/src/node.cc +++ b/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::AgentWriterHandle tracing_file_writer_; NodePlatform* platform_; #else // !NODE_USE_V8_PLATFORM void Initialize(int thread_pool_size) {} diff --git a/src/tracing/agent.cc b/src/tracing/agent.cc index a3ddfb61a95328..9cc21863e3ed42 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(arg); + uv_run(&agent->tracing_loop_, UV_RUN_DEFAULT); + }, this)); started_ = true; } -Agent::ClientHandle Agent::AddClient(const std::set& categories, - std::unique_ptr writer) { +AgentWriterHandle Agent::AddClient( + const std::set& categories, + std::unique_ptr 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(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(arg); - uv_run(&agent->tracing_loop_, UV_RUN_DEFAULT); -} - void Agent::Enable(const std::string& categories) { if (categories.empty()) return; std::set 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& 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 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 writer( @@ -138,24 +132,24 @@ void Agent::Enable(const std::set& 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& 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 fd7984275969ba..022e86f11afba9 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 #include @@ -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, - void (*)(std::pair*)>; - - 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& categories, - std::unique_ptr writer); + AgentWriterHandle AddClient(const std::set& categories, + std::unique_ptr 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& categories); void Disable(const std::set& 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* 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> 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> categories_; std::unordered_map> writers_; + TracingController* tracing_controller_ = nullptr; + AgentWriterHandle file_writer_; std::multiset 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