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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
