Make ~num_domains in Task.setup_pool optional

[dra27]

The first commit closes #87

I then updated the tests to utilise this default - this changes the behaviour of dune runtest which now runs those tests in parallel (which presumably was wanted??).

I changed the argument order for all tests to put the number of domains last - that's an optional change, but I think it looks slightly better for the execution of the sequential and parallel implementations. fib_par 42 and fib 42 now both compute $Fib_{42}$ where before fib_par 42 computed $Fib_{43}$ using 42 domains.

• Making this change makes me wonder if ~num_domains should be changed to be $> 0$ rather than $\ge 0$. I think from a caller's perspective, it's more logical to say that you want a pool of $n$ domains and it's an implementation detail that ~num_domains is the number of additional domains that should be spawned to facilitate that. It also results in a lot of - 1 being peppered around the examples! What do you think?

[gasche]

The latter part of your question ("-1 peppered around") is related to the discussion in #77. We have to be a bit careful about "a pool of n domains" means.

Your default choice of recommended_count - 1 assumes that thre is a single pool, and that there is a single non-pool domain. Are those reasonable assumptions? (Over-estimating the domain pool size by a bit is not very costly.) Do we have experience with programs using several Domainslib pools, and why are they doing it?

[kayceesrk]

I think from a caller's perspective, it's more logical to say that you want a pool of domains and it's an implementation detail that ~num_domains is the number of additional domains that should be spawned to facilitate that. It also results in a lot of - 1 being peppered around the examples! What do you think?

This is related to #77 (comment).

[kayceesrk]

The idea to have multiple pools came from @yurug for the Tezos usecase. The idea was that you could have separate pools for parallelising crypto and serialization. This would allow Tezos to model an ideal machines with a fixed number of domains for crypto and serialisation so that they can estimate the gas costs well.

[gasche]

Slightly more comments;

• I think that the suggestion of Make num_domains argument to Task.setup_pool optional #87 is reasonable: make the simple code easy. It will also be easier to adapt later if we figure out a better "best practice" than Domain.recommended_domain_count () - 1, all the users using the default value need no change.
• This being said, an alternative is to show a good code example in the documentation of setup_pool that does the correct thing (with the -1, etc.) and explains why. I think that we should in fact do both. Explain stuff to users scales better to their complex cases than picking the right default.
• In the long run, why do "simple users" have to setup a pool explicitly? I think that we could consider having a richer API for Task.run that would be in charge of setting up a cached pool by default, without users having to think of the pool as a separate object.

(What's a reasonable "simplest workflow" for Task.run ? Do we expect people to call Task.run separately on separate tasks in their application, for example at the granularity of a single parmap or toplevel paralell_for call, or do they stick a single Task.run handler in their main function and forget about it?)

[Sudha247]

Your default choice of recommended_count - 1 assumes that there is a single pool, and that there is a single non-pool domain. Are those reasonable assumptions?

I think that's a reasonable assumption for default values. For other cases we can expect users to customise pools for their needs. As an example, I've found this to not work on our benchmarking servers with isolated cores. Domain.recommended_domain_count returns the number of non-isolated cores, consistent with its intended behaviour, while we want the benchmarks to run on isolated cores. Right now we pass hard-coded values as arguments for setting up pools. ocaml-processor has the functionality to automate it.

This being said, an alternative is to show a good code example in the documentation of setup_pool that does the correct thing (with the -1, etc.) and explains why.

Agreed! That's definitely useful to have.

I think that we could consider having a richer API for Task.run that would be in charge of setting up a cached pool by default, without users having to think of the pool as a separate object.

We discussed this a while ago (to be fair, before effect handlers were introduced to manage tasks); having a default pool would indeed simplify the Task API. A downside to this is when there is more than one application using domainslib interacting with each other, with a default pool of its own. Also, some applications could use the default pool and others set up their own, again possibly creating more domains than ideal.