This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Stop exposing Futures from ClangdServer operations.
ClosedPublic

Authored by sammccall on Feb 9 2018, 6:33 AM.

Details

Summary

LSP has asynchronous semantics, being able to block on an async operation
completing is unneccesary and leads to tighter coupling with the threading.

In practice only tests depend on this, so we add a general-purpose "block until
idle" function to the scheduler which will work for all operations.

To get this working, fix a latent condition-variable bug in ASTWorker, and make
AsyncTaskRunner const-correct.

Event Timeline

sammccall created this revision.Feb 9 2018, 6:33 AM
sammccall updated this revision to Diff 133609.Feb 9 2018, 6:36 AM

Tidy up comment, and revert notify_all to notify_one - it was a red herring.

ilya-biryukov added inline comments.Feb 12 2018, 4:47 AM
clangd/Threading.h
60

Maybe remove the comment or add more context (i.e. add references) on why the overflow is buggy?

66

Maybe move this helper to .cpp file?

68

Maybe keep the locking part out of this helper? It's often desirable to hold the lock after wait(). This will model how std::condition_variable::wait is defined.

83–84

Add LLVM_NODICARD here?
For that particular method maybe we could have two overload: with and without the deadline, i.e.

void waitForAll() const;
LLVM_NODISCARD bool waitForAll(Deadline D) const;

There are places (like the destructor of this class) where the first overload is used and consuming the return value is just adding noise, but clients passing the deadline (e.g. blockUntilIdle()) should definitely consume the return value.

unittests/clangd/ClangdTests.cpp
785–786

Do we need to change this test?
It was specifically designed to keep the second request from overriding the first one before it was processed.

sammccall updated this revision to Diff 133857.Feb 12 2018, 7:05 AM
sammccall marked 3 inline comments as done.

Change AsyncTaskRunner::wait() to be LLVM_NODISCARD when used with a deadline.
Restore NoConcurrentDiagnostics test to its former glory (with comments)
Other comment fixes.

clangd/Threading.h
60

Done. (Making specific reference to time_point::max() + overflow probably makes it clear why it's unusable?)

66

it's public, and it's a template - what am I missing?

68

I tried this, but find the API just way too grungy - we need to pass a unique_lock<std::mutex>&?
Generally I'd hope we're aiming for something a little higher level than the standard library!

We don't actually ever need to do anything with the lock held. If we did, what about passing a lambda?

unittests/clangd/ClangdTests.cpp
785–786

You're right. It wasn't obvious to me that this was what it was doing or why, so it seemed easy to simplify.
Added some comments and a sanity check assertion, reverted the logic changes.

(The change might make sense anyway - we rely on sleep already, a second sleep would get rid of all the synchronization without adding latency or changing behavior, but not in this patch)

ilya-biryukov added inline comments.Feb 12 2018, 7:38 AM
clangd/TUScheduler.cpp
286

Change to notify_all()? Otherwise we might wake up some thread waiting on blockUntilIdle(), but not the worker thread.

314

We should probably call notify_all() here.
Having multiple blockUntilIdle might not work otherwise.

clangd/Threading.h
66

Sorry, I missed that it's used outside Threading.h

68

The higher-level abstraction would certainly be appealing to me as well and I would welcome it.
I don't think we get it here, though. It's actually a bit confusing that the function calls into STL's versions of wait and wait_until, has a similar name, but does a slightly different thing.

unittests/clangd/ClangdTests.cpp
783

Maybe use paren initializers? Looks a bit less curly :-)

std::atomic<int> Count(0);
sammccall updated this revision to Diff 133868.Feb 12 2018, 8:07 AM
sammccall marked 3 inline comments as done.

Review comments

clangd/TUScheduler.cpp
286

Done.

In fact I had changed this to notify_all, and changed it back because it's safe:

  • this class isn't threadsafe
  • therefore there are only two interesting threads, the caller thread and the worker thread
  • each thread can only wake up the other one, so there's no unsatisfied waiter

But as discussed offline, notify_all() is just a less surprising default, and will fail in more obvious ways when we write threading bugs.

clangd/Threading.h
68

Yeah, fair enough.

unittests/clangd/ClangdTests.cpp
783

This is a member initializer, I think I have to use this syntax :-(

ilya-biryukov accepted this revision.Feb 13 2018, 12:15 AM

LGTM

unittests/clangd/ClangdTests.cpp
783

You're right, you have to do braced init here.
Sorry, my mistake.

This revision is now accepted and ready to land.Feb 13 2018, 12:15 AM
This revision was automatically updated to reflect the committed changes.