Restore 'caml_unregister_frametable' in OCaml 5

[recoules]

This PR is a "fork" of the PR #12756. It focuses only on restoring the function caml_unregister_frametable in OCaml 5.

In order to be able to use caml_unregister_frametable at any moment (even in a custom block finaliser), I use a pending list of frames to remove. The frames will only be removed the next time we want to register a new frame (STW event).

A mutex protects the shared table to concurent call to caml_unregister_frametable.

This PR also introduces a new mechanism to register a frametable (caml_copy_and_register_frametables). The copy is attached to the caml_frametable_list element and will be automatically released with the unregister process.

[recoules]

Hi @gasche,

Here is a tentative to restore caml_unregister_frametable while trying to introduce as fewer change as possible.
This PR will work fine for my use case and I hope it will be easier to review.

Regards,

[gasche]

I had a superficial first look at this PR on the occasion of meeting @recoules in-person at JFLA'24. Here is my first batch of comments.

[gasche]

I would find it more natural/predictable/future-proof to clean at the next STW of some sort, instead of waiting an indefinite amount of time to do the cleanup.

[gasche]

One place where to do this would be in major_gc.c, right before or after the call to caml_code_fragment_cleanup_from_stw_single within stw_cycle_all_domains.

[gasche]

I guess that we may want to do the cleanup here (in register_frametables) and there (in the major GC), instead of doing in just one place. If we did it only in the GC, we avoid unbounded delays but then the addition logic in register_frametables may still encounter stale entries.

[recoules]

I agree, but I face a difficulty : major_gc.c did not depend on frame_descriptors.c (the latter appears after in the compilation file order). Changing this order is "possible" but will mess with the actual structure of the Makefile (macros

ocaml/Makefile

Line 1094 in 733c2c5

runtime_COMMON_C_SOURCES = \

and

ocaml/Makefile

Line 1159 in 733c2c5

runtime_NATIVE_ONLY_C_SOURCES = \

)

[gasche]

You are the C expert, can we get away with a extern void clean_frame_descriptors(void); declaration somewhere in major_gc.c, without changing the linking order?

[recoules]

Well, actually, it is not an issue with the declaration (we can include frame_descriptors.h without any issue) but I think it is about the order in which we feed the object file .o to the compiler.

A possible workaround would be to expose a register_cleanup_callback in the GC, so the latter code can register cleanup routine without being a link dependency (it would also avoid the issue that the function is a "native only function" while the GC code is common with bytecode too).
Note: I did not check if there is not already this kind of hook.

[recoules]

Hi @gasche, I get a simple workaround that will not deeply modify the Makefile but I do not know if it fits the coding standard of the project.
The idea is to simply (conditionally) import the frame_descriptors.c file into major_gc.c file.
Do you think it is ok ?

[gasche]

This sounds like unwanted complexity to me. I propose to just drop my suggestion and keep things as you wrote them initially, where the removal is only forced when adding more frametables later.

[recoules]

Ok, so should I drop the commit 6bc2386 ?

[gasche]

I guess so. Sorry for the trouble.

[gasche]

a newline before the prototype would be nice to avoid confusions around: which comments documents what.

[recoules]

I added a space / reorder the declaration in 6965188

[recoules]

Hi @gasche,

So, if you agree, I can drop 6bc2386, squash in the history the commits ba09794 and 7a35a41.
Then, I think we will have to add a line in the Changes file in order to pass the CI.

After that, this PR should be ready to merge, isn't it?

[gasche]

I did another round of looking at the code (and comparing with the 4.x implementation), and I agree that this is fine, but I would like to see slightly more documentation of the synchronization mechanisms for removal.

(Someone should be able to understand the code and make the right design decision to evolve it from the code comments alone, without trying to go back to read the PR discussions.)

[gasche]

Nitpick: the house style is to use block comments for the whole block, not once per line (except in the license header). Could you move to that?

[gasche]

So, if you agree, I can drop 6bc2386, squash in the history the commits ba09794 and 7a35a41.
Then, I think we will have to add a line in the Changes file in order to pass the CI.

Sounds good, plus hopefully some more comments as suggested in my review and a minor style change. Yes, I think this would be ready to merge afterwards. The changes entry should go in the "Working version" section: this is a new feature so it will not be backported in the upcoming 5.2 release, it will be included in 5.3.

[recoules]

Thank you, I will try to address the few changes this week and ping you again.

[recoules]

@gasche Just a small word to tell you I do not forget this one but I was quite busy lately and will address your points the next week.
Regards,

[recoules]

Hi @gasche, I finally took the time to rebase the branch and update the comments as requested.
Feel free to report any changes if the comments are not clear enough.
Regards,

[recoules]

Hi @gasche, do you have any insight on this?

[recoules]

Hi @gasche, I rebased the branch yesterday, everything seems to be ok.

[gasche]

Apologies for the delay. This looks good! Approved. (See comment for minor grammatical nitpick.)

[gasche]

Merged! Thanks for the tenacious contribution.

[recoules]

Thank you :)

[gadmm]

Hi @recoules, I came across caml_unregister_frametable while reviewing locks in the runtime, and I have a couple of questions if you don't mind helping me reviewing it:

• Why is the lock taken only during remove_frame_descriptors, and not during other operations (e.g. register)? Are all other operations specifically during a STW and remove specifically outside of STW?
• Is remove_frame_descriptors meant to be called from the mutator while holding the domain lock specifically (as opposed to, possibly, places where the status of the domain lock is unclear)? (If so, I'd like to replace caml_plat_lock_blocking with caml_plat_lock_non_blocking.)

[recoules]

Hi @gadmm,

Why is the lock taken only during remove_frame_descriptors, and not during other operations (e.g. register)? Are all other operations specifically during a STW and remove specifically outside of STW?

All operations but remove_frame should be called outside STW and will trigger a STW.
So, there is no need to lock things since there is only one domain that do the job.
For my use case, I needed that remove_frame can be called from a STW (for instance, in a custom bloc finalizer).
So it is performed in two steps:

• first we put the elements in the "todolist" ;
• then, in the next STW, we perform all the actual pending action of removing.

Is remove_frame_descriptors meant to be called from the mutator while holding the domain lock specifically (as opposed to, possibly, places where the status of the domain lock is unclear)? (If so, I'd like to replace caml_plat_lock_blocking with caml_plat_lock_non_blocking.)

It am not sure to understand everything but, in your proposition, the code will call caml_enter_blocking_section_no_pending and I believe it would not work well within a STW.

Regards,

[gadmm]

Thanks @recoules! If remove_frame_descriptor can be called from a custom block finalizer, then it should call caml_plat_lock_blocking indeed. I will add a comment with this information.