Skip to content

Simplify fill! bootstrapping and remove @inbounds for AbstractArray #59181

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LilithHafner
Copy link
Member

Notably, the compiler will not be able to elide bounds checking for all arrays so this could be a performance hit in some cases (otoh better effects could be a performance boon).

I think that

for i in eachindex(a::AbstractVector{T})
    @inbounds a[i] = x::T
end

is one of the safest possible uses of @inbounds on an abstract array and any resulting UB is the fault of the array type so I'm not actually enthusiastic about this change. I made the change based on #59174 (comment) and #59174 (comment) but am now having second thoughts.

cc. @giordano @oscardssmith

@LilithHafner LilithHafner added the arrays [a, r, r, a, y, s] label Aug 1, 2025
@oscardssmith
Copy link
Member

imo for arbitrary abstractarray the compiler should be able to figure out the inbounds itself (but I acknowledge that in practice that often won't happen)

@mbauman
Copy link
Member

mbauman commented Aug 1, 2025

I'm also hesitant to start ripping out abstract array @inbounds (even though I really want to do this!). I just didn't think the compiler was smart enough about this yet... but in looking just at some simple views and reshapes, I've not yet found cases O(n) boundschecking. I'm sure they're there, but they're much farther away than I expected.

julia> function myfill!(A::AbstractArray{T}, x) where T
           xT = convert(T, x)
           for I in eachindex(A)
               A[I] = xT
           end
           A
       end
myfill! (generic function with 1 method)

julia> @btime fill!(reshape(view($(Array{Int}(undef, 10000,2)), 1:2:10000,1:2), 2500,4), 1);
  5.465 μs (0 allocations: 0 bytes)

julia> @btime myfill!(reshape(view($(Array{Int}(undef, 10000,2)), 1:2:10000,1:2), 2500,4), 1);
  5.472 μs (0 allocations: 0 bytes)

@oscardssmith
Copy link
Member

SparseMatrix would be my guess.

@mbauman
Copy link
Member

mbauman commented Aug 1, 2025

SparseMatrix would be my guess.

Good news, this boundscheck is the least of your performance woes when you fill! a sparse matrix.

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 this pull request may close these issues.

3 participants