[PATCH] D96027: [clangd] Trace queue state for each TUScheduler action.

Kadir Cetinkaya via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Feb 5 04:26:52 PST 2021


kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

LGTM!



================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1054
     Requests.push_back({std::move(Task), std::string(Name), steady_clock::now(),
-                        std::move(Ctx), Update, Invalidation,
-                        std::move(Invalidate)});
+                        std::move(Ctx), std::move(QueueCtx), Update,
+                        Invalidation, std::move(Invalidate)});
----------------
sammccall wrote:
> kadircet wrote:
> > what if we just pushed the span here and kept it alive in the queue? it is move-able at the moment.
> > 
> > our tracing contract seem to be saying beginSpan/endSpan calls would always form a proper stack, i am not sure about its importance to the jsontracer, but I don't think rest of our tracers cares (and i think it would be nice for that trace to actually span the whole time in queue).
> > what if we just pushed the span here and kept it alive in the queue?
> Mechanically it's a little more complicated than that, but I get what you're saying - unfortunately it doesn't quite work.
> 
> > it is move-able at the moment.
> (in fact not: it has a WithContext member whose move operators are deleted)
> 
> > our tracing contract seem to be saying beginSpan/endSpan calls would always form a proper stack
> Yes - this is in fact documented as the *only* point of endSpan - it marks the strictly-stacking lifetimes of stack-allocated trace::Spans objects themselves. Tracers that don't need this strict stacking are supposed to watch for the destruction of the context frames created by beginSpan instead. This extends the span over context-propagation, at the cost of no longer strictly stacking.
> 
> > i am not sure about its importance to the jsontracer
> The chrome trace viewer (which jsontracer targets) really does require strict stacking. I'm not 100% sure, but I think the behavior was just to silently discard events that don't nest properly.
> 
> > I don't think rest of our tracers cares (and i think it would be nice for that trace to actually span the whole time in queue).
> Right, so our private out-of-tree tracer follows request/context timelines rather than thread ones, and doesn't require strict nesting. It completely ignores endSpan(), and ends events when the context gets destroyed instead. That's why we bundle up the QueueCtx into the request and then destroy it as soon as we dequeue - it makes that trace actually span the whole time in queue :-)
> 
> (Happy to chat more about this or add some comments/diagrams/something if we should make this more clear)
thanks makes sense.

> The chrome trace viewer (which jsontracer targets) really does require strict stacking. I'm not 100% sure, but I think the behavior was just to silently discard events that don't nest properly.

I was expecting this to be the case, but wanted to be sure.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96027/new/

https://reviews.llvm.org/D96027



More information about the cfe-commits mailing list