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

view(::Memory, ::UnitRange) should not create a Vector #54768

Closed
MasonProtter opened this issue Jun 11, 2024 · 6 comments · Fixed by #54927
Closed

view(::Memory, ::UnitRange) should not create a Vector #54768

MasonProtter opened this issue Jun 11, 2024 · 6 comments · Fixed by #54927
Labels
arrays [a, r, r, a, y, s]

Comments

@MasonProtter
Copy link
Contributor

I wasn't around to complain about #53896, but I think that PR was a bad idea. That PR made view worse at doing the regular job that view is used for, and repurposes it for a different job.

  1. This now makes it so that generic functions that call
view(::AbstractVector, ::UnitRange)

might end up doing an unnecessary allocation depending on if you get a Memory (because Vector is a mutable struct). This means that any generic function that takes a view at every iteration of a loop is in danger of accidentally performing N allocations if it is passed a Memory. For example:

function f(v)
    s = 0.0
    for i  firstindex(v):lastindex(v)-1
        s += sum(@view v[i:i+1])
    end
    s
end;

julia> let m = Memory{Float64}(undef, 10000) .= 1
           @btime f($m)
       end
  43.232 μs (9999 allocations: 312.47 KiB)
19998.0

julia> let v = Vector{Float64}(undef, 10000) .= 1
           @btime f($v)
       end
  10.129 μs (0 allocations: 0 bytes)
19998.0
  1. If you actually wanted a regular SubArray{<:Memory}, you cannot use view for that and instead need to call the SubArray constructor directly

  2. The only reason you'd want this to return a Vector instead of a SubArray is if you wanted to push! or otherwise resize the result, but it's generally not good practice to encourage people to write push!(view(m, idxs), x) because that'd normally error.


I propose we remove this method before 1.11 drops.

@Seelengrab Seelengrab added this to the 1.11 milestone Jun 11, 2024
@LilithHafner LilithHafner added the triage This should be discussed on a triage call label Jun 11, 2024
@tecosaur tecosaur added the arrays [a, r, r, a, y, s] label Jun 12, 2024
@KristofferC
Copy link
Sponsor Member

KristofferC commented Jun 18, 2024

So what to do? Revert the wrap -> view PR and the wrap feature PR? Path of least resistance to me and 1.12 can be used to figure out exactly what to do.

@oscardssmith
Copy link
Member

I think for 1.11, a good enough solution would be to revert wrap->view and unexport wrap. We do need an internal way of doing this, but we don't need a public interface.

@KristofferC
Copy link
Sponsor Member

KristofferC commented Jun 18, 2024

We do need an internal way of doing this

We do? Julia seemed to work fine before #52049. I reverted both view and wrap on 1.11.

@KristofferC KristofferC removed this from the 1.11 milestone Jun 18, 2024
@jakobnissen
Copy link
Contributor

Reverting sounds good to me for now. Also much less risky compared to adding a new function this late in the 1.11 release cycle.

@KristofferC
Copy link
Sponsor Member

KristofferC commented Jun 19, 2024

#53192 makes a straight-up revert convoluted so I had to resort to just unexporting wrap in the end.

@JeffBezanson
Copy link
Sponsor Member

Looks like the decision has been made; removing triage tag.

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants