This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Implemented logging using Context
ClosedPublic

Authored by ilya-biryukov on Nov 27 2017, 4:51 AM.

Event Timeline

ilya-biryukov created this revision.Nov 27 2017, 4:51 AM
sammccall edited edge metadata.Nov 30 2017, 2:17 PM

This is pretty bikesheddy, but I wonder what you think about passing Ctx as the first vs last parameter.
First has some advantages (maybe I just read too much Go though):

  • it's a short expr, and F(short, long) tends to format better than F(long, short). Particularly with lambdas but also long strings.
  • it doesn't interfere with default args

It would be nice if we could be completely uniform here.

  • Updated to match the new Context implementation.
  • Logger is now stored in a global variable instead of the global Context.
  • Removed json-specific RequestContext, now storing all required information in Context.
  • reply,replyError and call are freestanding functions now.

Additionally:

  • All async methods now take Context by-value and pass it to their Callbacks or return them in futures.
  • All sync methods take Context by-ref

This is pretty bikesheddy, but I wonder what you think about passing Ctx as the first vs last parameter.
First has some advantages (maybe I just read too much Go though):

  • it's a short expr, and F(short, long) tends to format better than F(long, short). Particularly with lambdas but also long strings.
  • it doesn't interfere with default args

It would be nice if we could be completely uniform here.

To be fair it was just a random thought and I totally agree with your points. I'll update the implementation and place the Context as the first parameter everywhere instead.
Could you take a look at other changes in the patch while I'm at it?

Mostly nits - throughout there's going to be lots of judgement calls about where to propagate context and where not.
No need to get all those "exactly right", but trying to get a feel for what these answers are likely to be.

Most of the interesting stuff is around the logger itself of course :-)

clangd/ClangdServer.cpp
31

Up to you, but I like the old name better - this class is local so it's OK to be a bit opinionated.

36

Yikes, I can see how we got here, but we really don't get to move out of something we received an lvalue-reference to...

I think a safer idiom here is turning a whole lambda into a destructor:

auto Fulfil = MakeDestructor([&]{
   DonePromise.set_value(std::move(Ctx));
});

I'm not sure if LLVM has a shared utility for this, I've seen it in other codebases. Either way we could just define it here.

(Or we could copy the context to avoid the awkwardness)

220–223

You dropped the end of this comment

221

I don't quite understand what this is saying should be better. Is it saying we do these two things together instead of running them in parallel?

And why not clone the context instead?

clangd/ClangdUnit.cpp
436

why are we dropping the context here?
It's important that in general we don't store the ctx in the object and blindly reuse it, but passing it into the constructor for use *in* the constructor seems fine.

Reading on... OK, it saves a *lot* of plumbing to avoid attributing this one API call. I still don't totally understand how the CppFile API/lifetime works, so up to you.

533

nit: this wrapping has become *really* weird over time - can we change the comment to /* allow changing OldPreamble */ to try to fix it?

clangd/ClangdUnit.h
257

I think this is a bad merge - signatureHelp has moved to CodeComplete.h

clangd/GlobalCompilationDatabase.h
37

Do we want Ctx in this interface?
It's an extension point, and the ability for embedders to track calls flowing between their API calls and their extension points is one of the reasons we have ctx...

clangd/JSONRPCDispatcher.h
29

does this class need to be public? Is it staying around for the long haul?

39

Is this temporary?

sammccall added inline comments.Dec 6 2017, 8:36 AM
clangd/JSONRPCDispatcher.cpp
22

Maybe we don't need the Key suffix, as it's in the type... we should spend our verbosity elsewhere

23

RequestTracer?

24

RequestID

47

why?

clangd/Logger.cpp
16

Any need to have an empty logger, rather than just have log() branch?

22

There are no non-global loggers. Just L, or Current, or something?

26

I actually think this would be clearer without the assert - this can statically never be null

clangd/Logger.h
28

why is this not called log()?

31–32

Why does this need to be public?

37–38

nit: these seem like low SNR. Are we actually worried about people trying to move sessions?

sammccall added inline comments.Dec 7 2017, 1:34 AM
clangd/JSONRPCDispatcher.cpp
23

actually, RequestSpan I think - "tracer" is pretty confusing at global scope

70

Depending on a span tracer existing seems needlessly brittle. What about

if (auto *Tracer = Ctx->get(TracerKey))
  SPAN_ATTACH(*Tracer, ...)
  • Pass Context by const-ref instead of by-ref.
  • Don't expose global logger, it is only used in log method now.
  • Renamed logImpl to log.

Udpated the patch after changes in Context

ilya-biryukov marked 15 inline comments as done.
  • Copy Context in forceReparse's call to scheduleCancelRebuild.
  • Renamed Key<> for ID, Out and Span.
  • Removed the FIXME
  • Got rid of getExisting(RequestSpan)
  • Remove EmptyLogger, branch in log instead.
  • Renamed GlobalLogger to L
  • Addressed review comments.
ilya-biryukov added inline comments.Dec 11 2017, 4:40 AM
clangd/ClangdServer.cpp
220–223

Missed that. Thanks.

221

The first implementation did not allow to copy the Context, hence the comment. I've removed it and the context is now copied.

clangd/ClangdUnit.cpp
436

Exactly, plumbing it here is hard, so we're dropping the context here for now. Most of the useful work is still done outside the constructor, so this seems fine.
The threading patch will make it easier to pass proper Context here. I've added a FIXME.

clangd/ClangdUnit.h
257

It is totally a bad merge. Thanks.

clangd/GlobalCompilationDatabase.h
37

I think we do, but I'd add this in a separate patch. (We're currently not changing the behavior, simply plumbing the Context through all the APIs).
Happy to come up with a follow-up patch, though.

clangd/JSONRPCDispatcher.cpp
47

This was a reminder to myself to discuss whether we want it globally in tracing+logging, not only in JSONOutput.
Removed it from the code.

70

Done.
Turned out to be SPAN(**Tracer, .... (Note the double dereference).

clangd/JSONRPCDispatcher.h
29

I'm not sure. I'd rather keep it private, but we don't have the right abstractions to hide it yet, right?
I'd say this is not directly related to the current patch and should be addressed separately.

39

Renamed back to log.
It's a leftover from an older version of the patch.

clangd/Logger.cpp
22

This is really just a preference. To me the global prefix is not about distinguishing it from other things in this file, but rather about signalling what is this thing so that I don't have to read the declaration before reading the usages to understand what's going on. Having L or Current requires to be in the context of the file.

Updated the code to use L instead.

clangd/Logger.h
31–32

It shouldn't be. Removed.

37–38

If they do, that will lead to a bug (destructor being called twice), so it feels safer to delete the corresponding constructors.

  • Remove mention of globalLogger() from the comments.
  • Use derive(key, val) instead of derive().add(key, val).
ilya-biryukov added inline comments.Dec 11 2017, 8:50 AM
clangd/ClangdServer.cpp
36

Thanks for suggestion. I totally agree, callbacks on scope exit are better.

I couldn't find an utility like that in LLVM, but I think it's a useful one. I'll add this locally to this file, but maybe we should make this helper public and put it into clangd?
What are you thoughts on that?

I'll update the implementation and place the Context as the first parameter everywhere instead.
Could you take a look at other changes in the patch while I'm at it?

Just pinging this - there's still places where Context is in a different position (at least in ClangdServer)

clangd/ClangdServer.cpp
36

Sounds good. I'm wary of adding lots of tiny utility headers - maybe if we squint it could go in Function.h?

clangd/ProtocolHandlers.h
32

We should use one name or the other. Fine to do this to keep the patch small, but leave a FIXME?

ilya-biryukov marked 7 inline comments as done.
  • Added a helper that runs functions in destructor.
  • Fixed compilation after rebase.
  • Got rid of FulfilContextPromiseGuard
  • Added a FIXME to remove the Ctx typedef.

I think all the comments are addressed now and this change is ready for another round of review.
Let me know if I missed something.

clangd/ClangdServer.cpp
36

Then Function.h it is :-)

clangd/ProtocolHandlers.h
32

Added a FIXME, will send a follow-up change that removes it after this change lands.

  • Make Context the first parameter everywhere.
  • Add a SFINAE check to UniqueFunction's constructor to avoid creating it from non-callable objects.
ilya-biryukov added inline comments.Dec 12 2017, 8:03 AM
clangd/Function.h
44 ↗(On Diff #126551)

We need this check to make ClangdServer::codeComplete overloads work. Without it, overloading thinks UniqueFunction can be constructed from any type, so we end up getting ambig between the two overloads when calling it in a following way: codeComplete(Ctx, File, Pos, CCOpts, OverridenContents).

This is now ready for review.

  • Also check that return type of Callable is correct.
  • Make Context the first parameter in rename and a few helper methods.

These should be the last usages where Context is not a first paramter.

  • Make Context the first parameter in findDefinitions and CppFile::rebuild.
  • Make Context the first parameter in invokeCodeCompletion.
sammccall accepted this revision.Dec 13 2017, 1:35 AM

Thanks! Just style and comment nits left really.

clangd/ClangdServer.cpp
231

this is a pretty gross return type.
Even after (if?) we replace tagged with context, we'll still have completionlist and context.
Should we define a struct here?

(If not, we should consider having the context first, for consistency)

536–537

This wrapping has gone beyond the point of readability for me.
Pull out using RebuildAction = llvm::Optional<...>(const Context &)?

clangd/ClangdServer.h
256

Is this comment obsolete? (and others below)
Ctx is passed by value so I don't understand what it could mean.

262–269

(and this comment)

301

ctx to first param

317–319

ctx to first param

321–322

ctx to first param

clangd/ClangdUnit.cpp
426

(while here, this should really be make_shared)

clangd/ClangdUnit.h
175–176

ctx to first param

269

ctx to first param

clangd/CodeComplete.cpp
594

ctx to first param

clangd/Function.h
143 ↗(On Diff #126554)

FWIW I think this assert adds more noise than value - this can clearly only be triggered by someone instantiating the template directly, which is probably better avoided using namespace detail or so.

157 ↗(On Diff #126554)

I'm struggling to think of cases where moving these guards is the right solution.
Can we delete the move ops until we need them? It seems to make this class almost trivial.

clangd/GlobalCompilationDatabase.cpp
107–109

Add a fixme to get a context in here?

clangd/JSONRPCDispatcher.cpp
139

There are *lots* of Context::empty() calls in this file.
Maybe conceptually clearer for JSONRPCDispatcher to own a "background" context that it derives from? It'd be empty for now in either case, so not a big deal.

The idea being that explicit Context::empty() calls are a bit suspect, but these ones aren't really "different" from each other - we may want to provide an extension point to customize it, but there'd only be one.

Up to you.

142

I'm not sure this is better than unconditionally attaching the optional ID.
Being able to distinguish between request-with-no-ID and no-request seems like a good thing.

clangd/JSONRPCDispatcher.h
29

It's related in that context takes some of the job of JSONOutput, so now we have a messy overlap.
A FIXME to work out how JSONOutput's role should shrink in light of Context is enough, I think.

clangd/Protocol.h
24

I think tihs is unused?

unittests/clangd/ClangdTests.cpp
31

nit: revert?

This revision is now accepted and ready to land.Dec 13 2017, 1:35 AM
ilya-biryukov marked 13 inline comments as done.
  • Removed obsolete comments.
  • Moved ScopeExitGuard to namespace detail.
  • Added FIXMEs to pass context to compilation database.
  • Addded a FIXME to JSONOutput.
  • Removed unused #include directive.
  • Revert accidental whitespace change.
ilya-biryukov marked 13 inline comments as done.Dec 13 2017, 3:15 AM
ilya-biryukov added inline comments.
clangd/ClangdServer.cpp
231

I agree, callback-based APIs look way better though.
Since we'll eventually switch to callback-based versions anyway, I'm gonna leave it as pair for now.

536–537

I'd refrain from doing that as this code will go away in the threading patch anyway. Hope that's ok.

clangd/ClangdUnit.cpp
426

It can't, because CppFile's constructor is private. This code is also going away in the threading patch.

clangd/Function.h
143 ↗(On Diff #126554)

Moved it to namespace detail.

157 ↗(On Diff #126554)

We need moves as we return the class by value from the onScopeGuard function. Neither of these will compile without move constructors:

auto Guard = onScopeGuard([]() { /*...*/ });
auto Guard2(onScopeGuard([]() { /*...*/ }));
auto Guard3{onScopeGuard([]() { /*...*/ })};
clangd/JSONRPCDispatcher.cpp
139

Since these usages are easy to spot (almost all uses of Context::empty in JSONRPCDispatcher.cpp), I'd leave them as is for now. It's easy to update them later.

142

I like that we can avoid having a two-level unwrap for RequestID. If we're going down that route, maybe we should have a dedicated struct instead:

// name may be bad, just for the sake of example.
struct RequestData { 
  llvm::Optional<json::Expr> RequestID;
  Span RequestSpan;
  JSONOutput Out;
};

Key<RequestData> ReqData;

That would also make it clear that we either attach all of those fields or none of them.
Given that RequestID is not yet public, I'd keep it as is and change it in a separate patch.

sammccall accepted this revision.Dec 13 2017, 3:38 AM
sammccall added inline comments.
clangd/Function.h
157 ↗(On Diff #126554)

Of course :-)
As you pointed out offline, making the member Optional<Func> would let you use the default move operators at least.

ilya-biryukov marked 2 inline comments as done.
  • Simplify ScopeExitGuard by using llvm::Optional<>.

Merged with head

This revision was automatically updated to reflect the committed changes.