Add basic code for binding partition revalidation #56649
Conversation
| ct->world_age = jl_typeinf_world; | ||
| jl_invalidate_binding_refs(gr, new_max_world); | ||
| } JL_CATCH { | ||
| JL_UNLOCK(&world_counter_lock); |
There was a problem hiding this comment.
You are forbidden from unlocking a lock that you didn't acquire in the same scope
| JL_UNLOCK(&world_counter_lock); |
There was a problem hiding this comment.
so the try/catch will automatically unlock it?
base/invalidation.jl
Outdated
| if src.code !== old_stmts | ||
| method.debuginfo = src.debuginfo | ||
| method.source = src | ||
| method.source = ccall(:jl_compress_ir, Ref{String}, (Any, Ptr{Cvoid}), method, C_NULL) | ||
| end |
There was a problem hiding this comment.
I don't love that we are adding a lot of data races here, and still not even getting the result correct, since lowering happens a long time before taking world_counter_lock (commonly in an entirely different process), so anything that got loaded from a package image afterwards (which is most things) will not be able to be handled correctly. Could we fix lowering/inference not to need these attempted and unreliable hacks?
There was a problem hiding this comment.
We did a fair bit of work in the opposite direction, but we could reverse course and allow GlobalRef effects in argument position again at least for inference (the ir conversion could still move them out as before). If we did that, we would not have to update the source here.
base/invalidation.jl
Outdated
| end | ||
| end | ||
|
|
||
| function invalidate_code_for_globalref!(gr::GlobalRef, src::CodeInfo) |
There was a problem hiding this comment.
For completeness, I think we will basically need to teach inference to create all of these as edges every time it encounters a GlobalRef, so that we can scan each MethodInstance separately for all of its GR uses, and then we can see later if it is worth the optimization of filtering out any edges that it knows it could re-discover by scanning the source code in each of the Methods?
There was a problem hiding this comment.
Yes, the idea is to add edges for all GlobalRefs that are not manifest in the source. There aren't that many of them, mostly only those created by getglobal.
| bpart = lookup_binding_partition(world, binding) | ||
| if is_some_const_binding(binding_kind(bpart)) | ||
| isdefined(bpart, :restriction) || continue | ||
| v = partition_restriction(bpart) |
There was a problem hiding this comment.
According to CI, apparently this function is unsound currently? (may return some invalid reference, possibly a NULL)
There was a problem hiding this comment.
Yeah, there's a bit of an awkward state where you have a something declared constant but the value is undefined. Originally that was what the isdefined check on the line before this was for, but on current master, it doesn't work, because restriction is the magic pointer-value union. I'm planning to fix it up by making that case its own binding_kind that's considered a guard kind, not a constant kind, so that constant kinds always have values.
This partially reverts #51970, once again allowing throwing GlobalRefs in value position for `CodeInfo`, (but not `IRCode`). The primary motivation for this reversal is that after binding partition the throwiness of a global is world-age dependent and we do not want to have lowering be world-age dependent, because that would require us to rewrite the lowered code upon invalidation (#56649). With respect to the original motivation for this change (being able to use the statement flag in inference), we retain the property that the statement flags apply only to the optimizer version of the statement (i.e. with the GlobalRefs moved out). The only place in inference where we were not doing this, we can rely on `exct` instead. This does assume that `exct` is precise, but we've done a lot of work on that in the past year, so hopefully we can rely on that now. The revert is partial, because we go in the opposite direction in a few places: For `GotoIfNot` cond and `EnterNode` scope operands, we just forbid any `GlobalRef` entirely. Constants are rare in these, so no need to burden inference with the possibility of handling these. We do however, keep (and add appropriate handling code) for GlobalRef arguments to `ReturnNode` to make sure that trivial code remains one statement.
This adds the binding partition revalidation code from #54654. This is the last piece of that PR that hasn't been merged yet - however the TODO in that PR still stands for future work. This PR itself adds a callback that gets triggered by deleting a binding. It will then walk all code in the system and invalidate code instances of Methods whose lowered source referenced the given global. This walk is quite slow. Future work will add backedges and optimizations to make this faster, but the basic functionality should be in place with this PR.
Keno
force-pushed
the
kf/bpartrevalidation
branch
from
726e037 to
ae19646
Compare
This adds the binding partition revalidation code from #54654. This is the last piece of that PR that hasn't been merged yet - however the TODO in that PR still stands for future work.
This PR itself adds a callback that gets triggered by deleting a binding. It will then walk all code in the system and invalidate code instances of Methods whose lowered source referenced the given global. This walk is quite slow. Future work will add backedges and optimizations to make this faster, but the basic functionality should be in place with this PR.