Skip to content

Major GC hooks are no longer allowed to interact with the GC heap.#8711

Merged
xavierleroy merged 4 commits intoocaml:trunkfrom
jhjourdan:major_gc_hooks_no_allocation
Jun 4, 2019
Merged

Major GC hooks are no longer allowed to interact with the GC heap.#8711
xavierleroy merged 4 commits intoocaml:trunkfrom
jhjourdan:major_gc_hooks_no_allocation

Conversation

@jhjourdan
Copy link
Copy Markdown
Contributor

@jhjourdan jhjourdan commented Jun 3, 2019 •
edited
Loading

As discussed in #8691 (comment) (by @stedolan):

BTW, the fact that the major GC is calling hooks that are allowed to allocate and execute arbitrary OCaml code (see comment in misc.h) completely kills the point of this PR. I did not know about them!

Would it be possible to add the restriction that these functions should not call an OCaml callback? Where are these functions used, in practice?

I didn't know about these functions either!

I think it would be possible to add this restriction. I can only find two uses of this feature: one in flow and one in Jane Street's internal code. Neither allocates, performs callbacks, or indeed calls any OCaml runtime functions at all.

Since this is nominally a breaking change, I think there should be a seperate PR which just edits the comment in misc.h.

@jhjourdan jhjourdan force-pushed the major_gc_hooks_no_allocation branch from deeb79e to 78d2ebe Compare June 3, 2019 18:21
@stedolan
Copy link
Copy Markdown
Contributor

stedolan commented Jun 4, 2019

In fact, the original PR proposing these hooks (#6675) says this:

I imagine there will be constraints on the C callbacks -- for example, the callbacks must not modify or allocate OCaml memory.

I can't find any uses of this feature that don't already conform to this restriction.

damiendoligez
damiendoligez approved these changes Jun 4, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@damiendoligez damiendoligez left a comment

Choose a reason for hiding this comment

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

I don't think allowing allocations in these hooks was a good idea.

@xavierleroy xavierleroy merged commit 63a16e8 into ocaml:trunk Jun 4, 2019
@xavierleroy
Copy link
Copy Markdown
Contributor

xavierleroy commented Jun 4, 2019

I merged in trunk after light editing.

@stedolan stedolan mentioned this pull request Jun 5, 2019
Merged
@gasche gasche mentioned this pull request Jan 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@damiendoligez damiendoligez damiendoligez approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jhjourdan @stedolan @xavierleroy @damiendoligez