delay setindex! inbounds check indexlinear

[johnnychen94]

This is needed to bypass the @inbounds check in the fallback implementation, and use our own bounds checking, otherwise, setindex! for indexlinear will always be set with @inbounds mode.

I'm not very sure if this is the right fix, perhaps I've used @boundscheck wrongly

PaddedViews.jl/src/PaddedViews.jl

Lines 172 to 187 in ff689b1

	Base.@propagate_inbounds function Base.setindex!(A::PaddedView{T, N}, v, i::Vararg{Int, N}) where {T, N}
	@boundscheck begin
	# This gives some performance boost https://github.com/JuliaLang/julia/issues/33273
	_throw_argument_error() = throw(ArgumentError("PaddedViews do not support (re)setting the padding value. Consider making a copy of the array first."))
	_throw_bounds_error(A, i) = throw(BoundsError(A, i))
	if checkbounds(Bool, A, i...)
	checkbounds(Bool, A.data, i...) || _throw_argument_error()
	else
	_throw_bounds_error(A, i)
	end
	end
	setindex!(A.data, v, i...)
	return A
	end

[johnnychen94]

The weird thing here is that even if this is not added, the test still passes with pkg> test but not with pkg> include("test/runtest.jl"). Either I'm using @boundscheck wrongly or there's some bug upstream...

Edit: might be related to JuliaLang/Pkg.jl#2109

[timholy]

Hmm, that "fallback" may date from the days where Julia supported partial linear indexing. What circumstances does it come up in? Generally iteration should automatically use the "correct" scheme.

[johnnychen94]

This is the current behavior in master:

julia> using PaddedViews

julia> A = reshape(collect(1:6), 2, 3);

julia> Ap = PaddedView(missing, A, (0:4, 0:4));

julia> Ap[1] = 0 # this is dangerous
0

julia> Ap
5×5 PaddedView(missing, ::Array{Int64,2}, (0:4, 0:4)) with eltype Union{Missing, Int64} with indices 0:4×0:4:
 missing   missing   missing   missing  missing
 missing  1         3         5         missing
 missing  2         4         6         missing
 missing   missing   missing   missing  missing
 missing   missing   missing   missing  missing

The bounds check is incomplete because of the @inbounds meta in the fallback implementation. So the patch in this PR is to move the bounds check to the IndexCartesian case and thus make it to correctly throw errors:

julia> Ap[1] = 0
ERROR: ArgumentError: PaddedViews do not support (re)setting the padding value. Consider making a copy of the array first.
Stacktrace:
 [1] (::PaddedViews.var"#_throw_argument_error#3")() at /Users/jc/Documents/Julia/PaddedViews.jl/src/PaddedViews.jl:175
 [2] setindex! at /Users/jc/Documents/Julia/PaddedViews.jl/src/PaddedViews.jl:179 [inlined]
 [3] setindex!(::PaddedView{Union{Missing, Int64},2,Tuple{UnitRange{Int64},UnitRange{Int64}},Array{Int64,2}}, ::Int64, ::Int64) at /Users/jc/Documents/Julia/PaddedViews.jl/src/PaddedViews.jl:191
 [4] top-level scope at REPL[5]:1

[timholy]

Sorry, what I meant was "should we delete that Vararg{Int, M} method? Why is it there? If there is no such method, won't the fallbacks in Base generate the fully-cartesian call automatically?

[johnnychen94]

Hmmm, the indexing implementation is still opaque to me.

Looks like is still needed. If I remove that method, then it goes to

https://github.com/JuliaLang/julia/blob/c24a93253db45131983c665373d17759f9d4b758/base/multidimensional.jl#L874-L898

and then errors

julia> Ap[1] = 1
ERROR: ArgumentError: indexed assignment with a single value to many locations is not supported; perhaps use broadcasting `.=` instead?
Stacktrace:
 [1] setindex_shape_check(::Int64, ::Int64)
   @ Base ./indices.jl:258
 [2] _unsafe_setindex!(#unused#::IndexCartesian, A::Base.ReshapedArray{Union{Missing, Int64}, 1, PaddedView{Union{Missing, Int64}, 2, Tuple{UnitRange{Int64}, UnitRange{Int64}}, Matrix{Int64}}, Tuple{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64}}}, x::Int64, I::Int64)
   @ Base ~/Documents/Julia/julia/base/multidimensional.jl:886
 [3] _setindex!
   @ ~/Documents/Julia/julia/base/multidimensional.jl:877 [inlined]
 [4] setindex!(A::PaddedView{Union{Missing, Int64}, 2, Tuple{UnitRange{Int64}, UnitRange{Int64}}, Matrix{Int64}}, v::Int64, I::Int64)
   @ Base ~/Documents/Julia/julia/base/abstractarray.jl:1217
 [5] top-level scope
   @ REPL[6]:1

[johnnychen94]

Things like this works, though. I guess this is something you expected? (I didn't do make test to check if this breaks other codes)

# https://github.com/JuliaLang/julia/blob/c24a93253db45131983c665373d17759f9d4b758/base/multidimensional.jl#L874-L898
function _setindex!(l::IndexStyle, A::AbstractArray, x, I::Union{Real, AbstractArray}...)
    @_inline_meta
    @boundscheck checkbounds(A, I...)
    _unsafe_setindex!(l, _maybe_ind2sub(_maybe_reshape(l, A, I...)), x, I...)
    A
end

_maybe_ind2sub(l, A, I) = I
_maybe_ind2sub(::IndexCartesian, A, I) = _unsafe_ind2sub(size(A), I...)

[timholy]

the indexing implementation is still opaque to me

Key design principle: Base has lots of unspecific methods that convert all of Julia's fancy indexing into either linear index calls A[i::Int] or cartesian index calls A[i::Int, j::Int]. Specific methods intercept whatever calls they are designed to handle. You get more for less by only handling very specific indexing calls, because it allows more of the Base machinery to normalize into these standard forms.

_setindex!(l::IndexStyle, A::AbstractArray, x, I::Union{Real, AbstractArray}...)

Avoid creating such methods if at all possible. If you have them, you're essentially signing up to support the entire gamut of indexing methods, and that's basically a nightmare to support. Let Base do the work.

OK, now I see the problem: to_indices gets called here but it returns the Int directly, then this checkbounds occurs before the indexes are normalized. From there, everything is inside an @inbounds, so the @propagate_inbounds ends up eliding the checks you've added.

This seems to be an argument to remove the bounds-checking from the Vararg{Int,M} method and just wait to do bounds-checking after the _to_subscript_indices has normalized everything. @mbauman, do you remember why it was done that way? It was a long time ago (JuliaLang/julia#19958).

Alternatively we could implement a custom checkbounds(A, I::Int) method that expanded to cartesian indexes, but then we'd be calling _to_subscript_indices twice and that seems dumb.

[timholy]

xref JuliaLang/julia#38012

[mbauman]

ERROR: ArgumentError: ... that doesn't look like a bounds error to me. I don't know if it is feasible to wedge this behavior into a bounds check — because it's not one. You are inbounds, but it's an unsettable element. It feels like a bounds check because PaddedViews happen to have their padding around the bounds, but it's the same as a Diagonal for example.

[johnnychen94]

It is in the paddedview bounds, but not in the parent data array bounds. The bounds check here is actually doing some eager bounds check for the parent data array, so I think that still lies in the concept of bounds check :P.

[mbauman]

But you can access it at that location, right? So it's inbounds for getindex but "out of bounds" for setindex!, right? That means that there are two meanings of "bounds" for this array — one for getting and a different one for setting.

The bounds check here is actually doing some eager bounds check for the parent data array, so I think that still lies in the concept of bounds check

That's circular. It's only eagerly doing the bounds check for you because you've currently implemented the test/error in a @boundscheck block. I'm saying that you shouldn't do that because others (including Base) assume that if checkbounds(A, I...) works, then it's safe to @inbounds both A[I...] and A[I...] = x. The linear -> cartesian conversion is a bit of a red herring, I think.

[johnnychen94]

This is a hard truth and a valid mental model. I'm not sure if this fits the PaddedViews case, because it can also be weird that checkbounds(A, I...) works while setindex!(A.data, I...) then immediately throws a boundserror. And this can be more difficult if we want to skip this kind of validity checks to reduce the overhead. @inbounds macro seems like the only mechanism we have to achieve this.

[timholy]

Personally, I would say the only valid way of eliminating the overhead of the "interior check" (I'm going to stop referring to this as a "bounds check" because as @mbauman says, it isn't) is to write code that directly reaches in to the private implementation of PaddedView. That's ugly, but this is such a special case that it's also not reasonable to introduce a whole parallel infrastructure like bounds-checking and its elision to handle it.

I think this is one of those "just live with the performance overhead" cases.