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

Preserving static size information in no_offset_view #301

Open
jishnub opened this issue Jun 6, 2022 · 3 comments
Open

Preserving static size information in no_offset_view #301

jishnub opened this issue Jun 6, 2022 · 3 comments

Comments

@jishnub
Copy link
Member

jishnub commented Jun 6, 2022

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}
1nothing%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}
1nothing%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 JuliaLang/julia#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.

jishnub added a commit to jishnub/OffsetArrays.jl that referenced this issue Jun 6, 2022
@johnnychen94
Copy link
Member

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
Copy link
Member Author

jishnub commented Jun 7, 2022

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 added a commit that referenced this issue Jun 7, 2022
* fix IdOffsetRange kwargs constructor

* preserve types if indices are 1-based

* no_offset_view in values

* remove test for SOneTo (#301)

* Add tests

* bump version to v1.12.4
@jishnub
Copy link
Member Author

jishnub commented Jun 30, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants