[PATCH] D42499: [clangd] Modify the Span API so that Spans propagate with contexts.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL323511: [clangd] Modify the Span API so that Spans propagate
with contexts. (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D42499?vs=131546&id=131547#toc
Repository:
rL LLVM
https://reviews.llvm.org/D42499
Files:
clang-tools-extra/trunk/clangd/ClangdUnit.cpp
clang-tools-extra/trunk/clangd/Context.h
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
clang-tools-extra/trunk/clangd/Trace.cpp
clang-tools-extra/trunk/clangd/Trace.h
clang-tools-extra/trunk/unittests/clangd/TraceTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/TraceTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/TraceTests.cpp
+++ clang-tools-extra/trunk/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");
+ trace::Span Tracer(Context::empty(), "A");
+ trace::log(Tracer.Ctx, "B");
}
}
Index: clang-tools-extra/trunk/clangd/Trace.h
===
--- clang-tools-extra/trunk/clangd/Trace.h
+++ clang-tools-extra/trunk/clangd/Trace.h
@@ -32,17 +32,15 @@
/// 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 *Args, only complete when the event ends.
+ virtual Context beginSpan(const Context &Ctx, llvm::StringRef Name,
+json::obj *Args) = 0;
/// Called for instant events.
virtual void instant(const Context &Ctx, llvm::StringRef Name,
@@ -70,30 +68,35 @@
void log(const Context &Ctx, const llvm::Twine &Name);
/// Records an event whose duration is the lifetime of the Span object.
+/// This lifetime is extended when the span's context is reused.
+///
/// 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);
+///
/// 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.
+ /// Mutable 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;
+ json::obj *const Args;
+ /// Propagating this context will keep the span alive.
+ const Context Ctx;
};
+/// 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 a Span event.
+/// This is not threadsafe when used with the same Span.
#define SPAN_ATTACH(S, Name, Expr) \
do { \
-if ((S).args() != nullptr) { \
- (*((S).args()))[(Name)] = (Expr);\
-} \
+if (auto *Args = (S).Args) \
+ (*Args)[Name] = Expr;\
} while (0)
} // namespace trace
Index: clang-tools-extra/trunk/clangd/Context.h
===
--- clang-tools-extra/trunk/clangd/Context.h
+++ clang-tools-extra/trunk/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 plumbi
[PATCH] D42499: [clangd] Modify the Span API so that Spans propagate with contexts.
sammccall updated this revision to Diff 131546.
sammccall marked 2 inline comments as done.
sammccall added a comment.
Address review comments
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/CodeCompleteTests.cpp
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");
+ trace::Span Tracer(Context::empty(), "A");
+ trace::log(Tracer.Ctx, "B");
}
}
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -537,12 +537,13 @@
/*StorePreamblesInMemory=*/true);
FS.Files[getVirtualTestFilePath("bar.h")] =
- R"cpp(namespace ns { int preamble; })cpp";
+ R"cpp(namespace ns { struct preamble { int member; }; })cpp";
auto File = getVirtualTestFilePath("foo.cpp");
Annotations Test(R"cpp(
#include "bar.h"
namespace ns { int local; }
- void f() { ns::^ }
+ void f() { ns::^; }
+ void f() { ns::preamble().$2^; }
)cpp");
Server.addDocument(Context::empty(), File, Test.code()).wait();
clangd::CodeCompleteOptions Opts = {};
@@ -562,6 +563,11 @@
.second.Value;
EXPECT_THAT(WithIndex.items,
UnorderedElementsAre(Named("local"), Named("index")));
+ auto ClassFromPreamble =
+ Server.codeComplete(Context::empty(), File, Test.point("2"), Opts)
+ .get()
+ .second.Value;
+ EXPECT_THAT(ClassFromPreamble.items, Contains(Named("member")));
}
TEST(CompletionTest, DynamicIndexMultiFile) {
Index: clangd/Trace.h
===
--- clangd/Trace.h
+++ clangd/Trace.h
@@ -32,17 +32,15 @@
/// 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 *Args, only complete when the event ends.
+ virtual Context beginSpan(const Context &Ctx, llvm::StringRef Name,
+json::obj *Args) = 0;
/// Called for instant events.
virtual void instant(const Context &Ctx, llvm::StringRef Name,
@@ -70,30 +68,35 @@
void log(const Context &Ctx, const llvm::Twine &Name);
/// Records an event whose duration is the lifetime of the Span object.
+/// This lifetime is extended when the span's context is reused.
+///
/// 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);
+///
/// SomeJSONExpr is evaluated and copied only if actually needed.
class Span {
-public:
+ public:
Span(const Context &Ctx, llvm::StringRef Name);
- ~Span();
- /// Returns mutable span metadata if this span is interested.
+ /// Mutable 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;
+ json::obj * const Args;
+ /// Propagating this context will keep the span alive.
+ const Context Ctx;
};
+/// 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 a Span event.
+/// This is not threadsafe when used with the same Span.
#define SPAN_ATTACH(S, Name, Expr) \
do { \
-if ((S).args() != nullptr) {
[PATCH] D42499: [clangd] Modify the Span API so that Spans propagate with contexts.
sammccall marked 2 inline comments as done.
sammccall added inline comments.
Comment at: clangd/JSONRPCDispatcher.cpp:31
+ RequestArgs(json::obj *Args) : Args(Args) {}
+ std::mutex Mu;
+ json::obj *Args;
ilya-biryukov wrote:
> We could make the fields private and expose only the function to modify args
> under lock (similar to `FillRequestInfo`, i.e. the last two line of it).
> That would make the class safer to use. But that's work and the class itself
> is an implementation detail, so we could also leave it as is.
Yeah, good point. Encapsulated the whole mechanism in this class.
Comment at: clangd/Trace.cpp:138
+ Args)
+: Ctx.derive(kSpanArgs, nullptr)) {}
ilya-biryukov wrote:
> Would be nice for `Span`s to have zero cost when no tracer is active. We
> could probably achieve that be not storing the `kSpanArgs` when `T` is null.
> WDYT?
> That's really not a big deal, so I don't think we should block the patch on
> this.
Good catch! This was left over from an older revision where SPAN_ATTACH looked
up the args in the context.
Now this is for ownership only, so I skip derive() if there's no tracer, and
made the key anonymous.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42499
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42499: [clangd] Modify the Span API so that Spans propagate with contexts.
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM. And sorry for confusion with https://reviews.llvm.org/D42524, if any.
Comment at: clangd/Function.h:147
- ScopeExitGuard(Func F) : F(std::move(F)) {}
+ ScopeExitGuard(Func F) : F(llvm::make_unique(std::move(F))) {}
~ScopeExitGuard() {
sammccall wrote:
> ilya-biryukov wrote:
> > Why do we need `unique_ptr` instead of `Optional` here?
> ScopeExitGuard was broken by the recent Optional changes (when Func is
> trivially copyable).
> This is a minimal workaround, let's discuss on the other patch.
LG, thanks. Wasn't aware of the breakage when writing the comment.
Comment at: clangd/JSONRPCDispatcher.cpp:31
+ RequestArgs(json::obj *Args) : Args(Args) {}
+ std::mutex Mu;
+ json::obj *Args;
We could make the fields private and expose only the function to modify args
under lock (similar to `FillRequestInfo`, i.e. the last two line of it).
That would make the class safer to use. But that's work and the class itself is
an implementation detail, so we could also leave it as is.
Comment at: clangd/Trace.cpp:138
+ Args)
+: Ctx.derive(kSpanArgs, nullptr)) {}
Would be nice for `Span`s to have zero cost when no tracer is active. We could
probably achieve that be not storing the `kSpanArgs` when `T` is null. WDYT?
That's really not a big deal, so I don't think we should block the patch on
this.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42499
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42499: [clangd] Modify the Span API so that Spans propagate with contexts.
sammccall updated this revision to Diff 131460.
sammccall marked 4 inline comments as done.
sammccall added a comment.
Address review comments
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");
+ trace::Span Tracer(Context::empty(), "A");
+ trace::log(Tracer.Ctx, "B");
}
}
Index: clangd/Trace.h
===
--- clangd/Trace.h
+++ clangd/Trace.h
@@ -32,17 +32,15 @@
/// 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 *Args, only complete when the event ends.
+ virtual Context beginSpan(const Context &Ctx, llvm::StringRef Name,
+json::obj *Args) = 0;
/// Called for instant events.
virtual void instant(const Context &Ctx, llvm::StringRef Name,
@@ -70,30 +68,35 @@
void log(const Context &Ctx, const llvm::Twine &Name);
/// Records an event whose duration is the lifetime of the Span object.
+/// This lifetime is extended when the span's context is reused.
+///
/// 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);
+///
/// SomeJSONExpr is evaluated and copied only if actually needed.
class Span {
-public:
+ public:
Span(const Context &Ctx, llvm::StringRef Name);
- ~Span();
- /// Returns mutable span metadata if this span is interested.
+ /// Mutable 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;
+ json::obj * const Args;
+ /// Propagating this context will keep the span alive.
+ const Context Ctx;
};
+/// 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 a Span event.
+/// This is not threadsafe when used with the same Span.
#define SPAN_ATTACH(S, Name, Expr) \
do { \
-if ((S).args() != nullptr) { \
- (*((S).args()))[(Name)] = (Expr);\
-} \
+if (auto *Args = (S).Args) \
+ (*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,
+json::obj *Args) 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)}});
-};
+
[PATCH] D42499: [clangd] Modify the Span API so that Spans propagate with contexts.
sammccall marked 5 inline comments as done.
sammccall added a comment.
In https://reviews.llvm.org/D42499#987583, @ilya-biryukov wrote:
> I've tried experimenting with that and came up with an alternative that has
> less changes to the API. Could you take a look, too?
> https://reviews.llvm.org/D42524
As discussed offline, I've tried to incorporate the big improvements from there:
- moved back to Span objects, that for now expose the derived contexts as in
your patch (and later will switch the TLS context for their lifetime)
- SPAN_ATTACH doesn't support the weird lookup that JSONRPCDispatcher uses.
Rather than changing that class's traces, I've added locks to support that
pattern locally.
Also as discussed offline, The proposed TraceEvent's endSpan/endEvent
distinction doesn't seem necessary.
The EventTracer needs to be able to:
- store some data until the span ends
- get notified when the span ends
UniqueFunction does provide this, but the interface is slightly confusing:
operator() and ~UniqueFunction both denote "when the span ends".
An object with a destructor seems a more natural way to express this, so I've
kept that change - I don't feel really strongly though, happy to change this to
something else simple.
Comment at: clangd/Context.h:165
+static Key::type> Private;
+return derive(Private, std::forward(Value));
+ }
ilya-biryukov wrote:
> This will call the `const&` version of `derive(Key, Value)`.
> To move we need to do `std::move(*this).derive(Private,
> std::forward(Value))`
Oops, good catch!
Comment at: clangd/Context.h:172
+ friend bool operator==(const Context &A, const Context &B) {
+return A.DataPtr == B.DataPtr;
+ }
ilya-biryukov wrote:
> I wonder what are the use-cases for this `operator ==`?
Sorry, this was dead code from a previous iteration. Removed
Comment at: clangd/Function.h:147
- ScopeExitGuard(Func F) : F(std::move(F)) {}
+ ScopeExitGuard(Func F) : F(llvm::make_unique(std::move(F))) {}
~ScopeExitGuard() {
ilya-biryukov wrote:
> Why do we need `unique_ptr` instead of `Optional` here?
ScopeExitGuard was broken by the recent Optional changes (when Func is
trivially copyable).
This is a minimal workaround, let's discuss on the other patch.
Comment at: clangd/Trace.h:92
+/// 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)
\
ilya-biryukov wrote:
> This is too easy to misuse unintentionally. Will this go away if we move to
> thread-local contexts? I.e. will the API leave out mutations of the context
> that can be used concurrently
> This is too easy to misuse unintentionally.
Agreed. I've restored the Span object, and restored the requirement that you
pass the span itself to SPAN_ATTACH.
This disallows "action at a distance" that can make for nice traces e.g. for
request, but can lead to thread-unsafety.
The only place where we actually use this is JSONRPCDispatcher, so I made that
do it explicitly using a lock.
It seems mostly reasonable - if you feel we should remove this feature instead
to avoid the complexity, happy to discuss that, but at least it's not in the
main tracer interface now.
> Will this go away if we move to thread-local contexts? I.e. will the API
> leave out mutations of the context that can be used concurrently
You've convinced me that requiring the user to pass SPAN_ATTACH to the span
makes sense and we should keep it even once we have implicit context passing.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42499
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42499: [clangd] Modify the Span API so that Spans propagate with contexts.
ilya-biryukov added a comment. I've tried experimenting with that and came up with an alternative that has less changes to the API. Could you take a look, too? https://reviews.llvm.org/D42524 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42499 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42499: [clangd] Modify the Span API so that Spans propagate with contexts.
ilya-biryukov added inline comments.
Comment at: clangd/Context.h:163
+
+ template Context derive(Type &&Value) const && {
+static Key::type> Private;
s/`const &&`/`&&`
Comment at: clangd/Context.h:165
+static Key::type> Private;
+return derive(Private, std::forward(Value));
+ }
This will call the `const&` version of `derive(Key, Value)`.
To move we need to do `std::move(*this).derive(Private,
std::forward(Value))`
Comment at: clangd/Context.h:172
+ friend bool operator==(const Context &A, const Context &B) {
+return A.DataPtr == B.DataPtr;
+ }
I wonder what are the use-cases for this `operator ==`?
Comment at: clangd/Function.h:147
- ScopeExitGuard(Func F) : F(std::move(F)) {}
+ ScopeExitGuard(Func F) : F(llvm::make_unique(std::move(F))) {}
~ScopeExitGuard() {
Why do we need `unique_ptr` instead of `Optional` here?
Comment at: clangd/Trace.h:92
+/// 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)
\
This is too easy to misuse unintentionally. Will this go away if we move to
thread-local contexts? I.e. will the API leave out mutations of the context
that can be used concurrently
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42499
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42499: [clangd] Modify the Span API so that Spans propagate with contexts.
sammccall updated this revision to Diff 131335.
sammccall added a comment.
Remove obsolete typedef
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
@@ -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,
-
[PATCH] D42499: [clangd] Modify the Span API so that Spans propagate with contexts.
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 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
===
