JuliaArrays / OffsetArrays.jl

Fortran-like arrays with arbitrary, zero or negative starting indices.
Other
198 stars 40 forks source link

Preserving static size information in `no_offset_view` #301

Open jishnub opened 2 years ago

jishnub commented 2 years ago

Currently, no_offst_view has special methods defined for Bsae.OneTo, but nothing for SOneTo axes that a StaticArray has. As a consequence, OffsetArrays doesn't realize that a SOneTo is 1-based. This leads to

julia> OffsetArrays.no_offset_view(SOneTo(4)) |> typeof
UnitRange{Int64}

that is, it loses the static size information. As a consequence,

julia> A = Float64.(reshape(1:16, 4, 4));

julia> B = @view A[SOneTo(4)];

julia> @code_warntype size(B) # size is constant-propagated
MethodInstance for size(::SubArray{Float64, 1, Vector{Float64}, Tuple{SOneTo{4}}, true})
  from size(V::SubArray) in Base at subarray.jl:63
Arguments
  #self#::Core.Const(size)
  V::SubArray{Float64, 1, Vector{Float64}, Tuple{SOneTo{4}}, true}
Body::Tuple{Int64}
1 ─      nothing
│   %2 = Base.axes(V)::Core.Const((SOneTo(4),))
│   %3 = Base.map(Base.length, %2)::Core.Const((4,))
└──      return %3

julia> @code_warntype size(OffsetArrays.no_offset_view(B)) # static size information is lost
MethodInstance for size(::SubArray{Float64, 1, Vector{Float64}, Tuple{UnitRange{Int64}}, true})
  from size(V::SubArray) in Base at subarray.jl:63
Arguments
  #self#::Core.Const(size)
  V::SubArray{Float64, 1, Vector{Float64}, Tuple{UnitRange{Int64}}, true}
Body::Tuple{Int64}
1 ─      nothing
│   %2 = Base.axes(V)::Tuple{Base.OneTo{Int64}}
│   %3 = Base.map(Base.length, %2)::Tuple{Int64}
└──      return %3

julia> B = @view A[SOneTo(4)];

julia> C = OffsetArrays.no_offset_view(B);

julia> @btime sum($B);
  3.356 ns (0 allocations: 0 bytes)

julia> @btime sum($C);
  5.870 ns (0 allocations: 0 bytes)

One way to resolve this is to simply define _no_offset_view for Union{Base.OneTo, StaticArrays.SOneTo} over here, but this will require us to explicitly depend on StaticArrays. The second way might be something like https://github.com/JuliaLang/julia/issues/41946, where Base defines OneTo <: AbstractOneTo, and we define methods for AbstractOneTo in this package. However, it doesn't appear that there's a clear direction to this in Base at the moment. I wonder if there's another way to preserve the size, without one package explicitly depending on the other.

johnnychen94 commented 2 years ago

I know people dislike Requires for many reasons but that seems unavoidable. -- since StaticArrays is already heavy enough, maybe it doesn't matter much to add an extra Requires dependency to StaticArrays?

But this seems to be a marginal improvement to me -- I can't imagine how would people utilize this in practice. Perhaps we can just leave this issue here until someone really finds it a performance bottleneck? I'm not sure.

jishnub commented 2 years ago

Yes, this isn't urgent, as it's only a performance improvement. Maybe a IsOneTo trait in Base will be useful, so that custom ranges might opt into that.

jishnub commented 1 year ago

Perhaps this is worth considering in v1.9+ as a package extension