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

Small number of invalidations when defining new AbstractUnitRange{Int} type #47143

Open
Tokazama opened this issue Oct 12, 2022 · 4 comments
Open
Labels
compiler:latency Compiler latency ranges Everything AbstractRange

Comments

@Tokazama
Copy link
Contributor

I consistently (across multiple Julia versions) have found a couple of invalidations whenever I define a new AbstractUnitRange{Int} type. It usually starts with a method like last(::OrdinalRange) but usually I can trace the issue back to last(::Slice). It's gotten a lot better in more recent releases (on v8.2 it only causes 2 invalidations), but I figured enough code interacts with Slice it was worth being proactive about it becoming a bigger issue later on.

I think it could be fixed if we changed its definition from Slice{T<:AbstractUnitRange} <: AbstractUnitRange{Int} to Slice{T<:AbstractUnitRange{Int} <: AbstractUnitRange{Int}. Is there any reason we don't enforce the indices have this subtype?

@jishnub
Copy link
Contributor

jishnub commented Oct 14, 2022

Wouldn't this impact something like this:

julia> view(big(1):2, :, 1)[:]
2-element Vector{BigInt}:
 1
 2

which involves constructing a Base.Slice{Base.OneTo{BigInt}}} at an intermediate step?

@Tokazama
Copy link
Contributor Author

The type of the indices shouldn't change the resulting vector's elements

julia> view(big(1):2, 1:2, 1)[1:2]
2-element Vector{BigInt}:
 1
 2

Maybe it's an attempt to support indices bigger than typemax(Int)? If so, I'm not sure it's actually accomplishing that. Base.Slice still subtypes AbstractUnitRange{Int} so the element types should (and are Int) anyway

julia> s = Base.Slice(big(1):2)
Base.Slice(1:2)

julia> typeof(s[1])
Int64

julia> typeof(s[big(1)])
Int64

@jishnub
Copy link
Contributor

jishnub commented Oct 14, 2022

It's not fully consistent currently, but I agree that indexing should be equivalent to indexing into an AbstractVector{Int}.

julia> typeof(s[big(1)])
Int64

julia> typeof(s[big(1):big(1)])
UnitRange{BigInt}

Perhaps I didn't provide the best example, it's not the indexing that's the issue, rather constructing the view:

julia> @code_warntype view(big(1):2, :, 1)
MethodInstance for view(::UnitRange{BigInt}, ::Colon, ::Int64)
  from view(A::AbstractArray, I::Vararg{Any, N}) where N in Base at subarray.jl:174
Static Parameters
  N = 2
Arguments
  #self#::Core.Const(view)
  A::UnitRange{BigInt}
  I::Tuple{Colon, Int64}
Locals
  #185::Base.var"#185#186"{UnitRange{BigInt}}
  J::Tuple{Base.Slice{Base.OneTo{BigInt}}, Int64}
Body::SubArray{BigInt, 1, Base.ReshapedArray{BigInt, 2, UnitRange{BigInt}, Tuple{}}, Tuple{Base.Slice{Base.OneTo{BigInt}}, Int64}, true}
1nothing%2  = Base.:(var"#185#186")::Core.Const(Base.var"#185#186")
│   %3  = Core.typeof(A)::Core.Const(UnitRange{BigInt})
│   %4  = Core.apply_type(%2, %3)::Core.Const(Base.var"#185#186"{UnitRange{BigInt}})
│         (#185 = %new(%4, A))%6  = #185::Base.var"#185#186"{UnitRange{BigInt}}%7  = Base.to_indices(A, I)::Tuple{Base.Slice{Base.OneTo{BigInt}}, Int64}
│         (J = Base.map(%6, %7))
└──       goto #3 if not $(Expr(:boundscheck))
2%10 = Core.tuple(A)::Tuple{UnitRange{BigInt}}
└──       Core._apply_iterate(Base.iterate, Base.checkbounds, %10, J)
3%12 = Core._apply_iterate(Base.iterate, Base.index_ndims, J)::Core.Const((true, true))
│   %13 = Base._maybe_reshape_parent(A, %12)::Base.ReshapedArray{BigInt, 2, UnitRange{BigInt}, Tuple{}}%14 = Core.tuple(%13)::Tuple{Base.ReshapedArray{BigInt, 2, UnitRange{BigInt}, Tuple{}}}%15 = Core._apply_iterate(Base.iterate, Base.unsafe_view, %14, J)::Core.PartialStruct(SubArray{BigInt, 1, Base.ReshapedArray{BigInt, 2, UnitRange{BigInt}, Tuple{}}, Tuple{Base.Slice{Base.OneTo{BigInt}}, Int64}, true}, Any[Base.ReshapedArray{BigInt, 2, UnitRange{BigInt}, Tuple{}}, Tuple{Base.Slice{Base.OneTo{BigInt}}, Int64}, Int64, Core.Const(1)])
└──       return %15

lists Base.to_indices(A, I)::Tuple{Base.Slice{Base.OneTo{BigInt}}, Int64}. However, on further testing, I'm uncertain if this is necessary, as the following doesn't work:

julia> view(1:big(2)^64, :, 1)
ERROR: InexactError: Int64(18446744073709551616)
Stacktrace:
 [1] Type
   @ ./gmp.jl:365 [inlined]
 [2] to_shape
   @ ./abstractarray.jl:814 [inlined]
 [3] map
   @ ./tuple.jl:222 [inlined]
 [4] to_shape
   @ ./abstractarray.jl:810 [inlined]
 [5] reshape
   @ ./reshapedarray.jl:111 [inlined]
 [6] reshape
   @ ./reshapedarray.jl:142 [inlined]
 [7] _maybe_reshape_parent
   @ ./subarray.jl:127 [inlined]
 [8] view(::UnitRange{BigInt}, ::Function, ::Int64)
   @ Base ./subarray.jl:178
 [9] top-level scope
   @ REPL[21]:1

So perhaps this is broken currently after all. In this example, the issue seems to be the same as #40076, which is labelled as a bug, so it might be fixed in the future.

@Tokazama
Copy link
Contributor Author

It seems like we either need to have Slice use the same subtype as it's parent range or just force use of Int

@brenhinkeller brenhinkeller added the compiler:latency Compiler latency label Nov 22, 2022
@nsajko nsajko added the ranges Everything AbstractRange label Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:latency Compiler latency ranges Everything AbstractRange
Projects
None yet
Development

No branches or pull requests

4 participants