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

Release callback early in domain_thread_func #12408

Merged
merged 1 commit into from
Jul 23, 2023

Conversation

gadmm
Copy link
Contributor

@gadmm gadmm commented Jul 22, 2023

caml_callback is careful not to keep its callback alive for the duration of the call. This allows the closure to be collected as early as possible.

Similarly, this change makes sure not to keep the functional argument to Domain.spawn alive longer than needed.

cc @kayceesrk

@gasche
Copy link
Member

gasche commented Jul 22, 2023

The outcome of my review of this PR (which I believe is correct) is that I wrote the following alternative proposal, which allows dynamic allocations and global roots, and I think results in simpler code overall:

trunk...gasche:ocaml:domain_spawn_callback_liveness

Do you have an opinion about this proposal?

@gasche
Copy link
Member

gasche commented Jul 22, 2023

(I would be fine with merging your changes and then discussing my suggestion as a follow-up PR if you prefer. In that case: did you intentionally not include a Changes entry?)

@gadmm
Copy link
Contributor Author

gadmm commented Jul 22, 2023

I have reviewed your patch, I like it as you are getting rid of global roots entirely. I suspect that formally the reasoning of correctness is trickier but I think it is correct.

It's a bit simpler for me if you can rebase it on top of #12409 as there is some diff overlap. Would you mind rebasing it over the latter? Otherwise I don't gain much in discussing it separately from the present PR and thus we could as well include it here.

I have added a Changes entry.

@gadmm
Copy link
Contributor Author

gadmm commented Jul 22, 2023

Note: If the answer is that you prefer if I do the rebasing, then I can quickly base the present PR on your suggestion to avoid having another PR.

@gasche
Copy link
Member

gasche commented Jul 23, 2023

I propose to go ahead with the current PR as-is, review #12409 and merge if applicable. Then I can rebase my own PR and we can discuss it.

I had done the work of rebasing my patch on top of the present PR, and in the process made a minor change to your comment and variable name. I pushed this change to the current PR.

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.

Good to merge if @gadmm agrees.

@gadmm
Copy link
Contributor Author

gadmm commented Jul 23, 2023

Yes, please, thanks.

@gasche gasche merged commit 01d1a0f into ocaml:trunk Jul 23, 2023
9 checks passed
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

2 participants