Allow user supplied executor/thread-pool (and potentially other options) to be used in go, thread macros and chan

Question

Right now thread pools used by core.async are defined as globals, they are shared by all go or thread macro on their respective pools:

see https://github.com/clojure/core.async/blob/ac0f1bfb40237a18dc0f03c0db5df41657cd23a6/src/main/clojure/clojure/core/async.clj#L391-L411
and https://github.com/clojure/core.async/blob/ac0f1bfb40237a18dc0f03c0db5df41657cd23a6/src/main/clojure/clojure/core/async/impl/dispatch.clj#L16

It would be very useful to have more control over this, hopefully not via a simple setter, like the one we have in core for agents (set-agent-send-executor!), since it only allows one pool to be used at a time.
One possible solution would be to do that via an option map parameter to thread-call, and/or some other mechanism for go blocks (go-call with the same kind of signature). Making this parameter a map would also allow to further extend internals in the future, if necessary, without requiring heavy changes to the API.

Answer 1

Comment made by: mpenet

I have a patch here https://github.com/mpenet/core.async/commit/1c2b0a9af9a5e891af0f1631b8debd337a73999d that adds this features.

It adds 2 macros: go* and thread**. These are identical to go/thread, but takes first an option map, then the body. The option map can receive an :executor key with an clojure.core.async.impl.protocols/Executor implementation.

I took the liberty to move the thread executor in the same namespace as the ioc/go one, and have it support the same interface instead of using a raw j.u.c Executor instance. Both instances declared in this namespace are now lazy (in a delay block), preventing useless thread pool initialisations/leaks.

Lastly I also modified thread-call to support that same option map.

I'll attach the .patch file to the ticket shortly.

Answer 2

Comment made by: mpenet

well actually this patch is incomplete, I am just discovering that ioc-macros rewriting leads to calls on the global thread pool (via put(image: , take) callbacks in particular).

We could allow to pass an executor at channel creation to force ops on this channel to run there relatively easily (partial example here https://github.com/mpenet/core.async/commit/1449b3842052033cdf917ae92259ad9789722fdb).

I believe this is how manifold handles it as well, but there might be more unknowns for me at this point.

Answer 3

Comment made by: mpenet

patch with chan modifications included. chan now can take an additional parameter that would be an clojure.core.async.impl.protocols/Executor compatible executor, this executor will then be used instead of the global default pool when supplied.

Answer 4

Comment made by: mpenet

provide patch as single commit (more readable). You can also view the changes here: https://github.com/mpenet/core.async/commit/e7dea04553935863d1abb1880d84bbdc273854ec

Answer 5

Comment made by: gshayban

Hi Max. I think that having some better control over this is necessary.

There are basically two needs:
1) callbacks need to wake up parked go-blocks.
2) running heavier threads (which may block and thus cannot share the same pool)

Right now channel callbacks don't know what they are waking up. I think that's a good design. If they are waking up a thread, it's only indirectly doing so by delivering a promise (link: 1)
I think there should be no mention of a specific executor in channels.clj Having multiple different places to run go-blocks is probably an antipattern as go-blocks should not be blocking or doing I/O.

I very much welcome some control over the thread block's executors though.

(link: 1) https://github.com/clojure/core.async/blob/master/src/main/clojure/clojure/core/async.clj#L137

Answer 6

Comment made by: mpenet

Hi Ghadi,

Thanks for the feedback, but I don't think you read my patch correctly.

I do not change the case you mention (promise), the only thing that's changed is that for the few cases where clojure.core.async.impl.dispatch/run is called, meaning the function is run in a globally defined threadpool used by everything in core.async with the exception of thread, it can ***optionally be done via a function argument supplied threadpool. The default is unchanged, if you don't pass an executor you get the current master core.async behavior without any change at all.

Few people mentioned that need, some publicly, some in private, myself included, and I know of a team who expressed the same concerns and just switched one of their systems from core.async to manifold because the latter offer more knobs of that sort.

In the end it's about control, it changes nothing to core.async and it's current execution model if you don't care about it, but it's important to people who needs that kind of fine grained tuning.

It came up again a couple of days ago on twitter: https://twitter.com/puredanger/status/576378306062262272 and you could certainly find reference of it in the irc logs as well.

I would have released this as a lib if that was possible, but the code implicated is sometimes very deeply rooted and make it nearly impossible to do without forking the whole library, hence that patch.

Answer 7

Comment made by: mpenet

same as previous patch, added missing arity to promise-chan

Answer 8

Comment made by: stu

Consider my upvote to be for the problem, not necessarily for the approach taken here.

It seems to me that go and thread are separate cases, deserving of separate consideration. It isn't clear to me why chan needs consideration at all, and I don't want to have to read a patch (or learn about impl namespaces) to discover why. Can somebody explain the case for chan without reference to implementation?

Is there a dev list discussion on this?

Answer 9

Comment made by: mpenet

Sure, as long as this is improved/fixed.

If you would read the patch, or the source for channels you would see
that channels run their callbacks on a threadpool.

When this patch was submitted I spent time discussing it with Timothy
Baldridge on IRC and made some changes at his suggestion. Since you
probably share offices with him I suggest you discuss the current
approach with him as he is more familiar with the codebase than both
of us.

Answer 10

Comment made by: lxsameer

Hey guys,

I added couple of functions to threadpool ns in order to allow user to provide an executor for the main thread pool and the one for the thread macro.

It's a draft at the moment, I wanted to know other people opinion on it before spend more time on it. Please checkout the
commit on my repo at:

https://github.com/Codamic/core.async/commit/5cfa3ac34f963dc67329722149d041629a6c6e6f

It should gives you the idea. Looking forward to your opinion.

Cheers

Answer 11

Comment made by: mpenet

After re-reading the details of this issue I created years ago I am actually now agreeing with both Stu and Ghadi comments, I think my understanding of core.async go internals is a lot better now, threading at this level (and callbacks) is merely for "supervision"/context switching, so fine grained control over these is not so important. I think I was probably influenced by the design of manifold, which differs quite a bit in that respect (and other ways), but these 2 libraries are quite different.

I think there is still a case for a better thread-call function and potentially another macro like thread that takes a user supplied executor.

• Would a patch that adds an arity to tread-call to allow to pass the executor be considered?

• What form could take a macro version of that new thread-call arity (if it's something wanted; I think a new macro would make more sense as thread takes a &body that might be a bit tricky to retrofit there).

• Would a patch that makes the various threadpool initialization "lazy" (delayed) be considered?

Answer 12

Reference: https://clojure.atlassian.net/browse/ASYNC-94 (reported by mpenet)