From e1b82327636aed75374988a490a7e86dff76afe8 Mon Sep 17 00:00:00 2001 From: ChrisRackauckas-Claude Date: Thu, 8 Oct 2026 07:05:13 -0400 Subject: [PATCH 1/3] Add O(1) fast path to AbstractVectorOfArray getindex getindex(A::AbstractVectorOfArray, I::Vararg{Int,N}) bounds-checked via checkbounds(A, I...), which calls size(A). size(A) scans every inner array (maximum(size(u,d) for u in VA.u)) to find the ragged maximum, so every scalar getindex was O(n) in the number of inner arrays. Add 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 it directly without computing size(A). Falls back to the existing checkbounds + zero-padding path for anything outside the selected inner array's own bounds (truly out-of-range indices, and ragged reads that must return zero), so behavior for ragged arrays is unchanged. N == 1 (where u's elements are themselves scalars) skips the fast path since size(A) is already O(1) there. At n=1000 inner arrays: A[i,j] 1219ns -> 4.4ns; scalar iteration over the whole array 2.42ms -> 4.4us (both Julia 1.10 and 1.12). Co-Authored-By: Chris Rackauckas Co-Authored-By: Claude Agent-Harness: Claude Code 2.1.294 Agent-Model: claude-sonnet-5 Agent-Session: https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv --- src/vector_of_array.jl | 18 ++++++++++++++++-- test/Core/basic_indexing.jl | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/src/vector_of_array.jl b/src/vector_of_array.jl index af4da6e9..f5e65b11 100644 --- a/src/vector_of_array.jl +++ b/src/vector_of_array.jl @@ -955,10 +955,24 @@ end Base.@propagate_inbounds function Base.getindex( A::AbstractVectorOfArray{T, N}, I::Vararg{Int, N} ) where {T, N} - @boundscheck checkbounds(A, I...) col = I[N] inner_I = Base.front(I) - u_col = A.u[col] + u = A.u + # Fast path: `checkbounds(A, I...)` below calls `size(A)`, which scans every + # inner array to find the ragged maximum (O(n) in the number of columns). + # When `I` is inside the selected inner array's own bounds, it is always a + # valid index into `A`, so this is always correct and avoids that scan. + # N == 1 means `u`'s elements are themselves scalars (see the + # `VectorOfArray(::AbstractVector)` constructor), so `size(A)` is already + # O(1) there and `inner_I` is empty; skip the fast path in that case. + if N > 1 && checkbounds(Bool, u, col) + u_col = @inbounds u[col] + if checkbounds(Bool, u_col, inner_I...) + return @inbounds u_col[inner_I...] + end + end + @boundscheck checkbounds(A, I...) + u_col = u[col] # Return zero for indices outside ragged storage for d in 1:(N - 1) if inner_I[d] > size(u_col, d) diff --git a/test/Core/basic_indexing.jl b/test/Core/basic_indexing.jl index f3b05cbd..94f3d923 100644 --- a/test/Core/basic_indexing.jl +++ b/test/Core/basic_indexing.jl @@ -1,6 +1,16 @@ using RecursiveArrayTools, Test using RecursiveArrayToolsShorthandConstructors +# Counts calls to `size` on its wrapped vector, used to check that an inner +# array is not scanned once per column of an `AbstractVectorOfArray`. +const counter = Ref(0) +struct CountingVec{T} <: AbstractVector{T} + data::Vector{T} +end +Base.size(c::CountingVec) = (counter[] += 1; size(c.data)) +Base.getindex(c::CountingVec, i::Int) = c.data[i] +Base.IndexStyle(::Type{<:CountingVec}) = IndexLinear() + # Example Problem recs = [[1, 2, 3], [4, 5, 6], [7, 8, 9]] testa = cat(recs..., dims = 2) @@ -394,3 +404,28 @@ x = VectorOfArray(StructArray{SVector{1, Float64}}(ntuple(_ -> [1.0, 2.0], 1))) y = 2 * x @. x = y @test all(all.(y .== x)) + +@testset "getindex O(1) fast path keeps BoundsError/ragged semantics" begin + # In-bounds getindex is O(1) in the number of inner arrays: the old path + # bounds-checked via `size(A)`, which calls `size` on every inner array to + # find the ragged maximum (O(n)). Count `size` calls on the inner arrays + # with `CountingVec` to check this without relying on timing. + counter[] = 0 + n = 1000 + A = VectorOfArray([CountingVec(rand(2)) for _ in 1:n]) + @test A[2, 7] == A.u[7][2] + @test counter[] < n # far fewer than one `size` call per inner array + + # Ragged: zero-padding inside the max size is unchanged + R = VA[[1.0, 2.0], [3.0], [4.0, 5.0, 6.0]] + @test R[1, 1] == 1.0 + @test R[2, 1] == 2.0 + @test R[3, 2] == 0.0 # inner array 2 (length 1) padded with zero at row 3 + @test R[1, 3] == 4.0 + + # Truly out-of-range indices still throw, on both dimensions + @test_throws BoundsError R[4, 1] # row 4 exceeds the global max size (3) + @test_throws BoundsError R[1, 4] # column 4 exceeds the number of inner arrays + @test_throws BoundsError R[0, 1] + @test_throws BoundsError R[1, 0] +end From 1a353f132845b5e4ad65c20f40bace330fd83dea Mon Sep 17 00:00:00 2001 From: ChrisRackauckas-Claude Date: Thu, 8 Oct 2026 10:17:26 -0400 Subject: [PATCH 2/3] Fix OffsetArray BoundsError regression in getindex fast path Round-1 review (Cursor, pr-668/round1): the fast path used checkbounds(Bool, u_col, inner_I...), which respects the selected column's own axes. AbstractVectorOfArray's index contract is 1-based over size(A, d) regardless of the inner array's own axes, so for an OffsetArray column (or any non-1-based inner array) this wrongly accepted indices that must throw -- e.g. O0[0,1] with inner arrays on axes 0:2 returned a value instead of BoundsError, same for O2[4,1] on axes 2:4. Replace the checkbounds(u_col, ...) check with an explicit 1-based bound against size(u_col, d): size(u_col, d) <= size(A, d) always holds, so this is a sufficient in-bounds check that matches the slow path's contract exactly, without touching the inner array's own axes at all. Falls back to the existing checkbounds(A, I...) path -- unaffected by this change -- for anything the fast check doesn't clear, so the known OffsetArray garbage-read issue (#675) is neither fixed nor worsened here. Verified against master bit-for-bit on both Julia 1.10 and 1.12: O0[0,1] and O2[4,1] both throw BoundsError (matching master exactly, previously returned 1.0/3.0); in-bounds OffsetArray reads return the same (garbage, per #675) values as master. The plain-Array win is unaffected: A[i,j] at n=1000 is still 1219ns -> 4.4ns. Also: the master CountingVec call count is 1001 (not 1000 as the PR body said) -- one extra size() call from the ragged zero-pad check on the selected column; the existing `counter[] < n` test already covers this correctly in either direction. A minimal OffsetVec test type (avoiding a new OffsetArrays dependency) with O0[0,1]/O2[4,1] regression tests is added to test/Core/basic_indexing.jl, reproducing the review's finding against round 1 (0/2 pass) and passing against this fix (2/2) on both Julia versions. Co-Authored-By: Chris Rackauckas Co-Authored-By: Claude Agent-Harness: Claude Code 2.1.294 Agent-Model: claude-sonnet-5 Agent-Session: https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv --- src/vector_of_array.jl | 23 +++++++++++++++-------- test/Core/basic_indexing.jl | 29 +++++++++++++++++++++++------ 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/src/vector_of_array.jl b/src/vector_of_array.jl index f5e65b11..90110861 100644 --- a/src/vector_of_array.jl +++ b/src/vector_of_array.jl @@ -958,16 +958,23 @@ Base.@propagate_inbounds function Base.getindex( col = I[N] inner_I = Base.front(I) u = A.u - # Fast path: `checkbounds(A, I...)` below calls `size(A)`, which scans every - # inner array to find the ragged maximum (O(n) in the number of columns). - # When `I` is inside the selected inner array's own bounds, it is always a - # valid index into `A`, so this is always correct and avoids that scan. - # N == 1 means `u`'s elements are themselves scalars (see the - # `VectorOfArray(::AbstractVector)` constructor), so `size(A)` is already - # O(1) there and `inner_I` is empty; skip the fast path in that case. + # Fast path avoiding the O(n) `size(A)` scan below. `size(u_col, d) <= + # size(A, d)` always holds, so a 1-based bound against `size(u_col, d)` is + # sufficient for an in-bounds index into `A`. Must not use + # `checkbounds(u_col, ...)`: that respects a non-1-based inner axis (e.g. + # OffsetArray) and would wrongly accept an index `A`'s 1-based contract + # must reject. N == 1 stores scalars in `u` directly (no inner array), + # where `size(A)` is already O(1); skip the fast path there. if N > 1 && checkbounds(Bool, u, col) u_col = @inbounds u[col] - if checkbounds(Bool, u_col, inner_I...) + in_inner_bounds = true + for d in 1:(N - 1) + if !(1 <= inner_I[d] <= size(u_col, d)) + in_inner_bounds = false + break + end + end + if in_inner_bounds return @inbounds u_col[inner_I...] end end diff --git a/test/Core/basic_indexing.jl b/test/Core/basic_indexing.jl index 94f3d923..91bc07aa 100644 --- a/test/Core/basic_indexing.jl +++ b/test/Core/basic_indexing.jl @@ -11,6 +11,17 @@ Base.size(c::CountingVec) = (counter[] += 1; size(c.data)) Base.getindex(c::CountingVec, i::Int) = c.data[i] Base.IndexStyle(::Type{<:CountingVec}) = IndexLinear() +# A vector whose own indices do not start at 1, e.g. `OffsetVec(v, -1)` has +# axes `0:length(v)-1`, like `OffsetArrays.OffsetArray(v, 0:length(v)-1)`. +struct OffsetVec{T} <: AbstractVector{T} + data::Vector{T} + offset::Int +end +Base.axes(o::OffsetVec) = (o.offset .+ (1:length(o.data)),) +Base.size(o::OffsetVec) = size(o.data) +Base.getindex(o::OffsetVec, i::Int) = o.data[i - o.offset] +Base.IndexStyle(::Type{<:OffsetVec}) = IndexLinear() + # Example Problem recs = [[1, 2, 3], [4, 5, 6], [7, 8, 9]] testa = cat(recs..., dims = 2) @@ -406,26 +417,32 @@ y = 2 * x @test all(all.(y .== x)) @testset "getindex O(1) fast path keeps BoundsError/ragged semantics" begin - # In-bounds getindex is O(1) in the number of inner arrays: the old path - # bounds-checked via `size(A)`, which calls `size` on every inner array to - # find the ragged maximum (O(n)). Count `size` calls on the inner arrays - # with `CountingVec` to check this without relying on timing. + # count `size` calls on the inner arrays to check the fast path is O(1) + # without relying on timing counter[] = 0 n = 1000 A = VectorOfArray([CountingVec(rand(2)) for _ in 1:n]) @test A[2, 7] == A.u[7][2] @test counter[] < n # far fewer than one `size` call per inner array - # Ragged: zero-padding inside the max size is unchanged + # ragged: zero-padding inside the max size is unchanged R = VA[[1.0, 2.0], [3.0], [4.0, 5.0, 6.0]] @test R[1, 1] == 1.0 @test R[2, 1] == 2.0 @test R[3, 2] == 0.0 # inner array 2 (length 1) padded with zero at row 3 @test R[1, 3] == 4.0 - # Truly out-of-range indices still throw, on both dimensions + # truly out-of-range indices still throw, on both dimensions @test_throws BoundsError R[4, 1] # row 4 exceeds the global max size (3) @test_throws BoundsError R[1, 4] # column 4 exceeds the number of inner arrays @test_throws BoundsError R[0, 1] @test_throws BoundsError R[1, 0] end + +@testset "getindex fast path rejects a non-1-based inner axis (OffsetArray)" begin + # minimal non-1-based AbstractVector, avoiding an OffsetArrays dependency + O0 = VA[OffsetVec([1.0, 2.0, 3.0], -1), OffsetVec([4.0, 5.0, 6.0], -1)] # axes 0:2 + O2 = VA[OffsetVec([1.0, 2.0, 3.0], 1), OffsetVec([4.0, 5.0, 6.0], 1)] # axes 2:4 + @test_throws BoundsError O0[0, 1] + @test_throws BoundsError O2[4, 1] +end From bf2e5fee081f66de1ed5b0ece3c8e4d29e5caaaf Mon Sep 17 00:00:00 2001 From: ChrisRackauckas-Claude Date: Thu, 8 Oct 2026 15:38:19 -0400 Subject: [PATCH 3/3] Fix OffsetVector container BoundsError regression in getindex fast path Round-2 review (Grok 4.7, pr-668/round2): the column gate followed checkbounds(Bool, u, col), which respects VA.u's own axes. The constructors (VectorOfArray(vec::AbstractVector{VT}) and the AbstractArray 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/checkbounds(A, I...). So for a container with axes 0:3 or 2:5, a column outside 1:length(u) (but still inside axes(u)) wrongly returned a value instead of throwing -- the same mistake round 2 fixed for inner arrays, one level up, for the container itself. Add 1 <= col <= length(u) to the fast-path gate, alongside the existing checkbounds(Bool, u, col) (both are still needed: a column inside 1:length(u) but outside u'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 it and the fallback's own u[col] throws natively). Both checks are O(1), so the fast path keeps its cost. Verified bit-for-bit against master on both Julia 1.10 and 1.12 across the reviewer's full grid: columns outside 1:length(u) (C0[1,0], C0[2,0], C0[CartesianIndex(1,0)], C2[1,5], C2[2,5]) throw BoundsError, matching master exactly (round-2 code returned 1.0/2.0/1.0/7.0/8.0 for these); columns inside 1:length(u) but outside axes(u) (C2[1,1], C0[1,4], C2[1,6]) still throw, matching master; columns inside both return the stored value, matching master (C0[1,1]==3.0, C0[1,3]==7.0, C2[1,2]==1.0, C2[1,4]==5.0). The plain-Array win is unaffected: A[i,j] at n=1000 is still 1219ns -> 4.4ns. Also tightens the existing O(1) call-count regression test from `counter[] < n` to the measured exact count (`== 1`), per the review's non-blocking note that `< n` is a weak lock (any count up to 999 would also pass). Co-Authored-By: Chris Rackauckas Co-Authored-By: Claude Agent-Harness: Claude Code 2.1.294 Agent-Model: claude-sonnet-5 Agent-Session: https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv --- src/vector_of_array.jl | 6 +++++- test/Core/basic_indexing.jl | 30 +++++++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/vector_of_array.jl b/src/vector_of_array.jl index 90110861..709f686c 100644 --- a/src/vector_of_array.jl +++ b/src/vector_of_array.jl @@ -965,7 +965,11 @@ Base.@propagate_inbounds function Base.getindex( # OffsetArray) and would wrongly accept an index `A`'s 1-based contract # must reject. N == 1 stores scalars in `u` directly (no inner array), # where `size(A)` is already O(1); skip the fast path there. - if N > 1 && checkbounds(Bool, u, col) + # `A`'s column dimension is likewise 1-based over `length(u)` regardless + # of `u`'s own axes (e.g. an OffsetVector container): `1 <= col <= + # length(u)` matches that contract, and `checkbounds(Bool, u, col)` is + # still required since `u`'s own axes need not be `1:length(u)`. + if N > 1 && 1 <= col <= length(u) && checkbounds(Bool, u, col) u_col = @inbounds u[col] in_inner_bounds = true for d in 1:(N - 1) diff --git a/test/Core/basic_indexing.jl b/test/Core/basic_indexing.jl index 91bc07aa..ae5208e9 100644 --- a/test/Core/basic_indexing.jl +++ b/test/Core/basic_indexing.jl @@ -423,7 +423,7 @@ y = 2 * x n = 1000 A = VectorOfArray([CountingVec(rand(2)) for _ in 1:n]) @test A[2, 7] == A.u[7][2] - @test counter[] < n # far fewer than one `size` call per inner array + @test counter[] == 1 # exactly the selected column's own size, not one per inner array # ragged: zero-padding inside the max size is unchanged R = VA[[1.0, 2.0], [3.0], [4.0, 5.0, 6.0]] @@ -446,3 +446,31 @@ end @test_throws BoundsError O0[0, 1] @test_throws BoundsError O2[4, 1] end + +@testset "getindex fast path rejects a non-1-based VA.u container" begin + # VectorOfArray(::AbstractVector{VT}) stores a non-1-based container (like + # OffsetVec) as-is; this is the same column-index mistake as the inner-axis + # case above, one level up. + cols = [[1.0, 2.0], [3.0, 4.0], [5.0, 6.0], [7.0, 8.0]] + C0 = VectorOfArray(OffsetVec(cols, -1)) # axes 0:3 + C2 = VectorOfArray(OffsetVec(cols, 1)) # axes 2:5 + + @test_throws BoundsError C0[1, 0] + @test_throws BoundsError C0[2, 0] + @test_throws BoundsError C0[CartesianIndex(1, 0)] + @test_throws BoundsError C2[1, 5] + @test_throws BoundsError C2[2, 5] + + # a column inside both `1:length(u)` and `u`'s own axes still returns the + # stored value + @test C0[1, 1] == 3.0 + @test C0[1, 3] == 7.0 + @test C2[1, 2] == 1.0 + @test C2[1, 4] == 5.0 + + # a column inside `1:length(u)` but outside `u`'s own axes still throws, + # matching master (indexing `u` itself fails natively there) + @test_throws BoundsError C0[1, 4] + @test_throws BoundsError C2[1, 1] + @test_throws BoundsError C2[1, 6] +end