subtype: isolate Union state in subtype_ccheck to prevent exponential hang#61316
subtype: isolate Union state in subtype_ccheck to prevent exponential hang#61316
Union state in subtype_ccheck to prevent exponential hang#61316Conversation
521b886 to
c92a8f4
Compare
c92a8f4 to
d5e6056
Compare
so applying another seems like fair game 😉. my claude makes a convincing argument that the it gives me this diffdiff --git a/src/subtype.c b/src/subtype.c
index d99e4b79cf..06abd293e7 100644
--- a/src/subtype.c
+++ b/src/subtype.c
@@ -739,8 +739,7 @@ NOINLINE static void ccheck_merge_env(
jl_value_t *x, jl_value_t *y, jl_stenv_t *e,
jl_savedenv_t *se,
jl_saved_unionstate_t *oldLunions,
- int16_t oldRunions_used,
- jl_value_t **saved_envout, int envout_n)
+ int16_t oldRunions_used)
{
jl_savedenv_t me;
int nmerge = 0;
@@ -748,9 +747,6 @@ NOINLINE static void ccheck_merge_env(
nmerge = merge_env(e, &me, se, nmerge);
while (next_union_state_from(e, 1, oldRunions_used)) {
restore_env(e, se, 1);
- if (saved_envout)
- memcpy(&e->envout[e->envidx], saved_envout,
- envout_n * sizeof(jl_value_t *));
pop_unionstate(&e->Lunions, oldLunions);
e->Runions.more = 0;
e->Lunions.more = 0;
@@ -761,9 +757,6 @@ NOINLINE static void ccheck_merge_env(
if (nmerge > 0) {
restore_env(e, &me, 1);
ccheck_restore_metadata(e, se);
- if (saved_envout)
- memcpy(&e->envout[e->envidx], saved_envout,
- envout_n * sizeof(jl_value_t *));
free_env(&me);
}
}
@@ -798,36 +791,25 @@ static int subtype_ccheck(jl_value_t *x, jl_value_t *y, jl_stenv_t *e)
return 0;
if (obviously_in_union(y, x))
return 1;
+ // Bounds checks happen after all outer-chain right-side UnionAlls have
+ // been entered, so envidx >= envsz and restore_env cannot touch envout.
+ assert(e->envidx >= e->envsz);
jl_saved_unionstate_t oldLunions; push_unionstate(&oldLunions, &e->Lunions);
jl_saved_unionstate_t oldRunions; push_unionstate(&oldRunions, &e->Runions);
jl_savedenv_t se;
save_env(e, &se, 1);
int sub = local_forall_exists_subtype(x, y, e, 0, 1);
if (e->Runions.used > oldRunions.used) {
- // Right-side Union choices were made. Preserve envout
- // for subtype_unionall before any env restore.
- int envout_n = 0;
- jl_value_t **saved_envout = NULL;
- if (e->envout && e->envidx < e->envsz) {
- envout_n = e->envsz - e->envidx;
- saved_envout = (jl_value_t **)alloca(
- envout_n * sizeof(jl_value_t *));
- memcpy(saved_envout, &e->envout[e->envidx],
- envout_n * sizeof(jl_value_t *));
- }
+ // Right-side Union choices were made during the bounds check.
if (sub && !e->ccheck_merging && !e->intersection) {
// Merge constraints across all successful ∃ branches
// (lb via simple_meet, ub via simple_join).
- ccheck_merge_env(x, y, e, &se, &oldLunions, oldRunions.used,
- saved_envout, envout_n);
+ ccheck_merge_env(x, y, e, &se, &oldLunions, oldRunions.used);
}
else {
// sub == 0 or re-entrance/intersection: restore env
// to clean up stale constraints from the check.
restore_env(e, &se, 1);
- if (saved_envout)
- memcpy(&e->envout[e->envidx], saved_envout,
- envout_n * sizeof(jl_value_t *));
}
}
free_env(&se);this "proof"Proof:
|
`envidx >= envsz` always holds on entry to `subtype_ccheck`, because bounds checks only occur after all outer-chain right-side UnionAlls have been entered. This means `restore_env` cannot touch `envout` and the `saved_envout` tracking was dead code. Replace with an assert documenting the invariant. Suggested-by: adienes (#61316) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
d5e6056 to
3d88c14
Compare
|
@adienes Thank you for the suggestion, and for providing a proof! I applied it and confirmed the tests and the MRE of the original issue are still passing as before. |
Make `next_union_state` a thin wrapper around `next_union_state_from` to eliminate duplicated logic. Remove the `if (nmerge > 0)` guard in `ccheck_merge_env` since `merge_env` always returns > 0. Suggested-by: adienes (#61316) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
to the extent I can review this I have no other comments. not sure if someone with more than my limited experience with the subtyping algorithm also wants to review |
|
IIUC, this PR would break
As for this specific hang pattern, perhaps we can monitor whether |
…tial hang
`subtype_ccheck` calls `local_forall_exists_subtype`, which
leaks right-side Union choices (via `pick_union_decision`)
into the shared `Runions` statestack. When many
Union-bounded type parameters share a common variable
(e.g. 6 parameters all bounded by `Union{Ref{F},Val{F}}`),
each bounds check adds decisions that `exists_subtype` must
iterate over combinatorially, causing O(2^N) iterations.
Make `subtype_ccheck` save and restore both the `Runions`
state and variable environment (`save_env`/`restore_env`).
When `pick_union_decision` added new entries during the
check (detected via `Runions.used` growth) and the check
succeeded, a merge loop (`ccheck_merge_env`) enumerates all
successful right-side ∃ branches and merges their variable
constraints via `simple_meet` (lb) / `simple_join` (ub).
This yields the widest constraints valid across any
successful branch, instead of discarding all constraints.
A separate `ccheck_restore_metadata` step then overrides
the metadata fields (`occurs_inv`, `occurs_cov`,
`max_offset`, `innervars`) with the original values, since
`merge_env`'s metadata merging (max for occurs, min for
max_offset) tightens constraints — the opposite of what
`subtype_ccheck` needs.
The merge loop uses `next_union_state_from` to iterate only
the ccheck's own ∃ decisions (starting from
`oldRunions.used`), avoiding interference with outer Union
iteration. Each branch runs a full ∀∃ check via
`local_forall_exists_subtype`. The `ccheck_merging` flag
prevents re-entrant merge loops from nested `subtype_ccheck`
calls; the `!e->intersection` guard avoids conflicts with
`intersect_all`'s own `merge_env` handling.
When no Union splitting occurred, the variable constraints
are deterministic and safe to keep; restoring them
unconditionally introduces false negatives that break the
`obvious_subtype` invariant. The `envout` array is preserved
across the restore because inner `subtype_unionall` calls
may have already written inferred type-parameter values that
must survive.
Known limitation: detection via `Runions.used` growth is
imperfect — `local_forall_exists_subtype`'s exists-free
path (path 1) internally pushes/pops its own `Runions`,
so Union choices made there do not increase the outer
`Runions.used`. Currently harmless because exists-free
inputs have no exists-variable constraints to corrupt,
but the detection mechanism has a structural gap.
Fixes aviatesk/JETLS.jl#509.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
`envidx >= envsz` always holds on entry to `subtype_ccheck`, because bounds checks only occur after all outer-chain right-side UnionAlls have been entered. This means `restore_env` cannot touch `envout` and the `saved_envout` tracking was dead code. Replace with an assert documenting the invariant. Suggested-by: adienes (#61316) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Make `next_union_state` a thin wrapper around `next_union_state_from` to eliminate duplicated logic. Remove the `if (nmerge > 0)` guard in `ccheck_merge_env` since `merge_env` always returns > 0. Suggested-by: adienes (#61316) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the merge_env-based approach for preventing exponential Union state blowup in subtype_ccheck with a simpler strategy suggested by N5N3: monitor whether variable bounds (lb/ub) change during the bounds check, and reset Runions.more to its previous value when they do not. This avoids leaking purely-internal Union choices into the outer exists_subtype iteration. The merge_env approach was incorrect for subtyping because merge_env was designed for typeintersect only — it widens env via simple_meet/ simple_join, which does not match the existential semantics of subtype. The new approach is both simpler and correct: it removes ccheck_merge_env, ccheck_restore_metadata, next_union_state_from, and the ccheck_merging flag entirely. Suggested-by: N5N3 (#61316) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
a7ab740 to
f296c3b
Compare
|
I'd add the examples as regression tests |
…tial hang (#61316) Investigation of aviatesk/JETLS.jl#509 revealed that the root cause is a hang in the subtyping algorithm. A minimal reproducer has been added to `test/subtype.jl`, which does not terminate in a reasonable time on the current master branch. The fix was developed using Claude and Codex, with iterative cross-review between the two to arrive at what I believe is the most sound approach. However, I am not very familiar with the subtype algorithm implementation, so there may be implementation issues I have not caught. Note that the bulk of this patch was written by AI. --- `subtype_ccheck` calls `local_forall_exists_subtype`, which leaks right-side Union choices (via `pick_union_decision`) into the shared `Runions` statestack. When many Union-bounded type parameters share a common variable (e.g. 6 parameters all bounded by `Union{Ref{F},Val{F}}`), each bounds check adds decisions that `exists_subtype` must iterate over combinatorially, causing O(2^N) iterations. Make `subtype_ccheck` save and restore both the `Runions` state and variable environment (`save_env`/`restore_env`). When `pick_union_decision` added new entries during the check (detected via `Runions.used` growth) and the check succeeded, a merge loop (`ccheck_merge_env`) enumerates all successful right-side ∃ branches and merges their variable constraints via `simple_meet` (lb) / `simple_join` (ub). This yields the widest constraints valid across any successful branch, instead of discarding all constraints. A separate `ccheck_restore_metadata` step then overrides the metadata fields (`occurs_inv`, `occurs_cov`, `max_offset`, `innervars`) with the original values, since `merge_env`'s metadata merging (max for occurs, min for max_offset) tightens constraints — the opposite of what `subtype_ccheck` needs. The merge loop uses `next_union_state_from` to iterate only the ccheck's own ∃ decisions (starting from `oldRunions.used`), avoiding interference with outer Union iteration. Each branch runs a full ∀∃ check via `local_forall_exists_subtype`. The `ccheck_merging` flag prevents re-entrant merge loops from nested `subtype_ccheck` calls; the `!e->intersection` guard avoids conflicts with `intersect_all`'s own `merge_env` handling. When no Union splitting occurred, the variable constraints are deterministic and safe to keep; restoring them unconditionally introduces false negatives that break the `obvious_subtype` invariant. The `envout` array is preserved across the restore because inner `subtype_unionall` calls may have already written inferred type-parameter values that must survive. Known limitation: detection via `Runions.used` growth is imperfect — `local_forall_exists_subtype`'s exists-free path (path 1) internally pushes/pops its own `Runions`, so Union choices made there do not increase the outer `Runions.used`. Currently harmless because exists-free inputs have no exists-variable constraints to corrupt, but the detection mechanism has a structural gap. Fixes aviatesk/JETLS.jl#509. --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…tial hang (#61316) Investigation of aviatesk/JETLS.jl#509 revealed that the root cause is a hang in the subtyping algorithm. A minimal reproducer has been added to `test/subtype.jl`, which does not terminate in a reasonable time on the current master branch. The fix was developed using Claude and Codex, with iterative cross-review between the two to arrive at what I believe is the most sound approach. However, I am not very familiar with the subtype algorithm implementation, so there may be implementation issues I have not caught. Note that the bulk of this patch was written by AI. --- `subtype_ccheck` calls `local_forall_exists_subtype`, which leaks right-side Union choices (via `pick_union_decision`) into the shared `Runions` statestack. When many Union-bounded type parameters share a common variable (e.g. 6 parameters all bounded by `Union{Ref{F},Val{F}}`), each bounds check adds decisions that `exists_subtype` must iterate over combinatorially, causing O(2^N) iterations. Make `subtype_ccheck` save and restore both the `Runions` state and variable environment (`save_env`/`restore_env`). When `pick_union_decision` added new entries during the check (detected via `Runions.used` growth) and the check succeeded, a merge loop (`ccheck_merge_env`) enumerates all successful right-side ∃ branches and merges their variable constraints via `simple_meet` (lb) / `simple_join` (ub). This yields the widest constraints valid across any successful branch, instead of discarding all constraints. A separate `ccheck_restore_metadata` step then overrides the metadata fields (`occurs_inv`, `occurs_cov`, `max_offset`, `innervars`) with the original values, since `merge_env`'s metadata merging (max for occurs, min for max_offset) tightens constraints — the opposite of what `subtype_ccheck` needs. The merge loop uses `next_union_state_from` to iterate only the ccheck's own ∃ decisions (starting from `oldRunions.used`), avoiding interference with outer Union iteration. Each branch runs a full ∀∃ check via `local_forall_exists_subtype`. The `ccheck_merging` flag prevents re-entrant merge loops from nested `subtype_ccheck` calls; the `!e->intersection` guard avoids conflicts with `intersect_all`'s own `merge_env` handling. When no Union splitting occurred, the variable constraints are deterministic and safe to keep; restoring them unconditionally introduces false negatives that break the `obvious_subtype` invariant. The `envout` array is preserved across the restore because inner `subtype_unionall` calls may have already written inferred type-parameter values that must survive. Known limitation: detection via `Runions.used` growth is imperfect — `local_forall_exists_subtype`'s exists-free path (path 1) internally pushes/pops its own `Runions`, so Union choices made there do not increase the outer `Runions.used`. Currently harmless because exists-free inputs have no exists-variable constraints to corrupt, but the detection mechanism has a structural gap. Fixes aviatesk/JETLS.jl#509. --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…l_∀_∃_subtype` Following up on JuliaLang#61316, this commit moves the `env_unchange` check and the reset of `Runion.more` to `local_forall_exists_subtype`, ensuring that similar optimizations are applied more broadly. Additionally, `env_unchange` now also detects diagonality changes in variables, which would fix ```julia Tuple{NTuple{2,Int}, Int8} <: Tuple{<:Union{NTuple{2,T},Tuple{S,T}}, <:T} where {S,T>:Int} ```
…l_∀_∃_subtype` Following up on JuliaLang#61316, this commit moves the `env_unchange` check and the reset of `Runion.more` to `local_forall_exists_subtype`, ensuring that similar optimizations are applied more broadly. Additionally, `env_unchange` now also detects diagonality changes in variables, which would fix ```julia Tuple{NTuple{2,Int}, Int8} <: Tuple{<:Union{NTuple{2,T},Tuple{S,T}}, <:T} where {S,T>:Int} ```
…l_∀_∃_subtype` (#61503) Following up on #61316, this commit moves the `env_unchange` check and the reset of `Runion.more` to `local_forall_exists_subtype`, ensuring that similar optimizations are applied more broadly, and preventing unnecessary `save_env` cost. Additionally, `env_unchanged` now also detects diagonality changes in variables, which would fix ```julia Tuple{NTuple{2,Int}, Int8} <: Tuple{<:Union{NTuple{2,T},Tuple{S,T}}, <:T} where {S,T>:Int} ```
Investigation of aviatesk/JETLS.jl#509 revealed that the root cause is a hang in the subtyping algorithm. A minimal reproducer has been added to
test/subtype.jl, which does not terminate in a reasonable time on the current master branch.The fix was developed using Claude and Codex, with iterative cross-review between the two to arrive at what I believe is the most sound approach. However, I am not very familiar with the subtype algorithm implementation, so there may be implementation issues I have not caught. Note that the bulk of this patch was written by AI.
subtype_ccheckcallslocal_forall_exists_subtype, which leaks right-side Union choices (viapick_union_decision) into the sharedRunionsstatestack. When manyUnion-bounded type parameters share a common variable (e.g. 6 parameters all bounded by
Union{Ref{F},Val{F}}), each bounds check adds decisions thatexists_subtypemust iterate over combinatorially, causing O(2^N) iterations.Make
subtype_cchecksave and restore both theRunionsstate and variable environment (save_env/restore_env).When
pick_union_decisionadded new entries during the check (detected viaRunions.usedgrowth) and the check succeeded, a merge loop (ccheck_merge_env) enumerates all successful right-side ∃ branches and merges their variable constraints viasimple_meet(lb) /simple_join(ub). This yields the widest constraints valid across any successful branch, instead of discarding all constraints. A separateccheck_restore_metadatastep then overrides the metadata fields (occurs_inv,occurs_cov,max_offset,innervars) with the original values, sincemerge_env's metadata merging (max for occurs, min for max_offset) tightens constraints — the opposite of whatsubtype_ccheckneeds.The merge loop uses
next_union_state_fromto iterate only the ccheck's own ∃ decisions (starting fromoldRunions.used), avoiding interference with outer Union iteration. Each branch runs a full ∀∃ check vialocal_forall_exists_subtype. Theccheck_mergingflag prevents re-entrant merge loops from nestedsubtype_ccheckcalls; the!e->intersectionguard avoids conflicts withintersect_all's ownmerge_envhandling.When no Union splitting occurred, the variable constraints are deterministic and safe to keep; restoring them unconditionally introduces false negatives that break the
obvious_subtypeinvariant. Theenvoutarray is preserved across the restore because innersubtype_unionallcalls may have already written inferred type-parameter values that must survive.Known limitation: detection via
Runions.usedgrowth is imperfect —local_forall_exists_subtype's exists-free path (path 1) internally pushes/pops its ownRunions, so Union choices made there do not increase the outerRunions.used. Currently harmless because exists-free inputs have no exists-variable constraints to corrupt, but the detection mechanism has a structural gap.Fixes aviatesk/JETLS.jl#509.