stdlib: remove Domain.at_each_spawn#11595
Merged
Octachron merged 5 commits intoocaml:trunkfrom Oct 6, 2022
Merged
Conversation
gadmm
approved these changes
Oct 3, 2022
runtime/domain.c
Outdated
|
|
||
| caml_reset_young_limit(domain_state); | ||
|
|
||
| caml_domain_initialize_hook(); |
Contributor
There was a problem hiding this comment.
This is called while holding the all_domains lock. It is better to wait that the lock is released. I would call caml_domain_initialize_hook right before the call to caml_callback inside domain_thread_func.
Member
Author
There was a problem hiding this comment.
That seems better indeed.
kayceesrk
approved these changes
Oct 4, 2022
Contributor
kayceesrk
left a comment
There was a problem hiding this comment.
The change looks good to me. Removing Domain.at_each_spawn is a good move.
Systhreads can use this hook to initialize the systhreads machinery before any OCaml code runs on the corresponding domain.
8c7309c to
edb3955
Compare
Member
Author
|
I have cleaned the history and I am planning to merge tomorrow in order to move forward the release cycle. |
Octachron
added a commit
that referenced
this pull request
Oct 6, 2022
stdlib: remove `Domain.at_each_spawn` (cherry picked from commit 021619f)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR proposes to remove the function
Domain.at_each_spawn.One major issue with
Domain.at_each_spawnis that it is impossible to predict in which context the registered function will end up being called.A good illustration of the issue at hand is the use of
Domain.at_each_spawnin the threads library in order to initialize the thread machinery. This approach breaks down if the order of the initialization is incorrect as described in #11434. Consider for instance, a moduleEarly_hookregistering a function withDomain.at_each_spawnand the following main function:
With this setup, any program compiled with
early_hooklinked before thesysthreadslibrary crashes because the function registered byearly_hookcallsThread.selfbefore the thread machinery initialization.The first commit in this PR fixes this issue by adding a new C hook
caml_domain_initialize_hookfor the systhreads library which is run during domain initialization just before the registration with the stop-the-world mechanism. (I am not sure of the right location for the hook during initialization). The second commit adds the example described above as an ephemeral test.The two last commits removes the use of
Domain.at_each_spawninFormatby moving the registration of theDomain.at_exitfunctions to the initialization of the by-domain stdout and stder formatter.Finally, the last commit completely remove
Domain.at_each_spawnfunction since the function seems more harmful than helpful in its current state. It is thus probably better in term of forward compatibility to remove it.Close #11434