improve construction of `<:Tuple` types from iterators #53265

nsajko · 2024-02-09T17:45:16Z

Make tuple construction from overlong iterators throw instead of truncating the input iterator.

Trying to construct a tuple from an infinite iterator now throws an ArgumentError instead of succeeding or throwing a MethodError.

Fixes #40495

Fixes #52657

Make tuple construction from known constant-length iterators inferrable and prevent allocation.

Fixes #52993

Compute more accurate tuple element types in many cases using typeintersect, with a fallback to the old behavior. This prevents some cases of convert throwing due to ambiguity.

Instead of just calling typeintersect on the tuple types, do typeintersect again once fieldtype computes the element type.

Fixes #53181

Separate the tuple construction from tuple element conversion, allowing dropping the tuple_type_tail dependency.

Fixes #53182

Make tuple construction from overlong iterators throw instead of truncating the input iterator. Trying to construct a tuple from an infinite iterator now throws an `ArgumentError` instead of succeeding or throwing a `MethodError`. Fixes JuliaLang#40495 Fixes JuliaLang#52657 Make tuple construction from known constant-length iterators inferrable and prevent allocation. Fixes JuliaLang#52993 Compute more accurate tuple element types in many cases using `typeintersect`, with a fallback to the old behavior. This prevents some cases of `convert` throwing due to ambiguity. Instead of just calling `typeintersect` on the tuple types, do `typeintersect` again once `fieldtype` computes the element type. Fixes JuliaLang#53181 Separate the tuple construction from tuple element conversion, allowing dropping the `tuple_type_tail` dependency. Fixes JuliaLang#53182

oscardssmith · 2024-02-09T21:43:37Z

base/Base.jl

@@ -196,6 +197,7 @@ end

 # core operations & types
 include("promotion.jl")
+include("tuple_from_iterator.jl")


Does this really need to exist this early? The code would be a lot cleaner if it could be moved to after int (so you can use +)

oscardssmith · 2024-02-09T21:44:46Z

base/tuple_from_iterator.jl

+
+function _totuple_err(@nospecialize T)
+    @noinline
+    throw(ArgumentError("too few or too many elements for tuple type $T"))


should this use a LazyString?

oscardssmith · 2024-02-09T21:45:52Z

base/tuple_from_iterator.jl

+end
+
+function iterator_to_ntuple(::Type{T}, iter) where {len,T<:NTuple{len,Any}}
+    if len < 100


100 seems pretty high for this. Does the recursive version really infer that well?

oscardssmith · 2024-02-09T21:49:43Z

base/type_intersect_exact.jl

+        AB = typeintersect(A, B)
+        BA = typeintersect(B, A)
+        AC = typeintersect(A, C)
+        CA = typeintersect(C, A)
+        BC = typeintersect(B, C)
+        CB = typeintersect(C, B)
+
+        AB_C = typeintersect(AB, C)
+        C_AB = typeintersect(C, AB)
+        BA_C = typeintersect(BA, C)
+        C_BA = typeintersect(C, BA)
+        AC_B = typeintersect(AC, B)
+        B_AC = typeintersect(B, AC)
+        CA_B = typeintersect(CA, B)
+        B_CA = typeintersect(B, CA)
+        BC_A = typeintersect(BC, A)
+        A_BC = typeintersect(A, BC)
+        CB_A = typeintersect(CB, A)
+        A_CB = typeintersect(A, CB)


Can't you simplify this to something like

type_intersect_exact(C, type_intersect_exact(A,B)) type_intersect_exact(B, type_intersect_exact(A,C)) type_intersect_exact(A, type_intersect_exact(B,C))

oscardssmith · 2024-02-09T21:50:52Z

base/type_intersect_exact.jl

+        A_CB = typeintersect(A, CB)
+
+        (
+            A, B, C,


I'm pretty sure A, B, C do not need to be considered because if one of them was valid, it would be successfully found as the intersection.

I think you're saying that typeintersect(A, B) can be relied on to return the "smaller" type of the two when they're ordered? Maybe you're right, but that's not documented, and I didn't read the C implementation, so I don't know if we can rely on that. If we can rely on that behavior of typeintersect, perhaps we should document it.

I think you're saying that typeintersect(A, B) can be relied on to return the "smaller" type of the two when they're ordered?

yes. If it didn't, it would be doing a really bad job of returning an intersection.

oscardssmith · 2024-02-09T21:51:01Z

base/type_intersect_exact.jl

+
+        (
+            A, B, C,
+            AB, BA, AC, CA, BC, CB,


oscardssmith · 2024-02-09T21:52:01Z

base/type_intersect_exact.jl

+When `OK <: ok`, `R` is the exact type intersection. When `ok <: Union{}`, the
+exact type intersection wasn't found and `R` has no significance.
+"""
+struct Result{R, ok<:OK} end


The fact that this is implementing an Option type seems pretty sketchy to me.

How is it sketchy? It just seems like an elegant encoding to me?

The idea is to let the caller know if the exact intersection couldn't be found.

That is, the idea is to enable the implementation of get_result.

that's relatively sensible, the weird parts are that

the OK value is a type? Why not a bool?

Implimenting a custom option type specifically for this functionality seems somewhat weird. Why is an option type inherently embedded in the implementation of a typeintersect wrapper?

it may be elegant, but something like that should be a separate PR, and not live under TypeIntersectExact namespace.

Now that I think about this again, OK isn't necessary at all, and thus Result is also unnecessary: I think it would be better to just return a Type when the search was successful and nothing when the search failed.

@adienes what do you mean by "not live under TypeIntersectExact namespace"? Do you object to the name, or to the addition of a namespace?

Do you have a suggestion for a better name?

vtjnash · 2024-02-09T22:56:21Z

base/type_intersect_exact.jl

+        is_exact = (T <: A) && (T <: B) && (T <: C)
+        is_exact && (return result(T)::Result)
+    end
+    failure()::Result


This would be a breaking change. We cannot merge the type_intersect_exact.jl file into Base in any form, as it promises something that it cannot provide

I'm not at all sure what you mean? It's just an internal helper function, given that it's not a change it can't be a breaking change??

Do you just don't like the name? The documentation is clear about the search for the exact intersection not always succeeding, but even so it's safe given that the result says whether the intersection was found or not.

vtjnash

Just at a high level review comment, it sounds like this is trying to change too many things at once, several of which are likely breaking changes that therefore must be handled carefully and likely will need to be in separate PRs

As per the discussion in JuliaLang#53265.

As per the discussion in #53265.

nsajko · 2024-02-11T19:04:20Z

I realized that, while this is an improvement for constant-length iterators, it's a (runtime) performance regression for non-constant-length iterators when the tuple type is known-length, so I need to rethink this a bit, apart from the pointed out issues.

nsajko added types and dispatch Types, subtyping and method dispatch bugfix This change fixes an existing bug collections Data structures holding multiple items, e.g. sets performance Must go faster labels Feb 9, 2024

nsajko mentioned this pull request Feb 9, 2024

improve construction of <:Tuple types from constant-length iterators #53152

Closed

nsajko force-pushed the tuple_from_iterator branch 4 times, most recently from aaf9ebd to d4127a7 Compare February 9, 2024 20:40

nsajko force-pushed the tuple_from_iterator branch from d4127a7 to 039fc1b Compare February 9, 2024 21:12

oscardssmith reviewed Feb 9, 2024

View reviewed changes

base/type_intersect_exact.jl

(

A, B, C,

AB, BA, AC, CA, BC, CB,

Copy link

Member

oscardssmith Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

nsajko reacted with thumbs up emoji

oscardssmith reviewed Feb 9, 2024

View reviewed changes

vtjnash reviewed Feb 9, 2024

View reviewed changes

nsajko added a commit to nsajko/julia that referenced this pull request Feb 11, 2024

document exactness for typeintersect(T, S) when T <: S

f35cbc8

As per the discussion in JuliaLang#53265.

nsajko mentioned this pull request Feb 11, 2024

document exactness for typeintersect(T, S) when T <: S #53283

Merged

fingolfin pushed a commit that referenced this pull request Feb 11, 2024

document exactness for typeintersect(T, S) when T <: S (#53283)

790a4eb

As per the discussion in #53265.

nsajko marked this pull request as draft February 11, 2024 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve construction of `<:Tuple` types from iterators #53265

improve construction of `<:Tuple` types from iterators #53265

nsajko commented Feb 9, 2024 •

edited

Loading

oscardssmith Feb 9, 2024

oscardssmith Feb 9, 2024

oscardssmith Feb 9, 2024

oscardssmith Feb 9, 2024

oscardssmith Feb 9, 2024

nsajko Feb 9, 2024

oscardssmith Feb 9, 2024

oscardssmith Feb 9, 2024

oscardssmith Feb 9, 2024

nsajko Feb 9, 2024

nsajko Feb 9, 2024

nsajko Feb 9, 2024

oscardssmith Feb 9, 2024

adienes Feb 9, 2024

nsajko Feb 10, 2024

nsajko Feb 10, 2024

nsajko Feb 10, 2024

vtjnash Feb 9, 2024

nsajko Feb 10, 2024

vtjnash left a comment

nsajko commented Feb 11, 2024

improve construction of <:Tuple types from iterators #53265

Are you sure you want to change the base?

improve construction of <:Tuple types from iterators #53265

Conversation

nsajko commented Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash left a comment

Choose a reason for hiding this comment

nsajko commented Feb 11, 2024

improve construction of `<:Tuple` types from iterators #53265

improve construction of `<:Tuple` types from iterators #53265

nsajko commented Feb 9, 2024 •

edited

Loading