[clangd] Stop capturing trace args if the tracer doesn't need them.

[sammccall]

Summary

The tracer is now expected to allocate+free the args itself.

[kadircet]

clang-tools-extra/clangd/support/Trace.cpp

91

move this near other fileds?

clang-tools-extra/clangd/support/Trace.h

86

nit: at first i parsed request in RequestArgs as a verb (e.g. i request arguments), which was confusing a little bit. Hence the comment also didn't make much sense, I only managed to error correct after seeing the usage :D

Maybe something like SetRequestArgs to emphasize that one provides request arguments using this lambda rather than receiving them?

[sammccall]

clang-tools-extra/clangd/support/Trace.cpp

91

It's public unlike the other fields, but moved it to the bottom of the public section

303

added assertions here (nonnull and empty) to guard against any API confusion

clang-tools-extra/clangd/support/Trace.h

86

This *is* actually the intention.

Changed to AttachDetails to avoid ambiguity, and rewrote the comment.

[sammccall]

address comments

[kadircet]

still LGTM. thanks!

clang-tools-extra/clangd/support/Trace.h

 class EventTracer {
 public:
   virtual ~EventTracer() = default;
 
   /// 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(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.
   // Tracers should not override this unless they need to observe strict
   // per-thread nesting. Instead they should observe context destruction.
   virtual void endSpan() {}
 
   /// Called for instant events.
public:
   ~Span();
 
   /// Mutable metadata, if this span is interested.
   /// Prefer to use SPAN_ATTACH rather than accessing this directly.
   /// The lifetime of Args is the whole event, even if the Span dies.
   llvm::json::Object *const Args;
 
 private:
+  // Awkward constructor works around constant initialization.
+  Span(std::pair<Context, llvm::json::Object *>);
   WithContext RestoreCtx;
 };
 
 /// 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 (auto *Args = (S).Args)                                                 \
       (*Args)[Name] = Expr;                                                    \
   } while (0)
 
 } // namespace trace
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/support/Trace.cpp

~JSONTracer() {
     Out.arrayEnd();
     Out.attributeEnd();
     Out.objectEnd();
     Out.flush();
   }
 
   // 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.
   // So we need to mark only duration that the span was active on the thread.
   // (Hopefully any off-thread activity will be connected by a flow event).
   // Record the end time here, but don't write the event: Args aren't ready yet.
   void endSpan() override {
     Context::current().getExisting(SpanKey)->markEnded();
void jsonEvent(llvm::StringRef Phase, llvm::json::Object &&Contents,
     Contents["tid"] = int64_t(TID);
     std::lock_guard<std::mutex> Lock(Mu);
     rawEvent(Phase, Contents);
   }
 
 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();
 
       // We don't record begin events here (and end events in the destructor)
       // because B/E pairs have to appear in the right order, which is awkward.
       // Instead we send the complete (X) event in the destructor.
 
       // If our parent was on a different thread, add an arrow to this span.
JSONSpan(JSONTracer *Tracer, llvm::StringRef Name)
             TID);
       }
     }
 
     ~JSONSpan() {
       // 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);
     }
 
     // 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};
       return Next++;
     }
 
     double StartTime;
     std::atomic<double> EndTime; // Filled in by markEnded().
     std::string Name;
     uint64_t TID;
     JSONTracer *Tracer;
-    llvm::json::Object *Args;
   };
   static Key<std::unique_ptr<JSONSpan>> SpanKey;
 
   // Record an event. ph and pid are set.
   // Contents must be a list of the other JSON key/values.
   void rawEvent(llvm::StringRef Phase,
                 const llvm::json::Object &Event) /*REQUIRES(Mu)*/ {
     // PID 0 represents the clangd process.

 }
 
 void log(const llvm::Twine &Message) {
   if (!T)
     return;
   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(
       [StartTime = Clock::now(), Name = Name.str(), &LatencyMetric] {
         LatencyMetric.record(
             std::chrono::duration_cast<std::chrono::milliseconds>(Clock::now() -
                                                                   StartTime)
                 .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::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
 // metric. Labels are span names.
 constexpr Metric SpanLatency("span_latency", Metric::Distribution, "span_name");
 
 // Span keeps a non-owning pointer to the args, which is how users access them.
 // The args are owned by the context though. They stick around until the
 // 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)
     T->endSpan();
 }
 
 void Metric::record(double Value, llvm::StringRef Label) const {
   if (!T)
     return;
   assert((LabelName.empty() == Label.empty()) &&
          "recording a measurement with inconsistent labeling");
   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
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/support/TraceTests.cpp

TEST_F(CSVMetricsTracerTest, Escaping) {
   Dist.record(1, "a\"b");
   Dist.record(1, "a\nb");
 
   EXPECT_THAT(outputLines(), ElementsAre(_, StartsWith(R"(d,dist,",",1)"),
                                          StartsWith(R"(d,dist,"a""b",1)"),
                                          StartsWith("d,dist,\"a\nb\",1"), ""));
 }
 
+TEST_F(CSVMetricsTracerTest, IgnoresArgs) {
+  trace::Span Tracer("Foo");
+  EXPECT_EQ(nullptr, Tracer.Args);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang