Skip to content
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

Merged
merged 2 commits into from
Jul 27, 2022
Merged

Rename num_additional_domains to num_domains for setup_pool #78

merged 2 commits into from
Jul 27, 2022

Conversation

favonia
Copy link
Contributor

@favonia favonia commented Jul 21, 2022

Close #77. This will probably break all current code, but I think it is worth it (especially before 5.0 is officially out).

Copy link
Contributor

@Sudha247 Sudha247 left a 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. *)
Copy link
Contributor

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?

Copy link
Contributor Author

@favonia favonia Jul 25, 2022

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is correct.

Copy link
Contributor Author

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thanks a lot.

@Sudha247 Sudha247 merged commit 94abdda into ocaml-multicore:master Jul 27, 2022
@favonia favonia deleted the fix-setup-pool branch July 27, 2022 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do new pools "own" the current domain?
2 participants