[PATCH] D42499: [clangd] Modify the Span API so that Spans propagate with contexts.

[Sam McCall via Phabricator via cfe-commits]

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<typename std::decay<Type>::type> Private;
+    return derive(Private, std::forward<Type>(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<Type>(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<Func>(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