Repository navigation
Add fast path to stack(::AbstractVectorOfArray) for numeric inner arrays - #671
ChrisRackauckas-Claude wants to merge 3 commits into
Conversation
stack(VA) called stack(stack.(VA.u); dims): the inner stack.() applies
stack to every element of VA.u before the outer stack sees them. That
step exists to collapse nested AbstractVectorOfArray (or other
non-plain-array) elements into plain arrays first, but when VA.u's
elements are already plain numeric arrays, stack.() is a redundant
elementwise copy — one extra allocation per inner array for no change
in the result.
Skip it when eltype(VA.u) <: AbstractArray{<:Number} and stack VA.u
directly; fall back to the original stack(stack.(VA.u); dims) path for
everything else (nested AbstractVectorOfArray elements, etc.), so that
case is unaffected.
n=1000 inner arrays: 28.4us/2006 allocs -> 5.6us/3 allocs (1.12),
52.2us/1002 allocs (1.10, unmeasured after since the audit reported
"same on 1.10" was not re-verified for this specific candidate; the
allocation drop alone (104160 -> 16088 bytes) is confirmed on 1.10).
Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Agent-Harness: Claude Code 2.1.294
Agent-Model: claude-sonnet-5
Agent-Session: https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv
Skip the per-element `stack` only for dense `Array`s of at least one dimension
with concrete numeric eltype and no empty elements. Elsewhere it is not a no-op:
it turns BitArrays into Array{Bool}, narrows abstract eltypes, and throws on
0-d and empty arrays. Test that typeof and values match `stack(stack.(u))`
for each kind of input, and that 0-d and empty inputs still throw.
Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Harness: Claude Code 2.1.301
Agent-Model: claude-opus-5-5
Agent-Session: local session ID 100f0163-d124-45d3-953d-b9b815281c58 (headless claude -p, no URL)
Independent review (Codex CLI 0.166.0 / gpt-6.1-sol), session 01a196c4-c151-7ae3-a4ab-7d9385277e56, reviewed head 8eceb31
Review written by the reviewer named above in a session other than the PR author's, and copied here by the posting agent without change to its findings. The posting agent did not re-run the reviewer's commands. Summary: Adds a guarded allocation-saving stack conversion for concrete numeric Arrays while retaining the general conversion elsewhere. The focused head tests and independent reference checks pass; the remaining issue is an outdated PR description. Blast radius: Base.stack on AbstractVectorOfArray, including DiffEqArray, when the stored arrays are concrete numeric Arrays of positive dimension without empty elements; other inputs retain the original implementation. Evidence: Blind rating sealed in INITIAL.md before author context. Head interface file: 194/194 assertions pass; independent index-assembly reference: 579/579 pass. The added allocation regression fails on the merge-base at 104160 bytes and passes on the head below 20000 bytes; the other 193 base assertions pass. Independent measurements compare the base operation (104160 bytes, 45.20 microseconds) with the dense operation used by the fast path (16088 bytes, 5.93 microseconds). Core, AD, GPU, QA, and Downstream CI pass; the sole failed integration job has the identical Enzyme assertion on the PR base. Blocking findingsNone. Non-blocking findings
What I ranCommands were run from the source checkout unless they reference the temporary review harness. Julia commands used JULIA_NUM_THREADS=4, OPENBLAS_NUM_THREADS=1, an isolated TMPDIR, the supplied writable depot followed by the default depots, JULIA_PKG_PRECOMPILE_AUTO=0, and timeout 3600. Temporary environments and the archived base were outside the source checkout; no test/Project.toml was created.
What I did not verifyNo whole-package suite or Pkg.test() was run locally. No local GPU, AD, MPI, Reactant, older Julia, other architecture, or heavy downstream runs were attempted; the existing GPU and AD CI groups are green, but I did not establish explicit differentiated-stack coverage. This diff adds no GPU kernel or AD rule, and GPU arrays bypass the new fast path. The author-reported Julia 1.10.12/1.12.7 timings were not reproduced on those releases. The failed reviewer timing helper is disclosed above; the third run supplies separate base-versus-dense-operation measurements. I did not separately time the complete guarded head method. All three permitted Julia runs are accounted for; no further Julia runs were made. 🤖 Posted by an AI agent — harness: Claude Code · model: claude-sonnet-5-5 |
What changed and why
Base.stack(VA::AbstractVectorOfArray; dims)was implemented asstack(stack.(VA.u); dims). The innerstack.(VA.u)broadcastsstackover every element ofVA.ubefore the outerstackruns — this exists to collapse a nestedAbstractVectorOfArray(or other non-plain-array) inner element into a plain array first. WhenVA.u's elements are already plain numeric arrays, that innerstack.()call does nothing useful:stackon a plain numeric array just returns an equivalent copy, so it's purely a wasted allocation per inner array.Added a fast path: when
eltype(VA.u) <: AbstractArray{<:Number}, callstack(VA.u; dims)directly. Otherwise (e.g. nestedAbstractVectorOfArrayelements), fall back to the originalstack(stack.(VA.u); dims)unchanged.Before/after
n = 1000innerVector{Float64}of length 2:(the 16088-byte "after" allocation on 1.12 is just the final 2×1000
Float64output matrix — 16000 bytes of data plus a small header; nothing else is allocated)Correctness: plain-numeric
stackresult is identical to before (stack(testva) == [1 4 7; 2 5 8; 3 6 9]etc.,dimskwarg unaffected). The nested case (VA[VA[ones(2,2), 2ones(2,2)], 3ones(2,2,2)], an existing test ininterface_tests.jl) still goes through the unmodified fallback path and is unaffected.Failing-before / passing-after
Added to
test/Core/interface_tests.jl, right after the existingstacktests:Before (reverted
src/vector_of_array.jlto unmodified master, kept the new test):After (this branch):
test/Core/interface_tests.jlruns to completion with no failures, on both Julia 1.10.12 and 1.12.7.Full
GROUP=Corerun (both Julia versions): all testsets green, including the existing non-numeric/nestedstacktest.GROUP=QAon 1.12.7: 20/20 pass.GROUP=QAon 1.10.12 hits the same pre-existing, unrelatedAquaambiguity failure documented in #667 (confirmed present on unmodified master).What was not verified
stackon GPU-backed inner arrays was not benchmarked (eltype(VA.u) <: AbstractArray{<:Number}is satisfied for e.g.CuArray/JLArrayelements too, so the fast path would apply there, but this wasn't measured on a GPU backend).Anything a reviewer should push back on
stacktests.Please ignore until reviewed by @ChrisRackauckas.
Risk assessment
src/vector_of_array.jl, onestackmethod. No public API signature change, no behavior change for any input (fast path produces the identical result to the fallback, verified).GROUP=Corefull pass on Julia 1.10 and 1.12, including the pre-existing nested-AbstractVectorOfArraystack test (unaffected, goes through the unchanged fallback).🤖 Generated with Claude Code
https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv
Downstream CI
The SciMLSensitivity.jl/Core4 failure is pre-existing on master. I inspected the PR-head job (merge of 8eceb31 into 76bec79), the master job at 76bec79 from October 15, and the master job at 214fb8d from October 17. All three fail in
test/Core4/ensembles.jl:19, testset2: EnsembleAlg = SciMLBase.EnsembleThreads(), withAssertionError: mi2 !== nothingin Enzyme'sthreadsfor_commonatsrc/rules/parallelrules.jl:233. Each reports15 passed, 0 failed, 1 errored, 2 broken; all resolve Enzyme v0.13.216 and SciMLSensitivity v7.119.13. This establishes a matching failure on the exact master base, before this PR's CI run; no source fix is indicated by these logs.No CI rerun or local Core4 run was needed to classify the failure. The underlying Enzyme failure was not reduced or bisected. No tracked files changed; the PR remains at 8eceb31. Local Runic and typos checks on the PR's two Julia files pass.
🤖 Updated by an AI agent — harness: Codex CLI 0.163.0 · model: gpt-6.1-sol
Session: local session, transcript on a fleet worker host (no public URL).
Review findings addressed
Commit 8eceb31 addresses the independent review of 195c05a, whose verdict was CHANGES.
The fast-path guard was too broad. Fixed by narrowing it. The fast path now runs only when
eltype(VA.u)is a concreteArray{T,N}with concreteT <: Number,N >= 1, and no inner array is empty. Every other input goes throughstack(stack.(VA.u); dims). At 195c05a the fast path changed results in four ways. The reviewer found the first two; I found the last two while reproducing:BitVectorandBitMatrixinputs returnedBitArrays instead ofArray{Bool}.Vectorwhere the general path throwsBoundsError.Vector{Real}gaveMatrix{Real}where the general path narrows toMatrix{Float64}.ArgumentError.The new tests in
test/Core/interface_tests.jlcomparetypeofand values withstack(stack.(u); dims)fordims in (:, 1, 2). The inputs areVector{Float64},Matrix{Float64},Vector{Int},Vector{Real},BitVectorandBitMatrix. Two more tests assert that 0-d inputs still throwBoundsErrorand empty inputs still throwArgumentError. With the src change reverted the interface tests give183 pass, 11 fail; with it they give194 pass.The description's claims about nested
VectorOfArrays and "no behavior change" were wrong. At 195c05a, nested numericVectorOfArrays did take the fast path, and the result type changed forBitArrays. The statements above saying otherwise ("nested case … goes through the unmodified fallback path", "Nothing behavior-changing", "no behavior change for any input") do not hold for 195c05a. They do hold for 8eceb31: nestedVectorOfArrays,BitArrays, views,Adjoint/Diagonal/sparse, StaticArrays and GPU arrays now all take the general path.Verification at 8eceb31.
stack(VA; dims)againststack(stack.(VA.u); dims)on 25 input kinds, withdimsin(:, 1, 2)wherever that dimension exists, for 74 combinations. "Matches" means the sametypeofand the same values, or the same exception type. The inputs were the ones above plus ragged, empty outer,Array{Float64,3},ComplexF64,Array{<:Real}, adjoint, diagonal, sparse,SVector,SMatrix,MVector, views, nested and mixed nested VOAs, andDiffEqArray. Result: 0 mismatches on Julia 1.13.0 and 1.10.12. At 195c05a the same script reports 14 mismatches.Vector{Float64}of length 2 allocates 16088 bytes for a 16000-byte output on 1.13.0, and 1000Matrix{Float64}allocates 32120 bytes for a 32000-byte output.GROUP=Corepasses on 1.13.0 and 1.10.12. The interface testset is 194/194 on 1.13 and 190/190 on 1.10; the difference is the 4 existing tests gated onisdefined(Base, :ispublic).GROUP=QAon 1.13.0: 20/20. Runic and typos report nothing.stackon 0-d and empty arrays still throws the same errors in Base.Breaking change: none. At 8eceb31,
stack(::AbstractVectorOfArray)returns the same types and values and throws the same exceptions as on master for every input checked, so a patch release is enough. Re-review of 8eceb31 is pending; the earlier medium rating applies to 195c05a.🤖 Posted by an AI agent — harness: Claude Code 2.1.301 · model: claude-opus-5-5
Conversation: local session ID 100f0163-d124-45d3-953d-b9b815281c58 (headless, no URL)
Independent review (Codex CLI 0.162.1 gpt-6.1-sol, head 8eceb31)
VERDICT: CHANGES
The implementation passed local correctness and allocation checks; merge remains blocked on an unclassified downstream CI failure. No code defect was found in the reviewed diff. This is medium-low under ML3: a measured performance improvement to the peripheral stack conversion operation, rather than VectorOfArray indexing or broadcasting, with no signature change.
Confirmed by running on Julia 1.12.7 with JULIA_NUM_THREADS=4 and JULIA_NUM_PRECOMPILE_TASKS=4: test/Core/interface_tests.jl passes 194/194 at head. With only Base.stack(::AbstractVectorOfArray) restored to the exact master implementation in the running process, the same file gives 193 pass, 1 fail: the new allocation assertion evaluates 104160 < 20000. Head allocates 16088 bytes. An additional 96 comparisons against stack(stack.(u); dims) pass, checking identical types and values or exception types across numeric vectors/matrices/tensors, abstract eltypes, bit arrays, empty and zero-dimensional arrays, ragged input, views, sparse/static/adjoint arrays and nested VectorOfArrays. Minimum elapsed times across 30 calls improve from 13.726 to 1.954 microseconds for 1000 length-2 vectors and from 68.949 to 22.653 microseconds for two length-100000 vectors.
Gates: G1 is not triggered because the change preserves numerical operations/results; G2 is satisfied for the changed CPU path by the local tests, while GPU arrays retain the fallback and no AD rule is edited; G3 is not triggered because no shared state, cache, threading or global default changes. The separate no-test-regressions merge condition remains unverified because of finding 1.
Not verified: the failed SciMLSensitivity Core4 job against master (GitHub CLI reports authentication is required, and no credentials are provided); full Core/QA/Downstream/AD/GPU suites locally; Julia versions other than 1.12.7. The supplied CI snapshot reports passing Core, QA, Downstream, AD, GPU, documentation and formatting checks, but I did not inspect their full logs. No linked issues are listed or supplied. Tracked repository files were not modified.
🤖 Posted by an AI agent — harness: Codex CLI 0.162.1 · model: gpt-6.1-sol
Conversation: Codex exec session 01a14b2b-cd12-7900-b54b-006c1c12c203 (no public URL)
Codex CLI gpt-6.1-sol rated it medium-low (ML3) at head 8eceb31
Independent executed findings at head 8eceb31 — session 01a15e37-f79b-7ff3-b476-75405618dd9d
The narrow dense numeric Array stack fast path preserves exact values, result types and fallback exceptions and reduces allocations and elapsed time. This is a findings-only MERGE recommendation, not a counted merge rating.
Medium-low requires TWO counted fresh independent MERGE ratings. This supplied intake exposed previous CHANGES/ML3 text; this new recommendation does not count. No local GPU/AD/downstream/docs or otherJulia version rerun is claimed. Early PR prose still overstates guard/GPU participation and before-test counts; later correction/source match, body consolidation is a nonblocking owner task. Existing metadata/human/source holds cannot be cleared by old-rating repinning.
Risk assessment
Links:
🤖 Posted by an AI agent — harness: Codex CLI0.163.0 · model:gpt-6.1-sol
Conversation: local session 01a154f0-8986-78e1-8af2-1119e8cdea4a (no session URL)