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

Adapt Gc alarms for multicore #12558

Merged
merged 4 commits into from
Sep 17, 2023
Merged

Adapt Gc alarms for multicore #12558

merged 4 commits into from
Sep 17, 2023

Conversation

gadmm
Copy link
Contributor

@gadmm gadmm commented Sep 14, 2023

Foreword: nobody cares about GC alarms, so let us not waste too much time on this. If you feel like suggesting corrections for typos or wording, please use the github feature for code suggestions so that I can apply them in one click.

  • Gc alarms do not actually run at the end every major cycle, this has not been the case for a while, though this has been made worse by the new GC. This is not worth fixing, we simply document the fact. A use-case is added, just because this snippet of code was laying around (and in fact is the original use-case of Gc alarms).
  • We delete GC alarms if their domain exits. There is no sense in having the alarm start running in a completely unrelated domain after orphaning.
  • 2nd commit: Remove an outdated and superfluous comment along the way, at_each_spawn no longer exists (be reassured that its useful contents already appear inside the interface documentation).

Changes Outdated Show resolved Hide resolved
Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

Looks good to me. (Thanks for the PR-description comment on at_exit, which was indeed answering a question I was bound to have.)

@kayceesrk
Copy link
Contributor

The PR looks good and happy to go ahead with this.

Why is it that we can't have the GC alarm reliably run at the end of every major cycle?

@gasche gasche merged commit 5ed5f59 into ocaml:trunk Sep 17, 2023
9 checks passed
gasche added a commit that referenced this pull request Sep 17, 2023
Swrup pushed a commit to Swrup/ocaml that referenced this pull request Sep 17, 2023
@gadmm
Copy link
Contributor Author

gadmm commented Sep 18, 2023

Why is it that we can't have the GC alarm reliably run at the end of every major cycle?

Oh, we could. As far as I can tell, this involves replacing the current lightweight implementation, which builds on finalisers, with an approach more integrated with the runtime. I do not think it is worth fixing, as explained, but if someone has a better idea then this could be considered.

@gadmm
Copy link
Contributor Author

gadmm commented Sep 18, 2023

By doing a quick code survey on github, I have found that GC alarms have become pretty popular, so I have to retract my claim that nobody cares.

I have found many new users of memory limits, for which I have already justified the change in behaviour. But I have also found uses where the alarm does not raise, but instead periodically cleans-up some weak data structure. Here again it does not make sense to start running this operation in an unspecified domain in parallel, as it stands, due to data races. But porting this code to multicore (with ownership transfer of those weak data structures between domains, or with some parallel variants of those data structures) might be tricky. Though it might be an intrinsic limitation of the GC alarm API.

@kayceesrk
Copy link
Contributor

Do you think it is useful to track reliable GC alarms as a separate issue?

@gadmm
Copy link
Contributor Author

gadmm commented Sep 19, 2023

It might be useful; the code survey did not suggest that it is necessary.

@kayceesrk
Copy link
Contributor

kayceesrk commented Sep 19, 2023

Ok. In that case, an issue isn't necessary. Thanks.

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.

None yet

3 participants