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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this helps me understand where previous patch is coming from!

I have some comments on the ThreadPool part, which basically amount to trying to represent the same abstract structure with fewer pieces.
But I still want to consider whether that's the right structure (specifically: whether the Queue abstraction makes it awkward to express task interactions like diagnostics debouncing).

So please don't do any work based on these comments yet! More thoughts to come...



================
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
----------------
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).


================
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.
----------------
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!


================
Comment at: clangd/ClangdServer.h:143
+///      executed in the FIFO order.
 class ThreadPool {
 public:
----------------
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42573