Don't @inbounds AbstractArray's iterate method; optimize checkbounds instead

[mbauman]

Split off from #58785, this simplifies iterate and removes the @inbounds call that was added in #58635. It achieves the same (or better!) performance, however, by targeting optimizations in checkbounds and — in particular — the construction of a linear eachindex (against which the bounds are checked).

[giordano]

It achieves the same (or better!) performance

Do you have any benchmarks handy?

[mbauman]

Using @jishnub's benchmark:

Nightly @ f61c640 (which has the @inbounds):

julia> using BenchmarkTools
Precompiling BenchmarkTools finished.
  8 dependencies successfully precompiled in 12 seconds. 9 already precompiled.

julia> using LinearAlgebra

julia> A = rand(1000,1000); v2 = view(A, 1:2:lastindex(A));

julia> @btime norm(Iterators.map(splat(-), zip($v2, $v2)));
  1.941 ms (0 allocations: 0 bytes)

vs. b09514e:

julia> @btime norm(Iterators.map(splat(-), zip($v2, $v2)));
  312.167 μs (0 allocations: 0 bytes)

[oscardssmith]

are these functions really so big that they aren't being inlined automatically?

[mbauman]

Before b09514e, yes, they were (well, kinda. length itself was not inlining; had I added @inline to it, though, it'd make this method not inline). That branch was enough to push things over the inlining limit. As I wrote in #58785 (comment),

this calls abstract infrastructure that could be large (and itself @inline'd)... which would then require these generic methods to be similarly @inline.

[mbauman]

@nanosoldier runbenchmarks()

[giordano]

Test failures (quite a lot) look relevant 😢

[mbauman]

@nanosoldier runbenchmarks(ALL, vs=":master")

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

[JeffBezanson]

Very good PR. Benchmarks are an interesting mixed bag of slowdowns and infinite speedups :) Not sure what to do about it.

[mbauman]

The regressions that look meaningful to me are in the iteration of Dict, IdDict and BitSet — those are reporting to be on the order of 1.8x slower. It may be possible to address those; my hunch is that they're hitting the range-length change. That branch is surely constant-folded or statically known in most cases. Maybe adding @inline to that method is the better solution.

[mbauman]

OK, I don't think any of the regressions are actually real. I can't reproduce the iteration ones locally, and BenchmarkTools is doing something wildly wrong for the in-place BitSet ones. Just looking at nightly:

julia> using Random, StableRNGs, BenchmarkTools

julia> const RNG = StableRNG(1)
StableRNGs.LehmerRNG(state=0x00000000000000000000000000000003)

julia> const iterlen = 1000;

julia> const ints = rand(RNG, 1:iterlen, iterlen);

julia> c = BitSet(ints);

julia> const newints = [rand(RNG, ints, 10); rand(RNG, 1:iterlen, 10); rand(RNG, iterlen:2iterlen, 10);];

julia> c2 = BitSet(newints);

julia> @benchmark union!(x, $c2) setup=(x=copy($c)) evals=1
BenchmarkTools.Trial: 10000 samples with 1 evaluation per sample.
 Range (min … max):   0.001 ns … 916.000 ns  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     42.000 ns               ┊ GC (median):    0.00%
 Time  (mean ± σ):   46.112 ns ±  33.456 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

  ▄          ▇█          ▂                       ▁▃            ▁
  █▁▁▁▁▁▁▁▁▁▁██▁▁▁▁▁▁▁▁▁▁██▁▁▁▁▁▁▁▁▁▁▁▆▁▁▁▁▁▁▁▁▁▁██▁▁▁▁▁▁▁▁▁▁▇ █
  0.001 ns      Histogram: log(frequency) by time       208 ns <

 Memory estimate: 736 bytes, allocs estimate: 1.

That is... weird.

@nanosoldier runbenchmarks(!("scalar" || "dates" || "io" || "problem" || "inference"), vs = "master")

[mbauman]

@nanosoldier runbenchmarks(["array","collection","find","misc","sort","sparse","tuple","union"], vs=":master")

[mbauman]

🤷

@nanosoldier runbenchmarks(ALL, vs=":master")

[vtjnash]

I don't think nanosoldier knows how to distribute ! over ||, and [ means a single test that matches all of those tags in order

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

[mbauman]

That looks much better. I think there's only two (non-scalar) regressions over 1.1x that's on both pages:

ID	time ratio (1)	memory ratio (1)	time ratio (2)	memory ratio (2)
["array", "setindex!", ("setindex!", 3)]	1.29 (5%) ❌	1.00 (1%)	1.17 (5%) ❌	1.00 (1%)
["tuple", "linear algebra", ("matvec", "(4, 4)", "(4,)")]	1.52 (5%) ❌	1.00 (1%)	1.13 (5%) ❌	1.00 (1%)

The perf improvements are stable across the two runs. This is looking good to me.

[mbauman]

Just for posterity, as I understand it, there are three separate considerations — each of which help increase the chance that the compiler does something smart here:

• Ensuring length inlines (here by making a branch do math instead)
• The iteration implementation here takes the form checkbounds(Bool, x, i) ? (x[i], i+1) : nothing... and x[i] itself should have the exact same checkbounds(Bool, x, i) predicate inside it. Making sure those two predicates match exactly increases the odds that the compiler skips the inner branch entirely. (That's what's behind the CodeUnits change)
• Making sure the checkbounds test itself is linear (purely adds and muls) helps LLVM track repeated checkbounds inside a for loop and hoist it out entirely. (That's what moving to unchecked_oneto does)

[giordano]

I'm not deeply familiar with this part of the code, but changes look sensible, the diff in base (excluding test) is net negative, we get new tests and more consistently better performance, looks like a win-win situation.

[mbauman]

OK, given that the length changes here are relatively ancillary to the main objective around iteration, I've split those off into #58864, and instead I just mark it (and its callers) @inline for now. Given the fundamental importance of the length of a range, I figure that's prudent. Specifically targeting the inlining of this method (and its callers) gets us some of the advantages in the original benchmark. That benchmark is now half-way between what I reported in #58793 (comment):

julia> using BenchmarkTools, LinearAlgebra

julia> A = rand(1000,1000); v2 = view(A, 1:2:lastindex(A));

julia> @btime norm(Iterators.map(splat(-), zip($v2, $v2)));
  931.917 μs (0 allocations: 0 bytes)