Execute preemptive systhread switching as a delayed pending action#12882
Execute preemptive systhread switching as a delayed pending action#12882gasche merged 2 commits intoocaml:trunkfrom
Conversation
|
This approach seems reasonable and beneficial to me, but I would appreciate the input of people with a deeper working knowledge of the runtime system. @mshinwell, @NickBarnes, would you by chance have an opinion? |
stedolan
left a comment
There was a problem hiding this comment.
This change looks correct to me, and definitely improves on the current behaviour.
|
Excellent news, thanks @stedolan! (The Changes entry needs updating.) |
|
Should this target 5.2 or trunk? |
|
I consider this as a bugfix, so 5.2. |
NickBarnes
left a comment
There was a problem hiding this comment.
I agree with @stedolan: this looks obviously right to me, and a distinct improvement to abstraction in the runtime.
Changes
Outdated
| (Guillaume Munch-Maccagnoni, review by Anil Madhavapeddy and KC | ||
| Sivaramakrishnan) | ||
|
|
||
| - #12875, #12879, #12881: Execute preemptive systhread switching as a |
|
(I will review this too) |
|
Thanks. I'll wait then. |
|
@mshinwell , would you be fine with merging the PR now in 5.2 and maybe doing a separate review later on your side? |
- Fix two bugs reported by Mark Shinwell: ocaml#12879 and ocaml#12875 - Reasoning about multithreaded code: it is now safe to assume that no mutation happens on the same domain after allocating on the OCaml from C. (This reverts to OCaml 4 behaviour.)
40b8a45 to
f22731b
Compare
|
I have rebased this PR, maybe we can proceed. |
Execute preemptive systhread switching as a delayed pending action (cherry picked from commit 572aeb5)
|
I went ahead and merged in trunk and 5.2. Thanks @gadmm for the nice code, and @stedolan and @NickBarnes for your reviews on this fairly tricky aspect. |
Background: in OCaml 4, preemptive systhread switching (happening every 50ms) used to be done via signal handlers. This means that there were the same guarantees for the C FFI regarding code running concurrently on the same domain, as there was regarding OCaml signal handlers running asynchronously. In particular, allocating on the OCaml heap from C could not cause another systhread on the same domain to run before the allocation function returned. This property has been lost with OCaml 5 and the rewriting of the systhreads runtime.
The rewriting of systhreads with multicore uses a more straightforward mechanism to communicate preemption (the atomic flag
requested_external_interrupt). Initially, multicore was not as careful regarding the guarantees about asynchronous callbacks, but this was progressively fixed. Systhread's yield being executed during allocations from C is a remnant of this old multicore implementation.In addition, @mshinwell has found two critical bugs related to the misplacement of preemptive systhread switching being called from
caml_poll_gc_work, which is not expected to execute OCaml code (#12875, #12879).With this PR, preemptive systhread switching is considered again as an asynchronous action whose execution can be delayed until an appropriate safe point is reached. This restores the guarantee described above, and fixes the two bugs.
This PR is targeted towards backporting to 5.2, given that it fixes critical bugs.
(Cc @kayceesrk with whom the change was previously discussed.)