From c101b396aacb9afed790ab2689ddff16774dd5f8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 17 Jul 2018 02:50:07 +0200 Subject: [PATCH] src: refactor default trace writer out of agent The agent code is supposed to manage multiple writers/clients. Adding the default writer into the mix breaks that encapsulation. Instead, manage default options through a separate "virtual" default client handle, and keep the file writer management all to the actual users. PR-URL: https://github.com/nodejs/node/pull/21867 Reviewed-By: James M Snell Reviewed-By: Eugene Ostroukhov Reviewed-By: Ali Ijaz Sheikh --- src/env-inl.h | 4 +- src/env.cc | 4 +- src/env.h | 8 +-- src/inspector/tracing_agent.cc | 5 +- src/node.cc | 26 ++++++--- src/node_trace_events.cc | 9 ++-- src/tracing/agent.cc | 97 +++++++++++++++------------------- src/tracing/agent.h | 47 +++++++++++----- src/util.cc | 14 +++++ src/util.h | 3 ++ 10 files changed, 130 insertions(+), 87 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index bc81400403bb16..542cfb6400c0ee 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -333,8 +333,8 @@ inline v8::Isolate* Environment::isolate() const { return isolate_; } -inline tracing::Agent* Environment::tracing_agent() const { - return tracing_agent_; +inline tracing::AgentWriterHandle* Environment::tracing_agent_writer() const { + return tracing_agent_writer_; } inline Environment* Environment::from_immediate_check_handle( diff --git a/src/env.cc b/src/env.cc index 76d1b6dd86d43e..bea42aa10774f8 100644 --- a/src/env.cc +++ b/src/env.cc @@ -103,10 +103,10 @@ void InitThreadLocalOnce() { Environment::Environment(IsolateData* isolate_data, Local context, - tracing::Agent* tracing_agent) + tracing::AgentWriterHandle* tracing_agent_writer) : isolate_(context->GetIsolate()), isolate_data_(isolate_data), - tracing_agent_(tracing_agent), + tracing_agent_writer_(tracing_agent_writer), immediate_info_(context->GetIsolate()), tick_info_(context->GetIsolate()), timer_base_(uv_now(isolate_data->event_loop())), diff --git a/src/env.h b/src/env.h index bd29e006e3c256..73b1c8bd72558d 100644 --- a/src/env.h +++ b/src/env.h @@ -55,7 +55,7 @@ class performance_state; } namespace tracing { -class Agent; +class AgentWriterHandle; } namespace worker { @@ -590,7 +590,7 @@ class Environment { Environment(IsolateData* isolate_data, v8::Local context, - tracing::Agent* tracing_agent); + tracing::AgentWriterHandle* tracing_agent_writer); ~Environment(); void Start(int argc, @@ -628,7 +628,7 @@ class Environment { inline bool profiler_idle_notifier_started() const; inline v8::Isolate* isolate() const; - inline tracing::Agent* tracing_agent() const; + inline tracing::AgentWriterHandle* tracing_agent_writer() const; inline uv_loop_t* event_loop() const; inline uint32_t watched_providers() const; @@ -877,7 +877,7 @@ class Environment { v8::Isolate* const isolate_; IsolateData* const isolate_data_; - tracing::Agent* const tracing_agent_; + tracing::AgentWriterHandle* const tracing_agent_writer_; uv_check_t immediate_check_handle_; uv_idle_t immediate_idle_handle_; uv_prepare_t idle_prepare_handle_; diff --git a/src/inspector/tracing_agent.cc b/src/inspector/tracing_agent.cc index 92c3597590dabf..6e962b289ab36f 100644 --- a/src/inspector/tracing_agent.cc +++ b/src/inspector/tracing_agent.cc @@ -74,10 +74,11 @@ DispatchResponse TracingAgent::start( if (categories_set.empty()) return DispatchResponse::Error("At least one category should be enabled"); - trace_writer_ = env_->tracing_agent()->AddClient( + trace_writer_ = env_->tracing_agent_writer()->agent()->AddClient( categories_set, std::unique_ptr( - new InspectorTraceWriter(frontend_.get()))); + new InspectorTraceWriter(frontend_.get())), + tracing::Agent::kIgnoreDefaultCategories); return DispatchResponse::OK(); } diff --git a/src/node.cc b/src/node.cc index f5be3f7954d53d..784dd157f14314 100644 --- a/src/node.cc +++ b/src/node.cc @@ -61,6 +61,7 @@ #include "req_wrap-inl.h" #include "string_bytes.h" #include "tracing/agent.h" +#include "tracing/node_trace_writer.h" #include "util.h" #include "uv.h" #if NODE_USE_V8_PLATFORM @@ -427,17 +428,24 @@ static struct { #endif // HAVE_INSPECTOR void StartTracingAgent() { - tracing_file_writer_ = tracing_agent_->AddClient( - trace_enabled_categories, - new tracing::NodeTraceWriter(trace_file_pattern)); + if (trace_enabled_categories.empty()) { + tracing_file_writer_ = tracing_agent_->DefaultHandle(); + } else { + tracing_file_writer_ = tracing_agent_->AddClient( + ParseCommaSeparatedSet(trace_enabled_categories), + std::unique_ptr( + new tracing::NodeTraceWriter(trace_file_pattern, + tracing_agent_->loop())), + tracing::Agent::kUseDefaultCategories); + } } void StopTracingAgent() { tracing_file_writer_.reset(); } - tracing::Agent* GetTracingAgent() const { - return tracing_agent_.get(); + tracing::AgentWriterHandle* GetTracingAgentWriter() { + return &tracing_file_writer_; } NodePlatform* Platform() { @@ -466,7 +474,9 @@ static struct { } void StopTracingAgent() {} - tracing::Agent* GetTracingAgent() const { return nullptr; } + tracing::AgentWriterHandle* GetTracingAgentWriter() { + return nullptr; + } NodePlatform* Platform() { return nullptr; @@ -3593,7 +3603,7 @@ Environment* CreateEnvironment(IsolateData* isolate_data, HandleScope handle_scope(isolate); Context::Scope context_scope(context); auto env = new Environment(isolate_data, context, - v8_platform.GetTracingAgent()); + v8_platform.GetTracingAgentWriter()); env->Start(argc, argv, exec_argc, exec_argv, v8_is_profiling); return env; } @@ -3652,7 +3662,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, HandleScope handle_scope(isolate); Local context = NewContext(isolate); Context::Scope context_scope(context); - Environment env(isolate_data, context, v8_platform.GetTracingAgent()); + Environment env(isolate_data, context, v8_platform.GetTracingAgentWriter()); env.Start(argc, argv, exec_argc, exec_argv, v8_is_profiling); const char* path = argc > 1 ? argv[1] : nullptr; diff --git a/src/node_trace_events.cc b/src/node_trace_events.cc index 06063a449b93d8..d59b92555795fb 100644 --- a/src/node_trace_events.cc +++ b/src/node_trace_events.cc @@ -58,7 +58,7 @@ void NodeCategorySet::New(const FunctionCallbackInfo& args) { if (!*val) return; categories.emplace(*val); } - CHECK_NOT_NULL(env->tracing_agent()); + CHECK_NOT_NULL(env->tracing_agent_writer()); new NodeCategorySet(env, args.This(), std::move(categories)); } @@ -69,7 +69,7 @@ void NodeCategorySet::Enable(const FunctionCallbackInfo& args) { CHECK_NOT_NULL(category_set); const auto& categories = category_set->GetCategories(); if (!category_set->enabled_ && !categories.empty()) { - env->tracing_agent()->Enable(categories); + env->tracing_agent_writer()->Enable(categories); category_set->enabled_ = true; } } @@ -81,14 +81,15 @@ void NodeCategorySet::Disable(const FunctionCallbackInfo& args) { CHECK_NOT_NULL(category_set); const auto& categories = category_set->GetCategories(); if (category_set->enabled_ && !categories.empty()) { - env->tracing_agent()->Disable(categories); + env->tracing_agent_writer()->Disable(categories); category_set->enabled_ = false; } } void GetEnabledCategories(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - std::string categories = env->tracing_agent()->GetEnabledCategories(); + std::string categories = + env->tracing_agent_writer()->agent()->GetEnabledCategories(); if (!categories.empty()) { args.GetReturnValue().Set( String::NewFromUtf8(env->isolate(), diff --git a/src/tracing/agent.cc b/src/tracing/agent.cc index 9cc21863e3ed42..641c476c1d8df7 100644 --- a/src/tracing/agent.cc +++ b/src/tracing/agent.cc @@ -1,23 +1,24 @@ #include "tracing/agent.h" -#include #include #include "tracing/node_trace_buffer.h" -#include "tracing/node_trace_writer.h" namespace node { namespace tracing { -namespace { - -class ScopedSuspendTracing { +class Agent::ScopedSuspendTracing { public: - ScopedSuspendTracing(TracingController* controller, Agent* agent) - : controller_(controller), agent_(agent) { - controller->StopTracing(); + ScopedSuspendTracing(TracingController* controller, Agent* agent, + bool do_suspend = true) + : controller_(controller), agent_(do_suspend ? agent : nullptr) { + if (do_suspend) { + CHECK(agent_->started_); + controller->StopTracing(); + } } ~ScopedSuspendTracing() { + if (agent_ == nullptr) return; TraceConfig* config = agent_->CreateTraceConfig(); if (config != nullptr) { controller_->StartTracing(config); @@ -29,8 +30,10 @@ class ScopedSuspendTracing { Agent* agent_; }; +namespace { + std::set flatten( - const std::unordered_map>& map) { + const std::unordered_map>& map) { std::set result; for (const auto& id_value : map) result.insert(id_value.second.begin(), id_value.second.end()); @@ -43,18 +46,17 @@ using v8::platform::tracing::TraceConfig; using v8::platform::tracing::TraceWriter; using std::string; -Agent::Agent(const std::string& log_file_pattern) - : log_file_pattern_(log_file_pattern) { +Agent::Agent() { tracing_controller_ = new TracingController(); tracing_controller_->Initialize(nullptr); + + CHECK_EQ(uv_loop_init(&tracing_loop_), 0); } void Agent::Start() { if (started_) return; - CHECK_EQ(uv_loop_init(&tracing_loop_), 0); - NodeTraceBuffer* trace_buffer_ = new NodeTraceBuffer( NodeTraceBuffer::kBufferChunks, this, &tracing_loop_); tracing_controller_->Initialize(trace_buffer_); @@ -71,18 +73,30 @@ void Agent::Start() { AgentWriterHandle Agent::AddClient( const std::set& categories, - std::unique_ptr writer) { + std::unique_ptr writer, + enum UseDefaultCategoryMode mode) { Start(); + + const std::set* use_categories = &categories; + + std::set categories_with_default; + if (mode == kUseDefaultCategories) { + categories_with_default.insert(categories.begin(), categories.end()); + categories_with_default.insert(categories_[kDefaultHandleId].begin(), + categories_[kDefaultHandleId].end()); + use_categories = &categories_with_default; + } + ScopedSuspendTracing suspend(tracing_controller_, this); int id = next_writer_id_++; writers_[id] = std::move(writer); - categories_[id] = categories; + categories_[id] = { use_categories->begin(), use_categories->end() }; return AgentWriterHandle(this, id); } -void Agent::Stop() { - file_writer_.reset(); +AgentWriterHandle Agent::DefaultHandle() { + return AgentWriterHandle(this, kDefaultHandleId); } void Agent::StopTracing() { @@ -99,54 +113,30 @@ void Agent::StopTracing() { } void Agent::Disconnect(int client) { + if (client == kDefaultHandleId) return; ScopedSuspendTracing suspend(tracing_controller_, this); writers_.erase(client); categories_.erase(client); } -void Agent::Enable(const std::string& categories) { +void Agent::Enable(int id, const std::set& categories) { if (categories.empty()) return; - std::set categories_set; - std::istringstream category_list(categories); - while (category_list.good()) { - std::string category; - getline(category_list, category, ','); - categories_set.emplace(std::move(category)); - } - Enable(categories_set); -} -void Agent::Enable(const std::set& categories) { - 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_.empty()) { - // Ensure background thread is running - Start(); - std::unique_ptr 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_.id_] = full_list; - } + ScopedSuspendTracing suspend(tracing_controller_, this, + id != kDefaultHandleId); + categories_[id].insert(categories.begin(), categories.end()); } -void Agent::Disable(const std::set& categories) { +void Agent::Disable(int id, const std::set& categories) { + ScopedSuspendTracing suspend(tracing_controller_, this, + id != kDefaultHandleId); + std::multiset& writer_categories = categories_[id]; for (const std::string& category : categories) { - auto it = file_writer_categories_.find(category); - if (it != file_writer_categories_.end()) - file_writer_categories_.erase(it); + auto it = writer_categories.find(category); + if (it != writer_categories.end()) + writer_categories.erase(it); } - if (file_writer_.empty()) - return; - ScopedSuspendTracing suspend(tracing_controller_, this); - categories_[file_writer_.id_] = { file_writer_categories_.begin(), - file_writer_categories_.end() }; } TraceConfig* Agent::CreateTraceConfig() const { @@ -178,5 +168,6 @@ void Agent::Flush(bool blocking) { for (const auto& id_writer : writers_) id_writer.second->Flush(blocking); } + } // namespace tracing } // namespace node diff --git a/src/tracing/agent.h b/src/tracing/agent.h index 022e86f11afba9..b62dc5fe5bc9d5 100644 --- a/src/tracing/agent.h +++ b/src/tracing/agent.h @@ -44,6 +44,11 @@ class AgentWriterHandle { inline bool empty() const { return agent_ == nullptr; } inline void reset(); + inline void Enable(const std::set& categories); + inline void Disable(const std::set& categories); + + inline Agent* agent() { return agent_; } + private: inline AgentWriterHandle(Agent* agent, int id) : agent_(agent), id_(id) {} @@ -58,19 +63,23 @@ class AgentWriterHandle { class Agent { public: - explicit Agent(const std::string& log_file_pattern); - void Stop(); + Agent(); TracingController* GetTracingController() { return tracing_controller_; } + enum UseDefaultCategoryMode { + kUseDefaultCategories, + kIgnoreDefaultCategories + }; + // Destroying the handle disconnects the client 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::unique_ptr writer, + enum UseDefaultCategoryMode mode); + // A handle that is only used for managing the default categories + // (which can then implicitly be used through using `USE_DEFAULT_CATEGORIES` + // when adding a client later). + AgentWriterHandle DefaultHandle(); // Returns a comma-separated list of enabled categories. std::string GetEnabledCategories() const; @@ -82,6 +91,9 @@ class Agent { TraceConfig* CreateTraceConfig() const; + // TODO(addaleax): This design is broken and inherently thread-unsafe. + inline uv_loop_t* loop() { return &tracing_loop_; } + private: friend class AgentWriterHandle; @@ -91,19 +103,22 @@ class Agent { void StopTracing(); void Disconnect(int client); - const std::string& log_file_pattern_; + void Enable(int id, const std::set& categories); + void Disable(int id, const std::set& categories); + uv_thread_t thread_; uv_loop_t tracing_loop_; + bool started_ = false; + class ScopedSuspendTracing; // Each individual Writer has one id. int next_writer_id_ = 1; + enum { kDefaultHandleId = -1 }; // These maps store the original arguments to AddClient(), by id. - std::unordered_map> categories_; + std::unordered_map> categories_; std::unordered_map> writers_; TracingController* tracing_controller_ = nullptr; - AgentWriterHandle file_writer_; - std::multiset file_writer_categories_; }; void AgentWriterHandle::reset() { @@ -124,6 +139,14 @@ AgentWriterHandle::AgentWriterHandle(AgentWriterHandle&& other) { *this = std::move(other); } +void AgentWriterHandle::Enable(const std::set& categories) { + if (agent_ != nullptr) agent_->Enable(id_, categories); +} + +void AgentWriterHandle::Disable(const std::set& categories) { + if (agent_ != nullptr) agent_->Disable(id_, categories); +} + } // namespace tracing } // namespace node diff --git a/src/util.cc b/src/util.cc index 3e808e13fe87d8..66be18eae2d150 100644 --- a/src/util.cc +++ b/src/util.cc @@ -24,6 +24,7 @@ #include "node_internals.h" #include "uv.h" #include +#include namespace node { @@ -118,4 +119,17 @@ void GetHumanReadableProcessName(char (*name)[1024]) { snprintf(*name, sizeof(*name), "%s[%d]", title, uv_os_getpid()); } +std::set ParseCommaSeparatedSet(const std::string& in) { + std::set out; + if (in.empty()) + return out; + std::istringstream in_stream(in); + while (in_stream.good()) { + std::string item; + getline(in_stream, item, ','); + out.emplace(std::move(item)); + } + return out; +} + } // namespace node diff --git a/src/util.h b/src/util.h index f7dcc5ea35abf8..a9fce79ebeaec1 100644 --- a/src/util.h +++ b/src/util.h @@ -36,6 +36,7 @@ #include #include // std::function +#include namespace node { @@ -476,6 +477,8 @@ struct FunctionDeleter { template using DeleteFnPtr = typename FunctionDeleter::Pointer; +std::set ParseCommaSeparatedSet(const std::string& in); + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS