Page MenuHomePhabricator

[clangd] Implemented logging using Context

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
sammccall added inline comments.Dec 6 2017, 8:36 AM
22 ↗(On Diff #126728)

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

23 ↗(On Diff #126728)


24 ↗(On Diff #126728)


48 ↗(On Diff #126728)


16 ↗(On Diff #126728)

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

22 ↗(On Diff #126728)

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

26 ↗(On Diff #126728)

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

29 ↗(On Diff #126728)

why is this not called log()?

32–33 ↗(On Diff #126728)

Why does this need to be public?

38–39 ↗(On Diff #126728)

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
23 ↗(On Diff #126728)

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

68 ↗(On Diff #126728)

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
199 ↗(On Diff #126728)

Missed that. Thanks.

200 ↗(On Diff #126728)

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

626 ↗(On Diff #126728)

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.

257 ↗(On Diff #126728)

It is totally a bad merge. Thanks.

37 ↗(On Diff #126728)

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.

48 ↗(On Diff #126728)

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.

68 ↗(On Diff #126728)

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

29–31 ↗(On Diff #126728)

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.

41 ↗(On Diff #126728)

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

22 ↗(On Diff #126728)

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.

32–33 ↗(On Diff #126728)

It shouldn't be. Removed.

38–39 ↗(On Diff #126728)

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
30–31 ↗(On Diff #126728)

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)

30–31 ↗(On Diff #126728)

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

32–33 ↗(On Diff #126728)

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.

30–31 ↗(On Diff #126728)

Then Function.h it is :-)

32–33 ↗(On Diff #126728)

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
44 ↗(On Diff #126728)

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.

206–207 ↗(On Diff #126728)

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)

560–561 ↗(On Diff #126728)

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

252 ↗(On Diff #126728)

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

259–264 ↗(On Diff #126728)

(and this comment)

308 ↗(On Diff #126728)

ctx to first param

332 ↗(On Diff #126728)

ctx to first param

334–335 ↗(On Diff #126728)

ctx to first param

616 ↗(On Diff #126728)

(while here, this should really be make_shared)

175–176 ↗(On Diff #126728)

ctx to first param

261–262 ↗(On Diff #126728)

ctx to first param

594 ↗(On Diff #126728)

ctx to first param

143 ↗(On Diff #126728)

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 #126728)

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.

106–111 ↗(On Diff #126728)

Add a fixme to get a context in here?

145 ↗(On Diff #126728)

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.

148 ↗(On Diff #126728)

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.

29–31 ↗(On Diff #126728)

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.

24 ↗(On Diff #126554)

I think tihs is unused?

31 ↗(On Diff #126728)

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.
206–207 ↗(On Diff #126728)

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.

560–561 ↗(On Diff #126728)

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

616 ↗(On Diff #126728)

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

143 ↗(On Diff #126728)

Moved it to namespace detail.

157 ↗(On Diff #126728)

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([]() { /*...*/ })};
145 ↗(On Diff #126728)

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.

148 ↗(On Diff #126728)

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.
157 ↗(On Diff #126728)

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

Closed by commit rCTE320576: [clangd] Implemented logging using Context (authored by ibiryukov, committed by ). · Explain WhyDec 13 2017, 4:52 AM
This revision was automatically updated to reflect the committed changes.