JuliaGPU / Adapt.jl

Other
90 stars 24 forks source link

Type piracy: ndims, eltype, parent #30

Closed maleadt closed 3 years ago

maleadt commented 3 years ago

We should make these private methods: https://github.com/JuliaGPU/Adapt.jl/blob/b8c86370b31611ebeb5ab44f6eafa4657ee785dc/src/wrappers.jl#L105-L110

timholy commented 3 years ago
julia> using SnoopCompileCore

julia> invs = @snoopr using Adapt;

julia> using SnoopCompile

julia> trees = invalidation_trees(invs)
1-element Vector{SnoopCompile.MethodInvalidations}:
 inserting eltype(::Type{var"#s15"} where var"#s15"<:(Union{Base.LogicalIndex{T, var"#s5"} where var"#s5"<:Src, Base.ReinterpretArray{T, N, var"#s1", var"#s2", IsReshaped} where IsReshaped where var"#s2"<:Union{SubArray{var"#s3", var"#s4", var"#s13", I, L} where L where I where var"#s4" where var"#s3", var"#s13"} where var"#s1" where var"#s13"<:Src, Base.ReshapedArray{T, N, var"#s4", MI} where MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64}, N} where N} where var"#s4"<:Union{Base.ReinterpretArray{var"#s1", var"#s5", var"#s11", var"#s2", IsReshaped} where IsReshaped where var"#s2"<:Union{SubArray{var"#s3", var"#s4", var"#s14", I, L} where L where I where var"#s4" where var"#s3", var"#s14"} where var"#s11" where var"#s5" where var"#s1", SubArray{var"#s3", var"#s2", var"#s14", I, L} where L where I where var"#s2" where var"#s3", var"#s14"} where var"#s14"<:Src, SubArray{T, N, var"#s5", I, L} where L where I where var"#s5"<:Union{Base.ReinterpretArray{var"#s2", var"#s1", var"#s11", var"#s21", IsReshaped} where IsReshaped where var"#s21"<:Union{SubArray{var"#s3", var"#s4", var"#s15", I, L} where L where I where var"#s4" where var"#s3", var"#s15"} where var"#s11" where var"#s1" where var"#s2", Base.ReshapedArray{var"#s4", var"#s3", var"#s41", MI} where MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64}, N} where N} where var"#s41"<:Union{Base.ReinterpretArray{var"#s1", var"#s5", var"#s11", var"#s2", IsReshaped} where IsReshaped where var"#s2"<:Union{SubArray{var"#s3", var"#s4", var"#s15", I, L} where L where I where var"#s4" where var"#s3", var"#s15"} where var"#s11" where var"#s5" where var"#s1", SubArray{var"#s3", var"#s2", var"#s15", I, L} where L where I where var"#s2" where var"#s3", var"#s15"} where var"#s3" where var"#s4", var"#s15"} where var"#s15"<:Src, LinearAlgebra.Adjoint{T, var"#s1"} where var"#s1"<:Dst, LinearAlgebra.Diagonal{T, var"#s11"} where var"#s11"<:Dst, LinearAlgebra.LowerTriangular{T, var"#s7"} where var"#s7"<:Dst, LinearAlgebra.Transpose{T, var"#s6"} where var"#s6"<:Dst, LinearAlgebra.Tridiagonal{T, var"#s12"} where var"#s12"<:Dst, LinearAlgebra.UnitLowerTriangular{T, var"#s8"} where var"#s8"<:Dst, LinearAlgebra.UnitUpperTriangular{T, var"#s10"} where var"#s10"<:Dst, LinearAlgebra.UpperTriangular{T, var"#s9"} where var"#s9"<:Dst, PermutedDimsArray{T, N, var"#s4", var"#s3", var"#s2"} where var"#s2"<:Src where var"#s3" where var"#s4"} where Dst where Src where N)) where T in Adapt at /home/tim/.julia/packages/Adapt/8kQMV/src/wrappers.jl:108 invalidated:
   backedges: 1: superseding eltype(::Type{var"#s76"} where var"#s76"<:(AbstractArray{E, N} where N)) where E in Base at abstractarray.jl:188 with MethodInstance for eltype(::Type{SubArray{UInt8, 1, Vector{UInt8}, Tuple{UnitRange{Int64}}, true}}) (230 children)
   11 mt_cache

That's pretty bad (230 children). Now it contaminates a bunch of the ecosystem since it's loaded by OffsetArrays.

timholy commented 3 years ago

Cross-post from https://github.com/JuliaArrays/OffsetArrays.jl/issues/174; this has to be resolved or Adapt will get booted out of OffsetArrays.

maleadt commented 3 years ago

this has to be resolved or Adapt will get booted out of OffsetArrays

It's not that simplistic. Adapt is used by a huge number of packages, introducing a breaking release locks the ecosystem until everybody has upgraded. @chrisrackauckas probably still has nightmares from the last time we did so.

I'll have a look, we need to bite this bullet at some point anyway. I had hoped Base would have gained the AbstractWrappedArray type that obviates the Adapt.WrappedArray union, but that doesn't seem to be happening anytime soon.

ChrisRackauckas commented 3 years ago

@ChrisRackauckas probably still has nightmares from the last time we did so.

😢 I just got finished updating Reexport and StaticArrays, and IterativeSolvers, and ... please everyone I beg you 😢

I'll have a look, we need to bite this bullet at some point anyway. I had hoped Base would have gained the AbstractWrappedArray type that obviates the Adapt.WrappedArray union, but that doesn't seem to be happening anytime soon.

Yes, I think that's really the solution. There's some array interface portions from Adapt and ArrayInterface that really need to just be part of Base and in the standard sysimage anyways.

maleadt commented 3 years ago

I think I have a way to do it that wouldn't break the ecosystem: if we just make parent/ndims/eltype private and change downstream users to use Adapt.parent, it would still be compatible with the older Adapt.jl (as you can look up Base methods through a module). So the upgrade could happen gradually this time.

timholy commented 3 years ago

That sounds like a great solution, thanks a ton!