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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
