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

Implement all_assigned #362

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Implement all_assigned #362

wants to merge 3 commits into from

Conversation

Tokazama
Copy link
Member

all_assigned(x) performs isassigned(x, i) at each index for x. This implementation is very simple but may not be immediately obvious because it requires iterating over the indices not the values.

`all_assigned(x)` performs `isassigned(x, i)` at each index for `x`. This
implementation is very simple but may not be immediately obvious because
it requires iterating over the indices _not_ the values.
@codecov
Copy link

codecov bot commented Oct 30, 2022

Codecov Report

Merging #362 (5b51fdc) into master (0caea19) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #362   +/-   ##
=======================================
  Coverage   90.27%   90.27%           
=======================================
  Files           9        9           
  Lines        1337     1337           
=======================================
  Hits         1207     1207           
  Misses        130      130           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ChrisRackauckas
Copy link
Member

What needs to extend this?

@Tokazama
Copy link
Member Author

My current use case is supporting the :nothrow effect.
Bounds checking isn't sufficient for this because an unassigned index will still throw an UndefRefError.

unsafe_getindex1(x, i::Int) = @inbounds(x[i])
Base.@assume_effects :nothrow unsafe_getindex2(x, i::Int) = @inbounds(x[i])

In case like Vector it makes sense that the compiler would struggle to figure this out

julia> Core.Compiler.infer_effects(unsafe_getindex1, Tuple{Vector{Int},Int})
(?c,+e,?n,+t)

julia> Core.Compiler.infer_effects(unsafe_getindex2, Tuple{Vector{Int},Int})
(?c,+e,+n,+t)

But even cases where we can't have an undefined error and in bounds should be sufficient, the compiler still doesn't know there won't be an error.

julia> Core.Compiler.infer_effects(unsafe_getindex1, Tuple{Tuple{Int},Int})
(?c,+e,?n,+t)

julia> Core.Compiler.infer_effects(unsafe_getindex2, Tuple{Tuple{Int},Int})
(?c,+e,+n,+t)

So any array whose inbound elements are guaranteed to be assigned could benefit from extending this.

@ChrisRackauckas
Copy link
Member

I don't quite understand why that would need an interface function? Your example doesn't use all_assigned

@Tokazama
Copy link
Member Author

Tokazama commented Oct 31, 2022

The previous example was just demonstrating that even if we know something is inbounds the compiler often can't ensure an error isn't going to be thrown. Assuring that all indices are defined allows us to make some assumptions that can speed things up sometimes

function find_first1(v, x)
    @assert ArrayInterfaceCore.all_assigned(x)
    i = unsafe_find_first(v, x)
    i === 0 ? nothing : i
end
Base.@assume_effects :terminates_locally :nothrow function unsafe_find_first(v, x)
    for i in eachindex(x)
        isequal(@inbounds(x[i]), v) && return i
    end
    return 0
end
function find_first2(v, x)
    for i in eachindex(x)
        isequal(@inbounds(x[i]), v) && return i
    end
    return 0
end

In this case we get a small performance boost

julia> let x = rand(100);
       @btime find_first1($(x[end]), $x)
       @btime find_first2($(x[end]), $x)
       end
  121.440 ns (0 allocations: 0 bytes)
  155.948 ns (0 allocations: 0 bytes)

I can see how the utility of this may not be readily apparent. Maybe this sort of example needs to be in an extended help section for all_assigned?

@ChrisRackauckas
Copy link
Member

Update to the latest version?

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

Successfully merging this pull request may close these issues.

2 participants