Allow values reachable from ephemeron keys to be collected by minor GC #3402
Conversation
(cherry picked from commit fa197971cbda8290bca250ab6dda3628eaed73ae)
(cherry picked from commit f078a6a222678e29b592e9b1ff031b931559aa3b)
| @@ -74,6 +72,17 @@ struct caml_ephe_info { | |||
| #define Ephe_link(e) (*(Op_val(e) + CAML_EPHE_LINK_OFFSET)) | |||
| #define Ephe_data(e) (*(Op_val(e) + CAML_EPHE_DATA_OFFSET)) | |||
|
|
|||
| value caml_ephe_await_key(value ephe, uintnat i); | |||
|
|
|||
| Caml_inline value Ephe_key(value ephe, uintnat i) | |||
There was a problem hiding this comment.
I think perhaps this should be ephe_key, with a lower-case e.
There was a problem hiding this comment.
(as a hint that it does something, including possibly spin-waiting).
| @@ -413,12 +412,16 @@ static void oldify_one (void* st_v, value v, volatile value *p) | |||
| } | |||
| } | |||
|
|
|||
| typedef struct { | |||
There was a problem hiding this comment.
I'd like us to adopt a convention for naming struct types, and pointers to them, across the runtime. I'm doing a bit of this whenever I add or make a big modification to one:
typedef struct foo_s {
...
} foo_s, *foo_t;
There was a problem hiding this comment.
(I wholly approve of adding a struct here, BTW).
There was a problem hiding this comment.
But I think perhaps we should have two, a mopup_result and a promote_result, and transfer the locked_ephemerons field from one to the other. It's a bit weird for oldify_mopup to return a promote_result.
| if( do_ephemerons ) { | ||
| /* Limits of *this* minor heap, not other domains' */ |
There was a problem hiding this comment.
I think this code should probably be in weak.c, as it exposes and depends upon the inner workings of ephemerons.
There was a problem hiding this comment.
(that could be done as part of a general refactor, separate from this PR).
| v = caml_ephe_none; | ||
| Ephe_data(re->ephe) = caml_ephe_none; | ||
| } | ||
| atomic_store_release(Op_atomic_val(re->ephe) + re->offset, v); |
There was a problem hiding this comment.
Why is this an atomic_store_release while the one at line 815 is just an assignment?
There was a problem hiding this comment.
Should we have macros to atomically set ephemeron fields?
| @@ -114,6 +115,7 @@ Caml_inline void add_to_ephe_ref_table (struct caml_ephe_ref_table *tbl, | |||
| ephe_ref = tbl->ptr++; | |||
| ephe_ref->ephe = ar; | |||
| ephe_ref->offset = offset; | |||
| ephe_ref->locked = Val_unit; | |||
There was a problem hiding this comment.
I think there's something to be said for the "resting" value of this field to be caml_ephe_locked.
| @@ -52,6 +52,7 @@ struct caml_ref_table CAML_TABLE_STRUCT(value *); | |||
| struct caml_ephe_ref_elt { | |||
| value ephe; /* an ephemeron in major heap */ | |||
| mlsize_t offset; /* the offset that points in the minor heap */ | |||
| value locked; /* only used during minor GC; see minor_gc.c */ | |||
There was a problem hiding this comment.
Unhappy with the name of this field. It's the place where a locked field of an ephemeron is temporarily stored. stash perhaps?
|
|
||
| In theory the data need only be promoted if the ephemeron and all keys are | ||
| live, but determining this requires a multi-round synchronisation (consider | ||
| the case where the keys are live, but from different domains). So, we do it |
There was a problem hiding this comment.
s/we do it unconditionally here, and leave the hard cases/we do the hard cases unconditionally here, and leave them/
| @@ -663,7 +693,7 @@ void caml_empty_minor_heap_promote(caml_domain_state* domain, | |||
| CAML_EV_END(EV_MINOR_MEMPROF_ROOTS); | |||
|
|
|||
| CAML_EV_BEGIN(EV_MINOR_REMEMBERED_SET_PROMOTE); | |||
| oldify_mopup (&st, 1); /* ephemerons promoted here */ | |||
| promote_result result = oldify_mopup (&st, 1); /* ephemerons promoted here */ | |||
There was a problem hiding this comment.
On line 726 we also call oldify_mopup, with do_ephemerons set to 0, and silently ignore the result. I recommend explicitly disregarding the result (by cast to void) and adding a comment about why.
|
|
||
| static void ephe_clean_minor (caml_domain_state* domain) | ||
| { | ||
| struct caml_ephe_ref_table table = |
There was a problem hiding this comment.
why is this line broken?
|
I've made NickBarnes/nbarnes-minor-ephemerons with most of the minor style things. |
Port ocaml/ocaml#13643
(cherry picked from commit fa197971cbda8290bca250ab6dda3628eaed73ae)
cc @stedolan