Ensure the minor heap is actually empty before reallocating it#12879
Ensure the minor heap is actually empty before reallocating it#12879NickBarnes merged 1 commit intoocaml:trunkfrom
Conversation
(cherry picked from flambda-backend commit 85a505610bb3b1900d05347651435b323620f48b) (cherry picked from flambda-backend commit 3583cc128279820f5861b27fb58b095861fc8725)
NickBarnes
left a comment
There was a problem hiding this comment.
LGTM, modulo adding some more assertions. Again (as for #12875) it would be easier to work with or review this code if we had clearer design documentation. I know @stedolan has ideas for restructuring the STW systems to make the constraints clearer. I think that would help here, more separation of concerns. I would like to be able to assume that, after calling a function called something like caml_empty_minor_heaps_once(), the domain's minor heap is empty. But is it? Have finalizers (or memprof callbacks) run, and allocated something? We need to write this stuff down, and have assertions which check it.
|
|
||
| CAML_EV_END(EV_MINOR_CLEAR); | ||
| caml_gc_log("finished stw empty_minor_heap"); | ||
| CAMLassert(domain->young_ptr == domain->young_end); |
There was a problem hiding this comment.
I'd be tempted to put this same assertion in at the end of caml_empty_minor_heaps_once().
- 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.)
gadmm
left a comment
There was a problem hiding this comment.
Again thanks for digging up these issues. With a more drastic fix I propose at #12882, caml_minor_collection does ensure the assertion domain_state->young_ptr == domain_state->young_end, and so it fixes the current bug in a different way.
However I do not see anything wrong with this change which makes sense (we should remind ourselves to remove the outdated comment at some point if #12882 is accepted).
The added assertions are useful.
|
As a non-expert, I find it very tricky that The present PR has enough support/reviews to be merged, but I would like to see what people think of #12882 before moving in this direction or rebasing it on top of (Edit: typo fixed, thanks!) |
|
(@gasche typo in your last comment, it's 12882) I'm also interested in #12882 but will have to take time to look at it carefully. I think it might be better to push the two small fixes for the existing code, before making a larger change, though. (From what I have seen, the two fixes do solve at least the identified problems.) |
|
@mshinwell this makes sense |
|
Having looked at #12882 in more detail, I think we should merge both that and this. |
- 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.)
|
@NickBarnes , it looks that this PR should have been merged and has been forgotten by accident? Am I mistaken? |
|
@Octachron I think you're right. |
|
After the merge of #12882, only the two added assertions were still useful, and the added comment is erroneous and should have been removed before merging, as I explained in my comment above. Not sure what happened here but it appears that the PR author asked to delay the merge of #12882 for their review, but the review never arrived and then they never updated us nor this PR. @NickBarnes would you please kindly fix the erroneous comment? |
The
caml_domain_external_interrupt_hookstrikes again. Whencaml_set_minor_heap_sizeis called, the minor heap is emptied by callingcaml_minor_collection, before the existing minor heap for the domain concerned is freed and reallocated. Except thatcaml_minor_collectiondoesn't necessarily leave the heap empty, because of the hook.To fix this, in this PR we use
caml_empty_minor_heaps_once, which doesn't call the hook. The test case that was failing is a bit intermittent but I have reasonable confidence this fix has solved the problem.I added an assertion at the end of
caml_stw_empty_minor_heap_no_major_sliceto show that the minor heap is indeed empty at the end of that function, since this isn't that obvious.This makes me wonder: maybe under the debug runtime, we should make that hook always allocate a small value. At the moment the behaviour is intermittent because it is relying on thread yields.
(Fix originally from oxcaml/oxcaml#2208)