About the slowdown. In the patch of 2011, ephemerons used a specific tag and so codes that didn't use ephemerons where not impacted. Here weak pointers are implemented using ephemerons (one additional word) and none of them use a specific tag. I tried to reduce the slowdown but any attempt had the opposite effect. The only way I can think of is to separate in two different lists ephemerons and weak pointers. Currently you can go from one to the other, a weak pointer is just an ephemeron with an empty data.

I had wished for inclusion of ephemerons for a long time, and it's great to see some progress in this direction!

For users who don't care about the seed property, would it make sense to expose "Make" functors (with hash: t -> int), assuming this can be done without too much code bloat?

I'm also slightly reluctant to keeping Ephemeron.Obj in the same unit as Ephemeron. What about moving it to a submode of Obj (hence: Obj.Ephemeron), or creating a different unit?

The documentation of Ephemeron.Obj.create should explain the meaning of the argument (number of keys).

Those are only documentation comments:

1. I think the documentation of K1.MakeSeeded and K2.MakeSeeded should make it more clear to the casual programmer what they actually build (i.e. IIUC a hashtable were both keys and values are weak: whenever one or the other becomes unreachable the binding automagically disappear from the hashtable)
2. We now have two notion of weak hashtables in the stdlib: those of Weak and those of Ephemeron. I think they should be somehow distinguished from a terminological point of view to ease our programming discussions. I would call Weak's current hashtables, weak hashed sets (as I think to most programmers mind hashtables are understood as being maps). In that case Weak's doc should be updated to reflect that.
3. The toplevel comment of the module says ephemeron and weak tables. That's the only use of weak table, maybe this should be changed to ephemeron and weak hash tables, I was looking for those unhashed tables but didn't find them.

Other than that:

1. I share the sentiment of Alain that the unsafe stuff should go in Obj. A toplevel Obj is immediately suspicious and the spot of attention in case of hard problems.
2. I would be curious to test the slowdown on a real world React code base but unfortunately I don't have that at hand.

The last push took, I hope, into account the majority of the current remarks:

@alainfrisch :

1. Add Make functor: bobot/ocaml@178c475
2. I hesitated but since you and dbuenzil agree, I will put Ephemeron.Obj in Obj.Ephemeron
3. Ephemeron.obj.create fixed: bobot/ocaml@20e9cf9

@dbuenzli doc:

1. The values are not really weak since they are strong if the keys are alive. I added more explanation in bobot/ocaml@beed79b
2. Weak.Make is indeed hash set. bobot/ocaml@9bc9ab5 fix that.
3. Unhashed weak table can be fun :) but I fixed the missing "hash" in bobot/ocaml@f622cad

@dbuenzli code:

1. I will do that. But for the typed version should I add K3, K4,... until when?
2. I would love to see that, perhaps the new opam switch can simplify that.

I added a note about marshalling in bobot/ocaml@ec11031

Where does the restriction about marshaling come from?

Weak pointers already have this restriction.I think the problems are:

• Weak and ephemerons use the Abstract Tag, so special code must be written.
• I don't know during input how to recognize them in order to add them to the linked list caml_ephe_list_head.
• That makes sense only if you marshal with sharing.

But if you are guided by the type and you keep the sharing there is no problem, I think.

Another point I think we can lift the restriction about integer in Weak and Ephemeron. We can say that an integer value is always alive. That could remove some complication in the case of int Lazy.t Weak.t where the shortcut is forbidden. I can try to write the code after a first review of this merge-request or before if needed

Ephemeron.Obj moved in Obj.Ephemeron in commit bobot/ocaml@b0f6a7b

About integers, the first version of Weak allowed integers as always-live values but then people complained that the integers they put in their weak hash sets would never disappear, so I removed them. I don't think it's a good idea to add them.

I just added commit dc49f2e to remove code duplication between caml_ref_table and caml_ephe_ref_table. Needs to be reviewed.

Since Ephemeron.Obj is now in Obj.Ephemeron there is no ephemeron with arbitrary arity in Ephemeron. So I propose to add ephemerons Ephemeron.Kn.t with an arbitrary number of key with the same type [bobot/ocaml@e39fa5d](sorry the implementation is in [bobot/ocaml@0a3c081]). The module Ephemeron.K can be reserved for when somebody finds a nice interface for heterogeneous typed ephemerons with arbitrary arity.

We would like to merge this in trunk pretty soon, but @bobot needs time to rebase on top of the current trunk. I think the plan is to actually rebase on top of #297 (rather than the other way around).

@damiendoligez The branch is rebased on trunk. There is no big differences compared to before, more comments, I hope better naming. Just c038923, which factorize the short-circuiting of Forward tag, have been heavily modified. We discussed it before, but I think we forget that there are different use of it. I used flags for selecting the different mode. What do you think of it?

I need to review #297 before rebasing above it.

Some general remarks:

• alloc_generic_table was not marked as static in a previous version of the branch, so it was exported in byterun/libcamlrun.a. Perhaps we could check during make tests that no functions are exported without the camlprefixed. Currently two functions seems wronly exported.

# nm -g --defined-only -A byterun/libcamlrun.a | grep -v " caml"
byterun/libcamlrun.a:backtrace_prim.o:0000000000000020 T process_debug_events
byterun/libcamlrun.a:backtrace_prim.o:0000000000000810 T read_main_debug_info
byterun/libcamlrun.a:debugger.o:0000000000000010 D marshal_flags
byterun/libcamlrun.a:main.o:0000000000000000 T main

• Git Tips that I learned the hard way, if one have many fixup:... for fixing commits, you should rebase them one by one.
• Why do we have to modify boot/ocamlc when adding primitive? Can't we have the information about the primitive in a separate generated text file that will be read by boot/ocamlc? So that these binaries that can't be merged automatically have to be modified less often?

I only spent two minutes looking at this patch, but I have two comments:

1. Have you ensured that the refactoring in the major GC does not cause a performance degradation? In particular, I don't think the "inline" keyword is sufficient to force inlining of that function. I think for gcc you can ensure that using __attribute__((always_inline)) or something like that, though I don't remember offhand.
2. I suggest Obj.Ephemeron.eph be called Obj.Ephemeron.t instead.

@mshinwell For point 1. I have not read the assembly code obtained to see if the refactoring is inlined. However in the performance test I've done (in the header of this MR) there is no regression for the first commits (0,1,2). If people prefer I can try to force inlining.

For point 2. You are completely right, I do the substitution

By mail I received some comments for the documentation of the ephemerons by William (Romait Troit) that I will take into account shortly.

The MR #515 add the missing changes and modification of the documentation. (It target 4.03 but is tagged 4.04 because it contained other more invasive changes)

Hmm. Whilst that change does break the current code, I cannot see how it would cause the specific test failure. Has anything else changed about when weak pointers are cleared? It seems like a single Gc.full_major was previously enough to clear them in the test, but now two Gc.full_majors are required.

If there is a finalizer attached to it you need indeed two rounds. In the first the value is marked as to be finalized and alive ( since the finalizer will use the value). In the second it is marked dead and so can be removed from the weak pointer and swept.

Previously you needed only one round for the value to be removed from the weak pointer, but two rounds for the value to be swept. Since for fixing the semantic of weak pointer the value is removed from weak pointer only when dead so swept, it is done in the second round.

I have to say that this change to the ordering between finalisers and weak references is quite annoying. It is not now possible to get something to run after a weak has been emptied, which makes it hard to tidy up the data structure (e.g. hashtable) which contains the now empty weak reference.

I considered trying to replace the weak reference with an ephemeron pointing to a fresh block, and then place the finaliser on that block instead of on the target of the weak reference -- but looking through the patch it is clear that the finaliser may still run before we have entered the clean phase which means the ephemeron will not yet be cleared.

I appreciate the reasons for changing the order, but it would have been good to simultaneously add support for the other kind of finalisers (i.e. the ones which do not receive their target as an argument and so can safely be run after weak has been cleared) since it is now quite difficult to handle clean up of data structures containing weak pointers.

As a side note, whilst looking around the patch, it seemed to me that line 479 of major_gc.c:

ephes_to_check = ephes_checked_if_pure;

(the one just after calling caml_final_update) is incorrect.

• If caml_final_update did not darken any new values then we do not need to check the ephemerons again.
• If caml_final_update did darken some new values then caml_darken would have set ephe_list_pure back to 0. This means that we will start checking the ephemerons again with ephe_list_pure set to 0 rather than 1, so even if none of the ephemerons were effected by the newly dark values we will end up scanning them twice.

Either way it looks like it will tend to scan the ephemeron list one more time than is necessary.

Simply removing that line would fix it.

@lpw25

I have to say that this change to the ordering between finalisers and weak references is quite annoying. It is not now possible to get something to run after a weak has been emptied, which makes it hard to tidy up the data structure (e.g. hashtable) which contains the now empty weak reference.

I want to keep this problem on the radar. Could you open a Mantis PR?

@damiendoligez Doesn't MPR 7210 is enough for that ?

@lpw25 Do you tried to use Ephemerons.K1.Make or the same implementation since you have some additional functions ? I think the size of Ephemerons.K1.Make table will be at most twice the size of your table. A cleanup is done before resizing and the resizing is aborted if after cleanup the current number of element is less than half the current size. That keeps the constant amortized time complexity.

I appreciate the reasons for changing the order, but it would have been good to simultaneously add support for the other kind of finalizer.

I agree. It will be too late for 4.03 but for 4.04 I'm willing to propose patch to add the support if it is seen as needed in MPR 7210.

I considered trying to replace the weak reference with an ephemeron pointing to a fresh block, and then place the finalizer on that block instead of on the target of the weak reference -- but looking through the patch it is clear that the finalizer may still run before we have entered the clean phase which means the ephemeron will not yet be cleared.

It is even more sure than not, if I understand what you do correctly. The ephemeron is unset in the same gc round than the key is swept, so at least one round after the "finalization" of the key.

As a side note, whilst looking around the patch, it seemed to me that line 479 of major_gc.c

It is an interesting remark. I agree that it is unnecessary. I will do the GPR for 4.04.

Doesn't MPR 7210 is enough for that ?

Yes it is, thanks.