bpart: Start tracking backedges for bindings #57213
Conversation
Keno
force-pushed
the
kf/bindingbackedges
branch
from
00c949f to
c7006ec
Compare
This PR adds limited backedge support for Bindings. There are two classes of bindings that get backedges: 1. Cross-module `GlobalRef` bindings (new in this PR) 2. Any globals accesses through intrinsics (i.e. those with forward edges from #57009) This is a time/space trade-off for invalidation. As a result of the first category, invalidating a binding now only needs to scan all the methods defined in the same module as the binding. At the same time, it is anticipated that most binding references are to bindings in the same module, keeping the list of bindings that need explicit (back)edges small.
Keno
force-pushed
the
kf/bindingbackedges
branch
from
c7006ec to
d85c37a
Compare
| if !is_defined_const_binding(binding_kind(bpart)) || (bpart.max_world < max_world(ir.valid_worlds)) | ||
| if (!is_defined_const_binding(binding_kind(bpart)) || (bpart.max_world < max_world(ir.valid_worlds))) && | ||
| (op.mod !== Core) && (op.mod !== Base) | ||
| # Core and Base are excluded because the frontend uses them for intrinsics, etc. |
There was a problem hiding this comment.
Does this mean the verifier (and also somewhat the IR semantics) are also broken for any topmod other than Base (like Compiler)? I don't think the frontend directly references Base anywhere (although I know there are plenty of references to Core)
There was a problem hiding this comment.
Yes. This was a little bit of a band-aid since we didn't move these globalrefs out into statement position. We need to take a decision on the semantics of these, but I want to punt that to a later PR
|
|
||
| function should_invalidate_code_for_globalref(gr::GlobalRef, src::CodeInfo) | ||
| isgr(g::GlobalRef) = gr.mod == g.mod && gr.name === g.name | ||
| isgr(g) = false |
| if isdefined(b, :backedges) | ||
| for edge in b.backedges |
There was a problem hiding this comment.
This appears to trigger UB when julia runs with threads (the default) and does any inference, since b.backedges is accessed without a lock here
| @@ -910,6 +910,7 @@ static NOINLINE void _finish_julia_init(JL_IMAGE_SEARCH rel, jl_ptls_t ptls, jl_ | |||
| jl_n_threads_per_pool[JL_THREADPOOL_ID_INTERACTIVE] = 0; | |||
| jl_n_threads_per_pool[JL_THREADPOOL_ID_DEFAULT] = 1; | |||
| } else { | |||
| jl_current_task->world_age = jl_atomic_load_acquire(&jl_world_counter); | |||
There was a problem hiding this comment.
This seems a little sketchy, since any valid entry point to the code should already have this, though not strictly wrong, it just wasn't expected to be observable.
| @@ -3271,12 +3271,13 @@ void jl_init_types(void) JL_GC_DISABLED | |||
|
|
|||
| jl_binding_type = | |||
| jl_new_datatype(jl_symbol("Binding"), core, jl_any_type, jl_emptysvec, | |||
| jl_perm_symsvec(4, "globalref", "value", "partitions", "flags"), | |||
| jl_svec(4, jl_any_type/*jl_globalref_type*/, jl_any_type, jl_binding_partition_type, jl_uint8_type), | |||
| jl_perm_symsvec(5, "globalref", "value", "partitions", "backedges", "flags"), | |||
There was a problem hiding this comment.
Any idea of the rough cost of this? I wondered if we should combine them more (e.g. put this backedge list on the containing Module instead, so that any mutation of a Binding in the Module has to scan though all edges to any Bindings used outside that Module). Depends really on how costly this level of tracking detail seems though. (and because then we have the obvious module->write_lock we can use to fix up some of the data race UB here–though we can probably use that lock anyways)
| } else if (jl_array_len(b->backedges) > 0 && | ||
| jl_array_ptr_ref(b->backedges, jl_array_len(b->backedges)-1) == edge) { |
There was a problem hiding this comment.
IIUC, this should be scanning the whole list, not just checking the last one (particularly because of the thread-safety issues this currently incurs)?
This PR adds limited backedge support for Bindings. There are two classes of bindings that get backedges:
GlobalRefbindings (new in this PR)This is a time/space trade-off for invalidation. As a result of the first category, invalidating a binding now only needs to scan all the methods defined in the same module as the binding. At the same time, it is anticipated that most binding references are to bindings in the same module, keeping the list of bindings that need explicit (back)edges small.