Skip to content

Caml_alloc: Document new behaviour and upgrade path (check_urgent_gc)#8941

Merged
gasche merged 1 commit intoocaml:trunkfrom
gadmm:breaking-check_urgent
Sep 30, 2019
Merged

Caml_alloc: Document new behaviour and upgrade path (check_urgent_gc)#8941
gasche merged 1 commit intoocaml:trunkfrom
gadmm:breaking-check_urgent

Conversation

@gadmm
Copy link
Copy Markdown
Contributor

@gadmm gadmm commented Sep 14, 2019

This is a follow-up to #8691 (comment). I find it a good idea to document the new behaviour of allocation functions and give an upgrade path for users of the C FFI in case they could be affected, e.g. if they have long-running C code. Currently this does not change any code.

However, I find three issues with the current interface and I may improve it according to feedback.

  • I find it better to give the FFI user a function that does not take an extra root as argument. What would be the name?
  • The advice to call check_urgent_gc is currently hard to follow in resource-conscious C code, because when an asynchronous exception must be raised, it automatically jumps back to the OCaml caller and prevents cleanup. (This is an issue the Coq VM is facing for instance.) A solution would be to have a variant of check_urgent_gc_exn that returns the exception, and document that it has to be reraised with raise_in_async_callback. (It looks like some amount of work, but I see how it could help in situations like the Coq VM.). The same should be done for {enter,leave}_blocking_section.
  • check_urgent_gc is not called when an allocating C function returns to OCaml, unlike I had documented at first. This can have unexpected consequences given that OCaml is currently poor in emitted safe points (cf. the problem of uninterruptible loops). There are possibilities that this causes bugs in current programs. I read that a solution is to change caml_c_call, but this could also have a much simpler fix after the better safe points patch.

Note: it's good to consider this for 4.10

@gadmm gadmm changed the title Document new behaviour and upgrade path (check_urgent_gc) Caml_alloc: Document new behaviour and upgrade path (check_urgent_gc) Sep 14, 2019
execution. These asychronous callbacks can execute arbitrary OCaml
code, including raising asynchronous exceptions.

The function \verb"caml_check_urgent_gc" from "memory.h" checks for
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the docs @gadmm. Isn't that a very bad name for what it's doing ? I mean if I'm checking something I rather expect an existence test, not side effects. Maybe caml_exec_urgent_gc would be a better name ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name cannot change for backwards compat, but I'm open to suggestions for the name of a new function that takes void as argument instead. caml_async_callbacks, caml_safe_point...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry didn't get this was already in the wild.

@gadmm
Copy link
Copy Markdown
Contributor Author

gadmm commented Sep 15, 2019

With this additional commit, I change alloc_custom_gen to not trigger asynchronous callbacks, like in caml_alloc. This fixes the discrepancy between the doc and the implementation, and makes the FFI consistent.
@jhjourdan
The change to alloc_custom_gen has been moved to #8993; now this PR is only about documentation.

@gadmm gadmm force-pushed the breaking-check_urgent branch from 9e68476 to aa13b8c Compare September 28, 2019 00:10
Copy link
Copy Markdown
Contributor

@jhjourdan jhjourdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @gadmm, for this PR. The added documentation looks fine to me. I'll add comments on the other proposals in the other PRs.

Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging on behalf of @jhjourdan's approval. Thanks @dbuenzli and @jhjourdan for your help, and thanks @gadmm for the documentation.

@gasche gasche merged commit dc56217 into ocaml:trunk Sep 30, 2019
@gadmm
Copy link
Copy Markdown
Contributor Author

gadmm commented Sep 30, 2019

This documents a new behaviour of caml_check_urgent_gc which is still being discussed at #8691 , and there is also was an unchecked todo box "updated after #8993". Let us remind each other that this needs to be updated before 4.10 is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants