Add O(1) fast path to AbstractVectorOfArray getindex

[ChrisRackauckas-Claude]

What changed and why

getindex(A::AbstractVectorOfArray{T,N}, I::Vararg{Int,N}) (src/vector_of_array.jl) bounds-checks via @boundscheck checkbounds(A, I...), and checkbounds(A, I...) calls size(A). size(A) is (maximum(size(u,d) for u in VA.u for d in 1:N-1)..., length(VA.u)) — it scans every inner array to find the ragged maximum. So every scalar A[i, j] was O(n) in the number of inner arrays, even for the overwhelmingly common non-ragged case.

Added a fast path: if the index is inside the selected inner array's own bounds, it is always a valid index into A, so return directly without computing size(A). Anything outside the selected inner array's own bounds (a genuinely out-of-range index, or a ragged read that must return zero) falls back to the existing checkbounds(A, I...) + zero-padding path unchanged. N == 1, where u's elements are scalars rather than arrays, skips the fast path since size(A) is already O(1) there.

Round 2 (addressing Cursor's review): the round-1 fast path checked bounds with checkbounds(Bool, u_col, inner_I...), which respects the selected column's own axes rather than A's 1-based contract. Fixed by checking 1 <= inner_I[d] <= size(u_col, d) directly, which never touches the inner array's own axes.

Round 3 (this update, addressing Grok 4.7's review): round 2 fixed the inner-array axis mistake but left the identical mistake one level up, in the column gate: checkbounds(Bool, u, col) respects VA.u's own axes, but VectorOfArray(vec::AbstractVector{VT}) and the AbstractArray constructor overload store a non-1-based container (e.g. an OffsetVector) as-is, while A's own column dimension is 1-based over length(VA.u) (via size(A)/checkbounds(A, I...)). So for a container with axes 0:3 or 2:5, a column outside 1:length(u) (but still inside the container's own axes(u)) wrongly returned a value instead of throwing. Fixed by adding 1 <= col <= length(u) to the gate, alongside the existing checkbounds(Bool, u, col) — both checks are still needed: a column inside 1:length(u) but outside the container's own axes (e.g. column 1 on a container with axes 2:5) must still throw, which it does because checkbounds(Bool, u, col) catches that case and the fallback's own u[col] throws natively there too, matching master.

Before/after

n = 1000 inner arrays, Vector{Float64} of length 2 each (Julia 1.12.7; 1.10.12 numbers are within noise of these):

before  A[i,j]:               min 1219.0 ns   med 1223.0 ns   allocs 0
after   A[i,j]:                min    4.4 ns   med    4.4 ns   allocs 0  (unaffected by rounds 2 and 3)

Ragged correctness unchanged throughout all three rounds (R[3,2] == 0.0 zero-pad, R[4,1]/R[1,4] BoundsError).

Round-3 verification — bit-for-bit identical to master on both Julia 1.10.12 and 1.12.7, across the reviewer's full grid (a VectorOfArray built directly over a non-1-based OffsetVector-style container of four Vector{Float64} columns, C0 on axes 0:3, C2 on axes 2:5):

C0[1,0]              -> BoundsError   # round-2 PR returned 1.0 (bug); round-3 matches master
C0[2,0]              -> BoundsError   # round-2 PR returned 2.0 (bug); round-3 matches master
C0[CartesianIndex(1,0)] -> BoundsError # round-2 PR returned 1.0 (bug); round-3 matches master
C2[1,5]              -> BoundsError   # round-2 PR returned 7.0 (bug); round-3 matches master
C2[2,5]              -> BoundsError   # round-2 PR returned 8.0 (bug); round-3 matches master

C0[1,1] == 3.0        # identical on master and round-3 (column inside 1:length(u) and axes(u))
C0[1,3] == 7.0        # identical
C2[1,2] == 1.0        # identical
C2[1,4] == 5.0        # identical

C0[1,4]  -> BoundsError  # column inside 1:length(u), outside axes(u) -- identical on master and round-3
C2[1,1]  -> BoundsError  # identical
C2[1,6]  -> BoundsError  # identical

Failing-before / passing-after

Round 2's test (getindex O(1) fast path keeps BoundsError/ragged semantics): tightened the call-count assertion from counter[] < n to the measured exact count counter[] == 1, per the round-2 review's non-blocking note that < n is a weak lock (anything up to 999 would also have passed). 10/10 pass on this branch.

Round 2's OffsetVec inner-array test (getindex fast path rejects a non-1-based inner axis (OffsetArray)): unaffected by round 3, still 2/2.

New round-3 test — test/Core/basic_indexing.jl::"getindex fast path rejects a non-1-based VA.u container", covering the reviewer's exact grid above:

Before (round-2 code, commit 1a353f1, kept the new test):

Test Summary:                                           | Pass  Fail  Total  Time
getindex fast path rejects a non-1-based VA.u container |    7     5    12   2.5s

(5 failures: exactly the 5 out-of-1:length(u) column cases that round 2 got wrong)

After (this commit, bf2e5fe): 12/12 pass, on both Julia 1.10.12 and 1.12.7.

Full GROUP=Core run (both Julia versions, after round 3): all testsets green. GROUP=QA on 1.12.7: 20/20 pass. GROUP=QA on 1.10.12 hits the same pre-existing, unrelated Aqua ambiguity failure documented in #667 (confirmed present on unmodified master). Runic and typos: clean on both changed files.

What was not verified

• Downstream, GPU, NoPre and AD test groups were not run; this is an internal getindex fast path with no public-API surface change.
• GPU array backends (JLArray/CuArray) were not benchmarked.
• Issue VectorOfArray with OffsetArray inner arrays returns garbage from A[i,j] (@inbounds after size-only check) #675 (OffsetArray garbage reads on in-bounds access) is explicitly not addressed here, per the round-1 review — this PR only ensures the fast path's bounds decision (throw vs. not) matches master's, at both the inner-array and container level, not that master's in-bounds OffsetArray value is itself correct.
• A merge against the sibling VoA PRs (fill!(::AbstractVectorOfArray, x): fill inner arrays directly, fixing a silent ragged bug #669, Add fast path to stack(::AbstractVectorOfArray) for numeric inner arrays #671, Add comprehension fast path for A[ints..., cols] indexing #673), which also touch src/vector_of_array.jl/test/Core/basic_indexing.jl but different methods — no semantic conflict expected, not tested.

Anything a reviewer should push back on

• Two rounds of independent review have now found the identical mistake (respecting a non-1-based axis instead of A's 1-based contract) at two different levels of this one function — the inner array in round 2, the container in round 3. Worth a careful third look for whether there's a third place the same assumption is hiding (I don't see one — the function only indexes u once by col and u_col once by inner_I, which is now exactly two gates — but two misses in a row on the same kind of mistake is a reason to look again rather than assume round 3 is the last one).

Please ignore until reviewed by @ChrisRackauckas.

Risk assessment

• Risk: low
• Blast radius: src/vector_of_array.jl, one getindex method used throughout scalar AbstractVectorOfArray/AbstractDiffEqArray indexing (so touches a hot path), but the change is a pure fast-path addition that falls back to the existing, unmodified logic whenever it doesn't apply — no behavior change for any input, now verified against both OffsetArray inners and OffsetVector containers. No public API signature change.
• Evidence: failing-before (5/12 on the container regression against round 2) / passing-after (12/12) shown above; before/after timing confirms the claimed speedup is unaffected; GROUP=Core full pass on Julia 1.10 and 1.12.
• Independent review: two rounds of independent review (Cursor on round 1, Grok 4.7 on round 2), both returning CHANGES with a confirmed-by-running blocking finding, both addressed. Round-3 independent review still pending.
• Merge: needs human review — a hot indexing path with two prior rounds of review findings on the same class of bug (non-1-based axis handling); wants a careful third look before merge rather than auto-merging.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv

[ChrisRackauckas]

🤖 Automated comment from an AI agent running as @ChrisRackauckas — not written or reviewed by Chris.

Independent review (Devin CLI 3000.11.3, model fusion-claude-opus-5-5-high-sidekick-swe-2-medium): MERGE, risk medium.

Full review

VERDICT: MERGE
RISK: medium

Medium rather than low: this is the hot scalar getindex that every ODESolution/DiffEqArray sol[i, j] goes through downstream. I found no behaviour difference from master in any probe, but I did not run the AD, GPU or Downstream groups.

Blocking findings

None.

Non-blocking findings

1. Too many comments (CLAUDE.md asks for under ~10% comment lines). Suspected from reading. In src/vector_of_array.jl:961-971, 11 of the roughly 27 added lines are one comment block. It is justification rather than narration, but it repeats itself: both the inner-axis note and the container-axis note explain the same 1-based contract. Two or three lines would cover it. In test/Core/basic_indexing.jl:452-454, the comment "this is the same column-index mistake as the inner-axis case above, one level up" describes how the PR evolved, not what the test checks. CLAUDE.md treats that kind of comment as a defect. Suggested fix: cut the src comment down and drop the history wording from the test comment.
2. The OffsetVec testsets can't catch the real OffsetArrays hazard. Confirmed by reading and by probing. The local OffsetVec.getindex is not @propagate_inbounds, so @inbounds u_col[i] still bounds-checks inside Vector. The two OffsetVec testsets therefore only check whether the code throws; they could never see an unchecked out-of-range read. I checked the real-OffsetArrays case myself (What I ran, item 1): the PR decides to throw exactly when master does for inner axes 0:2, 2:4 and -5:-3. In-size reads still return garbage, as on master (issue VectorOfArray with OffsetArray inner arrays returns garbage from A[i,j] (@inbounds after size-only check) #675). This is not a regression.
3. The two OffsetVec testsets pass on master too. Confirmed by running. They only guard against regressions in earlier versions of this PR. The only test that fails before the fix and passes after is the counter[] == 1 assertion. On master it fails: 9 pass, 1 fail at basic_indexing.jl:426. That is enough for an optimization PR, but the PR body's "failing-before" evidence compares against the PR's own round-2 commit, not master.
4. Small-n @inbounds cost. Confirmed by running. With n=3 under @inbounds, the PR's minimum is 3.25 ns vs master's 2.83 ns on 1.12, and 2.42 vs 2.00 ns on 1.10. The fast-path checks now run even when bounds checks are elided. This doesn't matter in practice and is far outweighed by the O(n)→O(1) gain without @inbounds.
5. Merge friction with Add comprehension fast path for A[ints..., cols] indexing #673. Suspected from reading. That PR appends testsets at the same place (end of test/Core/basic_indexing.jl, hunk @@ -394,3), so whichever merges second will get a trivial textual conflict. fill!(::AbstractVectorOfArray, x): fill inner arrays directly, fixing a silent ragged bug #669 and Add fast path to stack(::AbstractVectorOfArray) for numeric inner arrays #671 touch other methods. The PR has no linked or parent issue (closingIssuesReferences is empty).

What I ran

All runs used Julia 1.12.7 unless marked LTS (1.10.12). TMPDIR and scratch were under the review directory.

1. Differential probe against master (scratch/probe.jl), on 1.12 and LTS. The probe defines master's getindex body as a free function master_gi. For each case it compares the outcome (value, or exception type) of A[I...], master_gi(A, I...) and A[CartesianIndex(I...)] over every index in (-2):(size+2) on each dimension.
   • Cases: dense, ragged and empty vectors; dense and ragged matrices; ragged 3-D; SVector; StructArray; DiffEqArray (dense and ragged); nested VoA; Int; BigFloat; and real OffsetArrays.OffsetVector containers with axes 0:3 and 2:5.
   • Result: 0 mismatches across 4,794 index tuples on both Julia versions.
   • Real OffsetArray inner arrays (axes 0:2, 2:4, -5:-3), indices (-6:6) × (-1:3): the throw / no-throw decision matched master everywhere.
   • @inbounds ragged reads, including zero-pad positions, matched master.
   • Base.return_types matched master for every case.
2. Benchmarks (BenchmarkTools), n=1000, inner length 2.
   • 1.12: PR 3.7 ns vs master 832 ns. 3-D: 3.7 vs 1658 ns.
   • LTS: 2.9 vs 833 ns.
   • 0 allocations in all of these.
   • This reproduces the speedup claimed in the PR body. The absolute numbers differ from the body's because they were measured on a different machine.
3. GROUP=Core julia +1.12 --project -e 'using Pkg; Pkg.test()': passed. "VecOfArr Indexing Tests" shows 204 pass and 2 broken; the PR adds no @test_broken. Log is scratch/core112.log.
4. The PR's basic_indexing.jl against master's src (via git archive origin/master into scratch): only counter[] == 1 fails. The other 9 assertions in that testset pass, so the new test does detect the O(n) scan.
5. Runic and typos: Runic --check on both changed files is clean, and typos over the diff is clean.
6. Commit trailers: all three commits carry the harness, model and session trailers.

What I did not verify

• The QA group (Aqua/JET/ExplicitImports) on either Julia version, and Core on LTS. On LTS I only ran the probe.
• The AD group (Zygote/ReverseDiff/Tracker/Mooncake differentiating through scalar getindex), the Downstream, GPU and NoPre groups, and the RecursiveArrayToolsRaggedArrays sublibrary.
• GPU inner arrays (JLArray/CuArray). Scalar indexing there behaves the same as on master as far as I can tell from reading, but I didn't run it.
• 32-bit.
• The PR body's claim that QA fails on 1.10 and that the failure is pre-existing (Fix two silent memory-safety bugs in recursivecopy!/copyat_or_push! #667).

Links

• Add O(1) fast path to AbstractVectorOfArray getindex #668: the PR under review
• VectorOfArray with OffsetArray inner arrays returns garbage from A[i,j] (@inbounds after size-only check) #675: OffsetArray inner arrays read garbage on master; not addressed here, and not made worse
• Add comprehension fast path for A[ints..., cols] indexing #673: sibling PR, likely textual conflict in basic_indexing.jl
• fill!(::AbstractVectorOfArray, x): fill inner arrays directly, fixing a silent ragged bug #669: sibling PR (fill!)
• Add fast path to stack(::AbstractVectorOfArray) for numeric inner arrays #671: sibling PR (stack)
• Fix two silent memory-safety bugs in recursivecopy!/copyat_or_push! #667: cited for the pre-existing QA failure on 1.10

---

🤖 Posted by an AI agent — harness: Devin CLI 3000.11.3 (review), Claude Code 2.1.285 (posting) · model: fusion-claude-opus-5-5-high-sidekick-swe-2-medium
Conversation: local Claude session 3c311569-dda6-4687-bc9f-65febb212c33 (fleet master)