Index: clangd/ClangdUnit.cpp =================================================================== --- clangd/ClangdUnit.cpp +++ clangd/ClangdUnit.cpp @@ -506,6 +506,7 @@ std::unique_ptr 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 PreambleDiags; StoreDiagsConsumer PreambleDiagnosticsConsumer(/*ref*/ PreambleDiags); IntrusiveRefCntPtr PreambleDiagsEngine = @@ -587,8 +588,8 @@ // Compute updated AST. llvm::Optional 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); } 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 Context derive(Type &&Value) const & { + static Key::type> Private; + return derive(Private, std::forward(Value)); + } + + template Context derive(Type &&Value) const && { + static Key::type> Private; + return derive(Private, std::forward(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/Function.h =================================================================== --- clangd/Function.h +++ clangd/Function.h @@ -144,7 +144,7 @@ static_assert(std::is_same::type, Func>::value, "Func must be decayed"); - ScopeExitGuard(Func F) : F(std::move(F)) {} + ScopeExitGuard(Func F) : F(llvm::make_unique(std::move(F))) {} ~ScopeExitGuard() { if (!F) return; @@ -159,7 +159,7 @@ ScopeExitGuard &operator=(ScopeExitGuard &&Other) = default; private: - llvm::Optional F; + std::unique_ptr F; }; } // namespace detail Index: clangd/JSONRPCDispatcher.cpp =================================================================== --- clangd/JSONRPCDispatcher.cpp +++ clangd/JSONRPCDispatcher.cpp @@ -20,7 +20,7 @@ using namespace clangd; namespace { -static Key> RequestSpan; +static Key RequestCtx; static Key RequestID; static Key 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(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(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(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/Trace.h =================================================================== --- clangd/Trace.h +++ clangd/Trace.h @@ -32,17 +32,14 @@ /// 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; - /// 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 +66,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 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> kSpanArgs; - Args = llvm::make_unique(); +Context span(const Context &Ctx, llvm::StringRef Name) { + if (!T) + return Ctx.derive(kSpanArgs, nullptr); + return T->beginSpan(Ctx.derive(kSpanArgs, llvm::make_unique()), + 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: 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"); } }