sammccall updated this revision to Diff 297555.
sammccall marked an inline comment as done.
sammccall added a comment.
address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89135/new/
https://reviews.llvm.org/D89135
Files:
clang-tools-extra/clangd/support/Trace.cpp
clang-tools-extra/clangd/support/Trace.h
clang-tools-extra/clangd/unittests/support/TraceTests.cpp
Index: clang-tools-extra/clangd/unittests/support/TraceTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/support/TraceTests.cpp
+++ clang-tools-extra/clangd/unittests/support/TraceTests.cpp
@@ -186,6 +186,11 @@
StartsWith("d,dist,\"a\nb\",1"), ""));
}
+TEST_F(CSVMetricsTracerTest, IgnoresArgs) {
+ trace::Span Tracer("Foo");
+ EXPECT_EQ(nullptr, Tracer.Args);
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/support/Trace.h
===================================================================
--- clang-tools-extra/clangd/support/Trace.h
+++ clang-tools-extra/clangd/support/Trace.h
@@ -79,8 +79,13 @@
/// 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(llvm::StringRef Name, llvm::json::Object *Args);
+ /// The tracer may capture event details provided in SPAN_ATTACH() calls.
+ /// In this case it should call AttachDetails(), and pass in an empty Object
+ /// to hold them. This Object should be owned by the context, and the data
+ /// will be complete by the time the context is destroyed.
+ virtual Context
+ beginSpan(llvm::StringRef Name,
+ llvm::function_ref<void(llvm::json::Object *)> AttachDetails);
// Called when a Span is destroyed (it may still be active on other threads).
// beginSpan() and endSpan() will always form a proper stack on each thread.
// The Context returned by beginSpan is active, but Args is not ready.
@@ -146,6 +151,8 @@
llvm::json::Object *const Args;
private:
+ // Awkward constructor works around constant initialization.
+ Span(std::pair<Context, llvm::json::Object *>);
WithContext RestoreCtx;
};
Index: clang-tools-extra/clangd/support/Trace.cpp
===================================================================
--- clang-tools-extra/clangd/support/Trace.cpp
+++ clang-tools-extra/clangd/support/Trace.cpp
@@ -53,9 +53,12 @@
// We stash a Span object in the context. It will record the start/end,
// and this also allows us to look up the parent Span's information.
- Context beginSpan(llvm::StringRef Name, llvm::json::Object *Args) override {
- return Context::current().derive(
- SpanKey, std::make_unique<JSONSpan>(this, Name, Args));
+ Context beginSpan(
+ llvm::StringRef Name,
+ llvm::function_ref<void(llvm::json::Object *)> AttachDetails) override {
+ auto JS = std::make_unique<JSONSpan>(this, Name);
+ AttachDetails(&JS->Args);
+ return Context::current().derive(SpanKey, std::move(JS));
}
// Trace viewer requires each thread to properly stack events.
@@ -85,9 +88,9 @@
private:
class JSONSpan {
public:
- JSONSpan(JSONTracer *Tracer, llvm::StringRef Name, llvm::json::Object *Args)
+ JSONSpan(JSONTracer *Tracer, llvm::StringRef Name)
: StartTime(Tracer->timestamp()), EndTime(0), Name(Name),
- TID(llvm::get_threadid()), Tracer(Tracer), Args(Args) {
+ TID(llvm::get_threadid()), Tracer(Tracer) {
// ~JSONSpan() may run in a different thread, so we need to capture now.
Tracer->captureThreadMetadata();
@@ -125,7 +128,7 @@
// Finally, record the event (ending at EndTime, not timestamp())!
Tracer->jsonEvent("X",
llvm::json::Object{{"name", std::move(Name)},
- {"args", std::move(*Args)},
+ {"args", std::move(Args)},
{"dur", EndTime - StartTime}},
TID, StartTime);
}
@@ -133,6 +136,8 @@
// May be called by any thread.
void markEnded() { EndTime = Tracer->timestamp(); }
+ llvm::json::Object Args;
+
private:
static int64_t nextID() {
static std::atomic<int64_t> Next = {0};
@@ -144,7 +149,6 @@
std::string Name;
uint64_t TID;
JSONTracer *Tracer;
- llvm::json::Object *Args;
};
static Key<std::unique_ptr<JSONSpan>> SpanKey;
@@ -277,12 +281,11 @@
T->instant("Log", llvm::json::Object{{"Message", Message.str()}});
}
-// Returned context owns Args.
-static Context makeSpanContext(llvm::Twine Name, llvm::json::Object *Args,
- const Metric &LatencyMetric) {
+// The JSON object is event args (owned by context), if the tracer wants them.
+static std::pair<Context, llvm::json::Object *>
+makeSpanContext(llvm::Twine Name, const Metric &LatencyMetric) {
if (!T)
- return Context::current().clone();
- WithContextValue WithArgs{std::unique_ptr<llvm::json::Object>(Args)};
+ return std::make_pair(Context::current().clone(), nullptr);
llvm::Optional<WithContextValue> WithLatency;
using Clock = std::chrono::high_resolution_clock;
WithLatency.emplace(llvm::make_scope_exit(
@@ -293,9 +296,15 @@
.count(),
Name);
}));
- return T->beginSpan(Name.isSingleStringRef() ? Name.getSingleStringRef()
- : llvm::StringRef(Name.str()),
- Args);
+ llvm::json::Object *Args = nullptr;
+ Context Ctx = T->beginSpan(
+ Name.isSingleStringRef() ? Name.getSingleStringRef()
+ : llvm::StringRef(Name.str()),
+ [&](llvm::json::Object *A) {
+ assert(A && A->empty() && "Invalid AttachDetails() placeholder!");
+ Args = A;
+ });
+ return std::make_pair(std::move(Ctx), Args);
}
// Fallback metric that measures latencies for spans without an explicit latency
@@ -307,8 +316,9 @@
// beginSpan() context is destroyed, when the tracing engine will consume them.
Span::Span(llvm::Twine Name) : Span(Name, SpanLatency) {}
Span::Span(llvm::Twine Name, const Metric &LatencyMetric)
- : Args(T ? new llvm::json::Object() : nullptr),
- RestoreCtx(makeSpanContext(Name, Args, LatencyMetric)) {}
+ : Span(makeSpanContext(Name, LatencyMetric)) {}
+Span::Span(std::pair<Context, llvm::json::Object *> Pair)
+ : Args(Pair.second), RestoreCtx(std::move(Pair.first)) {}
Span::~Span() {
if (T)
@@ -323,7 +333,9 @@
T->record(*this, Value, Label);
}
-Context EventTracer::beginSpan(llvm::StringRef Name, llvm::json::Object *Args) {
+Context EventTracer::beginSpan(
+ llvm::StringRef Name,
+ llvm::function_ref<void(llvm::json::Object *)> AttachDetails) {
return Context::current().clone();
}
} // namespace trace
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
