Move last finalisers book keeping to the end of minor gc#12001
Move last finalisers book keeping to the end of minor gc#12001kayceesrk merged 1 commit intoocaml:trunkfrom
Conversation
|
Would it make sense to add a test to the testsuite ? |
|
The fix looks reasonable. Can you add the test from #11995 to the testsuite? |
sadiqj
left a comment
There was a problem hiding this comment.
This bug has been present since the first stop-the-world minor implementation - I'm very surprised nobody has hit it until now. It does require you to have a finalised value that is only reachable via the local roots, which is probably a rare occurrence.
The fix looks correct.
There was a problem hiding this comment.
It's just occurred to me this might not be correct. We do this after the final barrier (with the opportunistic major slice in it), there are no more barriers and so we need to think through if it's safe for this to be run concurrently with another domain having re-entered the mutator (which may be filling it's minor heap with data again).
|
On further pondering this could be fixed with: barrier -> finalisers -> final barrier with opportunistic work (This also has the advantage that if there are finalisers which are long running, the other domains get to do some useful work hopefully) |
|
It is not clear to me why we can’t process finalise_last finalisers while other domains may be running mutators. The minor heaps are empty at the end of minor collection. The domains only examine finalisers installed on their own minor heaps and no other domains will touch other domains' minor heap. What am I missing? |
sadiqj
left a comment
There was a problem hiding this comment.
I looked through last night and this morning and agree with @kayceesrk , it should be safe to run the finalisers while other domains have returned to mutators and cleared their minor heaps (because a domain's young finalisers only point to their own minor heap).
kayceesrk
left a comment
There was a problem hiding this comment.
Please add a test to the testsuite.
…men_bin and dolmen (0.8.1) CHANGES: ### UI - Fix handling of size/time limits on windows (PR#117) - Fix spurious printing of backtraces (PR#118) - Add release binaries for windows ### LSP - Add option for the lsp to read preludes before checking each file (PR#116) - The LSP now sends an empty list of diagnostics upon closing a file (PR#116) ### Parsing - Fix a bug related to alt-ergos function definition, which were previously alwyas non-recursive. Now, alt-ergo's function definitions are always recursive (PR#123) - Add `parse_raw_lazy` to parse a string into a lazy list of statements (PR#125) - Add support for mutually recursive functions and predicates in Alt-ergo's native language (PR#129) ### Typing - Properly add binding locations for implicit type variables (PR#123) - Ensure that type of recursively defined symbols are freshened to avoid type variables sharing between declaration and definition (PR#123) ### Loop - Use `GC.finalise` instead of `Gc.finalise_last` in `loop/parser.ml` in order to avoid a bug in the ocaml 5.0 runtime, see ocaml/ocaml#12001 (PR#128) - New module to implement Alarms (size/time limits) (PR#117) - Add optional argument to `Pipeline.run` to specify an alarm implementation (PR#117) - Add a `bt` key to the state to record whether we should print backtraces (PR#118) - Use `parse_raw_lazy` to parse raw contents in full mode if/when necessary (PR#125)
|
I pushed the test case (slightly modified to add a However it does not currently pass the test suite, because it seems there's an inconsistency between the behaviour of the byte code and native version. The byte code version does not seem to call the finaliser even if we call Is this inconsistency expected, or is it another bug? |
|
When something becomes unreachable may differ between native and bytecode. Can you try with this testcase? let [@inline never] foo () =
let s = "Hello" ^ " world!" in
Gc.finalise_last (fun () -> print_endline "finalised") s;
Gc.minor ();
s
let [@inline never] bar () =
let s = foo () in
print_endline s
let _ =
bar ();
Gc.full_major ()Here's what I see: |
…e end of the minor cycle This commit implements a fix on how last finalisers are updated during the minor cycle. Previously, the book keeping was done during the minor cycle, before the values are promoted. As a result, last finalisers are wrongly run after the end of the minor cycle, precisely because the book keeping did not account the the promoted values. The fix is to move said book keeping to the end of the minor gc cycle, and can be done outside of the last barrier for the minor cycle, as a domain will only process finalisers within its own minor heap. Fixes ocaml#11995
26d3774 to
a302ea9
Compare
|
This test case behave more consistently (regarding the byte code vs native difference.), Thank you very much! |
|
I went through the file and saw no references to finalisers. Can you confirm that this is indeed an orthogonal failure (and file an issue if this is the case)? |
|
Given that the CI failure was unrelated to the changes here, I'm merging this PR. |
This PR is an implementation of @kayceesrk 's suggestion in #11995
This commit implements a fix on how last finalisers are updated during the minor cycle.
Currently, the book keeping is done during the minor cycle, before the values are promoted.
As a result, last finalisers are wrongly run after the end of the minor cycle even is the value is promoted (and as such, live), precisely because the book keeping did not account for the promoted values.
The fix suggested by @kayceesrk is to move said book keeping to the end of the minor gc cycle, and can be done outside of the last barrier for the minor cycle, as a domain will only process finalisers within its own minor heap.