-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename num_additional_domains
to num_domains
for setup_pool
#78
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @favonia! The change certainly makes sense. I'll cut a release with this before 5.0.
|
||
Raises {!Invalid_argument} when [num_additional_domains] is less than 0. *) | ||
Raises {!Invalid_argument} when [num_domains] is less than 0. *) | ||
|
||
val teardown_pool : pool -> unit | ||
(** Tears down the task execution pool. *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unable to comment below - do you think it's worth mentioning in the run
function that the calling domain also participates in executing the task, thereby running it on n+1
domains where n
is the number of domains in the pool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and I'm happy to update this PR after understanding the semantics better. I suppose it would happen only when await
or parallel_*
is called. (That is, the calling domain could be blocked.) Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sudha247 I believe I have updated the documentation. Please check. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks a lot.
Close #77. This will probably break all current code, but I think it is worth it (especially before 5.0 is officially out).