This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Pass Context implicitly using TLS.
ClosedPublic

Authored by sammccall on Jan 24 2018, 6:49 PM.

Details

Summary

Instead of passing Context explicitly around, we now have a thread-local Context object Context::current() which is an implicit argument to every function.
Most manipulation of this should use the WithContextValue helper, which augments the current Context to add a single KV pair, and restores the old context on destruction.

Advantages are:

  • less boilerplate in functions that just propagate contexts
  • reading most code doesn't require understanding context at all, and using context as values in fewer places still
  • fewer options to pass the "wrong" context when it changes within a scope (e.g. when using Span)
  • contexts pass through interfaces we can't modify, such as VFS
  • propagating contexts across threads was slightly tricky (e.g. copy vs move, no move-init in lambdas), and is now encapsulated in the threadpool

Disadvantages are all the usual TLS stuff - hidden magic, and potential for higher memory usage on threads that don't use the context. (In practice, it's just one pointer)

Diff Detail

Event Timeline

sammccall created this revision.Jan 24 2018, 6:49 PM

LG. Let's move the APIs to callbacks and start passing Contexts everywhere.

sammccall updated this revision to Diff 131564.Jan 26 2018, 2:48 AM

Rebase on the Span changes that landed as r323511

sammccall updated this revision to Diff 132132.Jan 31 2018, 3:13 AM

Rebase and fix tests.

Rather than deleting the future<Context> return values, made them future<void>.
This is NFC because only the tests ever used the future at all, and they
ignored the context.

sammccall retitled this revision from [clangd] RFC: Pass Context implicitly using TLS. to [clangd] Pass Context implicitly using TLS..Jan 31 2018, 3:19 AM
sammccall edited the summary of this revision. (Show Details)
ilya-biryukov added inline comments.Jan 31 2018, 3:54 AM
clangd/Context.h
77

Maybe we could return const Context& here and have a separate setCurrent() method for replacing the current context?
Most of the clients shouldn't mutate the contexts anyway, and finding the few that do seems easier if there's a separate method for that.

202

Where do we use it? Can't we make move ctor deleted too?

clangd/JSONRPCDispatcher.h
60

reply's signature is scary now.
Should we change it to accept something request-specific that can be found in the Context by the clients? WDYT?

PS we could change it after the patch lands, just wanted to get your opinion on this

clangd/Threading.cpp
37

Maybe replace with std::tie(Request, Ctx) = std::move(RequestQueue.front())?

clangd/Trace.cpp
131

Maybe use parentheses instead of braces for initialization?

sammccall marked 3 inline comments as done.Jan 31 2018, 4:50 AM

Thanks!

clangd/Context.h
77

Done (named swap).

In fact the only caller is WithContext, which is a good sign.

It would be possible to make swap() private, since WithContext is in this file. I tend to lean against it because embedders will do explicit context manipulation and WithContext may not be the right primitive for them. So I've left the WithContext guidance as "just a comment". Could be convinced though.

202

Good catch! This was used in an old version of the patch.

clangd/Trace.cpp
131

Braces here are to avoid most-vexing-parse: with parens this is a function declaration.

(I'm not sure this is worth a comment, as the compiler flags it if you change it)

I don't see a patch corresponding to the fixed comments. arc diff didn't go through?

clangd/Trace.cpp
131

LG. No need for a comment.

sammccall marked 2 inline comments as done.Jan 31 2018, 4:56 AM
sammccall added inline comments.
clangd/JSONRPCDispatcher.h
60

Oops, missed this.
To me, the scariest part is that "reply" is a very generic name and its parameter is approximately "anything".

In principle your suggestion makes sense, but passing Context::current() explicitly is plain weird, and passing Context::current().get(kRequestToken) seems pretty grungy in the context.

I think I'd be more partial to either giving it a more specific name like replyToLSPRequest which hints at the current-context requirement, or actually making all our LSP handlers accept a UniqueFunction<Expected<T>> and obviating the need for reply altogether.

Either way, I'd prefer to address this separately (partly because I'm not sure which is best).

sammccall updated this revision to Diff 132145.Jan 31 2018, 5:06 AM

Address review comments

I don't see a patch corresponding to the fixed comments. arc diff didn't go through?

Indeed, sorry. Should be up to date now.

ilya-biryukov added inline comments.Jan 31 2018, 5:07 AM
clangd/JSONRPCDispatcher.h
60

Totally agree, let's address it separately.

ilya-biryukov accepted this revision.Jan 31 2018, 5:12 AM

LGTM modulo two NITs.
Really excited to see this land.

clangd/Context.cpp
32

Maybe call it swapCurrent. It's longer, but won't allow to mistake it for a member swap function.
Since it's almost never used, having a longer name is not a big deal.

clangd/Context.h
196

I think it can't be null after move ctor is deleted.
Maybe always store a Context instead of unique_ptr and call swap unconditionally?

This revision is now accepted and ready to land.Jan 31 2018, 5:12 AM
sammccall marked 2 inline comments as done.Jan 31 2018, 5:40 AM
sammccall added inline comments.
clangd/Context.h
196

Right! much nicer.

sammccall updated this revision to Diff 132151.Jan 31 2018, 5:41 AM
sammccall marked an inline comment as done.

address review comments

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.