This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Fix tracing now that spans lifetimes can overlap on a thread.
ClosedPublic

Authored by sammccall on Feb 13 2018, 7:09 PM.

Details

Summary

The chrome trace viewer requires events within a thread to strictly nest.
So we need to record the lifetime of the Span objects, not the contexts.

But we still want to show the relationship between spans where a context crosses
threads, so do this with flow events (i.e. arrows).

Before: https://photos.app.goo.gl/q4Dd9u9xtelaXk1v1
After: https://photos.app.goo.gl/5RNLmAMLZR3unvY83

(This could stand some further improvement, in particular I think we want a
container span whenever we schedule work on a thread. But that's another patch)

Event Timeline

sammccall created this revision.Feb 13 2018, 7:09 PM
ilya-biryukov accepted this revision.Feb 14 2018, 5:28 AM

LG with a few NITs.

clangd/Trace.cpp
133

Maybe make all fields except EndTime const?

clangd/Trace.h
46

It feels awkward that endSpan() does not have any parameters explicitly connecting it to beginSpan().
We could change beginSpan() to return a callback that would be called when Span ends instead, that would make the connection clear, albeit it's not very useful for JSONTracer.

Not sure we should change it, just thinking out loud :-)

This revision is now accepted and ready to land.Feb 14 2018, 5:28 AM
sammccall added inline comments.Feb 15 2018, 12:40 AM
clangd/Trace.cpp
133

Good idea - what's safe to read vs write wasn't clear.
Ended up just making this a class and encapsulating EndTime instead though.

clangd/Trace.h
46

So destructor-of-context is still the preferred way of observing the end of an event. endSpan() is kind of an attractive nuisance that we need for chrome tracing.

Added comments to explain this.

Given that, I think we want a signature that:

  • you basically can't use without first implementing and understanding the primary interface
  • can be defaulted to do nothing without interfering with the primary interface (this also gives us backwards compatibility).

I'm not sure think it's a problem that this is a bit obscure...

address review comments

This revision was automatically updated to reflect the committed changes.

Hi,

It seems that your submission have broken the Windows and Linux Builds for 'clangd'.

This is the error message:

/home/xxx/llvm-root/llvm/tools/clang/tools/extra/clangd/Trace.cpp:54:76: error: call of overloaded ‘make_unique(clang::clangd::trace::{anonymous}::JSONTracer*, llvm::StringRef&, clang::clangd::json::obj*&)’ is ambiguous

make_unique<JSONSpan>(this, Name, Args));
                                      ^

Thanks,

r325239 should fix, sorry!

r325239 should fix, sorry!

Build now is OK. Thanks @sammccall.