Resource-safe C interface, part 0 by gadmm · Pull Request #9037 · ocaml/ocaml

gadmm · 2019-10-11T17:28:48Z

This is the first part of #8993, where I apply the semantics change for caml_check_urgent_gc agreed upon at #8691 (comment).

In 4.9, the public function caml_check_urgent_gc checks for finalisers, but not other callbacks. In 4.10, the check for finalisers is removed from allocation functions called from C code at #8691. At the same time, execution of all pending callbacks is added to caml_check_urgent_gc, and several new polling points are added. I think the motivation is to ensure, in many places, that memprof callbacks are called as soon as possible after an allocation.

There is a desire to build on #8691 to make the C API safe in the presence of asynchronous exceptions both for the runtime and the user (#8993, #8997). This requires: reducing the number of implicit polling points in C, and for functions that need to poll, replace them with _exn variants that return the exception, to be used to perform appropriate resource clean-up. But this is hard to use in practice if the functions that can raise asynchronous exceptions are many.

So, the current PR furthers #8691 by changing again caml_check_urgent_gc, such that it does not call any callback. We take advantage that in 4.9, it is not yet a reliable polling location. The new code is roughly equivalent to the OCaml 4.9 behaviour modulo fewer checks for finalisers. Any place where the check for finaliser could be relied on has been kept as a polling location.

Some polling points added explicitly with #8691 were in primitives, right before returning to OCaml. I removed them and propose a more robust strategy to ensure the memprof callback is called no later in those cases (not implemented in this PR): have non-noalloc C calls poll when returning (this should be easy to do during the work on better safe points). In addition, if in the future it is decided and enforced that memprof callbacks should not raise exceptions, it will also be possible to add more explicit calls to them independently of the possibly-raising callbacks.

Since memprof is not going to be ready for 4.10, there will still be some time to discuss the appropriate polling of memprof callbacks in the near future.

The first 3 clean-up commits have already been reviewed at #8993, they are included here to avoid me a tedious rebase.

This PR is targeted at 4.10 because it relies on the fact that polling points added in #8691 have not reached users yet, and also because #8993 is currently based on it (the current PR enabled a simpler public API at #8993).

(@jhjourdan, @stedolan)

gadmm · 2019-10-12T05:57:22Z

@jhjourdan I can anticipate one remark: sometimes I call one set of callbacks without calling the other ones. This matches 4.9. An alternative is to always call all callbacks when I call some callbacks. This is debatable, let me know what you think.

jhjourdan

Thanks for this new proposal. This goes to the right direction, we are almost there! I have two concerns:

@jhjourdan I can anticipate one remark: sometimes I call one set of callbacks without calling the other ones. This matches 4.9. An alternative is to always call all callbacks when I call some callbacks. This is debatable, let me know what you think.

I would indeed prefer not calling these functions directly anywhere, except in caml_process_pending_callbacks.

Apart from that, I would prefer if we could use the polling versions everywhere it is possible in the OCaml runtime. This include functions like Array.make: they are not designed to be called from C code, so that they just correspond to allocation from OCaml code, which we decided were polling.

runtime/caml/signals.h

runtime/finalise.c

runtime/gc_ctrl.c

runtime/memprof.c

runtime/weak.c

runtime/obj.c

runtime/minor_gc.c

runtime/meta.c

gadmm · 2019-10-13T10:42:46Z

About the question about removed polling points in primitives that allocate (which you introduced in order to call memprof callbacks), and your request:

Apart from that, I would prefer if we could use the polling versions everywhere it is possible in the OCaml runtime. This include functions like Array.make: they are not designed to be called from C code, so that they just correspond to allocation from OCaml code, which we decided were polling.

So you were not convinced by my proposition to do it once and for all allocating primitives. This solution is not ready, but neither is memprof so I felt like there was no incentive to rush this into 4.10. But maybe you have other plans. I agree to add them (in the case of allocating primitives) and I trust you that it makes a real difference.

You are going to great lengths to call the memprof callback immediately after the allocation. If so, isn't this an information that the memprof callback could use, i.e. a boolean saying whether the call was immediate or delayed? That distinction could also justify adding such calls, in which case it could take the form of a specific function rather than the generic process_pending_callbacks. (It does not need to be implemented right now.)

I am done with my questions for this one!

jhjourdan · 2019-10-13T11:01:42Z

So you were not convinced by my proposition to do it once and for all allocating primitives. This solution is not ready, but neither is memprof so I felt like there was no incentive to rush this into 4.10. But maybe you have other plans. I agree to add them (in the case of allocating primitives) and I trust you that it makes a real difference.

Ah, sorry, I don't remember this proposal (the discussions around these PRs really start being too long... and I am partly responsible for this, I know). Could you recall me your proposal?

Note, also, that many of these caml_check_urgent_gc calls follow calls to caml_initialize, so that it's a good thing to check for urgent GC, because of the many back-references.

You are going to great lengths to call the memprof callback immediately after the allocation. If so, isn't this an information that the memprof callback could use, i.e. a boolean saying whether the call was immediate or delayed? That distinction could also justify adding such calls, in which case it could take the form of a specific function rather than the generic process_pending_callbacks. (It does not need to be implemented right now.)

I'd be happy to add this Boolean, but it seems rather hard to compute to me. If you use a specific function for this, how do you distinguish in the memprof queue the callbacks that have just been added from the old ones? I'm open to discuss this, but let's focus for now on this PR. If you have ideas, I'd be happy if you give them in another channel.

gadmm · 2019-10-13T11:31:54Z

Ah, sorry, I don't remember this proposal (the discussions around these PRs really start being too long... and I am partly responsible for this, I know). Could you recall me your proposal?

Have all non-noalloc C calls (from OCaml) poll when returning. This should be easy to implement as part of the work on better safe points. In addition, if in the future it is decided and enforced that memprof callbacks should not raise exceptions, it will also be possible to add more explicit calls to them independently of the possibly-raising callbacks.

Note, also, that many of these caml_check_urgent_gc calls follow calls to caml_initialize, so that it's a good thing to check for urgent GC, because of the many back-references.

I did not remove such calls (apart from the one discussed above which I did not interpret as such).

I'd be happy to add this Boolean, but it seems rather hard to compute to me. If you use a specific function for this, how do you distinguish in the memprof queue the callbacks that have just been added from the old ones? I'm open to discuss this, but let's focus for now on this PR. If you have ideas, I'd be happy if you give them in another channel.

The idea was to distinguish the most recent one, but I trust you if you say it is more complicated than that. (Future evolution can influence what I do now.)

Also, would you still be comfortable in calling caml_process_pending_callbacks instead of memprof callbacks if the order in which callbacks are called (memprof before anything else) was changed, or will this decision rigidify caml_process_pending_callbacks? (I was thinking that for latency in reaction to signals, it is better to do signals first).

gadmm · 2019-10-13T12:25:40Z

Here's my current impression: I am not opposed to calling memprof callbacks directly in allocating primitives in addition to a systematic polling on return (which I think is going to be there in any case in the end), however it seems premature to decide the exact form it takes: a generic process_pending_callbacks that will check signals first, a generic call to memprof callbacks with an enforced non-raising behaviour, a specific call to memprof callbacks that knows it is called first thing after the allocation... I do not think that it is urgent to discuss and decide this before 4.10, so the default is to keep it as it was in 4.9.

(If you tell me they do make a difference for 4.10, I add them.)

gadmm · 2019-10-13T23:22:37Z

The new last commit concerns what still has pending questions above. (new check_urgent_gcs & polling ASAP after major allocations).

For polling ASAP after major allocations, there is no one solution that satisfies all constraints so I came up with what I think is the best: call memprof callbacks directly. I will move the calls to signals handlers to first position in process_pending_callbacks in the other patch to fix a bug discovered there. This voids the certainty that the allocation just happened. In addition, calling process_pending_callbacks is likely going to be redundant at some point because non-noalloc C calls poll on return. Calling memprof callbacks directly instead is closer to your original intent of calling them ASAP and will keep making sense in the future. In addition it is the most flexible option which you can change anyway in the future.

The drawback is that it breaks the rule that all callbacks are executed via process_pending_callbacks (which was already broken in the case of memprof_handle_postponed).

In any case I did not take a decision there, its purpose is to continue the discussion and let you check that I took all other remarks into account. But I am satisfied with this one.

(Edit: except for the part where the first callback in memprof_handle_postponed will poll at the next allocation... I'll try something else.)

jhjourdan · 2019-10-14T09:16:03Z

Have all non-noalloc C calls (from OCaml) poll when returning. This should be easy to implement as part of the work on better safe points. In addition, if in the future it is decided and enforced that memprof callbacks should not raise exceptions, it will also be possible to add more explicit calls to them independently of the possibly-raising callbacks.

Alright. I confirm this is a promising idea, but for now this is not obvious this will work, and will need at least a few benchmarks to check that performance is not affected too much. Said otherwise, I agree that would be a solution to this problem, but for now we don't have it. And it does not cost much to do manual polling instead for now, and revert that once automatic polling will be available.

however it seems premature to decide the exact form it takes: a generic process_pending_callbacks that will check signals first, a generic call to memprof callbacks with an enforced non-raising behaviour, a specific call to memprof callbacks that knows it is called first thing after the allocation... I do not think that it is urgent to discuss and decide this before 4.10, so the default is to keep it as it was in 4.9.

That's not only about 4.10. I'd like to have a version that works well in the trunk branch independently of how long the safe point change will take. I don't think it's so premature to doe this change, since we can revert it easily without creating incompatibilities.

jhjourdan · 2019-10-14T09:21:57Z

For polling ASAP after major allocations, there is no one solution that satisfies all constraints

Which are the constraints? Is there a problem with polling signals there too?

This voids the certainty that the allocation just happened.

Actually, I don't think. What we want in memprof is that the callback gets called during the call of e.g., Array.make and not after. The user cannot observe whether the actual call to caml_alloc_shr happened before or after the signal handler.

In addition, calling process_pending_callbacks is likely going to be redundant at some point because non-noalloc C calls poll on return.

And at that point, my proposal is to remove that call. But only then.

runtime/array.c

runtime/signals.c

jhjourdan · 2019-10-14T13:28:44Z

This new version seems good to me! I reviewed and added a few nitpicks, but that's ok to merge as-is.

gasche · 2019-10-14T15:36:57Z

I'll approve (on @jhjourdan's behalf; I haven't reviewed myself) and merge once @gadmm is okay with it (he may want to take care of the nitpicks first) and the CI is green. Probably tomorrow.

gadmm · 2019-10-14T15:47:13Z

Alright, I was coming to the same conclusion after the failure of my idea of only calling memprof callbacks (which in addition you convinced me was useless), but your point about having trunk in good shape for experimenting convinced me it's the right approach. In the end we are very close to your original code (but avoid polling in sensitive places).

I'll turn to #8993 now and deal with the nitpicks in the meanwhile, so yes please wait for my greenlight.

the discussions around these PRs really start being too long...

I am responsible for this as well, I need to be sure I understand everything, and maybe some parts would have been easier to discuss in person. I could try to pass by Paris more often. Thanks for the patient replies.

jhjourdan · 2019-10-15T08:35:12Z

The CI failure corresponds to the repetitive failures that we see on the Inria CI for a few days. I don't know what this is, but this is probably unrelated to this PR.

runtime/signals.c

gadmm · 2019-10-15T09:59:35Z

The CI failure corresponds to the repetitive failures that we see on the Inria CI for a few days. I don't know what this is, but this is probably unrelated to this PR.

Good to know. I was taking it personally!

This is dead code since removal of vmthreads.

…on)" caml_raise_if_exception remains private; all public _exn functions will be provided with a directly-raising variant to keep supporting current programming styles.

gasche · 2019-10-15T12:17:01Z

I'll turn to #8993 now and deal with the nitpicks in the meanwhile, so yes please wait for my greenlight.

Note: I think it would be nice to merge this one soon, if it's ready to be merged, instead of trying to get both this one and #8993 perfect at the same time. You can always rebase #8993 on top of the final version of the current PR, and you can also do some changes in #8993 to the changes that were done here if everyone agrees. But keeping PRs open and in-flight has a cost (it introduces a risk of conflicts for other PRs, etc.), and I think we should try to polish and merge quickly when people have converged on a PR.

jhjourdan · 2019-10-15T12:23:58Z

There is a conflict introduced by #8809. This is just a matter of renaming a caml_check_urgent_gc into caml_process_pending_callbacks.

gadmm · 2019-10-15T15:05:03Z

Just pushed one based on your latest suggestion. caml_process_pending_callbacks becomes caml_do_urgent_gc_and_callbacks. Indeed, we do not care if check_urgent_gc was done twice, and in the latest version of the code this would happen only in caml_garbage_collection. I think this is clearer indeed, and made possible because I now call signal handlers directly in enter/leave_blocking_section, and for the functions in gc_ctrl.c the change does not matter. In the end the distinction caml_do_urgent_gc_and_callbacks vs. caml_check_urgent_gc_and_callbacks reflects well the clarification we did during this PR.

Let me now have a look at your proposed patch.

gadmm · 2019-10-15T15:06:12Z

I also added your bugfix from the other PR, thanks!

gadmm · 2019-10-15T15:14:22Z

I have added the change to caml_make_array from your patch. Other than that it is almost identical. The difference is your change to enter/leave_blocking_section. I say that the fact that some signals do not interact at all with the GC is enough to treat them specially, and the fact that there is no need to "fix" the previous behaviour is enough to leave it as in 4.9. (The rule that all callbacks have the same entry point is already broken inside caml_memprof_track_young.)

jhjourdan · 2019-10-15T15:20:02Z

Ok, thanks! Let's wait for CI, and if it succeeds, then I think this is good to merge, and I no longer have any more remarks (I think? ;) ). The only thing that you will need to do is, of course, squashing your "fixup" commit.

gadmm · 2019-10-15T15:23:28Z

Done! Thanks.

gasche

(approval on @jhjourdan's behalf; I did not review the code myself)

gadmm · 2019-10-15T16:52:15Z

There's still a bug to fix.

In 8691, caml_check_urgent_gc was merged with the function that runs asynchronous callbacks. The rationale was that caml_check_urgent_gc already runs finalisers, and so could have run any asynchronous callbacks. We agreed on a different strategy: we know that users could not rely on asynchronous callbacks being called at this point, so take the opportunity to make it callback-safe, like was done for allocation functions. The new check_urgent_gc no longer calls finalisers (nor any callbacks), and instead two internal functions are introduced: * caml_do_urgent_gc_and_callbacks : function to perform actions unconditionally. * caml_check_urgent_gc_and_callbacks : function that checks for something to do, and then executes all actions (GC and callbacks).

gadmm · 2019-10-15T17:28:30Z

The following change which I forgot when deciding to directly call caml_process_pending_signals was causing a loop, because caml_something_to_do was never reset.

diff --git a/runtime/signals.c b/runtime/signals.c
index 7ccf60cdf..11a47e5ae 100644
--- a/runtime/signals.c
+++ b/runtime/signals.c
@@ -169,7 +169,7 @@ CAMLexport void caml_enter_blocking_section(void)
     caml_enter_blocking_section_hook ();
     /* Check again for pending signals.
        If none, done; otherwise, try again */
-    if (! caml_something_to_do) break;
+    if (! signals_are_pending) break;
     caml_leave_blocking_section_hook ();
   }
 }

jhjourdan · 2019-10-15T19:19:51Z

The following change which I forgot when deciding to directly call caml_process_pending_signals was causing a loop, because caml_something_to_do was never reset.

Alright. Another fix would be to call caml_check_pending_signals_and_urgent_gc.

gadmm · 2019-10-15T19:30:50Z

Is there any issue with this version?

jhjourdan · 2019-10-15T19:36:08Z

Not really :D That's mostly a matter of respecting abstraction barriers: only one entry point for async callbacks.

gadmm · 2019-10-15T19:48:17Z

One way to see it is that the GC, memprof_track_young and enter/leave_blocking_section are "friends" with signals.c. The rest of the runtime is asked to use check_urgent_gc_and_callbacks or one of the public functions. Do you want me to document it?

jhjourdan · 2019-10-15T20:05:35Z

No, that's fine. Leave it as-is.

jhjourdan · 2019-10-16T09:07:19Z

Another failure of OCamlDebug on Windows.... I really don't know what this is, but this really looks like something we have to look at.

jhjourdan · 2019-10-16T09:27:06Z

However, it really seems unrelated to this PR, since it triggers repeatedly on different PR since Monday. Thus, it should not prevent merging this PR.

I opened an issue for the CI failure : #9043.

jhjourdan · 2019-10-16T11:32:51Z

@gasche : could we merge this?

gasche · 2019-10-16T11:58:36Z

It's really not easy for me, looking at the conversation, to tell that you have converged to a final state. (When I previously thought you had converged I was later told there was a bug, and I'm not completely sure which bug and how/whether it was solved.) I'm going to trust you on this.

gadmm · 2019-10-16T12:11:11Z

Thanks for merging. Indeed, we could have been clearer. I am very satisfied with the end result, in particular thanks to @jhjourdan's thorough review and explanations I think the good choices have been made.

jhjourdan · 2019-10-16T12:15:33Z

It's really not easy for me, looking at the conversation, to tell that you have converged to a final state. (When I previously thought you had converged I was later told there was a bug, and I'm not completely sure which bug and how/whether it was solved.) I'm going to trust you on this.

Sorry for that. I know this has not been clear, mostly my fault: I turned the green light on while I have not done my final, thorough review.

gadmm force-pushed the resource-safe-api-pt0 branch from a856314 to a03cca9 Compare October 11, 2019 17:40

gadmm mentioned this pull request Oct 11, 2019

Resource-safe C interface, part 1 (the 4.10 backwards-compatibility edition) #8993

Merged

1 task

gadmm force-pushed the resource-safe-api-pt0 branch from a03cca9 to c9ab845 Compare October 11, 2019 18:33

jhjourdan reviewed Oct 12, 2019

View reviewed changes

gadmm force-pushed the resource-safe-api-pt0 branch 2 times, most recently from e13511e to 7b02c13 Compare October 13, 2019 22:50

jhjourdan reviewed Oct 14, 2019

View reviewed changes

runtime/array.c Outdated Show resolved Hide resolved

jhjourdan reviewed Oct 14, 2019

View reviewed changes

runtime/signals.c Outdated Show resolved Hide resolved

gadmm force-pushed the resource-safe-api-pt0 branch from 7b02c13 to 509b284 Compare October 14, 2019 13:07

xavierleroy added this to the 4.10 milestone Oct 14, 2019

gadmm force-pushed the resource-safe-api-pt0 branch from 509b284 to d19bfb5 Compare October 14, 2019 13:11

jhjourdan reviewed Oct 15, 2019

View reviewed changes

runtime/signals.c Show resolved Hide resolved

gadmm added 3 commits October 15, 2019 14:02

[cleanup] Remove async_action_hook

6dbbc26

This is dead code since removal of vmthreads.

[cleanup] Rename caml_set_something_to_do into caml_set_action_pending

e64a861

[cleanup] Factor "if (Is_exception_result) caml_raise(Extract_excepti…

85d712a

…on)" caml_raise_if_exception remains private; all public _exn functions will be provided with a directly-raising variant to keep supporting current programming styles.

gadmm force-pushed the resource-safe-api-pt0 branch from a0f0d4b to 2a6d038 Compare October 15, 2019 14:59

gadmm force-pushed the resource-safe-api-pt0 branch from 2a6d038 to a61ea89 Compare October 15, 2019 15:17

gadmm force-pushed the resource-safe-api-pt0 branch from a61ea89 to a0c82c7 Compare October 15, 2019 15:23

gadmm force-pushed the resource-safe-api-pt0 branch from a0c82c7 to 5e57970 Compare October 15, 2019 15:39

gasche approved these changes Oct 15, 2019

View reviewed changes

gadmm force-pushed the resource-safe-api-pt0 branch from 5e57970 to 2433b93 Compare October 15, 2019 17:29

gasche merged commit 8142044 into ocaml:trunk Oct 16, 2019

gadmm deleted the resource-safe-api-pt0 branch October 16, 2019 12:11

jhjourdan mentioned this pull request Jul 21, 2020

EINTR-based signals, again #9722

Merged

1 task

gasche mentioned this pull request Jan 19, 2023

statmemprof is absent in OCaml 5.0 #11911

Closed

gadmm mentioned this pull request Oct 5, 2023

Resource-safe C interface: caml/fail.h #12407

Merged

Conversation

gadmm commented Oct 11, 2019

Uh oh!

gadmm commented Oct 12, 2019

Uh oh!

jhjourdan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gadmm commented Oct 13, 2019

Uh oh!

jhjourdan commented Oct 13, 2019

Uh oh!

gadmm commented Oct 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gadmm commented Oct 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gadmm commented Oct 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhjourdan commented Oct 14, 2019

Uh oh!

jhjourdan commented Oct 14, 2019

Uh oh!

Uh oh!

Uh oh!

jhjourdan commented Oct 14, 2019

Uh oh!

gasche commented Oct 14, 2019

Uh oh!

gadmm commented Oct 14, 2019

Uh oh!

jhjourdan commented Oct 15, 2019

Uh oh!

Uh oh!

gadmm commented Oct 15, 2019

Uh oh!

gasche commented Oct 15, 2019

Uh oh!

jhjourdan commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gadmm commented Oct 15, 2019

Uh oh!

gadmm commented Oct 15, 2019

Uh oh!

gadmm commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhjourdan commented Oct 15, 2019

Uh oh!

gadmm commented Oct 15, 2019

Uh oh!

gasche left a comment

Choose a reason for hiding this comment

Uh oh!

gadmm commented Oct 15, 2019

Uh oh!

gadmm commented Oct 15, 2019

Uh oh!

jhjourdan commented Oct 15, 2019

Uh oh!

gadmm commented Oct 15, 2019

Uh oh!

jhjourdan commented Oct 15, 2019

Uh oh!

gadmm commented Oct 15, 2019

Uh oh!

jhjourdan commented Oct 15, 2019

gadmm commented Oct 13, 2019 •

edited

Loading

gadmm commented Oct 13, 2019 •

edited

Loading

gadmm commented Oct 13, 2019 •

edited

Loading

jhjourdan commented Oct 15, 2019 •

edited

Loading

gadmm commented Oct 15, 2019 •

edited

Loading