This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Tracing improvements
ClosedPublic

Authored by sammccall on Nov 16 2017, 8:09 AM.

Details

Summary

[clangd] Tracing improvements

  • Compose JSON using JSONExpr
  • Allow attaching metadata to spans (and avoid it if tracing is off)
  • Attach IDs and responses of JSON RPCs to their spans
  • When -pretty is passed, also prettyprint the trace

The downside is that large responses make the trace viewer sluggish.
We should make our responses less huge :-) Or fix trace viewer.

Event Timeline

sammccall created this revision.Nov 16 2017, 8:09 AM
sammccall edited the summary of this revision. (Show Details)Nov 16 2017, 8:10 AM
ilya-biryukov accepted this revision.Nov 22 2017, 8:06 AM
ilya-biryukov added inline comments.
clangd/JSONRPCDispatcher.h
61

NIT: maybe replace new with llvm::make_unique?

78

Why do we need unique_ptr? Are Spans non-movable?

clangd/Trace.cpp
50

Any reason why we use rval-ref instead of accepting by-value?

This revision is now accepted and ready to land.Nov 22 2017, 8:06 AM
ilya-biryukov requested changes to this revision.Nov 22 2017, 8:08 AM

Oops, accidentally accepted the revision.
There are no major issues, but still wanted to learn the reasons we use rval-refs and allocate span dynamically.

This revision now requires changes to proceed.Nov 22 2017, 8:08 AM
sammccall updated this revision to Diff 124069.Nov 23 2017, 5:04 AM
sammccall edited edge metadata.
sammccall marked an inline comment as done.

Address review comment.

clangd/JSONRPCDispatcher.h
78

Spans aren't movable, they have an explicitly declared destructor. (The copy constructor is only deprecated by this condition, but Span has a unique_ptr member).

We could make them movable, though it's not absolutely trivial (we need an extra bool to indicate that this is moved-from and the destructor should be a no-op).

I think wrapping them in a unique_ptr here is slightly simpler than implementing the right move semantics by hand, but LMK what you think.

clangd/Trace.cpp
50

Two reasons I prefer this for json::expr/obj/arr:

  • major: you almost always want to pass a new literal, or move an existing one. Passing a const Expr & is usually a mistake, and taking Expr&& makes it an error.
  • minor: expr/obj/arr aren't trivially cheap to move. We forward these around internally, taking && at every level means we only pay for the move constructor once, pass-by-value pays on every call.
ilya-biryukov accepted this revision.Nov 23 2017, 6:11 AM

LGTM

clangd/JSONRPCDispatcher.h
78

We could make them movable, though it's not absolutely trivial (we need an extra bool to indicate that this is moved-from and the destructor should be a no-op).

I kinda hate this part when dealing with movable objects, too. I still do that, mostly to avoid heap allocs, but we're not on a hot path, so it's fine both ways.
We could use llvm::Optional instead of unique_ptr to get rid of the heap alloc too.

clangd/Trace.cpp
50

Makes sense, thanks for expanding on this. I'll make sure to pass around by r-value ref too when dealing with JSON objects.

This revision is now accepted and ready to land.Nov 23 2017, 6:11 AM
ilya-biryukov added inline comments.Nov 23 2017, 7:39 AM
clangd/JSONRPCDispatcher.h
78

Oh, sorry, llvm::Optional surely doesn't help here, it would still be non-movable. Forget what I said.

This revision was automatically updated to reflect the committed changes.