[PATCH] D42573: [clangd] The new threading implementation

Fri Feb 2 04:30:10 PST 2018

ilya-biryukov marked 3 inline comments as done.
ilya-biryukov added inline comments.

================
Comment at: clangd/ClangdServer.h:140
+///      the working threads as soon as an idle thread is available.
+///    - scheduleOnQueue will schedule to a specific queue. Requests from the
+///      same queue are not processed concurrently. Requests in each queue are
----------------
sammccall wrote:
> As discussed offline, Queue's semantics are very similar to a thread, except:
>  - cooperatively scheduled: Queues do not yield until idle
>  - limited in parallelism by the size of the threadpool
> 
> In the interests of keeping things simple and familiar, I think we should start out by using `std::thread`.
> We can use a semaphore to limit the parallelism (I'm fine with doing this in the first iteration, but urge you to consider leaving it out until we see actual problems with using the CPU as our limit). This should give both properties above (if the Queue only releases the semaphore when idle).
> If we still have performance problems we may need to switch to a multiplexing version, though I have a hard time imagining it (e.g. even on windows, thread spawn should be <1us, and memory usage is trivial compared to anything that touches an AST).
Done exactly that. There's a new abstraction in the patch that manages creating threads and waiting for them to finish for us. It seems pretty simple, please take a look and let me know what you think.

================
Comment at: clangd/ClangdServer.h:141
+///    - scheduleOnQueue will schedule to a specific queue. Requests from the
+///      same queue are not processed concurrently. Requests in each queue are
+///      executed in the FIFO order.
----------------
sammccall wrote:
> Similarly, the free requests look a lot like standalone threads, with a few enhancements that are implementable but also possible YAGNI.
>  - running code complete first is roughly[1] equivalent to elevating the thread's priority (no portability shim in llvm yet, but it's pretty easy). I don't think this needs to be in the first patch.
>  - limiting parallelism can be done with semaphores. In fact we can easily express something like "up to 18 queued tasks, up to 20 free tasks, up to 20 total", which nice for latency.
> 
> [1] I see both advantages and disadvantages, happy to discuss more!
We're not setting the priorities in the first version, but certainly agree we should add that later.
And we're using a simple semaphore to limit the number of actively running threads.

================
Comment at: clangd/ClangdServer.h:143
+///      executed in the FIFO order.
 class ThreadPool {
 public:
----------------
sammccall wrote:
> So overall here, I think that we can drop `ThreadPool` without much impact on the design.
> `Queue` would stay, as a wrapper around `std::thread` that lets you add tasks to its runloop. It would be owned by FileData, and shutdown would be triggered by its destructor.
> 
> The advantage is one less layer here to understand, and an attractive nuisance to tweak over time.
> The downsides I see:
>   - threading is no longer abstracted away, so changes to it are less isolated
>     I think it's likely that we get away with the model staying simple. If it gets complicated, we'll want to add the abstraction back in. But this isn't java, we can abstract when we need it :-)
>   - RunSynchronously escapes into Scheduler. This is for the worse I think, but only slightly.
I tried fiddling with the idea and ended up abandoning the `RequestQueue` in favor of the `std::queue` with explicit locks.
There's only one place where we queue requests now (processing the updates/reads of the ASTs), everything else is managed by creating separate `std::thread`s.

================
Comment at: clangd/TUScheduler.cpp:23
+    return;
+  Worker = std::thread([&]() {
+    while (true) {
----------------
sammccall wrote:
> I tend to find thread bodies more readable as a member function, up to you
Moved it as a member function and made a (questionable) decision that clients of the class are responsible for running that function on a separate thread themselves. 
It allows to reuse the abstraction that runs the threads and waits for them to finish (also used when spawning threads for completion, and similar).
It shouldn't be too bad, given that we have just a single client. Even though the interface is not particularly nice.

Happy to chat about it.

================
Comment at: clangd/TUScheduler.cpp:258
+    std::lock_guard<CountingSemaphore> BarrierLock(Barrier);
+    // XXX: what if preamble got built by this time?
+    // if (!Preamble)
----------------
sammccall wrote:
> this seems like where shared_ptr might help us out if we're willing to deal with it.
> If we captured a shared_ptr<File> here, then we'd want to call getPossiblyStalePreamble inside rather than outside the lambda.
> 
> So this would look something like:
>  - FileASTThread does thread shutdown as part of destructor, and requires the TUScheduler to outlive it
>  - TUScheduler keeps (conceptually): active files `StringMap<shared_ptr<FileASTThread>>`, and shutdown handles `vector<weak_ptr<thread>>`
>  - removal takes the shared_ptr out of the map, transfers ownership of the shared_ptr to a new thread which does nothing (except destroy the shared_ptr), and stashes a weak_ptr to the thread as a shutdown handle, which get looped over in the destructor.
> 
> Is this simpler? I'm not sure. It's maybe more thematically consistent :-) and it solves your "preamble got built" problem.
> 
I ran into related problem with completion threads: we'd have to store somewhere in order to wait for there completion.
Ended up creating a thing that counts all running threads and allows to wait for all of them to finish.
I feel it looks very simple now, please let me know what you think

================
Comment at: clangd/TUScheduler.h:37
+/// Limits the number of threads that can acquire this semaphore.
+class CountingSemaphore {
+public:
----------------
sammccall wrote:
> Probably belongs in Threading.h instead.
> 
> Maybe just Semaphore? If we have *two* semaphores in clangd, we've jumped the shark!
> (Isn't a non-counting semaphore just a mutex?)
> 
Renamed to `Semaphore` and moved to `Threading.h`.

================
Comment at: clangd/TUScheduler.h:50
+
+class FileASTThread {
+public:
----------------
sammccall wrote:
> Maybe FileWorker? this has-a thread, rather than is-a
Renamed it to `ASTWorker`. It doesn't really know anything really caret about files directly, just manages the AST.

================
Comment at: clangd/TUScheduler.h:59
+
+  void setDone();
+
----------------
sammccall wrote:
> AFAICS, we require callers to setDone() before destroying, and the caller always does this immediately before destroying.
> 
> Why not just make the destructor do this?
I still think that `setDone()` is useful to check the invariant that new requests are not being scheduled after FileData is removed.

================
Comment at: clangd/TUScheduler.h:72
+  std::condition_variable RequestsCV;
+  RequestQueue Requests;
+  std::shared_ptr<CppFile> File;
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > Having trouble seeing which parts of this are really used.
> > My understanding is that the ThreadPool+Queue abstraction is going away.
> > This means this is a deque + lock + ... a "done" state of some kind? Is more of it still live?
> > 
> > I'd probably expect the 'done' state to be redundant with the one here... If it's just a deque + a lock, I'd lean towards making it really thin, maybe even a plain struct, and putting it in this file.
> > 
> > But not sure of your thoughts here.
> I'll probably remove `RequestsQueue` or turn it into something really simple like you suggest.
> It certainly has more state than we need with this patch. This is one of the reasons this patch is far from being ready.
queue + lock + done is how we do this in the new patch. `RequestQueue` is removed

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42573