Add Ephemerons to OCaml #22

Merged
merged 32 commits into from Jan 27, 2016

Conversation

Projects
None yet
Contributor

bobot commented Mar 25, 2014

Ephemerons

The current OCaml runtime proposes two kinds of pointers:

  • A points hardly to B: if A is alive, B is alive (usual pointers)
  • A points weakly to B: when the GC decide that B stops to be alive, A stops to point to B

Weak pointers are very handy when you want to be able to access a data without creating memory leaks. For example hashconsing can be done using the weaksets defined in Weak.Make. However you can't create general weaktable with the current runtime: associative table that keep a data until the key is not needed anymore. The problematic case is when the data points to the key(cf 10).

Barry Hayes proposed in 1997 (cf 9) the notion of ephemerons which allow to code such table. In its simplest form an ephemeron A with key K and data D keeps D alive while A and K are alive. To sum up ephemerons add the possibility to express conjunctions where you normally have only disjunctions.

Request for Comment and Merge-Request

This merge-request adds ephemerons to ocaml with particularly a modification of the runtime, 4, and a new module in the standard library, 6. An untyped API with an arbitrary number of keys is given in Ephemerons.Obj and two specialized API for one and two keys are provided in Ephemerons.K1 and Ephemerons.K2. Each of these three APIs define a functor for constructing weaktables with the same signature than Hashtbl.

Before an actual review of the C or OCaml implementation parts, comments about the OCaml interface are more than welcomed.

The modifications are splitted as follows:

  • 1, 2: code factorization that can be removed from the merge request but at the end without them some functions start to be very big
  • 3: A correction of the behavior of the weak pointer when more than one weak array points to one value
  • 4: Add the ephemerons in the runtime (simple but inefficient way). The stdlib is modified in another commit in order to simplify rebase and bootstrapping.
  • 5: just add a getter for Hashtbl.randomized
  • 6: Add the module Ephemeron in the stdlib and some tests in the testsuites
  • 7: Shortcut clean phase if possible (can be removed from the merge request)
  • 8: Optimize the implementation of the ephemerons and mitigate the worst case

The code have been evaluated on Why3, "don't use" correspond to why3 commit 11 and "use" to why3 commit 12 that replace the half-satisfying weak-table solution by weak-table provided by Ephemeron.

0 1 2 3 4 5 6 7 8
don't use 12.96 (0.02) 12.96 (0.13) 12.93 (0.03) 13.26 (0.06) 13.70 (0.27) 13.87 (0.16) 13.30 (0.04) 13.37 (0.10) 13.10 (0.09)
use NA NA NA NA NA NA 12.08 (0.06) 12.02 (0.05) 11.88 (0.05)

We can see that the slowdown for a code that heavily use weak pointers is of 1-2%, and the speedup when using weak-pointers (weaksets) and weaktable is of 8% without counting the fact that the weaktable is "perfect" and simple to use.

nb: the branch is not directly mergeable because of boot/ocaml..., but at the end of the review process I will rebase the branch and doing the bootstrap.

How to reproduce the bench

clone why3, configure --enable-local, bin/why3config --detect. Add this section to why3.conf:

[prover]
command = "/bin/true %t %f"
driver = "drivers/null_smt.drv"
editor = ""
in_place = false
interactive = false
name = "null"
version = "0"

Bench with the command:

(exec bin/why3session.opt output --output /tmp --filter-prover null $(git ls-files "examples/**/why3session.xml") -L examples/bitvectors -F -L examples/vacid_0_binary_heaps/) >& /dev/null
stdlib/ephemeron.mli
+
+ The data is considered by the garbage collector alive if all the
+ full keys are alive and if the ephemeron is alive. When one of the
+ keys is not considered alive anymore bu the GC, the data is
@chambart

chambart Mar 25, 2014

Contributor

typo bu -> by

@bobot

bobot Mar 26, 2014

Contributor

thx, the fix will be autosquashed

Contributor

bobot commented Mar 26, 2014

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.

Contributor

alainfrisch commented Mar 26, 2014

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).

stdlib/ephemeron.mli
+ val get_key_copy: ('k,'d) t -> 'k option
+ (** [Ephemeron.K1.get_key_copy eph] returns [None] if the key of [eph] is
+ empty, [Some x] (where [x] is a (shallow) copy of the key) if
+ it is full. This function as the same GC friendliness as {!Weak.get_copy}
@gasche

gasche Mar 26, 2014

Member

minor typo: "This function as" -> "This function has"

@bobot

bobot Mar 26, 2014

Contributor

fixed in bobot@43c37be

stdlib/ephemeron.mli
+ reason.
+*)
+
+module type SeededS = sig
@dbuenzli

dbuenzli Mar 26, 2014

Contributor

This should be documented (something like Output signature of {!K1.MakeSeeded} and {!K2.MakeSeeded}.).

@bobot

bobot Mar 26, 2014

Contributor

applied in bobot@4d223bc

Contributor

dbuenzli commented Mar 26, 2014

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.
stdlib/hashtbl.mli
@@ -165,6 +165,11 @@ val randomize : unit -> unit
@since 4.00.0 *)
+val is_randomize : unit -> bool
@dbuenzli

dbuenzli Mar 26, 2014

Contributor

Shouldn't that rather be is_randomized ?

@bobot

bobot Mar 26, 2014

Contributor

Usually I use get_A, set_A, is_A. But perhaps here is_randomized is better, if someone else agree I will change that.

@hcarty

hcarty Mar 26, 2014

Contributor

is_randomized is a better name in this case.

@bobot

bobot Mar 26, 2014

Contributor

ok, fixed in bobot@46d1105 and bobot@cb6ff07

Contributor

bobot commented Mar 26, 2014

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

@alainfrisch :

  1. Add Make functor: bobot@178c475
  2. I hesitated but since you and dbuenzil agree, I will put Ephemeron.Obj in Obj.Ephemeron
  3. Ephemeron.obj.create fixed: bobot@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@beed79b
  2. Weak.Make is indeed hash set. bobot@9bc9ab5 fix that.
  3. Unhashed weak table can be fun :) but I fixed the missing "hash" in bobot@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.
Contributor

bobot commented Mar 26, 2014

I added a note about marshalling in bobot@ec11031

Contributor

alainfrisch commented Mar 26, 2014

Where does the restriction about marshaling come from?

Contributor

bobot commented Mar 26, 2014

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

Contributor

bobot commented Mar 27, 2014

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

Owner

damiendoligez commented Mar 31, 2014

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.

Owner

damiendoligez commented Apr 2, 2014

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

Contributor

bobot commented Apr 2, 2014

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@e39fa5d](sorry the implementation is in [bobot@0a3c081]). The module Ephemeron.K can be reserved for when somebody finds a nice interface for heterogeneous typed ephemerons with arbitrary arity.

lpw25 pushed a commit to lpw25/ocaml that referenced this pull request Oct 1, 2014

Leo White
Merge pull request #22 from lpw25/test-examples
Move examples to the test suite

stedolan added a commit to stedolan/ocaml that referenced this pull request Aug 18, 2015

Merge pull request #22 from kayceesrk/reset-stack
Reset realloc'ed stack to avoid useless scanning.
Member

gasche commented Nov 19, 2015

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).

+ let histo = Array.make (mbl + 1) 0 in
+ Array.iter
+ (fun b ->
+ let l = bucket_length 0 b in
@martin-neuhaeusser

martin-neuhaeusser Nov 29, 2015

In stats_alive: Why is the number of bindings in bucket b counted via the bucket_length function?
Is the function expected to compute the number of bindings that are still alive? In that case, should the call to bucket_length be replaced by a call of bucket_length_alive?
I expect this would produce the actual number of bindings that are still alive within the bucket?

@bobot

bobot Nov 29, 2015

Contributor

Well spotted. In fact the new warnings used during the compilation of the trunk also found it. Fixed.

Contributor

bobot commented Nov 29, 2015

@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?
Contributor

mshinwell commented Nov 30, 2015

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.
Contributor

bobot commented Dec 1, 2015

@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.

byterun/major_gc.c
+
+/**
+ Ephemerons:
+ During mark phaese the list caml_ephe_list_head of ephemerons
@chambart

chambart Dec 1, 2015

Contributor

typo: phaese -> phase

stdlib/ephemeron.mli
+ (** Define hash table on generic containers. *)
+
+ (** It can be used in conjunction of {!Obj.Ephemeron}
+ for building weak hash table for specific type of keys *)
@chambart

chambart Dec 1, 2015

Contributor

I'm quite uncomfortable with the idea of advertising the use of the Obj module. It may mean that a preinstantiated version of this functor could be exported if there is a safe way to do that.

@bobot

bobot Dec 1, 2015

Contributor

All the previous module K1,K2,Kn are preinstantiated versions of this functor. Do you prefer if I move this module to Obj?

@bobot

bobot Dec 1, 2015

Contributor

Or I can just remove the comment about {!Obj.Ephemeron}. People that wants K3 will be able to read the code of K2 and extend it.

@damiendoligez

damiendoligez Dec 4, 2015

Owner

I agree with @chambart. If this module is safe, you should remove the comment. If it's unsafe it should be moved to Obj.

@bobot

bobot Dec 25, 2015

Contributor

ok comment removed 03c3cf6

Changes
+ (François Bobot)
+- GPR#22: Add the Ephemeron module that implements ephemerons and weak
+ hash table (François Bobot, review by Damien Doligez, Daniel Bünzli,
+ Alain Frisch)
@damiendoligez

damiendoligez Dec 4, 2015

Owner

Add Pierre to this list.

+ result in the value being removed from one weak arrays and kept in
+ another one. That breaks the property that a value is removed from a
+ weak pointer only when it is dead and garbage collected. (François
+ Bobot, review by Damien Doligez)
@damiendoligez

damiendoligez Dec 4, 2015

Owner

We generally don't add to Changes the fixes for bugs that were never in a released version.

@bobot

bobot Dec 17, 2015

Contributor

I think the bug, even if I have not been able to create a test case, is present since at least when I started working on ephemerons, which is unfortunately, several release ago.

@bobot

bobot Jan 1, 2016

Contributor

Finally I have been able to create a test case d62950b. It is failing in 4.02.3.

byterun/major_gc.c
@@ -52,8 +53,48 @@ extern char *caml_fl_merge; /* Defined in freelist.c. */
static char *markhp, *chunk, *limit;
-int caml_gc_subphase; /* Subphase_{main,weak1,weak2,final} */
-static value *weak_prev;
+int caml_gc_subphase; /* Subphase_{mark_main,make_final,
@damiendoligez

damiendoligez Dec 4, 2015

Owner

typo: mark_final

byterun/major_gc.c
+ ephemeron_again:
+ if (caml_replace_forward(&child, CAML_REPLACE_FORWARD_NO_LONG)){
+ Field (v, i) = child;
+ goto ephemeron_again;
@damiendoligez

damiendoligez Dec 4, 2015

Owner

Please! replace with a while loop.

@bobot

bobot Dec 25, 2015

Contributor

After doing that I wonder why caml_ephe_clean was not doing the same thing and in fact caml_replace_forward should be used only on block. So I fixed the code and kept the goto... 501a35c

Owner

damiendoligez commented Dec 4, 2015

I had a quick look, I'll do a more thorough review soon.

@damiendoligez damiendoligez added this to the 4.03.0 milestone Dec 23, 2015

Contributor

bobot commented Dec 25, 2015

I have not yet used the fact that we can be in NO_NAKED_POINTER mode. So the code for ephemerons always use the slow In_heap test.

@@ -37,7 +37,8 @@ uintnat caml_percent_free;
uintnat caml_major_heap_increment;
CAMLexport char *caml_heap_start;
char *caml_gc_sweep_hp;
-int caml_gc_phase; /* always Phase_mark, Phase_sweep, or Phase_idle */
+int caml_gc_phase; /* always Phase_mark, Pase_clean,
@damiendoligez

damiendoligez Dec 28, 2015

Owner

Typo: Phase_clean

byterun/caml/memory.h
+ /* Do not short-circuit the pointer. */
+ return flags & CAML_REPLACE_FORWARD_IS_FORWARD;
+ } else {
+ *v = fv;
@damiendoligez

damiendoligez Dec 28, 2015

Owner

You need to add v to the ref_table if it was pointing to the major heap and now points to the minor heap. AFAIR, this is a long-standing bug that was fixed in the gc_patches branch, so let's be careful not to reintroduce it. See lines 226-227 in commit 0225ca0.

@bobot

bobot Dec 31, 2015

Contributor

I removed the factorization of the shortcutting of the forward pointer because, after rebase, with your fix the call site diverged even more and so the factorization doesn't make sense anymore.

byterun/major_gc.c
+#else
+ if (Is_block (child) && Is_in_heap (child)) {
+#endif
+ hd = Hd_val (child);
@damiendoligez

damiendoligez Dec 28, 2015

Owner

Be careful when rebasing on top of gc_patches, this is now chd (don't miss any occurrences below).

Contributor

bobot commented Jan 1, 2016

The branch is rebased on trunk (on top of trunk-gc-patches).

Contributor

mshinwell commented Jan 1, 2016

Have you done any measurements on code apart from Why3 to make sure that the GC performance isn't affected in general?
I think we may be able to use the flambda benchmarking infrastructure to help with this, once the patch has been merged.

Contributor

bobot commented Jan 1, 2016

Early on I have done some on the compiler and alt-ergo. Now that I have rebased, I'm redoing them because you are right to be concerned by performance regression. It would be great if you can use your benchmarking infrastructure.

I found one for testsuite/tests/misc/weaklifetime.ml, but it is a very heavy user of weak pointer. I will look at it.

Contributor

mshinwell commented Jan 1, 2016

For this one my sense is that any performance regressions for typical code (e.g. not using weak pointers) are likely to be easily-fixed. As such, I would recommend merging it, and then we can use the existing benchmarking infrastructure (which enables us to compare between different builds of trunk on packages that we know build with trunk) to see if anything has happened.

Contributor

mshinwell commented Jan 1, 2016

(Although you might still check that inlining thing, that should be easy.)

Contributor

bobot commented Jan 1, 2016

Looking at the assembly generated by gcc, there is no call to mark_slice_darken in mark_slice. After inlining in fact all the calls (2 calls) to mark_slice_darken are in mark_slice, it seems that gcc duplicate part of the code of mark_slice_darken and factorize other parts.

bobot added some commits Nov 29, 2015

[GC] More comments for ephemerons
     and rename some variables
Add precisions on the equality for Weak.Make
    It can't be physical equality since it use `Weak.get_copy`.
Precise documentation for ephemerons
        (Thanks Romait Troit alias William)
Fix creation of Ephemeron.K2
    And use the tests for the classic hashtables for testing
    the (non-weak) behavior of weak hashtables.
Add a test specifically for weak table
    And fix a bug in the handling of ephemeron during minor collection
    found thanks to this test. Oldifying of ephemerons' data was not iterated
    enough because `oldify_todo_list` doesn't become non-null each time
    `caml_oldify_one` is run.
Move caml_ephe_clean to the weak.h
     So that major_gc.c can inline this function for clean phase.
Do cleaning less often. optimize No_naked_pointer
   During clean phase for consistency we need to check if the key
   checked or got is alive or not. Three possibilities:
     - Doesn't unset the key, but still return the right value as if it
       is unset.
     - Unset the key and the data
     - Clean the ephemeron

   Testing aliveness is a little costly, so it is better to amortize
   this cost. Previously the last possibility was implemented but a
   better trade-off seems the second one, since the clean phase is still
   going to clean the ephemeron eventually.
Contributor

bobot commented Jan 25, 2016

Again, rebased on #435, so rebased on #437, so rebased on trunk. @alainfrisch filter_map_inplace is added and advertised in Ephemeron. Even if it is possible to do some modifications of the weak tables during iteration, I think it is better not to.

Contributor

bobot commented Jan 25, 2016

The link to the travis build seems not to be added on this PR anymore (I don't know why) here the link. https://travis-ci.org/bobot/ocaml/jobs/104569150

Contributor

bobot commented Jan 25, 2016

Travis happy! (so debug runtime happy!)

Contributor

mshinwell commented Jan 26, 2016

@bobot no-naked-pointers could probably be a runtime variant, actually...

Contributor

mshinwell commented Jan 26, 2016

@bobot I was about to merge this, but then thought that maybe we should squash all these commits. Do you want to do that?

Contributor

bobot commented Jan 26, 2016

No, at least for the first about ten commits. All these commits pass make tests and I splitted the addition of the feature in the runtime in reasonable part ( factorization then first implementation then optimization). The last commits are less clean but it is just for documentation or further optimization. Thx for merging!

mshinwell added a commit that referenced this pull request Jan 27, 2016

@mshinwell mshinwell merged commit d708e2e into ocaml:trunk Jan 27, 2016

Contributor

alainfrisch commented Jan 27, 2016

This PR breaks build under Windows.

A first fix is:

--- a/byterun/caml/minor_gc.h
+++ b/byterun/caml/minor_gc.h
@@ -77,11 +77,12 @@ static inline void add_to_ref_table (struct caml_ref_table *tbl, value *p)
 static inline void add_to_ephe_ref_table (struct caml_ephe_ref_table *tbl,
                                           value ar, mlsize_t offset)
 {
+  struct caml_ephe_ref_elt *ephe_ref;
   if (tbl->ptr >= tbl->limit){
     CAMLassert (tbl->ptr == tbl->limit);
     caml_realloc_ephe_ref_table (tbl);
   }
-  struct caml_ephe_ref_elt *ephe_ref = tbl->ptr++;
+  ephe_ref = tbl->ptr++;
   ephe_ref->ephe = ar;
   ephe_ref->offset = offset;
 }

but then I get more errors:

minor_gc.c
minor_gc.c(86) : error C2036: 'void *' : unknown size
minor_gc.c(88) : error C2036: 'void *' : unknown size
minor_gc.c(88) : error C2036: 'void *' : unknown size
minor_gc.c(511) : error C2036: 'void *' : unknown size
minor_gc.c(521) : error C2036: 'void *' : unknown size
minor_gc.c(521) : error C2036: 'void *' : unknown size
minor_gc.c(522) : error C2036: 'void *' : unknown size
minor_gc.c(523) : error C2036: 'void *' : unknown size
Makefile.nt:44: recipe for target 'minor_gc.obj' failed

Of course, this is blocking for the release.

Contributor

alainfrisch commented Jan 27, 2016

I propose to change void into char in

struct generic_table CAML_TABLE_STRUCT(void);

Does this sound right?

Contributor

alainfrisch commented Jan 27, 2016

I've committed that, and everything seems to go well.

objmagic pushed a commit to objmagic/ocaml that referenced this pull request Feb 10, 2016

Merge pull request #22 from marklrh/fix_objinfo
Attempt to fix ocamlobjinfo.
Member

AltGr commented Feb 23, 2016

We have a bench on which it seems this merge may have had large effects (judging by the date when the slowdown appeared, I might be wrong)

http://bench.flambda.ocamlpro.com/graph?bench=numal-fft

The code is at https://github.com/AltGr/ocaml-numerical-analysis/blob/bench/fft/bench.ml ; may it be caused by the Gc.set ?

Here is a more detailed view of the comparison between Jan. 26th and Jan. 27th versions:

http://bench.flambda.ocamlpro.com/compare?test=2016-01-27-2300%2Ftrunk%2Bbench&reference=2016-01-26-2300%2Ftrunk%2Bbench

We get +60% time, +245% major words on this bench.

Owner

damiendoligez commented Feb 24, 2016

@AltGr you need to open a Mantis PR for this problem, because GitHub notifications are ephemeral (pun intended).

Contributor

lpw25 commented Apr 11, 2016

Does this patch change the order in which finalisers are run relative to weak pointers being cleared? Core's Weak_hashtbl seems to rely on this ordering.

The tests for this module are failing here:

https://github.com/janestreet/core/blob/master/src/weak_hashtbl.ml#L137

in 4.03, and I suspect this is the issue.

I can change the code to not rely on the ordering of weak pointers being cleared and finalisers being run, but I would like to confirm that this ordering has indeed changed before doing so.

Contributor

alainfrisch commented Apr 11, 2016

I think this has indeed been changed on purpose, to avoid the case where the finalizer would create a new live reference to the value (in which case it should not be removed from the weak table).

(I learned about this change through http://caml.inria.fr/mantis/view.php?id=7210 )

Contributor

bobot commented Apr 12, 2016

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)

Contributor

lpw25 commented Apr 12, 2016

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.

Contributor

bobot commented Apr 12, 2016

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.

Contributor

lpw25 commented Apr 15, 2016

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.

Contributor

lpw25 commented Apr 15, 2016

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.

Owner

damiendoligez commented Apr 18, 2016

@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?

Contributor

bobot commented Apr 18, 2016

@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.

Owner

damiendoligez commented Apr 28, 2016

Doesn't MPR 7210 is enough for that ?

Yes it is, thanks.

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