This is an archive of the discontinued LLVM Phabricator instance.

Performance tracing facility for clangd.
ClosedPublic

Authored by sammccall on Oct 19 2017, 2:19 AM.

Details

Summary

This lets you visualize clangd's activity on different threads over time,
and understand critical paths of requests and object lifetimes.
The data produced can be visualized in Chrome (at chrome://tracing), or
in a standalone copy of catapult (http://github.com/catapult-project/catapult)

This patch consists of:

  • a command line flag "-trace" that causes clangd to emit JSON trace data
  • an API (in Trace.h) allowing clangd code to easily add events to the stream
  • several initial uses of this API to capture JSON-RPC requests, builds, logs

Example result: https://photos.app.goo.gl/12L9swaz5REGQ1rm1

Caveats:

  • JSON serialization is ad-hoc (isn't it everywhere?) so the API is limited to naming events rather than attaching arbitrary metadata. I'd like to fix this (I think we could use a JSON-object abstraction).
  • The recording is very naive: events are written immediately by locking a mutex. Contention on the mutex might disturb performance.
  • For now it just traces instants or spans on the current thread. There are other things that make sense to show (cross-thread flows, non-thread resources such as ASTs). But we have to start somewhere.

Event Timeline

sammccall created this revision.Oct 19 2017, 2:19 AM
ioeric edited edge metadata.Oct 19 2017, 3:38 AM

Looks good in general. A few nits.

clangd/JSONRPCDispatcher.cpp
230

Why this empty block?

clangd/Trace.cpp
90

Would it make sense to have a helper class which handles starting/stopping in, say, constructor/destructor? ... so that we don't need to worry about forgetting to stop?

clangd/tool/ClangdMain.cpp
98

Is this intended? Start tracing even if there is an error?

unittests/clangd/TraceTests.cpp
47

Add a comment on why we skip unexpected keys?

sammccall updated this revision to Diff 119562.Oct 19 2017, 3:57 AM
sammccall marked 3 inline comments as done.

Address review comments.

clangd/JSONRPCDispatcher.cpp
230

Oops, was tracing here but it didn't turn out to be useful. Reverted.

clangd/Trace.cpp
90

I didn't do this for a couple of reasons:

  • it obscures the static-stateful nature of this code, making it look like e.g. you could dynamically turn on tracing while clangd is running. (Which would be cool, but wouldn't currently work)
  • it's some more boilerplate, and isn't actually very convenient to ClangdMain (since start() is conditional)
clangd/tool/ClangdMain.cpp
98

Oops, thanks!

ioeric accepted this revision.Oct 19 2017, 5:03 AM

lgtm

This revision is now accepted and ready to land.Oct 19 2017, 5:03 AM
ilya-biryukov added inline comments.Oct 22 2017, 11:41 PM
clangd/Trace.cpp
88

Maybe use unique_ptr<Tracer> (or even llvm::Optional<Tracer>)? Otherwise we leak memory and don't flush the stream if users of tracing API forget to call stop(). (I think I'm on the same page with @ioeric here about forgetting a call to stop()).

Having a static global of non-pod type is a no-go, but we could use static local variables:

llvm::Optional<Tracer>& getTracerInstance() {
 static llvm::Optional<Tracer> Tracer;
 return Tracer;
}
103

NIT: Use FIXME instead of TODO

sammccall marked an inline comment as done.Oct 23 2017, 9:08 AM
sammccall added inline comments.
clangd/Trace.cpp
88

So Eric's solution of a scoped object that would be owned by main() makes sense to me, and I'm happy to add it (though personally not sure it carries its weight).

However I don't understand *your* suggestion :-)

Having a static global of non-pod type is a no-go, but we could use static local variables:

llvm::Optional<Tracer>& getTracerInstance() {
  static llvm::Optional<Tracer> Tracer;
  return Tracer;
}

So it's not global, which means we're not calling Optional() on startup, which would actually be totally safe (I guess the constructor should be marked as constexpr).

But we *are* calling ~Optional() and therefore ~Tracer() at shutdown. And maybe ~Tracer() calls something that logs, and llvm::errs() has already been destroyed... *this* is why avoid static objects, right?

Otherwise we leak memory and don't flush the stream if users of tracing API forget to call stop()

The intended time to call stop() is at the end of the program. If you forget to call it, we won't be leaking memory :-) We indeed won't flush the stream, but that wouldn't be safe (see above).

The other option is the user is calling start() and stop() multiple times, and they call start() twice without stopping. We already assert on this, and anyone who doesn't reproducibly hit that assert is well into racy UB-land (logging methods may be called concurrently with each other, but everything else must be synchronized).

Overall I'm not really seeing what scenario we can detect and safely do something about.

sammccall updated this revision to Diff 119864.Oct 23 2017, 9:10 AM

Address FIXME now that r316330 is in.

ilya-biryukov added inline comments.Oct 24 2017, 1:09 AM
clangd/Trace.cpp
88

Forget what I said, you're right. Tracer has state, can be reconfigured multiple times and there's no guarantee our references won't be dangling on destruction of Tracer.

I do think Eric's solution is good, though. For all the same reasons why RAII is good.

sammccall updated this revision to Diff 121247.Nov 2 2017, 1:34 AM

Provide RAII-like interface to trace functionality.

Note that we may want to provide a backend API here.

ioeric accepted this revision.Nov 2 2017, 1:41 AM

still lgtm

clangd/Trace.h
39

createJSON is a bit confusing... maybe just create since json is just the underlying representation which users don't have to know about?

sammccall updated this revision to Diff 121252.Nov 2 2017, 2:00 AM
sammccall marked an inline comment as done.

createJSON -> create

sammccall added inline comments.Nov 2 2017, 2:01 AM
clangd/Trace.h
39

I think later we might have an API for non-JSON sessions (crazy google internal tracing tools) but until then, create is a fine name.

sammccall updated this revision to Diff 121253.Nov 2 2017, 2:04 AM

clang-format

This revision was automatically updated to reflect the committed changes.
RKSimon added inline comments.
clang-tools-extra/trunk/unittests/clangd/TraceTests.cpp
57 ↗(On Diff #121257)

@sammccall PVS Studio is reporting a potential null dereference here: https://www.viva64.com/en/b/0771/ (Snippet 8)

kadircet added inline comments.Oct 28 2020, 10:26 AM
clang-tools-extra/trunk/unittests/clangd/TraceTests.cpp
57 ↗(On Diff #121257)