Do new pools "own" the current domain?

[favonia]

I could not tell whether a newly created pool "owns" the current domain. In one of the test files, there are consecutive calls to setup_pool:

domainslib/test/test_task.ml

Lines 40 to 41 in 15f04f3

	let pool1 = Task.setup_pool ~num_additional_domains:2 ~name:"pool1" () in
	let pool2 = Task.setup_pool ~num_additional_domains:2 ~name:"pool2" () in

Therefore, it seems okay to have multiple pools sharing the same domain (that is, the current domain). However, the documentation gave me the impression that I have to manually spawn new domains before setting up pools to avoid overlapping:

domainslib/lib/task.mli

Lines 10 to 13 in 15f04f3

	val setup_pool : ?name:string -> num_additional_domains:int -> unit -> pool
	(** Sets up a task execution pool with [num_additional_domains + 1] domains
	including the current domain. If [name] is provided, the pool is mapped to
	[name] which can be looked up later with [lookup_pool name].

I wonder what is the intended semantics? What is the ownership relation between pools and domains? This might be related to #55.

[kayceesrk]

Thanks for bringing this up. The documentation attached to setup_pool is not correct. The current domain does not get attached to the pool that it sets up. The num_additional_domains terminology is also wrong and should be changed to num_domains to reflect how pools are really set up.

The confusion arises from the fact that during T.run pool task, the calling domain also participates in the execution of the task, waiting for the task to run to completion. If the aim is to only submit a task but not wait for completion, one may use T.async pool task, which adds a task to the pool and the domains in the pool execute the task. The result of executing the task can be obtained later using T.await.

CC @Sudha247.

[gasche]

I came here because I was bitten by the new API which I found confusing, and I was about to open an issue saying "hey maybe we should rename this parameter into number_additional_domains. Running in circles!

The code I wrote would do

let num_domains = Domain.recommended_domain_count () in
let pool = Domainslib.setup_pool ~num_domains () in
(* ... *)

and @Sudha247 sharp-eyedly remarked that this was wrong. It may be wrong but it is also very easy to write, which suggests that the current API is problematic.

Can we do another round of brainstorming on how to propose a clear API here?

Remark 1: there is a fundamental difference between the status of pool domains ("worker domains") and the status of the "caller domain". Worker domains loop doing pool work forever. The caller domain wants to do the smallest amount of pool computation to get its result. It is correct to say that the tasks run on [num_domains + 1] domains, but the status of both sides of the addition is very different.

Remark 2: currently the documentation and implementation of get_num_domains is inconsistent with the ~num_domains parameter:

val get_num_domains : pool -> int
(** [get_num_domains pool] returns the total number of domains in [pool]
    including the parent domain. *)

let get_num_domains pool =
  let pd = get_pool_data pool in
  Array.length pd.domains + 1

Short-term suggestions:

• num_worker_domains?
• Have a code example in setup_pool that uses recommended_domain_count in the ... recommended way.

Medium-term suggestion. Taking a step back, I think that the problem comes from the fact that Domainslib.Task mixes two things together, an abstraction for a pool of worker domains and an abstraction for asynchronous tasks. I think that the API could be reworked to separate the two, on one side have an asynchronous tasks abstractions with explicit schedulers, and on the other a domain pool abstraction with an explicit (concurrent) work-channel (the interface for other domains to send work to the domain pool). The connection between the two is that the library offers a task scheduler that expects a pool work-channel as parameter.

[gasche]

(If others agree that the current state is not really satisfying, I think the present issue could be reopened.)

[haesbaert]

Would just add a small nit here, IMHO it's easier to only expose an API that gives you "maximum number of workers", and to not write code that expects to be a participating worker. As in:
D0 -> forks recommended_domain_count -> wait for completion
It's ok to waste one extra domain, no one is going to jail, the cost is negligible.

[gasche]

I agree that it's a simpler model to think about. If we wanted to go this way (present this as the expected usage mode), we could probably also simplify the implementation of Task (which currently I find a bit hairy) to follow this model.

On the other hand: I can see how to use Domainslib to build "parallel building blocks" (parallel_for, parmap etc.) that should remain low-overhead in the case where they are given small inputs and it's not worth it to parallelize computations. (This way most users can call them unconditionally instead of having to implement their own fast-path-on-small-inputs logic.) Currently doing this with Domainslib always involves setting up an effect handler (for Task.run), but no inter-domain communication (if we end up in the sequential path of parallel_for where no fork-join is performed). With your proposed approach, if used in the implementation, we always pay for at least one inter-domain communication.

[haesbaert]

I agree that it's a simpler model to think about. If we wanted to go this way (present this as the expected usage mode), we could probably also simplify the implementation of Task (which currently I find a bit hairy) to follow this model.

On the other hand: I can see how to use Domainslib to build "parallel building blocks" (parallel_for, parmap etc.) that should remain low-overhead in the case where they are given small inputs and it's not worth it to parallelize computations. (This way most users can call them unconditionally instead of having to implement their own fast-path-on-small-inputs logic.) Currently doing this with Domainslib always involves setting up an effect handler (for Task.run), but no inter-domain communication (if we end up in the sequential path of parallel_for where no fork-join is performed). With your proposed approach, if used in the implementation, we always pay for at least one inter-domain communication.

And I think this is fine, if you can't afford to pay one IPC or whatever to do a parallel computation on N Domains (where N is close to be normally 8 in 2022) I'd say something is already wrong. If that's too expensive than the computation should not be parallelized (it wouldn't pay off anyway).

(full disclaimer, I'm not using Domainslib, so I might be talking out of line here)

[gasche]

On an unrelated note: one benefit I would expect from rethinking the API a bit is that the Task operations (async, await, parallel_for etc.) should not require a pool parameter anymore. Currently we can only call those functions under run, and we have to pass the pool to run, but also to these functions. (Note that, unlike await, async is not implemented with an effect currently, and this would have to change to be able to do this simplification.)

[favonia]

As the "author" of this issue, I support rethinking the API. My original concern is really about the obvious inconsistencies (which are now fixed). Maybe get_num_domains indeed should not add one to the size of the pool. I understand the collision of different intuitions/views and sadly do not have a good proposal here. 😅

[gasche]

There are also implementation issues arising from this API confusion. For example, currently if you share a pool between two independent domains and they both call Task.run on separate tasks, one of the domain may end up running tasks from the other domain instead of completing its own task. This is unexpected (to me), but closer to what the previous documentation would say (in a certain sense the calling domain participates to the pool as any of the worker domains during the duration of run). Many parts of the documentation are in fact consistent with this behavior, but some are not (for example the description of the behavior of pools with 0 domains).

[favonia]

This is unexpected (to me), but closer to what the previous documentation would say

Well, I think "previous documentation" here might be misleading. It was not clear (to me) before #78 whether the calling domain (which can be different from the domain setting up the pool) participates at all, and thus I made the PR to document the existing behaviors more precisely. Perhaps you meant the name num_additional_domains matches your intuition better? 🙂 Personally I think there will be confusing cases either way until we redesign the API. (BTW, I believe pools with 0 domains should still match the general description. I added that extra text in #82 just to make the documentation more complete, not because it's a special case handled differently.)

[Sudha247]

Thanks everyone for the comments! Reopening the issue to continue discussion.

@favonia is right in that the previous documentation was inconsistent with the implementation. I can see how the current one could be confusing too.

[kayceesrk]

I found num_additional_domains to be smelly. It is not clear what it is an "addition" of. What's the missing part? Similar to @gasche, my thinking was that we should separate out the notion of pools and asynchronous tasks being executed on the pools. Following that reasoning, a pool is an abstraction that has N "worker" domains. Hence, creating the pool should take num_domains and not num_additional_domains.

Given the above, we should fix the buggy get_num_domains.

I envision task abstraction to be separate from the pool. You can submit tasks to the pool to be executed. Task.async pool task submits the task to the pool. The caller returns immediately with a promise. Task.await pool promise waits for the promise to be resolved. If the promise is not resolved at the time of the call, rather than sitting idle, the current domain also services tasks on the pool.

I also agree with @gasche that, now that we have a top-level handler in Task.run, the pool argument for everything except Task.run could be made an optional argument. I said something to this effect earlier this week to Sudha.

I'm in support of revising the API. The types pool and promise may be in their own modules Pool and Promise to indicate that they are separate? Similarly, the parallel iterators may be moved to their own module Iterator?