[PATCH] D48240: Try again to implement a FIFO task queue

[Zachary Turner via Phabricator via llvm-commits]

zturner added a comment.

In https://reviews.llvm.org/D48240#1136073, @chandlerc wrote:

> In https://reviews.llvm.org/D48240#1136061, @zturner wrote:
>
> > In https://reviews.llvm.org/D48240#1136052, @chandlerc wrote:
> >
> > > I also wonder whether you want to decouple this serialized execution from the thread creation and management. Could you accept a thread pool instead, and enqueue work on the thread pool? You can do this by having each task on the thread pool, on completion, check the queue for more work, and if there is some, schedule a new task on the thread pool. The reason why this is interesting is because if you have, say, a system thread pool that maybe gets a lot of other work, you can intersperse these tasks on it without overloading the system. It also allows the client of this class to dictate which threads are allowable threads for these tasks to run by providing a correct thread pool. My hope is that worst case, creating a one-thread thread pool and handing it to this class would be the equivalent of what you have here, but with a more clean separation of roles. I understand it would add some complexity, for example you'd have to handle the case where no work is currently in-flight when `asyncImpl` finishes adding the task to the queue and start a new batch. But this seems reasonably tolerable.
> >
> >
> > Isn't using multiple threads strictly incompatible with the idea of it being serialized or synchronized?  With multiple threads, you will have work overlapping, which confuses the purpose of what this is for.  If someone comes up with a use case for it, then maybe, but otherwise, YAGNI?
>
>
> It isn't about *using* multiple threads, its about selecting which threads run things.
>
> Whether tasks are serialized in their execution is independent of how many threads they run on. Its just that there doesn't *need* to be more than one if they are serialized.
>
> But a system thread pool might expose the equivalent of <1 thread.
>
> Think of this example. You have three different systems which each want to use this infrastructure to run a series of asynchronous tasks, but for each system the series of tasks must run in-order (FIFO) and without overlap. You can re-use a single threadpool with potentially fewer than three threads for all three of these systems. This allows the client to control the rate of work -- perhaps only allowing at most two operations to happen concurrently.
>
> While with three systems and two threads this seems somewhat odd, if you have *many* systems (maybe 100s) all using this to coordinate a series of tasks, you might not want hundereds of threads, all spinning on their own queues. Instead, it seems nicer to have a single threadpool, sized appropriately for the amount of resources on the system, and have all of the users of this class enqueue their serial sequence of tasks into this shared threadpool.


Would you be willing to allow that as a followup patch?  I want to get the single threaded case working with this patch and the extension to allow tasks that return values, and then maybe add that on in a followup?  (I'm not punting on it either, I actually will do it, but as you said it seems orthogonal so maybe makes sense to do in a followup)


https://reviews.llvm.org/D48240