sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, ioeric, jkorous-apple, klimek.
This is probably the right behavior for distributed tracers, and makes unpaired
begin-end events impossible without requiring Spans to be bound to a thread.
The API is conceptually clean but syntactically awkward. As discussed offline,
this is basically a naming problem and will go away if (when) we use TLS to
store the current context.
The apparently-unrelated change to onScopeExit are because its move semantics
broken if Func is POD-like since r322838. This is true of function pointers,
and the lambda I use here that captures two pointers only.
I've raised this issue on llvm-dev and will revert this part if we fix it in
some other way.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42499
Files:
clangd/ClangdUnit.cpp
clangd/Context.h
clangd/Function.h
clangd/JSONRPCDispatcher.cpp
clangd/Trace.cpp
clangd/Trace.h
unittests/clangd/TraceTests.cpp
Index: unittests/clangd/TraceTests.cpp
===================================================================
--- unittests/clangd/TraceTests.cpp
+++ unittests/clangd/TraceTests.cpp
@@ -78,8 +78,8 @@
auto JSONTracer = trace::createJSONTracer(OS);
trace::Session Session(*JSONTracer);
{
- trace::Span S(Context::empty(), "A");
- trace::log(Context::empty(), "B");
+ Context Ctx = trace::span(Context::empty(), "A");
+ trace::log(Ctx, "B");
}
}
Index: clangd/Trace.h
===================================================================
--- clangd/Trace.h
+++ clangd/Trace.h
@@ -38,11 +38,12 @@
virtual ~EventTracer() = default;
- /// 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 when event that has a duration starts. \p Name describes the event.
+ /// Returns a derived context that will be destroyed when the event ends.
+ /// Usually implementations will store an object in the returned context
+ /// whose destructor records the end of the event.
+ /// The args are spanArgs(Ctx), only complete when the event ends.
+ virtual Context beginSpan(const Context &Ctx, llvm::StringRef Name) = 0;
/// Called for instant events.
virtual void instant(const Context &Ctx, llvm::StringRef Name,
@@ -69,31 +70,34 @@
/// Records a single instant event, associated with the current thread.
void log(const Context &Ctx, const llvm::Twine &Name);
-/// Records an event whose duration is the lifetime of the Span object.
+/// Records an event with a duration.
+/// Returns a derived context, the event ends when it is destroyed.
+///
/// This is the main public interface for producing tracing events.
///
/// Arbitrary JSON metadata can be attached while this span is active:
-/// SPAN_ATTACH(MySpan, "Payload", SomeJSONExpr);
+/// SPAN_ATTACH(spanCtx, "Payload", SomeJSONExpr);
+///
/// SomeJSONExpr is evaluated and copied only if actually needed.
-class Span {
-public:
- 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:
- std::unique_ptr<json::obj> Args;
- EventTracer::EndEventCallback Callback;
-};
-
-#define SPAN_ATTACH(S, Name, Expr) \
+///
+/// FIXME: this API is awkward to use. Migrate to TLS Context and scoped Span.
+/// Meanwhile, beware of this trap:
+/// foo(const Context &Ctx) {
+/// Context Ctx = trace::span(Ctx /* actually the uninit'd new context! */);
+/// }
+Context span(const Context &Ctx, llvm::StringRef Name);
+
+/// Returns mutable span metadata if this span is interested.
+/// Prefer to use SPAN_ATTACH rather than accessing this directly.
+json::obj *spanArgs(const Context &Ctx);
+
+/// Attach a key-value pair to the current trace span.
+/// Ctx (or some ancestor) must be created by span().
+/// This macro is not safe for use on the same span from multiple threads.
+#define SPAN_ATTACH(Ctx, Name, Expr) \
do { \
- if ((S).args() != nullptr) { \
- (*((S).args()))[(Name)] = (Expr); \
- } \
+ if (auto *Args = ::clang::clangd::trace::spanArgs(Ctx)) \
+ (*Args)[Name] = Expr; \
} while (0)
} // namespace trace
Index: clangd/Trace.cpp
===================================================================
--- clangd/Trace.cpp
+++ clangd/Trace.cpp
@@ -7,6 +7,8 @@
//
//===----------------------------------------------------------------------===//
+#include "Context.h"
+#include "Function.h"
#include "Trace.h"
#include "llvm/ADT/DenseSet.h"
#include "llvm/Support/Chrono.h"
@@ -43,14 +45,12 @@
Out.flush();
}
- EndEventCallback beginSpan(const Context &Ctx,
- llvm::StringRef Name) override {
+ Context beginSpan(const Context &Ctx, llvm::StringRef Name) override {
+ json::obj *Args = spanArgs(Ctx);
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)}});
- };
+ return Ctx.derive(onScopeExit([this, Args] {
+ jsonEvent("E", json::obj{{"args", std::move(*Args)}});
+ }));
}
void instant(const Context &Ctx, llvm::StringRef Name,
@@ -125,23 +125,17 @@
T->instant(Ctx, "Log", json::obj{{"Message", Message.str()}});
}
-Span::Span(const Context &Ctx, llvm::StringRef Name) {
- if (!T)
- return;
-
- Callback = T->beginSpan(Ctx, Name);
- if (!Callback)
- return;
+static Key<std::unique_ptr<json::obj>> kSpanArgs;
- Args = llvm::make_unique<json::obj>();
+Context span(const Context &Ctx, llvm::StringRef Name) {
+ if (!T)
+ return Ctx.derive(kSpanArgs, nullptr);
+ return T->beginSpan(Ctx.derive(kSpanArgs, llvm::make_unique<json::obj>()),
+ Name);
}
-Span::~Span() {
- if (!Callback)
- return;
-
- assert(Args && "Args must be non-null if Callback is defined");
- Callback(std::move(*Args));
+json::obj *spanArgs(const Context &Ctx) {
+ return Ctx.getExisting(kSpanArgs).get();
}
} // namespace trace
Index: clangd/JSONRPCDispatcher.cpp
===================================================================
--- clangd/JSONRPCDispatcher.cpp
+++ clangd/JSONRPCDispatcher.cpp
@@ -20,7 +20,7 @@
using namespace clangd;
namespace {
-static Key<std::unique_ptr<trace::Span>> RequestSpan;
+static Key<Context> RequestCtx;
static Key<json::Expr> RequestID;
static Key<JSONOutput *> RequestOut;
} // namespace
@@ -66,8 +66,8 @@
return;
}
- if (auto *Span = Ctx.get(RequestSpan))
- SPAN_ATTACH(**Span, "Reply", Result);
+ if (auto *EnclosingRequest = Ctx.get(RequestCtx))
+ SPAN_ATTACH(*EnclosingRequest, "Reply", Result);
Ctx.getExisting(RequestOut)
->writeMessage(json::obj{
@@ -80,8 +80,8 @@
void clangd::replyError(const Context &Ctx, ErrorCode code,
const llvm::StringRef &Message) {
log(Ctx, "Error " + Twine(static_cast<int>(code)) + ": " + Message);
- if (auto *Span = Ctx.get(RequestSpan))
- SPAN_ATTACH(**Span, "Error",
+ if (auto *EnclosingRequest = Ctx.get(RequestCtx))
+ SPAN_ATTACH(*EnclosingRequest, "Error",
(json::obj{{"code", static_cast<int>(code)},
{"message", Message.str()}}));
@@ -99,8 +99,8 @@
void clangd::call(const Context &Ctx, StringRef Method, json::Expr &&Params) {
// FIXME: Generate/Increment IDs for every request so that we can get proper
// replies once we need to.
- if (auto *Span = Ctx.get(RequestSpan))
- SPAN_ATTACH(**Span, "Call",
+ if (auto *EnclosingRequest = Ctx.get(RequestCtx))
+ SPAN_ATTACH(*EnclosingRequest, "Call",
(json::obj{{"method", Method.str()}, {"params", Params}}));
Ctx.getExisting(RequestOut)
->writeMessage(json::obj{
@@ -143,14 +143,13 @@
Ctx = std::move(Ctx).derive(RequestID, *ID);
// Create a tracing Span covering the whole request lifetime.
- auto Tracer = llvm::make_unique<trace::Span>(Ctx, *Method);
+ Ctx = trace::span(Ctx, *Method);
if (ID)
- SPAN_ATTACH(*Tracer, "ID", *ID);
- SPAN_ATTACH(*Tracer, "Params", Params);
-
- // Update Ctx to include Tracer.
- Ctx = std::move(Ctx).derive(RequestSpan, std::move(Tracer));
+ SPAN_ATTACH(Ctx, "ID", *ID);
+ SPAN_ATTACH(Ctx, "Params", Params);
+ // Stash the enclosing request context, so later calls can add metadata.
+ Ctx = Ctx.derive(RequestCtx, Ctx.clone());
Handler(std::move(Ctx), std::move(Params));
return true;
}
Index: clangd/Function.h
===================================================================
--- clangd/Function.h
+++ clangd/Function.h
@@ -144,7 +144,7 @@
static_assert(std::is_same<typename std::decay<Func>::type, Func>::value,
"Func must be decayed");
- ScopeExitGuard(Func F) : F(std::move(F)) {}
+ ScopeExitGuard(Func F) : F(llvm::make_unique<Func>(std::move(F))) {}
~ScopeExitGuard() {
if (!F)
return;
@@ -159,7 +159,7 @@
ScopeExitGuard &operator=(ScopeExitGuard &&Other) = default;
private:
- llvm::Optional<Func> F;
+ std::unique_ptr<Func> F;
};
} // namespace detail
Index: clangd/Context.h
===================================================================
--- clangd/Context.h
+++ clangd/Context.h
@@ -84,6 +84,9 @@
///
/// Keys are typically used across multiple functions, so most of the time you
/// would want to make them static class members or global variables.
+///
+/// FIXME: Rather than manual plumbing, pass Context using thread-local storage
+/// by default, and make thread boundaries deal with propagation explicitly.
class Context {
public:
/// Returns an empty context that contains no data. Useful for calling
@@ -150,8 +153,24 @@
std::move(Value))}));
}
+ /// Derives a child context, using an anonymous key.
+ /// Intended for objects stored only for their destructor's side-effect.
+ template <class Type> Context derive(Type &&Value) const & {
+ static Key<typename std::decay<Type>::type> Private;
+ return derive(Private, std::forward<Type>(Value));
+ }
+
+ template <class Type> Context derive(Type &&Value) const && {
+ static Key<typename std::decay<Type>::type> Private;
+ return derive(Private, std::forward<Type>(Value));
+ }
+
/// Clone this context object.
Context clone() const;
+ /// Contexts are equal if they're clones.
+ friend bool operator==(const Context &A, const Context &B) {
+ return A.DataPtr == B.DataPtr;
+ }
private:
class AnyStorage {
Index: clangd/ClangdUnit.cpp
===================================================================
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -506,6 +506,7 @@
std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =
llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, That->FileName);
+ auto &OuterCtx = Ctx;
// A helper function to rebuild the preamble or reuse the existing one. Does
// not mutate any fields of CppFile, only does the actual computation.
// Lamdba is marked mutable to call reset() on OldPreamble.
@@ -525,8 +526,8 @@
// deleted (if there are no other references to it).
OldPreamble.reset();
- trace::Span Tracer(Ctx, "Preamble");
- SPAN_ATTACH(Tracer, "File", That->FileName);
+ auto Ctx = trace::span(OuterCtx, "Preamble");
+ SPAN_ATTACH(Ctx, "File", That->FileName);
std::vector<DiagWithFixIts> PreambleDiags;
StoreDiagsConsumer PreambleDiagnosticsConsumer(/*ref*/ PreambleDiags);
IntrusiveRefCntPtr<DiagnosticsEngine> PreambleDiagsEngine =
@@ -587,8 +588,8 @@
// Compute updated AST.
llvm::Optional<ParsedAST> NewAST;
{
- trace::Span Tracer(Ctx, "Build");
- SPAN_ATTACH(Tracer, "File", That->FileName);
+ auto Ctx = trace::span(OuterCtx, "Build");
+ SPAN_ATTACH(Ctx, "File", That->FileName);
NewAST = ParsedAST::Build(Ctx, std::move(CI), std::move(NewPreamble),
std::move(ContentsBuffer), PCHs, Inputs.FS);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
