sammccall added inline comments.
================
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)});
----------------
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)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96027/new/
https://reviews.llvm.org/D96027
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
