Memory cleanup at exit

[ghost]

This is a work-in-progress support of "memory cleanup at exit" code (OCAMLRUNPARAM=c), see issue #10865.

This is implemented in two parts:

1. perform the equivalent of what domain_terminate does in the main thread upon exit.
2. invoke the finalizer, if any, for all remaining objects in the shared heap.

This appears to work when only one domain is left running at program termination time. When multiple domains are used, no effort to release memory is achieved due to the domains being in an unknown state. However, if memory cleanup at exit mode is enabled, the domain threads will be forcibly stopped.

[ghost]

In order to make progress for what is probably most of the use cases of that feature, I've currently restricted the cleanup to single-domain operation.

In this state, there are no segmentation faults or other misbehaviours upon exit, only memory leaks (i.e. it's as bad as before in the worst case.)

[jmid]

Allow me summarize my understanding of this work (feel free to correct me):

• with this PR both single-domain programs and "well-behaved multi-domain programs"¹ will free the malloc'ed memory at exit
• this will only happen in "memory cleanup at exit" mode (OCAMLRUNPARAM=c)

This strikes me as a good first step to get an initial "memory cleanup at exit" mode working in OCaml 5.

Footnotes

1. multi-domain programs that Domain.join any child Domains before reaching program exit, meaning that they will be single-domain at that point ↩

[ghost]

Your understanding is correct.

[NickBarnes]

Possibly I am misunderstanding something here. In the current runtime, when a domain terminates, its major heap (a.k.a. "shared heap") is "orphaned" by caml_teardown_shared_heap, and stashed away in the (misleadingly-named) pool_freelist until the next GC "cycle" when it is adopted by some other domain (in caml_cycle_heap). The memory is not freed, nor can it be as there may be pointers into it from the major heaps of other domains. Likewise, finalisers are not run because the blocks may still be alive. This change appears to break that, by freeing the shared heap of a domain in domain_terminate (via caml_teardown_domain calling caml_teardown_shared_heap which calls caml_finalise_heap, etc).
I think that, instead, we need to call caml_finalise_heap (which should do approximately what it does in this PR, i.e. iterate through the pool lists and large alloc linked list) only when the last domain terminates. When some other domain terminates, its heap should continue to be orphaned and then adopted, as at present; blocks in that heap which die due to the domain termination will be collected (and finalisers run) in the normal way at a subsequent collection.

[gasche]

(This PR is related to #13010 and I was not sure which one to comment in.)

Here is the behavior I would expect for caml_shutdown:

1. all mutator threads are stopped
2. all domains are terminated
3. all resources allocated by the runtime are released

In 4.x, the cleanup-at-exit mode would make caml_do_exit call caml_shutdown, and I see no harm in keeping the same behavior in 5.x. (There the use-case is not an OCaml plugin within a larger non-OCaml program, as we know the process will exit anyway, it is to use valgrind to detect resources leaks in our C code or something like this.) If we are not in cleanup-at-exit mode, we don't need to do anything.

I could see a use-case for another function that does a "soft shutdown" by raising the Thread.Exit exception in all mutator threads, and letting the mutator themselves cleanup their own resources. This is nice, it corresponds to @damiendoligez's suggestion, it is more work and could be dealt with separately.

[ghost]

This new version incorporates @NickBarnes's feedback, but should not be considered ready yet. There are still issues when using the debug runtime. On the other hand it will be easier to review as the cleanup code paths are more visible now.

[ghost]

Status update:

• single-domain programs, or those which Domain.join all the running domains prior to exiting, ought to have all their memory allocations released correctly.
• if there are "rogue" domains still running, a message is caml_gc_loged, and an attempt will be made to cause them to stop, but no particular attempt will be made to clean up their resources (this is considered too risky). If these domains are stuck in non-caml code (e.g. waiting for a resource in one of the pthread functions), then they won't get interrupted, and no attempt is currently made to forcibly pthread_kill them.

[gasche]

This sleep-based synchronization looks very fishy to me -- but then I'm not a domain expert. Is there not a standard synchronization primitive that we can use?

[ghost]

It's only ugly (-:

If we reach this state, there is absolutely no guarantee that the interrupt request will be processed by the domains, so we can not wait for them without an upper time bound, in case at least one domain is unresponsive. The time limit here is (number of currently running domains + 5) x 1 millisecond. That's an arbitrary number which hopefully allows every non-sleeping domain to be scheduled and receive the exit request.

[gasche]

I understand the concern, but I am not sure what scenarios you have in mind about delayed processing.

I am assuming that all domains have at least a thread that checks for interruptions on a regular basis -- otherwise this is a bug that should be fixed to restore this property, because the minor GC relies on it. I am not sure if you make a different assumption, or if you also assume this but then are thinking of the other threads on that same domain that are waiting on other blocking calls, or something else?

As long as the owner of the domain lock regularly handles interruptions, we know (I think?) that the other threads on the domain will not use the runtime again before they take the domain lock. So we could have a check for "we are forced to terminate" at that point that would shut them down before they return to OCaml code, and not have to wait at all for those threads right now.

[gasche]

The implementation mechanism for interrupting all domains in this PR is somewhat custom: it uses the same interruption mechanism as STW sections, but it is not implemented as a STW itself, instead it is a new code path in handle_incoming.

I find it hard to reason about, and in particular there are two subtleties that may or may not be handled properly:

1. What happens if a new domain is being created while we are trying to shutdown? You added a test for this in caml_domain_spawn, but I don't know if the test is enough. (What happens if a spawn was ongoing, exactly after the test, when you call shutdown?). In contrast, the STW mechanism has careful synchronization in place to block new spawns while a STW section is running, so reusing it may make things more correct here.

2. What happens if caml_shutdown is called while some threads are blocked, and the backup thread handles the shutdown interruption? I suppose that the backup thread may cleanup the domain structure, but then what happens when another thread on the domain is done with their blocking call and decides to take the domain lock and run OCaml code again?

I think that a robust solution should come with an explanation of its interaction with the other domain lifetime events (see for example the existing documentation comments about the backup thread cycle above BT_IN_BLOCKING_SECTION, and the synchronization mechanisms of STW section above caml_try_run_on_all_domains_with_spin_work), and of how it preserves or modifies the invariants governing several threads morally attached to the same domain (including the backup thread).

If I was trying to implement this, I would try the following:

• Implement the shutdown/cancellation logic inside the STW mechanism, possibly by just calling a STW section whose body calls pthread_exit for most domains, but maybe this does not work (it break the logic that comes in stw_handler after the callback) and the logic has to be added to stw_handler or some other part of the STW mechanism, conditioned on the global "we are forced to terminate" condition.
• Check the "we are forced to terminate" condition in caml_leave_blocking_section (or maybe caml_bt_enter_ocaml, but I think that this may not work in Thread-using program), think about which other places need such a check, and try to find a crisp explanation for which places need the check and why.

[ghost]

The implementation mechanism for interrupting all domains in this PR is somewhat custom: it uses the same interruption mechanism as STW sections, but it is not implemented as a STW itself, instead it is a new code path in handle_incoming.

An earlier work (#13010) did use an STW for convenience, and some people commented this was probably overkill. But I agree that it was simpler to write as an STW rendezvous.

I find it hard to reason about, and in particular there are two subtleties that may or may not be handled properly:

1. What happens if a new domain is being created while we are trying to shutdown? You added a test for this in `caml_domain_spawn`, but I don't know if the test is enough. (What happens if a spawn was ongoing, exactly after the test, when you call shutdown?). In contrast, the STW mechanism has careful synchronization in place to block new spawns while a STW section is running, so reusing it may make things more correct here.

If a spawn was ongoing and had already passed the added test, then it will spawn correctly, and then honour the exit request (almost) immediately.

2. What happens if `caml_shutdown` is called while some threads are blocked, and the backup thread handles the shutdown interruption? I suppose that the backup thread may cleanup the domain structure, but then what happens when another thread on the domain is done with their blocking call and decides to take the domain lock and run OCaml code again?

Argh, this is something I did not take into account. The interrupt handling needs to ignore the exit request if processed in the backup thread, i.e. if caml_bt_is_self.

[gasche]

An earlier work (#13010) did use an STW for convenience, and some people commented this was probably overkill. But I agree that it was simpler to write as an STW rendezvous.

Ah, I suppose you had comment #13010 (comment) in mind. It is possible that I am contradicting myself from 2 months ago, but I think that (1) notifying other domains that they should terminate (as we discussed then) should not require a STW (and I don't think that raising an exception from within a STW is currently supported), but that (2) cleaning up the runtime state shared by all domains (as discussed now) requires a synchronization with all mutators, which could be done with a STW. In any case, "simpler" is good here, as long as it aligns with "reasonably easy to convince ourselves that it is correct".

[gasche]

Aside: I thought of trying to implement a domain-safe caml_shutdown myself to try to understand this better, and then I remembered that writing tests for this is tedious. I thought of reusing your own testsuite, but I don't see any new tests in the current draft PR. How do you do your testing?

[ghost]

It is possible that I am contradicting myself from 2 months ago

You are not contradicting yourself - your opinion on the problem evolves as you are getting more familiar with it.

[ghost]

Aside: I thought of trying to implement a domain-safe caml_shutdown myself to try to understand this better, and then I remembered that writing tests for this is tedious. I thought of reusing your own testsuite, but I don't see any new tests in the current draft PR. How do you do your testing?

There are several tests in the existing testsuite which, in their current state, exit with multiple domains running, and which are representative enough:

• tests/lazy/lazy3.ml
• tests/parallel/backup_thread.ml
• tests/parallel/major_gc_wait_backup.ml
• tests/parallel/mctest.ml

[kayceesrk]

The PR has been marked as ready for review. @dustanddreams, can you summarise what has improved since it was set to draft a few weeks ago (#12964 (comment))?

Is the original PR message still an accurate summary of the guarantees provided? #12964 (comment).

[ghost]

I have updated the PR description to match the current reality.

Short summary: when cleanup mode is enabled, memory is released only if only one domain is left running at program shutdown, otherwise the leftover threads are cancelled but nothing else is done for we can't assume anything about the state they are in. When cleanup mode is not enabled, there is no change of behaviour at all.

[ghost]

Note that changes are to be expected after #13272 goes in, since there will be more dynamic allocations to clean up. If you're short on time, I'd suggesting postponing your review until then.

[gasche]

I don't think I will be able to do a full review soon, but I had a quick look at it seems that the PR is moving in the direction of being simpler, thanks! On the other hand, I remain unconvinced by the reasoning in stw_terminate_domain, whose documentation explains that "we are not in a state where we can safely release resources", and that therefore no resource cleanup is attempted.

Here is my intuition about this -- but it may very well be incorrect/simplistic.

• The thread on each domain that answers the STW interrupt owns the domain resources.
• So in fact it could release the domain resources right on the spot.
• We could either kill the other threads running on the domain at this point, or be more lazy by just making sure that they stop themselves if they regain the domain lock after termination has been ordered (before they try to access domain resources), or both.

If I was writing this myself, I would try to ensure that when a non-backup thread regains the domain lock, it starts by checking whether the domain has already been shut down, and terminating itself in this case. (Can we check this just by looking at domains_exiting? We want to be sure that the STW section has run to release all resources on the domain before we switch to this behavior, so maybe we need a separate boolean domains_have_exited, or we need to set domains_exiting at the end of the stw_terminate_domain STW section, not before it as currently.)

[kayceesrk]

@dustanddreams and I discussed on a different channel and decided that running the leak sanitizer tests in the CI should be considered in a different PR. I've made an issue to track this: #13515.

[gasche]

(I don't have the bandwidth to look at this right now, so please feel free to make a decision without me if you can.)

[kayceesrk]

@damiendoligez, IIUC, you have self-requested to review this PR. Do you plan to have a look?

[kayceesrk]

@dustanddreams the failing test_dropped_events is due to a new data race introduced by this PR and will need to be investigated: https://github.com/ocaml/ocaml/actions/runs/11159629323/job/31018666175?pr=12964#step:5:575. I've removed the merge-me label for now. Note that this test is known to be flaky, but IIUC, the failure observed here, is not due to this flakiness.

See related discussions at #13407 (comment).

[kayceesrk]

the failing test_dropped_events is due to a new data race introduced by this PR

Doesn't look like a new data race. I've proposed a fix in #13529.

[ghost]

Thanks @kayceesrk for your work on the runtime event races.

[kayceesrk]

I believe that the PR is ready. The test failures in CI runs have been fixed in #13529. Since we haven't heard from @damiendoligez, I'll go ahead and merge.

Thanks for the work and persevering through the various changes in the runtime @dustanddreams.