From cc7f30292f8e4b7e48e38428f4590a4a366f37d6 Mon Sep 17 00:00:00 2001 From: Santiago Gimeno Date: Thu, 22 Dec 2016 20:29:27 +0100 Subject: [PATCH 1/7] tracing: fix -Wreorder warning Initialize `InternalTraceBuffer::id_` the last. PR-URL: https://github.com/nodejs/node/pull/10416 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig --- src/tracing/node_trace_buffer.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/tracing/node_trace_buffer.cc b/src/tracing/node_trace_buffer.cc index 4773e08325a3..c1aa569a4473 100644 --- a/src/tracing/node_trace_buffer.cc +++ b/src/tracing/node_trace_buffer.cc @@ -5,8 +5,9 @@ namespace tracing { InternalTraceBuffer::InternalTraceBuffer(size_t max_chunks, uint32_t id, NodeTraceWriter* trace_writer, NodeTraceBuffer* external_buffer) - : id_(id), flushing_(false), max_chunks_(max_chunks), - trace_writer_(trace_writer), external_buffer_(external_buffer) { + : flushing_(false), max_chunks_(max_chunks), + trace_writer_(trace_writer), external_buffer_(external_buffer), + id_(id) { chunks_.resize(max_chunks); } From 4730c42091ac49c3bddd3fa0d676565003a63504 Mon Sep 17 00:00:00 2001 From: Santiago Gimeno Date: Thu, 22 Dec 2016 20:35:35 +0100 Subject: [PATCH 2/7] tracing: fix -Wunused-private-field warning Remove `external_buffer_` from `InternalTraceBuffer` as it seems not to be used anywhere. PR-URL: https://github.com/nodejs/node/pull/10416 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig --- src/tracing/node_trace_buffer.cc | 9 ++++----- src/tracing/node_trace_buffer.h | 4 +--- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/tracing/node_trace_buffer.cc b/src/tracing/node_trace_buffer.cc index c1aa569a4473..4e99361cbefe 100644 --- a/src/tracing/node_trace_buffer.cc +++ b/src/tracing/node_trace_buffer.cc @@ -4,10 +4,9 @@ namespace node { namespace tracing { InternalTraceBuffer::InternalTraceBuffer(size_t max_chunks, uint32_t id, - NodeTraceWriter* trace_writer, NodeTraceBuffer* external_buffer) + NodeTraceWriter* trace_writer) : flushing_(false), max_chunks_(max_chunks), - trace_writer_(trace_writer), external_buffer_(external_buffer), - id_(id) { + trace_writer_(trace_writer), id_(id) { chunks_.resize(max_chunks); } @@ -90,8 +89,8 @@ void InternalTraceBuffer::ExtractHandle( NodeTraceBuffer::NodeTraceBuffer(size_t max_chunks, NodeTraceWriter* trace_writer, uv_loop_t* tracing_loop) : tracing_loop_(tracing_loop), trace_writer_(trace_writer), - buffer1_(max_chunks, 0, trace_writer, this), - buffer2_(max_chunks, 1, trace_writer, this) { + buffer1_(max_chunks, 0, trace_writer), + buffer2_(max_chunks, 1, trace_writer) { current_buf_.store(&buffer1_); flush_signal_.data = this; diff --git a/src/tracing/node_trace_buffer.h b/src/tracing/node_trace_buffer.h index 619799fdb219..296c958b80e3 100644 --- a/src/tracing/node_trace_buffer.h +++ b/src/tracing/node_trace_buffer.h @@ -20,8 +20,7 @@ class NodeTraceBuffer; class InternalTraceBuffer { public: InternalTraceBuffer(size_t max_chunks, uint32_t id, - NodeTraceWriter* trace_writer, - NodeTraceBuffer* external_buffer); + NodeTraceWriter* trace_writer); TraceObject* AddTraceEvent(uint64_t* handle); TraceObject* GetEventByHandle(uint64_t handle); @@ -44,7 +43,6 @@ class InternalTraceBuffer { bool flushing_; size_t max_chunks_; NodeTraceWriter* trace_writer_; - NodeTraceBuffer* external_buffer_; std::vector> chunks_; size_t total_chunks_ = 0; uint32_t current_chunk_seq_ = 1; From 7fdeeb16125a0eb338746e4b8867fb1e9bcd95aa Mon Sep 17 00:00:00 2001 From: Matt Loring Date: Wed, 28 Dec 2016 14:37:47 -0800 Subject: [PATCH 3/7] src: always initialize tracing controller in agent PR-URL: https://github.com/nodejs/node/pull/10507 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- src/tracing/agent.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tracing/agent.h b/src/tracing/agent.h index 098f955192e0..f6cb5af97f4b 100644 --- a/src/tracing/agent.h +++ b/src/tracing/agent.h @@ -22,7 +22,7 @@ class Agent { uv_thread_t thread_; uv_loop_t tracing_loop_; v8::Platform* platform_ = nullptr; - TracingController* tracing_controller_; + TracingController* tracing_controller_ = nullptr; }; } // namespace tracing From 43f55afa82df984c4cd9c09be8a5843556ca0955 Mon Sep 17 00:00:00 2001 From: Jason Ginchereau Date: Wed, 4 Jan 2017 15:21:26 -0800 Subject: [PATCH 4/7] src: fix TracingController cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes an incorrect deletion of the `TracingController` instance, which in some environments could cause an error about an invalid pointer passed to `free()`. The `TracingController` instance is actually owned by a `unique_ptr` member of the platform, so calling `platform::SetTracingController(nullptr)` is the correct way to delete it. But before that, the `TraceBuffer` must be deleted in order for the tracing loop to exit; that is accomplished by calling `TracingController::Initialize(nullptr)`. PR-URL: https://github.com/nodejs/node/pull/10623 Reviewed-By: Matthew Loring Reviewed-By: Michaël Zasso --- src/tracing/agent.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/tracing/agent.cc b/src/tracing/agent.cc index 97a3e11a2c45..ceab09e5a278 100644 --- a/src/tracing/agent.cc +++ b/src/tracing/agent.cc @@ -56,7 +56,9 @@ void Agent::Stop() { // Perform final Flush on TraceBuffer. We don't want the tracing controller // to flush the buffer again on destruction of the V8::Platform. tracing_controller_->StopTracing(); - delete tracing_controller_; + tracing_controller_->Initialize(nullptr); + tracing_controller_ = nullptr; + // Thread should finish when the tracing loop is stopped. uv_thread_join(&thread_); v8::platform::SetTracingController(platform_, nullptr); From 0eaad57bdfcb36cb3820a5930d4ee3dbd40cddf4 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Tue, 17 Jan 2017 08:12:08 +0100 Subject: [PATCH 5/7] build: add missing src/tracing header files I noticed that only one header from src/tracing is included in the sources list in node.gyp. Not sure if this is intentional or not so I wanted to bring it up just in case this was overlooked. PR-URL: https://github.com/nodejs/node/pull/10851 Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Italo A. Casas --- node.gyp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/node.gyp b/node.gyp index 20167458d4c5..c12fb4c0cf79 100644 --- a/node.gyp +++ b/node.gyp @@ -234,6 +234,9 @@ 'src/stream_base.h', 'src/stream_base-inl.h', 'src/stream_wrap.h', + 'src/tracing/agent.h', + 'src/tracing/node_trace_buffer.h', + 'src/tracing/node_trace_writer.h', 'src/tracing/trace_event.h' 'src/tree.h', 'src/util.h', From 70a34da4712a192ba5aa8b8cedd6208ed92dfbae Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 23 Jan 2017 05:53:23 +0100 Subject: [PATCH 6/7] src: move trace_event.h include to internal header Move the include from src/node.h to src/node_internals.h. trace_event.h is not shipped in binary-only and headers-only tarballs, making it currently impossible to build add-ons against them. PR-URL: https://github.com/nodejs/node/pull/10959 Refs: https://github.com/nodejs/citgm/pull/226#issuecomment-274066280 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Matthew Loring Reviewed-By: Richard Lau Reviewed-By: Stephen Belanger --- src/node.h | 1 - src/node_internals.h | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node.h b/src/node.h index 24ebce87c8fd..1255a4af7f11 100644 --- a/src/node.h +++ b/src/node.h @@ -41,7 +41,6 @@ #include "v8.h" // NOLINT(build/include_order) #include "node_version.h" // NODE_MODULE_VERSION -#include "tracing/trace_event.h" #define NODE_MAKE_VERSION(major, minor, patch) \ ((major) * 0x1000 + (minor) * 0x100 + (patch)) diff --git a/src/node_internals.h b/src/node_internals.h index b68594162b8a..156d934d0ec0 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -8,6 +8,7 @@ #include "util-inl.h" #include "uv.h" #include "v8.h" +#include "tracing/trace_event.h" #include #include From 0842c60abde6155a40bc258efc40ae785ad37a0d Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 28 Jan 2017 21:47:14 -0800 Subject: [PATCH 7/7] test: increase strictness for test-trace-event Change test-trace-event such that it checks that all expected values are within the same trace object rather than scattered across multiple trace objects. PR-URL: https://github.com/nodejs/node/pull/11065 Reviewed-By: Anna Henningsen Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig Reviewed-By: Evan Lucas Reviewed-By: James M Snell --- test/parallel/test-trace-event.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-trace-event.js b/test/parallel/test-trace-event.js index 187347058344..fde718d7bc06 100644 --- a/test/parallel/test-trace-event.js +++ b/test/parallel/test-trace-event.js @@ -21,15 +21,19 @@ proc_no_categories.once('exit', common.mustCall(() => { proc.once('exit', common.mustCall(() => { assert(common.fileExists(FILE_NAME)); - fs.readFile(FILE_NAME, (err, data) => { + fs.readFile(FILE_NAME, common.mustCall((err, data) => { const traces = JSON.parse(data.toString()).traceEvents; assert(traces.length > 0); // Values that should be present on all runs to approximate correctness. - assert(traces.some((trace) => { return trace.pid === proc.pid; })); - assert(traces.some((trace) => { return trace.cat === 'v8'; })); assert(traces.some((trace) => { - return trace.name === 'V8.ScriptCompiler'; + if (trace.pid !== proc.pid) + return false; + if (trace.cat !== 'v8') + return false; + if (trace.name !== 'V8.ScriptCompiler') + return false; + return true; })); - }); + })); })); }));