[PATCH] D40489: [clangd] Changed tracing interfaces
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE320708: [clangd] Changed tracing interfaces (authored by
ibiryukov, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D40489?vs=126957&id=126959#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40489
Files:
clangd/Trace.cpp
clangd/Trace.h
Index: clangd/Trace.cpp
===
--- clangd/Trace.cpp
+++ clangd/Trace.cpp
@@ -44,10 +44,24 @@
Out.flush();
}
+ EndEventCallback beginSpan(const Context &Ctx,
+ llvm::StringRef Name) override {
+jsonEvent("B", json::obj{{"name", Name}});
+
+// The callback that will run when event ends.
+return [this](json::Expr &&Args) {
+ jsonEvent("E", json::obj{{"args", std::move(Args)}});
+};
+ }
+
+ void instant(const Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) override {
+jsonEvent("i", json::obj{{"name", Name}, {"args", std::move(Args)}});
+ }
+
// Record an event on the current thread. ph, pid, tid, ts are set.
// Contents must be a list of the other JSON key/values.
- void event(const Context &Ctx, StringRef Phase,
- json::obj &&Contents) override {
+ void jsonEvent(StringRef Phase, json::obj &&Contents) {
uint64_t TID = get_threadid();
std::lock_guard Lock(Mu);
// If we haven't already, emit metadata describing this thread.
@@ -109,30 +123,26 @@
void log(const Context &Ctx, const Twine &Message) {
if (!T)
return;
- T->event(Ctx, "i",
- json::obj{
- {"name", "Log"},
- {"args", json::obj{{"Message", Message.str()}}},
- });
+ T->instant(Ctx, "Log", json::obj{{"Message", Message.str()}});
}
-Span::Span(const Context &Ctx, std::string Name) {
+Span::Span(const Context &Ctx, llvm::StringRef Name) {
if (!T)
return;
- // Clone the context, so that the original Context can be moved.
- this->Ctx.emplace(Ctx.clone());
- T->event(*this->Ctx, "B", json::obj{{"name", std::move(Name)}});
+ Callback = T->beginSpan(Ctx, Name);
+ if (!Callback)
+return;
+
Args = llvm::make_unique();
}
Span::~Span() {
- if (!T)
+ if (!Callback)
return;
- if (!Args)
-Args = llvm::make_unique();
- T->event(*Ctx, "E",
- Args ? json::obj{{"args", std::move(*Args)}} : json::obj{});
+
+ assert(Args && "Args must be non-null if Callback is defined");
+ Callback(std::move(*Args));
}
} // namespace trace
Index: clangd/Trace.h
===
--- clangd/Trace.h
+++ clangd/Trace.h
@@ -19,6 +19,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TRACE_H_
#include "Context.h"
+#include "Function.h"
#include "JSONExpr.h"
#include "llvm/ADT/Twine.h"
#include "llvm/Support/raw_ostream.h"
@@ -28,12 +29,24 @@
namespace trace {
/// A consumer of trace events. The events are produced by Spans and trace::log.
+/// Implmentations of this interface must be thread-safe.
class EventTracer {
public:
+ /// A callback executed when an event with duration ends. Args represent data
+ /// that was attached to the event via SPAN_ATTACH.
+ using EndEventCallback = UniqueFunction;
+
virtual ~EventTracer() = default;
- /// Consume a trace event.
- virtual void event(const Context &Ctx, llvm::StringRef Phase,
- json::obj &&Contents) = 0;
+
+ /// Called when event that has a duration starts. The returned callback will
+ /// be executed when the event ends. \p Name is a descriptive name
+ /// of the event that was passed to Span constructor.
+ virtual EndEventCallback beginSpan(const Context &Ctx,
+ llvm::StringRef Name) = 0;
+
+ /// Called for instant events.
+ virtual void instant(const Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) = 0;
};
/// Sets up a global EventTracer that consumes events produced by Span and
@@ -50,9 +63,6 @@
///
/// The format is documented here:
/// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview
-///
-/// The implementation supports concurrent calls and can be used as a global
-/// tracer (i.e., can be put into a global Context).
std::unique_ptr createJSONTracer(llvm::raw_ostream &OS,
bool Pretty = false);
@@ -67,16 +77,16 @@
/// SomeJSONExpr is evaluated and copied only if actually needed.
class Span {
public:
- Span(const Context &Ctx, std::string Name);
+ Span(const Context &Ctx, llvm::StringRef Name);
~Span();
/// Returns mutable span metadata if this span is interested.
/// Prefer to use SPAN_ATTACH rather than accessing this directly.
json::obj *args() { return Args.get(); }
private:
- llvm::Optional Ctx;
std::unique_ptr Args;
+ EventTracer::EndEventCallback Callback;
};
#define SPAN_ATTACH(S, Name, E
[PATCH] D40489: [clangd] Changed tracing interfaces
ilya-biryukov updated this revision to Diff 126957.
ilya-biryukov added a comment.
Merged with head
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40489
Files:
clangd/Trace.cpp
clangd/Trace.h
Index: clangd/Trace.h
===
--- clangd/Trace.h
+++ clangd/Trace.h
@@ -19,6 +19,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TRACE_H_
#include "Context.h"
+#include "Function.h"
#include "JSONExpr.h"
#include "llvm/ADT/Twine.h"
#include "llvm/Support/raw_ostream.h"
@@ -28,12 +29,24 @@
namespace trace {
/// A consumer of trace events. The events are produced by Spans and trace::log.
+/// Implmentations of this interface must be thread-safe.
class EventTracer {
public:
+ /// A callback executed when an event with duration ends. Args represent data
+ /// that was attached to the event via SPAN_ATTACH.
+ using EndEventCallback = UniqueFunction;
+
virtual ~EventTracer() = default;
- /// Consume a trace event.
- virtual void event(const Context &Ctx, llvm::StringRef Phase,
- json::obj &&Contents) = 0;
+
+ /// Called when event that has a duration starts. The returned callback will
+ /// be executed when the event ends. \p Name is a descriptive name
+ /// of the event that was passed to Span constructor.
+ virtual EndEventCallback beginSpan(const Context &Ctx,
+ llvm::StringRef Name) = 0;
+
+ /// Called for instant events.
+ virtual void instant(const Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) = 0;
};
/// Sets up a global EventTracer that consumes events produced by Span and
@@ -50,9 +63,6 @@
///
/// The format is documented here:
/// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview
-///
-/// The implementation supports concurrent calls and can be used as a global
-/// tracer (i.e., can be put into a global Context).
std::unique_ptr createJSONTracer(llvm::raw_ostream &OS,
bool Pretty = false);
@@ -67,16 +77,16 @@
/// SomeJSONExpr is evaluated and copied only if actually needed.
class Span {
public:
- Span(const Context &Ctx, std::string Name);
+ Span(const Context &Ctx, llvm::StringRef Name);
~Span();
/// Returns mutable span metadata if this span is interested.
/// Prefer to use SPAN_ATTACH rather than accessing this directly.
json::obj *args() { return Args.get(); }
private:
- llvm::Optional Ctx;
std::unique_ptr Args;
+ EventTracer::EndEventCallback Callback;
};
#define SPAN_ATTACH(S, Name, Expr) \
Index: clangd/Trace.cpp
===
--- clangd/Trace.cpp
+++ clangd/Trace.cpp
@@ -44,10 +44,24 @@
Out.flush();
}
+ EndEventCallback beginSpan(const Context &Ctx,
+ llvm::StringRef Name) override {
+jsonEvent("B", json::obj{{"name", Name}});
+
+// The callback that will run when event ends.
+return [this](json::Expr &&Args) {
+ jsonEvent("E", json::obj{{"args", std::move(Args)}});
+};
+ }
+
+ void instant(const Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) override {
+jsonEvent("i", json::obj{{"name", Name}, {"args", std::move(Args)}});
+ }
+
// Record an event on the current thread. ph, pid, tid, ts are set.
// Contents must be a list of the other JSON key/values.
- void event(const Context &Ctx, StringRef Phase,
- json::obj &&Contents) override {
+ void jsonEvent(StringRef Phase, json::obj &&Contents) {
uint64_t TID = get_threadid();
std::lock_guard Lock(Mu);
// If we haven't already, emit metadata describing this thread.
@@ -109,30 +123,26 @@
void log(const Context &Ctx, const Twine &Message) {
if (!T)
return;
- T->event(Ctx, "i",
- json::obj{
- {"name", "Log"},
- {"args", json::obj{{"Message", Message.str()}}},
- });
+ T->instant(Ctx, "Log", json::obj{{"Message", Message.str()}});
}
-Span::Span(const Context &Ctx, std::string Name) {
+Span::Span(const Context &Ctx, llvm::StringRef Name) {
if (!T)
return;
- // Clone the context, so that the original Context can be moved.
- this->Ctx.emplace(Ctx.clone());
- T->event(*this->Ctx, "B", json::obj{{"name", std::move(Name)}});
+ Callback = T->beginSpan(Ctx, Name);
+ if (!Callback)
+return;
+
Args = llvm::make_unique();
}
Span::~Span() {
- if (!T)
+ if (!Callback)
return;
- if (!Args)
-Args = llvm::make_unique();
- T->event(*Ctx, "E",
- Args ? json::obj{{"args", std::move(*Args)}} : json::obj{});
+
+ assert(Args && "Args must be non-null if Callback is defined");
+ Callback(std::move(*Args));
}
} // namespace trace
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://list
[PATCH] D40489: [clangd] Changed tracing interfaces
ilya-biryukov added inline comments. Comment at: clangd/Trace.cpp:134 + Callback = T->beginSpan(Ctx, Name); + if (!Callback) +return; sammccall wrote: > I'm not sure this is useful. Tracers that aren't interested in args can't opt > out by providing a null callback, unless they also don't care about the end > time. > This seems unlikely (a null tracer, I guess) One use-case that came to mind is turning the tracing on or off on per-request basis Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40489 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40489: [clangd] Changed tracing interfaces
sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clangd/Trace.cpp:134 + Callback = T->beginSpan(Ctx, Name); + if (!Callback) +return; I'm not sure this is useful. Tracers that aren't interested in args can't opt out by providing a null callback, unless they also don't care about the end time. This seems unlikely (a null tracer, I guess) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40489 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40489: [clangd] Changed tracing interfaces
ilya-biryukov added inline comments. Comment at: clangd/Trace.h:39 + /// Called when event with \p Name starts. + virtual void begin_event(const ContextData &Ctx, llvm::StringRef Name) = 0; + /// Called when event with \p Name ends. sammccall wrote: > just call this begin? > Otherwise style is `beginEvent` I think The new name is `beginSpan`. Hope that sounds good. Comment at: clangd/Trace.h:41 + /// Called when event with \p Name ends. + virtual void end_event(const ContextData &Ctx, llvm::StringRef Name, + json::obj &&Args) = 0; sammccall wrote: > How is identity represented between begin/end event? via Name doesn't seem > robust, so why the need to pass it twice? Does providing different contexts > for start/end make sense? > > It might be cleaner/more flexible to have > > std::function begin() > and call the return value to signal end. > > This seems likely to be pretty easy for different providers to implement, and > is easy to use from Span. Done. As discussed offline, the interface you propose seems much nicer. Used names `beginSpan` and `instant`. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40489 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40489: [clangd] Changed tracing interfaces
ilya-biryukov updated this revision to Diff 126762.
ilya-biryukov marked 3 inline comments as done.
ilya-biryukov added a comment.
- Changed the EventTracer interface to return a callback from beginEvent
instead of using begin_event/end_event method pairs.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40489
Files:
clangd/Trace.cpp
clangd/Trace.h
Index: clangd/Trace.h
===
--- clangd/Trace.h
+++ clangd/Trace.h
@@ -19,6 +19,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TRACE_H_
#include "Context.h"
+#include "Function.h"
#include "JSONExpr.h"
#include "llvm/ADT/Twine.h"
#include "llvm/Support/raw_ostream.h"
@@ -28,12 +29,24 @@
namespace trace {
/// A consumer of trace events. The events are produced by Spans and trace::log.
+/// Implmentations of this interface must be thread-safe.
class EventTracer {
public:
+ /// A callback executed when an event with duration ends. Args represent data
+ /// that was attached to the event via SPAN_ATTACH.
+ using EndEventCallback = UniqueFunction;
+
virtual ~EventTracer() = default;
- /// Consume a trace event.
- virtual void event(const Context &Ctx, llvm::StringRef Phase,
- json::obj &&Contents) = 0;
+
+ /// Called when event that has a duration starts. The returned callback will
+ /// be executed when the event ends. \p Name is a descriptive name
+ /// of the event that was passed to Span constructor.
+ virtual EndEventCallback beginSpan(const Context &Ctx,
+ llvm::StringRef Name) = 0;
+
+ /// Called for instant events.
+ virtual void instant(const Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) = 0;
};
/// Sets up a global EventTracer that consumes events produced by Span and
@@ -50,9 +63,6 @@
///
/// The format is documented here:
/// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview
-///
-/// The implementation supports concurrent calls and can be used as a global
-/// tracer (i.e., can be put into a global Context).
std::unique_ptr createJSONTracer(llvm::raw_ostream &OS,
bool Pretty = false);
@@ -67,16 +77,16 @@
/// SomeJSONExpr is evaluated and copied only if actually needed.
class Span {
public:
- Span(const Context &Ctx, std::string Name);
+ Span(const Context &Ctx, llvm::StringRef Name);
~Span();
/// Returns mutable span metadata if this span is interested.
/// Prefer to use SPAN_ATTACH rather than accessing this directly.
json::obj *args() { return Args.get(); }
private:
- llvm::Optional Ctx;
std::unique_ptr Args;
+ EventTracer::EndEventCallback Callback;
};
#define SPAN_ATTACH(S, Name, Expr) \
Index: clangd/Trace.cpp
===
--- clangd/Trace.cpp
+++ clangd/Trace.cpp
@@ -44,10 +44,24 @@
Out.flush();
}
+ EndEventCallback beginSpan(const Context &Ctx,
+ llvm::StringRef Name) override {
+jsonEvent("B", json::obj{{"name", Name}});
+
+// The callback that will run when event ends.
+return [this](json::Expr &&Args) {
+ jsonEvent("E", json::obj{{"args", std::move(Args)}});
+};
+ }
+
+ void instant(const Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) override {
+jsonEvent("i", json::obj{{"name", Name}, {"args", std::move(Args)}});
+ }
+
// Record an event on the current thread. ph, pid, tid, ts are set.
// Contents must be a list of the other JSON key/values.
- void event(const Context &Ctx, StringRef Phase,
- json::obj &&Contents) override {
+ void jsonEvent(StringRef Phase, json::obj &&Contents) {
uint64_t TID = get_threadid();
std::lock_guard Lock(Mu);
// If we haven't already, emit metadata describing this thread.
@@ -109,30 +123,26 @@
void log(const Context &Ctx, const Twine &Message) {
if (!T)
return;
- T->event(Ctx, "i",
- json::obj{
- {"name", "Log"},
- {"args", json::obj{{"Message", Message.str()}}},
- });
+ T->instant(Ctx, "Log", json::obj{{"Message", Message.str()}});
}
-Span::Span(const Context &Ctx, std::string Name) {
+Span::Span(const Context &Ctx, llvm::StringRef Name) {
if (!T)
return;
- // Clone the context, so that the original Context can be moved.
- this->Ctx.emplace(Ctx.clone());
- T->event(*this->Ctx, "B", json::obj{{"name", std::move(Name)}});
+ Callback = T->beginSpan(Ctx, Name);
+ if (!Callback)
+return;
+
Args = llvm::make_unique();
}
Span::~Span() {
- if (!T)
+ if (!Callback)
return;
- if (!Args)
-Args = llvm::make_unique();
- T->event(*Ctx, "E",
- Args ? json::obj{{"args", std::move(*Args)}} : json::obj{});
+
+ assert(Args && "Args must be non-null if Callback is defined");
+ Callback
[PATCH] D40489: [clangd] Changed tracing interfaces
ilya-biryukov updated this revision to Diff 126329.
ilya-biryukov added a comment.
Updated the patch after changes to Context
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40489
Files:
clangd/Trace.cpp
clangd/Trace.h
Index: clangd/Trace.h
===
--- clangd/Trace.h
+++ clangd/Trace.h
@@ -28,12 +28,21 @@
namespace trace {
/// A consumer of trace events. The events are produced by Spans and trace::log.
+/// Calls to begin_event and end_event with the same name are always properly
+/// nested by using a RAII object (Span).
+/// Implmentations of this interface must be thread-safe.
class EventTracer {
public:
virtual ~EventTracer() = default;
- /// Consume a trace event.
- virtual void event(const Context &Ctx, llvm::StringRef Phase,
- json::obj &&Contents) = 0;
+
+ /// Called when event with \p Name starts.
+ virtual void begin_event(const Context &Ctx, llvm::StringRef Name) = 0;
+ /// Called when event with \p Name ends.
+ virtual void end_event(const Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) = 0;
+ /// Called for instant events.
+ virtual void instant_event(const Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) = 0;
};
/// Sets up a global EventTracer that consumes events produced by Span and
@@ -67,15 +76,16 @@
/// SomeJSONExpr is evaluated and copied only if actually needed.
class Span {
public:
- Span(const Context &Ctx, std::string Name);
+ Span(const Context &Ctx, llvm::StringRef Name);
~Span();
/// Returns mutable span metadata if this span is interested.
/// Prefer to use SPAN_ATTACH rather than accessing this directly.
json::obj *args() { return Args.get(); }
private:
llvm::Optional Ctx;
+ std::string Name;
std::unique_ptr Args;
};
Index: clangd/Trace.cpp
===
--- clangd/Trace.cpp
+++ clangd/Trace.cpp
@@ -44,10 +44,23 @@
Out.flush();
}
+ void begin_event(const Context &Ctx, llvm::StringRef Name) override {
+jsonEvent("B", json::obj{{"name", Name}});
+ }
+
+ void end_event(const Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) override {
+jsonEvent("E", json::obj{{"args", std::move(Args)}});
+ }
+
+ void instant_event(const Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) override {
+jsonEvent("i", json::obj{{"name", Name}, {"args", std::move(Args)}});
+ }
+
// Record an event on the current thread. ph, pid, tid, ts are set.
// Contents must be a list of the other JSON key/values.
- void event(const Context &Ctx, StringRef Phase,
- json::obj &&Contents) override {
+ void jsonEvent(StringRef Phase, json::obj &&Contents) {
uint64_t TID = get_threadid();
std::lock_guard Lock(Mu);
// If we haven't already, emit metadata describing this thread.
@@ -109,30 +122,24 @@
void log(const Context &Ctx, const Twine &Message) {
if (!T)
return;
- T->event(Ctx, "i",
- json::obj{
- {"name", "Log"},
- {"args", json::obj{{"Message", Message.str()}}},
- });
+ T->instant_event(Ctx, "Log", json::obj{{"Message", Message.str()}});
}
-Span::Span(const Context &Ctx, std::string Name) {
+Span::Span(const Context &Ctx, llvm::StringRef Name) : Name(Name) {
if (!T)
return;
// Clone the context, so that the original Context can be moved.
this->Ctx.emplace(Ctx.clone());
- T->event(*this->Ctx, "B", json::obj{{"name", std::move(Name)}});
+ T->begin_event(*this->Ctx, this->Name);
Args = llvm::make_unique();
}
Span::~Span() {
if (!T)
return;
- if (!Args)
-Args = llvm::make_unique();
- T->event(*Ctx, "E",
- Args ? json::obj{{"args", std::move(*Args)}} : json::obj{});
+ assert(Args && "Args can't be null at this point");
+ T->end_event(*this->Ctx, Name, std::move(*Args));
}
} // namespace trace
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40489: [clangd] Changed tracing interfaces
ilya-biryukov updated this revision to Diff 126318.
ilya-biryukov added a comment.
- Update the patch to accomodate changes from tracing and Context patches.
- Updated tracing after changes to Context. We now store a clone() of Context
in Span instead of ContextData.
- Pass Context by const-ref instead of by ref.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40489
Files:
clangd/JSONRPCDispatcher.cpp
clangd/Trace.cpp
clangd/Trace.h
Index: clangd/Trace.h
===
--- clangd/Trace.h
+++ clangd/Trace.h
@@ -28,12 +28,21 @@
namespace trace {
/// A consumer of trace events. The events are produced by Spans and trace::log.
+/// Calls to begin_event and end_event with the same name are always properly
+/// nested by using a RAII object (Span).
+/// Implmentations of this interface must be thread-safe.
class EventTracer {
public:
virtual ~EventTracer() = default;
- /// Consume a trace event.
- virtual void event(const ContextData &Ctx, llvm::StringRef Phase,
- json::obj &&Contents) = 0;
+
+ /// Called when event with \p Name starts.
+ virtual void begin_event(const Context &Ctx, llvm::StringRef Name) = 0;
+ /// Called when event with \p Name ends.
+ virtual void end_event(const Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) = 0;
+ /// Called for instant events.
+ virtual void instant_event(const Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) = 0;
};
/// Sets up a global EventTracer that consumes events produced by Span and
@@ -57,7 +66,7 @@
bool Pretty = false);
/// Records a single instant event, associated with the current thread.
-void log(Context &Ctx, const llvm::Twine &Name);
+void log(const Context &Ctx, const llvm::Twine &Name);
/// Records an event whose duration is the lifetime of the Span object.
/// This is the main public interface for producing tracing events.
@@ -67,15 +76,16 @@
/// SomeJSONExpr is evaluated and copied only if actually needed.
class Span {
public:
- Span(Context &Ctx, std::string Name);
+ Span(const Context &Ctx, llvm::StringRef Name);
~Span();
/// Returns mutable span metadata if this span is interested.
/// Prefer to use SPAN_ATTACH rather than accessing this directly.
json::obj *args() { return Args.get(); }
private:
- const ContextData &Ctx;
+ llvm::Optional Ctx;
+ std::string Name;
std::unique_ptr Args;
};
Index: clangd/Trace.cpp
===
--- clangd/Trace.cpp
+++ clangd/Trace.cpp
@@ -44,10 +44,23 @@
Out.flush();
}
+ void begin_event(const Context &Ctx, llvm::StringRef Name) override {
+jsonEvent("B", json::obj{{"name", Name}});
+ }
+
+ void end_event(const Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) override {
+jsonEvent("E", json::obj{{"args", std::move(Args)}});
+ }
+
+ void instant_event(const Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) override {
+jsonEvent("i", json::obj{{"name", Name}, {"args", std::move(Args)}});
+ }
+
// Record an event on the current thread. ph, pid, tid, ts are set.
// Contents must be a list of the other JSON key/values.
- void event(const ContextData &Ctx, StringRef Phase,
- json::obj &&Contents) override {
+ void jsonEvent(StringRef Phase, json::obj &&Contents) {
uint64_t TID = get_threadid();
std::lock_guard Lock(Mu);
// If we haven't already, emit metadata describing this thread.
@@ -106,30 +119,27 @@
return llvm::make_unique(OS, Pretty);
}
-void log(Context &Ctx, const Twine &Message) {
+void log(const Context &Ctx, const Twine &Message) {
if (!T)
return;
- T->event(*Ctx, "i",
- json::obj{
- {"name", "Log"},
- {"args", json::obj{{"Message", Message.str()}}},
- });
+ T->instant_event(Ctx, "Log", json::obj{{"Message", Message.str()}});
}
-Span::Span(Context &Ctx, std::string Name) : Ctx(*Ctx) {
+Span::Span(const Context &Ctx, llvm::StringRef Name) : Name(Name) {
if (!T)
return;
- T->event(this->Ctx, "B", json::obj{{"name", std::move(Name)}});
+ // Clone the context, so that the original Context can be moved.
+ this->Ctx.emplace(Ctx.clone());
+
+ T->begin_event(*this->Ctx, this->Name);
Args = llvm::make_unique();
}
Span::~Span() {
if (!T)
return;
- if (!Args)
-Args = llvm::make_unique();
- T->event(Ctx, "E",
- Args ? json::obj{{"args", std::move(*Args)}} : json::obj{});
+ assert(Args && "Args can't be null at this point");
+ T->end_event(*this->Ctx, Name, std::move(*Args));
}
} // namespace trace
Index: clangd/JSONRPCDispatcher.cpp
===
--- clangd/JSONRPCDispatcher.cpp
+++ clangd/JSONRPCDispatcher.cpp
@@ -60,14 +60,1
[PATCH] D40489: [clangd] Changed tracing interfaces
sammccall added inline comments.
Comment at: clangd/Trace.cpp:138
return;
- if (!Args)
-Args = llvm::make_unique();
- T->event(Ctx, "E",
- Args ? json::obj{{"args", std::move(*Args)}} : json::obj{});
+ assert(Args && "Args can't be null at this point");
+ T->end_event(Ctx, Name, std::move(*Args));
ilya-biryukov wrote:
> sammccall wrote:
> > why not?
> Because `T` must outlive the `Span`, so we can't really have `if (!T)`
> evaluate to different things in constructor and destructor.
> Am I missing something?
I was being disingenuous, I agree with you.
But can you change the message to "Changed tracer during a span"?
assert(Args) and "Args can't be null at this point" are basically synonyms :-)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40489
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40489: [clangd] Changed tracing interfaces
ilya-biryukov added inline comments.
Comment at: clangd/Trace.cpp:138
return;
- if (!Args)
-Args = llvm::make_unique();
- T->event(Ctx, "E",
- Args ? json::obj{{"args", std::move(*Args)}} : json::obj{});
+ assert(Args && "Args can't be null at this point");
+ T->end_event(Ctx, Name, std::move(*Args));
sammccall wrote:
> why not?
Because `T` must outlive the `Span`, so we can't really have `if (!T)` evaluate
to different things in constructor and destructor.
Am I missing something?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40489
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40489: [clangd] Changed tracing interfaces
sammccall added inline comments.
Comment at: clangd/Trace.cpp:138
return;
- if (!Args)
-Args = llvm::make_unique();
- T->event(Ctx, "E",
- Args ? json::obj{{"args", std::move(*Args)}} : json::obj{});
+ assert(Args && "Args can't be null at this point");
+ T->end_event(Ctx, Name, std::move(*Args));
why not?
Comment at: clangd/Trace.h:39
+ /// Called when event with \p Name starts.
+ virtual void begin_event(const ContextData &Ctx, llvm::StringRef Name) = 0;
+ /// Called when event with \p Name ends.
just call this begin?
Otherwise style is `beginEvent` I think
Comment at: clangd/Trace.h:41
+ /// Called when event with \p Name ends.
+ virtual void end_event(const ContextData &Ctx, llvm::StringRef Name,
+ json::obj &&Args) = 0;
How is identity represented between begin/end event? via Name doesn't seem
robust, so why the need to pass it twice? Does providing different contexts for
start/end make sense?
It might be cleaner/more flexible to have
std::function begin()
and call the return value to signal end.
This seems likely to be pretty easy for different providers to implement, and
is easy to use from Span.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40489
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40489: [clangd] Changed tracing interfaces
ilya-biryukov updated this revision to Diff 125696.
ilya-biryukov added a comment.
Herald added a subscriber: klimek.
- Update the patch to accomodate changes from tracing and Context patches.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40489
Files:
clangd/Trace.cpp
clangd/Trace.h
Index: clangd/Trace.h
===
--- clangd/Trace.h
+++ clangd/Trace.h
@@ -28,12 +28,21 @@
namespace trace {
/// A consumer of trace events. The events are produced by Spans and trace::log.
+/// Calls to begin_event and end_event with the same name are always properly
+/// nested by using a RAII object (Span).
+/// Implmentations of this interface must be thread-safe.
class EventTracer {
public:
virtual ~EventTracer() = default;
- /// Consume a trace event.
- virtual void event(const ContextData &Ctx, llvm::StringRef Phase,
- json::obj &&Contents) = 0;
+
+ /// Called when event with \p Name starts.
+ virtual void begin_event(const ContextData &Ctx, llvm::StringRef Name) = 0;
+ /// Called when event with \p Name ends.
+ virtual void end_event(const ContextData &Ctx, llvm::StringRef Name,
+ json::obj &&Args) = 0;
+ /// Called for instant events.
+ virtual void instant_event(const ContextData &Ctx, llvm::StringRef Name,
+ json::obj &&Args) = 0;
};
/// Sets up a global EventTracer that consumes events produced by Span and
@@ -67,15 +76,16 @@
/// SomeJSONExpr is evaluated and copied only if actually needed.
class Span {
public:
- Span(Context &Ctx, std::string Name);
+ Span(Context &Ctx, llvm::StringRef Name);
~Span();
/// Returns mutable span metadata if this span is interested.
/// Prefer to use SPAN_ATTACH rather than accessing this directly.
json::obj *args() { return Args.get(); }
private:
const ContextData &Ctx;
+ std::string Name;
std::unique_ptr Args;
};
Index: clangd/Trace.cpp
===
--- clangd/Trace.cpp
+++ clangd/Trace.cpp
@@ -44,10 +44,23 @@
Out.flush();
}
+ void begin_event(const ContextData &Ctx, llvm::StringRef Name) override {
+jsonEvent("B", json::obj{{"name", Name}});
+ }
+
+ void end_event(const ContextData &Ctx, llvm::StringRef Name,
+ json::obj &&Args) override {
+jsonEvent("E", json::obj{{"args", std::move(Args)}});
+ }
+
+ void instant_event(const ContextData &Ctx, llvm::StringRef Name,
+ json::obj &&Args) override {
+jsonEvent("i", json::obj{{"name", Name}, {"args", std::move(Args)}});
+ }
+
// Record an event on the current thread. ph, pid, tid, ts are set.
// Contents must be a list of the other JSON key/values.
- void event(const ContextData &Ctx, StringRef Phase,
- json::obj &&Contents) override {
+ void jsonEvent(StringRef Phase, json::obj &&Contents) {
uint64_t TID = get_threadid();
std::lock_guard Lock(Mu);
// If we haven't already, emit metadata describing this thread.
@@ -109,27 +122,21 @@
void log(Context &Ctx, const Twine &Message) {
if (!T)
return;
- T->event(*Ctx, "i",
- json::obj{
- {"name", "Log"},
- {"args", json::obj{{"Message", Message.str()}}},
- });
+ T->instant_event(*Ctx, "Log", json::obj{{"Message", Message.str()}});
}
-Span::Span(Context &Ctx, std::string Name) : Ctx(*Ctx) {
+Span::Span(Context &Ctx, llvm::StringRef Name) : Ctx(*Ctx), Name(Name) {
if (!T)
return;
- T->event(this->Ctx, "B", json::obj{{"name", std::move(Name)}});
+ T->begin_event(this->Ctx, this->Name);
Args = llvm::make_unique();
}
Span::~Span() {
if (!T)
return;
- if (!Args)
-Args = llvm::make_unique();
- T->event(Ctx, "E",
- Args ? json::obj{{"args", std::move(*Args)}} : json::obj{});
+ assert(Args && "Args can't be null at this point");
+ T->end_event(Ctx, Name, std::move(*Args));
}
} // namespace trace
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40489: [clangd] Changed tracing interfaces
ilya-biryukov planned changes to this revision. ilya-biryukov added a comment. Have to update to match the new `Context` implementation. https://reviews.llvm.org/D40489 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40489: [clangd] Changed tracing interfaces
luckygeck added inline comments. Comment at: clangd/Trace.h:41 + /// Context. static PtrKey CtxKey; ilya-biryukov wrote: > luckygeck wrote: > > This is a (1)static non-pod member in an (2) interface. Is it really a good > > idea? If we plan to have only one ctxkey, then maybe let's make it not > > bound to EventTracer? > It does not have any data members, has trivial default constructor and > trivial destructor. > I don't think there are any problems we're gonna hit with this one, or am I > missing something? > > > then maybe let's make it not bound to EventTracer? > Do you propose to move it out of the `class` into the `clangd::trace` > namespace instead? I've just looked through the change adding Context and Key<>\PtrKey<> thingy. Yes, these classes are POD, so it is fine to have them static (as you rely only on their address). Yes, I'd prefer to move the key out of `class` into a namespace - interface looks better without any data IMO. Comment at: clangd/Trace.h:49 + virtual void end_event(Context &Ctx, llvm::StringRef Name, + json::obj &&Args) = 0; + /// Called for instant events. ilya-biryukov wrote: > luckygeck wrote: > > Maybe it is better to calculate these Args only if the tracing is enabled? > > This might be done at least this two ways: > > 1) Have `bool is_tracing_enabled()` method that we can check before > > calculating args. > > 2) Pass a function that will produce json::obj when called. > The current implementation won't compute args if `EventTracer` inside a > `Context` is null, so I think this should cover our needs for per-request > tracing (see `SPAN_ATTACH` macro). > But `is_tracing_enabled()` makes sense if we'd like to turn the tracing off > in a middle of a single `Context` lifetime. This would make sense for global > tracer, is this the use-case you anticipate? > Oh, ok - setting tracer in a context only when we need to looks good enough. https://reviews.llvm.org/D40489 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40489: [clangd] Changed tracing interfaces
ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clangd/Trace.h:41 + /// Context. static PtrKey CtxKey; luckygeck wrote: > This is a (1)static non-pod member in an (2) interface. Is it really a good > idea? If we plan to have only one ctxkey, then maybe let's make it not bound > to EventTracer? It does not have any data members, has trivial default constructor and trivial destructor. I don't think there are any problems we're gonna hit with this one, or am I missing something? > then maybe let's make it not bound to EventTracer? Do you propose to move it out of the `class` into the `clangd::trace` namespace instead? Comment at: clangd/Trace.h:49 + virtual void end_event(Context &Ctx, llvm::StringRef Name, + json::obj &&Args) = 0; + /// Called for instant events. luckygeck wrote: > Maybe it is better to calculate these Args only if the tracing is enabled? > This might be done at least this two ways: > 1) Have `bool is_tracing_enabled()` method that we can check before > calculating args. > 2) Pass a function that will produce json::obj when called. The current implementation won't compute args if `EventTracer` inside a `Context` is null, so I think this should cover our needs for per-request tracing (see `SPAN_ATTACH` macro). But `is_tracing_enabled()` makes sense if we'd like to turn the tracing off in a middle of a single `Context` lifetime. This would make sense for global tracer, is this the use-case you anticipate? https://reviews.llvm.org/D40489 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40489: [clangd] Changed tracing interfaces
ilya-biryukov updated this revision to Diff 124371.
ilya-biryukov added a comment.
- Fixed a typo.
https://reviews.llvm.org/D40489
Files:
clangd/Trace.cpp
clangd/Trace.h
Index: clangd/Trace.h
===
--- clangd/Trace.h
+++ clangd/Trace.h
@@ -27,12 +27,29 @@
namespace clangd {
namespace trace {
+/// A sink for consuming tracing events. Calls to begin_event and end_event with
+/// the same name are always properly nested by using a RAII object (Span).
+/// However, global tracers (i.e., returned from the global Context object) may
+/// be called concurrently and have to be properly synchronized. For per-request
+/// EventTracers (i.e., returned from the request-specific Context objects) the
+/// calls to begin_event and end_event must be properly synchronized and must
+/// never be called concurrently.
class EventTracer {
public:
+ /// A key used by trace::Span and trace::log to find EventTracer in a \p
+ /// Context.
static PtrKey CtxKey;
virtual ~EventTracer() = default;
- virtual void event(llvm::StringRef Phase, json::obj &&Contents) = 0;
+
+ /// Called when event with \p Name starts.
+ virtual void begin_event(Context &Ctx, llvm::StringRef Name) = 0;
+ /// Called when event with \p Name ends.
+ virtual void end_event(Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) = 0;
+ /// Called for instant events.
+ virtual void instant_event(Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) = 0;
};
/// Create an instance of EventTracer that produces an output in the Trace Event
@@ -66,6 +83,7 @@
private:
Context &Ctx;
+ std::string Name;
std::unique_ptr Args;
};
Index: clangd/Trace.cpp
===
--- clangd/Trace.cpp
+++ clangd/Trace.cpp
@@ -45,9 +45,23 @@
Out.flush();
}
+ void begin_event(Context &Ctx, llvm::StringRef Name) override {
+jsonEvent("B", json::obj{{"name", Name}});
+ }
+
+ void end_event(Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) override {
+jsonEvent("E", json::obj{{"name", Name}, {"args", std::move(Args)}});
+ }
+
+ void instant_event(Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) override {
+jsonEvent("i", json::obj{{"name", Name}, {"args", std::move(Args)}});
+ }
+
// Record an event on the current thread. ph, pid, tid, ts are set.
// Contents must be a list of the other JSON key/values.
- void event(StringRef Phase, json::obj &&Contents) override {
+ void jsonEvent(StringRef Phase, json::obj &&Contents) {
uint64_t TID = get_threadid();
std::lock_guard Lock(Mu);
// If we haven't already, emit metadata describing this thread.
@@ -103,27 +117,23 @@
EventTracer *T = Ctx.get(EventTracer::CtxKey);
if (!T)
return;
- T->event("i", json::obj{
-{"name", "Log"},
-{"args", json::obj{{"Message", Message.str()}}},
-});
+ T->instant_event("Log", json::obj{{"Message", Message.str()}});
}
-Span::Span(Context &Ctx, std::string Name) : Ctx(Ctx) {
- EventTracer* T = Ctx.get(EventTracer::CtxKey);
+Span::Span(Context &Ctx, std::string Name) : Ctx(Ctx), Name(std::move(Name)) {
+ EventTracer *T = Ctx.get(EventTracer::CtxKey);
if (!T)
return;
- T->event("B", json::obj{{"name", std::move(Name)}});
+ T->begin_event(this->Name);
Args = llvm::make_unique();
}
Span::~Span() {
auto T = Ctx.get(EventTracer::CtxKey);
if (!T)
return;
- if (!Args)
-Args = llvm::make_unique();
- T->event("E", Args ? json::obj{{"args", std::move(*Args)}} : json::obj{});
+ assert(Args && "Args can't be null at this point");
+ T->end_event(Name, std::move(*Args));
}
} // namespace trace
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40489: [clangd] Changed tracing interfaces
luckygeck added inline comments. Comment at: clangd/Trace.h:41 + /// Context. static PtrKey CtxKey; This is a (1)static non-pod member in an (2) interface. Is it really a good idea? If we plan to have only one ctxkey, then maybe let's make it not bound to EventTracer? Comment at: clangd/Trace.h:45 + + /// Called when event with \p Name stars. + virtual void begin_event(Context &Ctx, llvm::StringRef Name) = 0; s/stars/starts. Comment at: clangd/Trace.h:49 + virtual void end_event(Context &Ctx, llvm::StringRef Name, + json::obj &&Args) = 0; + /// Called for instant events. Maybe it is better to calculate these Args only if the tracing is enabled? This might be done at least this two ways: 1) Have `bool is_tracing_enabled()` method that we can check before calculating args. 2) Pass a function that will produce json::obj when called. https://reviews.llvm.org/D40489 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40489: [clangd] Changed tracing interfaces
ilya-biryukov created this revision.
Introduced begin_event, end_event and instant_event instead of a
string phase name.
https://reviews.llvm.org/D40489
Files:
clangd/Trace.cpp
clangd/Trace.h
Index: clangd/Trace.h
===
--- clangd/Trace.h
+++ clangd/Trace.h
@@ -27,12 +27,29 @@
namespace clangd {
namespace trace {
+/// A sink for consuming tracing events. Calls to begin_event and end_event with
+/// the same name are always properly nested by using a RAII object (Span).
+/// However, global tracers (i.e., returned from the global Context object) may
+/// be called concurrently and have to be properly synchronized. For per-request
+/// EventTracers (i.e., returned from the request-specific Context objects) the
+/// calls to begin_event and end_event must be properly synchronized and must
+/// never be called concurrently.
class EventTracer {
public:
+ /// A key used by trace::Span and trace::log to find EventTracer in a \p
+ /// Context.
static PtrKey CtxKey;
virtual ~EventTracer() = default;
- virtual void event(llvm::StringRef Phase, json::obj &&Contents) = 0;
+
+ /// Called when event with \p Name stars.
+ virtual void begin_event(Context &Ctx, llvm::StringRef Name) = 0;
+ /// Called when event with \p Name ends.
+ virtual void end_event(Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) = 0;
+ /// Called for instant events.
+ virtual void instant_event(Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) = 0;
};
/// Create an instance of EventTracer that produces an output in the Trace Event
@@ -66,6 +83,7 @@
private:
Context &Ctx;
+ std::string Name;
std::unique_ptr Args;
};
Index: clangd/Trace.cpp
===
--- clangd/Trace.cpp
+++ clangd/Trace.cpp
@@ -45,9 +45,23 @@
Out.flush();
}
+ void begin_event(Context &Ctx, llvm::StringRef Name) override {
+jsonEvent("B", json::obj{{"name", Name}});
+ }
+
+ void end_event(Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) override {
+jsonEvent("E", json::obj{{"name", Name}, {"args", std::move(Args)}});
+ }
+
+ void instant_event(Context &Ctx, llvm::StringRef Name,
+ json::obj &&Args) override {
+jsonEvent("i", json::obj{{"name", Name}, {"args", std::move(Args)}});
+ }
+
// Record an event on the current thread. ph, pid, tid, ts are set.
// Contents must be a list of the other JSON key/values.
- void event(StringRef Phase, json::obj &&Contents) override {
+ void jsonEvent(StringRef Phase, json::obj &&Contents) {
uint64_t TID = get_threadid();
std::lock_guard Lock(Mu);
// If we haven't already, emit metadata describing this thread.
@@ -103,27 +117,23 @@
EventTracer *T = Ctx.get(EventTracer::CtxKey);
if (!T)
return;
- T->event("i", json::obj{
-{"name", "Log"},
-{"args", json::obj{{"Message", Message.str()}}},
-});
+ T->instant_event("Log", json::obj{{"Message", Message.str()}});
}
-Span::Span(Context &Ctx, std::string Name) : Ctx(Ctx) {
- EventTracer* T = Ctx.get(EventTracer::CtxKey);
+Span::Span(Context &Ctx, std::string Name) : Ctx(Ctx), Name(std::move(Name)) {
+ EventTracer *T = Ctx.get(EventTracer::CtxKey);
if (!T)
return;
- T->event("B", json::obj{{"name", std::move(Name)}});
+ T->begin_event(this->Name);
Args = llvm::make_unique();
}
Span::~Span() {
auto T = Ctx.get(EventTracer::CtxKey);
if (!T)
return;
- if (!Args)
-Args = llvm::make_unique();
- T->event("E", Args ? json::obj{{"args", std::move(*Args)}} : json::obj{});
+ assert(Args && "Args can't be null at this point");
+ T->end_event(Name, std::move(*Args));
}
} // namespace trace
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
