I don't like this change : even thought it may not be bogus in itself, it makes the code way less robust, for two reasons:

• In native mode, if caml_something_to_do is not set but the minor GC limit is set to young_alloc_end, then we end up in an loop, because we never call caml_update_young_limit.
• If there is a pending callback but caml_something_to_do is not set, then we need to wait for the next callback to get a chance of executing the first one.

Sure, if we maintain the invariant that caml_something_to_do is always set when there is a pending callback, then these two issues can never happen. But this is based on a rather subtle logic, and, moreover, accesses to caml_something_to_do and young_limit can be racy so that it's not clear that we see the right changes at the right moment.

Sorry for the loop, I should have realised because I knew it from my work on implementing masking.

Note that whether the user is tasked in calling caml_get_urgent_to_do themselves or this implementation, the problem is the same. The goal here is to avoid exposing assumptions in the public interface about the race between querying signals and running signal handlers. It is not yet clear to me how multicore is going to affect this, but this way we can change it more easily in the future.

What we should do here is implement it differently in native and bytecode, and in native we check for young_limit==young_alloc_end instead (I already have to introduce the symmetric function caml_urgent_notify_program for Sys.mask, see 41a8ced).

What we should do here is implement it differently in native and bytecode, and in native we check for young_limit==young_alloc_end instead (I already have to introduce the symmetric function caml_urgent_notify_program for Sys.mask, see 41a8ced).

I would still prefer not doing this test at all. Again, because of the race, caml_something_to_do may not be up-to-date in bytecode mode. The same applies for young_limit in native mode. And, moreover, the invariant you are relying on is subtle and may get broken in the future by bogus code. I would prefer being robust.

Do we have an idea of how cost a spurious call to the non-optimized version of this function? How much do we gain from this optimization?

For the cost, the argument is that you want a C interface to do like OCaml. For instance you might want to remain reactive in a tight loop. In OCaml, we are happy that polling only costs one comparison in that case. The premiss is that there should be a way to do exactly like OCaml (essentially if (check()) { perform(); }).

It is true that checking for young_limit==young_alloc_end is not exactly like young_ptr < young_limit, but I think making young_limit==young_alloc_end an explicit invariant of when pending actions should be performed is technically sound. (I may also be missing your point, in that case could you please give us a more detailed explanation of your objection?)

Furthermore, in bytecode it is a bit more complicated indeed given that now sometimes the test is young_ptr < young_limit, other times it is caml_something_to_do, but I would prefer to simplify bytecode by always checking for the same condition (so, young_limit==young_alloc_end).

I can propose to make the non-checking function internal and provide a public function that does the test before calling the internal function.

The following piece of code from ocamlcc.1.0/runtime/ocamlcc-signals.c makes me think further (it relies on internals):

  if (caml_something_to_do) {
    long ocamlcc_save_sp_offset;
    SaveSp(ocamlcc_save_sp_offset);
    sp[-frame_sz - 1] = sp[-frame_sz - 2] = sp[-frame_sz - 3] =
      sp[-frame_sz - 4] = sp[-frame_sz - 5] =
      sp[-frame_sz - 6] = Val_unit;
    OffsetSp(-frame_sz - 6);
    do {
      caml_something_to_do = 0;
      caml_process_event();
    } while (caml_something_to_do);
    ResetSp(-frame_sz - 6);
    RestoreSp(ocamlcc_save_sp_offset);
  }

If we do want to support the inner loop, it looks to me that we need to expose caml_get_urgent_to_do(), contrary to what I advocate. But I do not know if we want to support something like it. We can keep caml_get_urgent_to_do internal, since they already rely on internals.

We will see what I come up with.

I initially wrote that in bytecode sometimes the test is young_ptr < young_limit, but this is currently never because young_limit == young_alloc_end. Similarly in native code we can get to caml_garbage_collection by exceeding caml_memprof_young_trigger. In that case, as I now understand, it is not only brittle but wrong to check young_limit == young_alloc_end.

My point is that, in rare situations, caml_something_to_do and young_limit may not be set even though there is an async callback waiting. I can foresee several reasons:

• Because of the race, writes to these variables may not be visible to the OCaml thread at the right moment. Imagine we get into caml_check_urgent_gc when caml_something_to_do is seen as set but not caml_pending_signals: we may miss a signal in the first call to caml_check_urgent_gc and don't check for them later because we return from said function even before checking caml_pending_signals.
• The fact that caml_something_to_do is set when there are pending callbacks relies on subtle invariants which we will easily break in the future.
• If there are two different callbacks (say, a signal and a finalizer) arriving at the same time, then we set caml_something_to_do to 0 when starting the first one while the second one is still waiting. We may want to run the second one if the first one is taking long.

In the current situation, the code is robust in the sense that async callbacks are called at the next minor collection even if, for some reason, caml_something_to_do is not set properly. With what you are proposing, we have to wait for the next async callback, which may not come at all.

I can propose to make the non-checking function internal and provide a public function that does the test before calling the internal function.

Perhaps what we should do then is make caml_something_to_do part of the public interface and document that this is the thing one has to check before calling caml_check_urgent_gc. This would both cover ocamlcc and Coq use cases, and make polling very cheap as you desire.

Note, however, that because of the reasons I described above, it may be possible that we miss async callbacks in long-running C code. I guess that's the price to pay?

You explain well why we need to distinguish the function called internally from the function provided to the user (which will be guarded by a check to caml_something_to_do).

With masking, we will need to make it a hard invariant that caml_something_to_do has to be set if there are pending callbacks (outside the case of process_pending* where it is temporarily set to zero and set back to 1 later if needed). I think it is better to start doing it now.

In the case of a finaliser taking a long time while a signal is pending and caml_something_to_do is temporarily 0 (or vice-versa), we can think further about it but I think this is outside the scope.

These reasons convince me to not make public a check for caml_something_to_do, so that we keep the races under our control and it is easier to fix any issue later.

I am not sure we want to support this exact code of ocamlcc, because I don't think there is a meaning to "please run signals until there are no more".

I am not sure I see under which conditions async callbacks could be forgotten, what scenario do you think about exactly?

With masking, we will need to make it a hard invariant that caml_something_to_do has to be set if there are pending callbacks (outside the case of process_pending* where it is temporarily set to zero and set back to 1 later if needed). I think it is better to start doing it now.

Why so? Cannot we simply call caml_check_urgent_gc after any unmasking?

These reasons convince me to not make public a check for caml_something_to_do, so that we keep the races under our control and it is easier to fix any issue later.

What about exposing a getter then? This would make it easy to change its content if needed. This getter could even be an inline function, for performance.

I am not sure we want to support this exact code of ocamlcc, because I don't think there is a meaning to "please run signals until there are no more".

That's not what worries me in ocamlcc's use case. What worries me is that it seems important for its performance not to register its extra roots and perform the function call at every single check for signals.

I am not sure I see under which conditions async callbacks could be forgotten, what scenario do you think about exactly?

I don't get your question: I thought my previous message was all about answering it... If caml_something_to_do is 0 when there is a pending async callback, then, with your proposal, we would need to check for the next async callback to come for executing the first one. To me, this is essentially as if we lost it.

Just to make things clear: my current proposal is to expose in the public interface a getter for caml_something_to_do, and document that it can be used for fast polling in C code. This getter could be an inline function in the signals.h file to make its cost minimal. We would not check for caml_something_to_do at every minor GC, just to make sure we don't miss async callbacks.

Do you agree with this proposal? If not, then, what is the issue? It seems to me this addresses all your concerns.

Ok, I thought you meant that some signal would be dropped. Now I understand the issue with long-running C code, this is well spotted. I am proposing the following fix: allow users the obtain the same fallback behaviour as there is from OCaml if they cooperate, by setting caml_something_to_do at every minor collection or major slice in CAML_FROM_C mode.

Waiting for a minor collection or a major slice is still bad latency. It would be nice to fix the first race you explain between caml_something_to_do and caml_pending_signals (this beyond my expertise). I imagine that something will have to be done anyway with multicore (I thought I saw it mentioned at some point but could not find a dedicated bug report). (I am also thinking that caml_signals_are_pending might be redundant now with caml_something_to_do, but I am not sure I should do something about it right now.)

I am wary of exposing a getter for caml_something_to_do as much as exposing caml_something_to_do itself: I want to avoid hardcoding backwards-compatibility support for the form if (check()) { execute(); } and everything else users can come up with. This would mean losing control over the critical section between check and execute, limiting future evolution. In particular this limits the space of possible solutions of the two problems and of any other that could arise (allow me to remain abstract; if I had a concrete solution in mind right now I would propose it). Anyway, I do not see a need for it compared to your proposal now that we can replicate the fallback behaviour in caml_process_pending_actions.

Ocamlcc is a special because it is an OCaml compiler so it is not excessive to ask that they keep relying on internals. It is like saying that the public interface does not support the use-case from runtime/interp.c either. The goal of this new API is not to support the creation of alternative OCaml compilers, as this could require to make public many more internal runtime identifiers beyond those.

I hope the new version of the code addresses all your related concerns (I am waiting for tests to finish). I am documenting the invariant for caml_something_to_do so that we go towards fixing it and not breaking it.

(About unmasking: we do not execute callbacks right away for crucial reasons related to reasoning about critical sections for polling. I gave a thought to what made OCaml's polling behaviour nice to reason about in my experience, and I wrote it down here: ocaml-multicore/ocaml-multicore#187 (comment). It is entirely about almost your question. But I guess you just want to poll for signals unconditionally when unmasking and not rely on caml_something_to_do to the extent that it is unreliable: that is something else and very reasonable, do not hesitate to bring up this point again at #8961.)

Not sure this deserves a FIXME. The current solution is rather good, and I don't think there is anything better we could do.

IIUC, in these situations, instead of reacting at the next allocation, it react on the order of 10^5 to 10^6 allocations later. For finalisers the total latency remains on the same order, but for signals this is bad. In particular, have a look at the part of ocaml-multicore/ocaml-multicore#187 (comment) which says "guaranteed interrupt at fixed delays... Based on the maximum latency of interrupt required...". From their point of view of guaranteed latency, this is still buggy.

The second bullet point can be solved if we call signal handlers first, what do you think about it?

It would be nice to fix the first one as well.

The second bullet point can be solved if we call signal handlers first, what do you think about it?

Alright, let's do that. Note, however, that the issue is still there for finalisers or memprof. That's sort of an unsolvable problem.

Anyway, for both issues, they will only occur at very rare occasions, and we have a workaround. And it's unlikely we will find a "fix" any time. Hence I don't think the "FIXME" is deserved.

But that's a detail, I don't care, let's move forward.

The second bullet point has been fixed. Signal handlers are now safe from this (newly-introduced, AFAIU) race. It is no longer a FIXME, because it is not a bug for finalisers (their latency remains on the same order of magnitude), and it is not a bug for memprof callbacks either, because although we want them to run ASAP, they are served on a best-effort basis.

I keep the first item as a FIXME because it is a bug if we take into account code that tries to allocate little, or if there are latency guarantees as assumed in the work on better safe points. If it is a deviation to the intended behaviour and it does not appear easily solvable with the current mechanism, then I think a FIXME is the best option.