Page MenuHomePhabricator

[clangd] XPC transport layer
AbandonedPublic

Authored by jkorous on Jun 25 2018, 1:09 PM.

Details

Reviewers
sammccall
Summary

Implementation of alternative transport layer for macOS based on XPC.

Based on these two other patches:
https://reviews.llvm.org/D48559
https://reviews.llvm.org/D48560

Diff Detail

Event Timeline

jkorous created this revision.Jun 25 2018, 1:09 PM
arphaman added inline comments.
xpc/test-client/ClangdXPCTestClient.cpp
52

We should probably use the main queue here too to ensure that we don't get bit by concurrent handler invocations.

jkorous added inline comments.Jun 26 2018, 7:31 AM
xpc/test-client/ClangdXPCTestClient.cpp
52

As far as I understand it this is taken care of by calling dispatch_main().

Per man dispatch_main:
"MAIN QUEUE
[...]

Programs must call dispatch_main() at the end of main() in order to process blocks submitted to the main queue."
jkorous updated this revision to Diff 152888.Jun 26 2018, 7:34 AM

Two changes in test client based on internal review by XPC folks:

  • Removed cleanup code at the end of main() as dispatch_main() never returns.
  • Removed check for conn as xpc_connection_create() is guaranteed to succeed.
arphaman added inline comments.Jul 13 2018, 2:32 PM
xpc/test-client/ClangdXPCTestClient.cpp
52

Not really. The documentation for the parameter states the following:

The GCD queue to which the event handler block will be submitted. This parameter may be NULL, in which case the connection's target queue will be libdispatch's default target queue, defined as DISPATCH_TARGET_QUEUE_DEFAULT. The target queue may be changed later with a call to xpc_connection_set_target_queue().

So the handler blocks will be submitted to the concurrent queue (DISPATCH_TARGET_QUEUE_DEFAULT) which means that they might end up running concurrently.

Just an initial couple of thoughts here, haven't yet been through in detail.

Mostly I wonder if we can use slightly different abstractions in D48559 to so the JSON/XPC parts get the code reuse we want but the work required to call one vs the other is smaller. Need to think about this more.

Features.inc.in
1

nit: can we give these the same name?
I'd vote for dropping the _SUPPORT too: either CLANGD_ENABLE_XPC or CLANGD_BUILD_XPC.

Maybe the latter is better, since an environment variable controls the runtime behavior anyway.

tool/ClangdMain.cpp
261

is there a reason this is an environment variable rather than an -xpc flag?

Apart from this being (mostly) what we do elsewhere, it seems like if the process spawning clangd wants XPC suppport, but clangd was built without it, then you want the process to fail and exit rather than sit there listening on stdin. Flags have this behavior, env variables do not.

262

the duplication here suggests to me the factoring isn't quite right.

naively, ISTM we should be able to have two implementations of an interface here rather than an actual ifdef'd server loop. But I'll dig into the implementation to try to understand a bit more.

Hi Sam, I am definitely open to discussion about the right abstraction.

I will push patches rebased on TOT and changes based on your comments today or tomorrow and I am trying to figure out how could we use your Transport abstraction proposal (https://reviews.llvm.org/D49389) without Json in it's interface in the meantime.

Features.inc.in
1

Good point. I will change that.

tool/ClangdMain.cpp
261

I agree that flag would be a natural choice here (and was my original intention) but unfortunately Launchd doesn't support that for spawning bundled XPC services.

Do you mean I should check for the env variable also in case clangd is not built with XPC support and conditionally exit?

262

I totally understand your thoughts - I originally started with the same idea in the end wasn't able to implement it in simple enough way. Ultimately I decided to prefer simplicity over nice abstraction.
But I am happy to discuss this and make changes accordingly.

jkorous marked 7 inline comments as done.Jul 19 2018, 8:14 AM
jkorous added inline comments.
xpc/test-client/ClangdXPCTestClient.cpp
52

Sorry, my bad.

jkorous marked 2 inline comments as done.Jul 19 2018, 8:24 AM

BTW Just for the record - I put the rebased & updated patch here: [clangd] XPC WIP https://reviews.llvm.org/D49548

jkorous planned changes to this revision.Jul 25 2018, 6:41 AM
jkorous abandoned this revision.Aug 8 2018, 8:24 AM

We decided to go with a different solution:
https://reviews.llvm.org/D50452