Improve BoundsError reporting for AbstractArrays

[carlobaldassi]

This uses the new interface of BoundsError in the checkbounds functions, which is used by AbstractArrays (including Arrays in some cases). In order to do that, I had to split the external (exported and documented) version of checkbounds and the internal one, which used a different - undocumented - interface. I called _checkbounds the latter, but suggestions for better names are indeed welcome, even if it's just and internal function.

Using BitArrays as an example, here is a before/after:

Before:

julia-0.4> b = bitrand(3);

julia-0.4> b[4] = true # does not call checkbounds
ERROR: BoundsError: attempt to access 3-element BitArray{1}:
  true
  true
 false
  at index [4]

julia-0.4> b[4] = 1
ERROR: BoundsError
 in checkbounds at abstractarray.jl:78
 in checkbounds at abstractarray.jl:110
 in setindex! at multidimensional.jl:629

After:

julia-0.4> b = bitrand(3);

julia-0.4> b[4] = true # does not call checkbounds
ERROR: BoundsError: attempt to access 3-element BitArray{1}:
  true
  true
 false
  at index [4]

julia-0.4> b[4] = 1
ERROR: BoundsError: attempt to access 3-element BitArray{1}:
  true
  true
 false
  at index [4]
 in _checkbounds at abstractarray.jl:78
 in checkbounds at abstractarray.jl:110
 in setindex! at multidimensional.jl:629

I tried to avoid extra allocations etc, but I'm submitting as a PR for review just to confirm that there should be no performance issues (cc @vtjnash which did the new BoundsError interface).

Out of necessity, the reporting is also not particularly precise in more convoluted cases, e.g.:

julia-0.4> a = rand(2,3,2);

julia-0.4> a[2,bitrand(4),2]
ERROR: BoundsError: attempt to access 2x3x2 Array{Float64,3}:
[:, :, 1] =
 0.0514813   0.573538  0.150317
 0.00485321  0.118724  0.435752

[:, :, 2] =
 0.0525793  0.731588  0.813656
 0.0234425  0.894198  0.977163
  at index [2,Bool[true,true,false,false],2]
 in _checkbounds at abstractarray.jl:80
 in checkbounds at abstractarray.jl:108
 in getindex at multidimensional.jl:188

where the whole indexing expression is reported; it's still much better then before though.

If this is OK I'll proceed improving the error reporting in other areas as well (e.g. Strings).

[vtjnash]

i think the main optimization necessary to lessen the likelyhood of a performance issue to prohibit inlining of the vargs form of the BoundsError constructor

[carlobaldassi]

@vtjnash I'm not sure I follow. Do you mean avoiding the actual construction of tuples like J in

_checkbounds(sz::Int, i::Int, A::AbstractArray, J...) = 1 <= i <= sz || throw(BoundsError(A, J))

unless actually needed? And if so, would adding @noinline in the BoundsError constructor be sufficient? Or am I totally off-track?

[vtjnash]

Yes. Although the constructor is actually at

julia/base/base.jl

Line 9 in da5013b

call(T::Type{BoundsError}, args...) = Core.call(T, args...)

(the builtin types are a bit of an odd case)

[vtjnash]

The other effect of possible significance is in
function checkbounds(A::AbstractArray, I::Union(Real,AbstractArray)...)
Previously, Julia may have been able to avoid actually allocating I, and thus avoided the need for a gc frame.

[carlobaldassi]

Yes. Although the constructor is actually at

julia/base/base.jl

Line 9 in da5013b

call(T::Type{BoundsError}, args...) = Core.call(T, args...)

(the builtin types are a bit of an odd case)

ok, which means at the moment I have no idea what to do then... @noinline at that line of base.jl? If it's not that simple, since avoiding inlining of BoundsError would be helpful anyway, my guess is that the quickest route is that you do that change on master and then I'd rebase this PR. Otherwise I need suggestions, sorry.

The other effect of possible significance is in
function checkbounds(A::AbstractArray, I::Union(Real,AbstractArray)...)
Previously, Julia may have been able to avoid actually allocating I, and thus avoided the need for a gc frame.

What if we forced inlining of all the _checkbounds methods? Wouldn't that automatically splat everything always? Then if inlining is prevented in the BoundsError constructor tuple wouldn't be reassembled back unless needed.

[vtjnash]

@noinline at that line of base.jl?

Yes

What if we forced inlining of all the _checkbounds methods

That might work. Perhaps Jeff can comment. It also just might not be too much of an issue. I suspect Jeff's Tuple rewrite will fix this anyways

[carlobaldassi]

Ok thanks I'll give it a try and update this PR then.

[carlobaldassi]

Ok this is now updated with the discussed inline/noinline changes - see last commit.

[carlobaldassi]

I have redone the manual inlining/noinlining thing. Hopefully this time it should be working. I had to do some ugly things in order to make it compile.

[carlobaldassi]

Updated with just a little less ugliness. (I'm still non sure it's actually doing what it's supposed to.)

[carlobaldassi]

I tried a couple of benchmarks and couldn't find any significant difference. Any suggestions on what cases to test? Other comments? Otherwise this could be merged I think.

[timholy]

Sorry, I totally missed this when it came out. Nice!

Is another option to wait to define checkbounds until after @inline has been defined? That will fail only if something calls checkbounds before its definition during compilation. (I'm guessing this won't work, but just asking.)

Anyway, assuming we need to define it early, LGTM. I'd encourage merging (since you wrote this there are now merge conflicts---again, sorry for the delay in noticing this).

[carlobaldassi]

Yes I got sidetracked. I'll look into this again as soon as I have some time to make it up to date again, and then merge (after trying again to avoid the ugly @_crude_meta hack).

[mbauman]

I think a simpler solution to @inline this early in the bootstrap process is to use a macro to insert the Expr manually into the function body:

macro _inline_expr()
    Expr(:meta, :inline)
end

f() = (@_inline_expr(); …)

(See 7958f08 line 488)

[carlobaldassi]

@mbauman : that was my first attempt, but see 359d5d3#commitcomment-9175007. When I tested it, it did not work.

[mbauman]

That's different. This does work because it's using a macro to insert the appropriate syntax (and not just creating a value):

julia> macro _noinline_expr()
           Expr(:meta, :noinline)
       end

julia> f() = (@_noinline_expr(); 1)
       g() = f()
g (generic function with 1 method)

julia> @code_llvm g()

define i64 @julia_g_331067() {
top:
  %0 = call i64 @julia_f_331068(), !dbg !1795
  ret i64 %0, !dbg !1795
}

[carlobaldassi]

Ah I see. Nice trick!

[carlobaldassi]

This is now superseded by #10525, closing.