Cc @gadmm, @stedolan.

You mention caml_final_to_do but I didn't see it appear in the patches.

Ah right. I forgot to push that.

You mention caml_final_to_do but I didn't see it appear in the patches.

Ah right. I forgot to push that.

look good to me

A behaviour I noticed before but which is repeated with this patch: in long-running allocating C functions, since allocations do not reset caml_something_to_do, if a signal arrives or a finaliser is pending, then all subsequent small allocations are made slow because they will all poll and then decide to not do anything (until the callbacks are finally executed). Is this tradeoff accepted?

My implementation of masking tries hard to avoid that so maybe we can fix this in the future with masking.

This patch looks good to me. Well spotted/fixed!

It worries me that there is no automated testing of this. I don't think this should be too hard to write a test case for: do something which causes a callback to become pending, and then allocate and assert that the callback runs before the next GC cycle. There's an example of such a test in testsuite/tests/c-api/alloc_async.

A behaviour I noticed before but which is repeated with this patch: in long-running allocating C functions, since allocations do not reset caml_something_to_do, if a signal arrives or a finaliser is pending, then all subsequent small allocations are made slow because they will all poll and then decide to not do anything (until the callbacks are finally executed). Is this tradeoff accepted?

My implementation of masking tries hard to avoid that so maybe we can fix this in the future with masking.

FWIW, I think this overhead is fine. If there's a simple way to avoid it, then that would be nice, but I don't think it's worth spending much effort or runtime complexity on.

A behaviour I noticed before but which is repeated with this patch: in long-running allocating C functions, since allocations do not reset caml_something_to_do, if a signal arrives or a finaliser is pending, then all subsequent small allocations are made slow because they will all poll and then decide to not do anything (until the callbacks are finally executed). Is this tradeoff accepted?

Well, yes, I thought that should be the price to pay. Do you have figures in order to evaluate the slowdown?

My implementation of masking tries hard to avoid that so maybe we can fix this in the future with masking.

Still, we would need to enter caml_alloc_small_dispatch, which sometimes requires registering extra roots... That still has some costs. Again, figures would help. But that's an independent discussion.

It worries me that there is no automated testing of this. I don't think this should be too hard to write a test case for: do something which causes a callback to become pending, and then allocate and assert that the callback runs before the next GC cycle. There's an example of such a test in testsuite/tests/c-api/alloc_async.

Then, this test would only be relevant in native mode with #8805 because the bytecode interpreter checks caml_something_to_do directly. And in native mode, I am afraid that the test will fail on some platforms, because young_limit may be cached in a register...

Then, this test would only be relevant in native mode with #8805 because the bytecode interpreter checks caml_something_to_do directly

Good point. We should wait until #8805 is merged before adding this test.

And in native mode, I am afraid that the test will fail on some platforms, because young_limit may be cached in a register...

This is true for finalisers but not for signals, so the test should use signals. When a signal arrives, the register caching young_limit is updated from the signal handler. I think this is the right behaviour, actually: it's OK to delay a finaliser until the next GC cycle since finalisers are always delayed by a GC cycle, but signals should be handled promptly.

Still, we would need to enter caml_alloc_small_dispatch, which sometimes requires registering extra roots... That still has some costs. Again, figures would help. But that's an independent discussion.

(Agreed that this is an independent discussion, but if you're investigating this you can use CAMLdrop in the style of #8993 (comment) to avoid the overhead of registering roots in the fast case)

I'm all for merging this (@stedolan, I am assuming that you also approve of the patch?), but I think that indeed writing a test would be very nice if it's not too much work.

I'm all for merging this (@stedolan, I am assuming that you also approve of the patch?), but I think that indeed writing a test would be very nice if it's not too much work.

I agree!

Then, this test would only be relevant in native mode with #8805 because the bytecode interpreter checks caml_something_to_do directly

Thinking about this again, isn't this only true for Memprof callbacks? Signal callbacks work fine before #8805, so a test using those in native code should fail before this PR and pass afterwards.

Then, this test would only be relevant in native mode with #8805 because the bytecode interpreter checks caml_something_to_do directly

Thinking about this again, isn't this only true for Memprof callbacks? Signal callbacks work fine before #8805, so a test using those in native code should fail before this PR and pass afterwards.

Currently (until #8805), in native, caml_garbage_collection correctly calls signal handlers both before and after this PR. IMO this is less of a bugfix than finishing implementing a feature that will only become visible once #8805 lands, in that sense a test isn't strictly necessary, and I agree with @jhjourdan if he says that it's complicated to test.

A behaviour I noticed before but which is repeated with this patch: in long-running allocating C functions, since allocations do not reset caml_something_to_do, if a signal arrives or a finaliser is pending, then all subsequent small allocations are made slow because they will all poll and then decide to not do anything (until the callbacks are finally executed). Is this tradeoff accepted?

Well, yes, I thought that should be the price to pay. Do you have figures in order to evaluate the slowdown?

My implementation of masking tries hard to avoid that so maybe we can fix this in the future with masking.

Still, we would need to enter caml_alloc_small_dispatch, which sometimes requires registering extra roots... That still has some costs. Again, figures would help. But that's an independent discussion.

I agree that it's a separate discussion, and I do not have figures. It is very dependent on the user's code. If the user follows the advice of regularly calling a process_pending_* function, then they are safe from it. The trick with masking in order to avoid entering caml_alloc_small_dispatch is simply not to change young_limit when the mask is on. You remember that a callback is pending with caml_something_to_do, and you use it to set the young_limit when you turn off the mask. I think the same principle can apply for this problem, but we can discuss it again over at #8961 (once it is ready again for review after I rebase it).

Currently (until #8805), in native, caml_garbage_collection correctly calls signal handlers both before and after this PR. IMO this is less of a bugfix than finishing implementing a feature that will only become visible once #8805 lands, in that sense a test isn't strictly necessary, and I agree with @jhjourdan if he says that it's complicated to test.

Actually, I think we can write a test, which would fail before this PR in bytecode. Just give me a little more time for that!

I have just added a test which checks that signals are indeed handled at every allocation in the minor heap from OCaml code. I took care of not calling any function after raising the signal, so that we are sure that the signal is not handled by the other mechanism in interp.c. The test indeed fails without the current commit.

Thanks!