Optionally disallow defining new methods and drop backedges #198
Conversation
github-actions
bot
added
port-to-v1.10
Drvi
added
port-to-master
| static int erase_method_backedges(jl_typemap_entry_t *def, void *closure) | ||
| { | ||
| jl_method_t *method = def->func.method; | ||
| jl_value_t *specializations = jl_atomic_load_relaxed(&method->specializations); | ||
| if (jl_is_svec(specializations)) { | ||
| size_t i, l = jl_svec_len(specializations); | ||
| for (i = 0; i < l; i++) { | ||
| jl_method_instance_t *mi = (jl_method_instance_t*)jl_svecref(specializations, i); | ||
| if ((jl_value_t*)mi != jl_nothing) { | ||
| mi->backedges = NULL; | ||
| } | ||
| } | ||
| } | ||
| else { | ||
| jl_method_instance_t *mi = (jl_method_instance_t*)specializations; | ||
| mi->backedges = NULL; | ||
| } | ||
| return 1; | ||
| } | ||
|
|
||
| static int erase_all_backedges(jl_methtable_t *mt, void *env) | ||
| { | ||
| // removes all method caches | ||
| // this might not be entirely safe (GC or MT), thus we only do it very early in bootstrapping | ||
| mt->backedges = NULL; | ||
| jl_typemap_visitor(jl_atomic_load_relaxed(&mt->defs), erase_method_backedges, env); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Interesting that we just NULL the backedges field, and don't erase/free anything.
Are these GC-managed objects that we were pointing to?
There was a problem hiding this comment.
Yeah I am also curious to know how are these objects managed and freed
There was a problem hiding this comment.
backedges are jl_vector_ts which are managed by the GC unless they are explicitly constructed to opt-out (owned=0), IIUC. Them being GC tracked is relied on even elsewhere e.g. invalidate_method_instance also does this
There was a problem hiding this comment.
Maybe @d-netto can weigh in?
There was a problem hiding this comment.
Or, do we need jl_gc_wb(mt, mt->backedges); ?
There was a problem hiding this comment.
From documentation,
If the object being stored is a `jl_value_t`, the Julia garbage collector must be notified also:
It is NULL so it looks fine the way it currently is.
There was a problem hiding this comment.
Interesting that we just NULL the backedges field, and don't erase/free anything.
This should be fine. If the reference we erased happened to be the last one pointing to the given object, then the mark phase will detect that this object is unreachable and free it during sweeping.
It is NULL so it looks fine the way it currently is.
Yes, you don't need a write barrier here because you are storing NULL.
| // No special processing of `new_specializations` is required because recaching handled it | ||
| // Add roots to methods | ||
| jl_copy_roots(method_roots_list, jl_worklist_key((jl_array_t*)restored)); | ||
| // Insert method extensions |
There was a problem hiding this comment.
what difference does this move make?
There was a problem hiding this comment.
I don't know, but it was in the diff we were given, so I included it.
| static int erase_method_backedges(jl_typemap_entry_t *def, void *closure) | ||
| { | ||
| jl_method_t *method = def->func.method; | ||
| jl_value_t *specializations = jl_atomic_load_relaxed(&method->specializations); | ||
| if (jl_is_svec(specializations)) { | ||
| size_t i, l = jl_svec_len(specializations); | ||
| for (i = 0; i < l; i++) { | ||
| jl_method_instance_t *mi = (jl_method_instance_t*)jl_svecref(specializations, i); | ||
| if ((jl_value_t*)mi != jl_nothing) { | ||
| mi->backedges = NULL; | ||
| } | ||
| } | ||
| } | ||
| else { | ||
| jl_method_instance_t *mi = (jl_method_instance_t*)specializations; | ||
| mi->backedges = NULL; | ||
| } | ||
| return 1; | ||
| } | ||
|
|
||
| static int erase_all_backedges(jl_methtable_t *mt, void *env) | ||
| { | ||
| // removes all method caches | ||
| // this might not be entirely safe (GC or MT), thus we only do it very early in bootstrapping | ||
| mt->backedges = NULL; | ||
| jl_typemap_visitor(jl_atomic_load_relaxed(&mt->defs), erase_method_backedges, env); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Yeah I am also curious to know how are these objects managed and freed
PR Description
By disabling new method definitions we can assert that the loading of cache packages can skip the expensive back edge reinsertion step.
Checklist
Requirements for merging:
port-to-*labels that don't apply.