Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

subtype: minor clean up for fast path for lhs union and rhs typevar #55645

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

N5N3
Copy link
Member

@N5N3 N5N3 commented Aug 31, 2024

Follow up #55413.
The error pattern mentioned in #55413 (comment) care's ∃y's ub in env rather than its original ub.
So it seems more robust to check the bounds in env directly.
The equivalent typevar propagation is lifted from subtype_var for the same reason.

@N5N3 N5N3 added the types and dispatch Types, subtyping and method dispatch label Aug 31, 2024
@N5N3 N5N3 requested a review from vtjnash August 31, 2024 02:43
This commit makes the check more focused on the related error pattern.
@vtjnash
Copy link
Sponsor Member

vtjnash commented Aug 31, 2024

I was a bit concerned that checking the env here might cause the pick_union call after it to get out of sync with the structure of type walk. Do you know if that could be a real issue though or not?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Aug 31, 2024

Actually, I wonder if a better option here might be to see that just the left has no free type vars, so this check may be legal to simplify to just an eager check of both parts of the union being subtypes of the right, which does not need to involve picking one from the Lstack?

@N5N3
Copy link
Member Author

N5N3 commented Aug 31, 2024

Do you know if that could be a real issue though or not?

We only need to care right typevars here (left ones have frozen bounds).
Since bounds in env get restored between adjacent exists loops, ub should keeps the same and not affect afterward type walk, especially when we have checked the depth of right union.

@vtjnash vtjnash merged commit 53d3ca9 into JuliaLang:master Sep 4, 2024
5 of 7 checks passed
@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 4, 2024

Okay, yes, I am satisfied that sounds right now. I was thinking we could do something like below also, which passes our tests. There seems to be at least two, related, challenges with making this fast path more widely applicable: one is that the repeated subtype calls on each element can cause record_var_occurrence check to get over-incremented (avoided by calling subtype_var directly and checking for bound-in-env vars here), a second is that the environment is not restored between calls, which it appears can trip up the triangular rule in one test (avoided by checking jl_has_free_typevars(x) here). Is this addition correct / possibly worth doing?

julia> A = Tuple{T,Ptr{T}} where T;
julia> C = Tuple{Ptr{T},Ptr{S}} where {T>:Ptr, S>:Ptr};
julia> C <: A
true # returns false without jl_has_free_typevars check
diff --git a/src/subtype.c b/src/subtype.c
index 4118bbeab64..df6a46f9955 100644
--- a/src/subtype.c
+++ b/src/subtype.c
@@ -1314,23 +1314,23 @@ static int subtype(jl_value_t *x, jl_value_t *y, jl_stenv_t *e, int param)
     if (jl_is_uniontype(x)) {
         if (obviously_egal(x, y))
             return 1;
-        if (e->Runions.depth == 0 && jl_is_typevar(y) && !jl_has_free_typevars(x) && !jl_has_free_typevars(((jl_tvar_t*)y)->ub)) {
-            // Similar to fast path for repeated elements: if there have been no outer
-            // unions on the right, and the right side is a typevar, then we can handle the
-            // typevar first before picking a union element, under the theory that it may
-            // be easy to match or reject this whole union in comparing and setting the lb
-            // and ub of the variable binding, without needing to examine each element.
-            // However, if x contains any free typevars, then each element with a free
-            // typevar must be handled separately from the union of all elements without
-            // free typevars, since the typevars presence might lead to those elements
-            // getting eliminated (omit_bad_union) or degenerate (Union{Ptr{T}, Ptr}) or
-            // combined (Union{T, S} where {T, S <: T}).
-            return subtype_var((jl_tvar_t*)y, x, e, 1, param);
+        if (e->Runions.depth == 0 && !jl_has_free_typevars(x) && !jl_has_free_typevars(jl_is_typevar(y) ? ((jl_tvar_t*)y)->ub : y)) {
+            // if there are no free typevars or unions on the right,
+            // then we can handle this union eagerly, without using the decision stack on the left
+            // there must not be any free type vars, as that will break the record_var_occurrence counts
+            if (jl_is_typevar(y))
+                return subtype_var((jl_tvar_t*)y, x, e, 1, param);
+            while (jl_is_uniontype(x)) {
+               if (!subtype(((jl_uniontype_t*)x)->a, y, e, param))
+                   return 0;
+               x = ((jl_uniontype_t*)x)->b;
+            }
+            return subtype(x, y, e, param);
         }
         x = pick_union_element(x, e, 0);
     }
     if (jl_is_uniontype(y)) {
-        if (x == ((jl_uniontype_t*)y)->a || x == ((jl_uniontype_t*)y)->b)
+        if (obviously_egal(x, ((jl_uniontype_t*)y)->a) || obviously_egal(x, ((jl_uniontype_t*)y)->b))
             return 1;
         if (jl_is_unionall(x))
             return subtype_unionall(y, (jl_unionall_t*)x, e, 0, param);

@N5N3 N5N3 deleted the subtypecleanup branch September 5, 2024 10:23
@N5N3
Copy link
Member Author

N5N3 commented Sep 5, 2024

I haven't verified it locally, but it looks like that the widened branch could be replaced with jl_subtype, and we already have this fast path in subtype_tuple routine. If the check failed there, it seems unlikely to pass here, as we split left union first. So perhaps the gain won't be large?

KristofferC pushed a commit that referenced this pull request Sep 12, 2024
…55645)

Follow up #55413.
The error pattern mentioned in
#55413 (comment)
care's `∃y`'s ub in env rather than its original ub.
So it seems more robust to check the bounds in env directly.
The equivalent typevar propagation is lifted from `subtype_var` for the
same reason.
kshyatt pushed a commit that referenced this pull request Sep 12, 2024
…55645)

Follow up #55413.
The error pattern mentioned in
#55413 (comment)
care's `∃y`'s ub in env rather than its original ub.
So it seems more robust to check the bounds in env directly.
The equivalent typevar propagation is lifted from `subtype_var` for the
same reason.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants