Repository navigation
Instantiate per-partition Broadcasted in ArrayPartition copy/copyto!, use type-level homogeneity check - #670
Conversation
The tuple-recursion base case was missing @inline, forcing a runtime dispatch whenever an ArrayPartition broadcast mixes partition eltypes (e.g. Float32 and Float64 partitions). Homogeneous partitions were unaffected since the compiler could already fold the single-method recursion. 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 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv
… use type-level homogeneity check
unpack() drops a Broadcasted's axes to make recomputing them cheaper,
but the per-partition copyto!/copy never recomputed them before handing
the slice to the partition's own copyto!. On Julia 1.12 the resulting
Broadcasted{Style,Nothing,...} takes a slower, non-SIMD path in the
partition's own broadcast machinery. Wrap each per-partition unpack in
Broadcast.instantiate before copying/copying into it.
Also replace the runtime `all(x isa typeof(first(dest.x)) for x in dest.x)`
homogeneity check (which only looked at dest, allocated, and ran on every
call) with a type-level check over dest and every ArrayPartition argument
of the broadcast, so it constant-folds.
Builds on the @inline fix in the previous PR: without it, re-adding
Broadcast.instantiate on a mixed-partition-eltype broadcast makes the
existing unpack_args dispatch strictly worse (confirmed: AP3mixed
`b+c*d` 5203 -> 9390 ns, allocs 21 -> 42, with instantiate alone).
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
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv
_homog_ap(::ArrayPartition{T,S}) called fieldtype(S, 1) without guarding
fieldcount(S) == 0, so an in-place broadcast on ArrayPartition() threw
BoundsError(Tuple{}, 1) instead of succeeding as it does on master (the
old all(...) check was vacuously true on an empty iterator). Guard the
empty case so it still constant-folds to true.
Also trims a changelog-style comment per review feedback.
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
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv
Independent review (Devin CLI 3000.11.3, model fusion-claude-opus-5-5-high-sidekick-swe-2-medium): MERGE, risk medium. Full reviewVERDICT: MERGE Every Blocking findingsNone. Non-blocking findings
What I ranAll runs used
Differential probe (
Mixed-eltype in-place broadcasts still allocate 0 bytes, and OOP return types still infer concretely. Benchmarks (
What I did not verify
🤖 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 |
Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com> Co-Authored-By: Devin <noreply@cognition.ai> Agent-Harness: Devin CLI 3000.11.3 (conflict resolution), Claude Code 2.1.285 (orchestration) Agent-Model: fusion-claude-opus-5-5-high-sidekick-swe-2-medium Agent-Session: local Claude session 3c311569-dda6-4687-bc9f-65febb212c33 (fleet master); Devin log /Users/chrisrackauckas/sandbox/master/fusion-fixes/SciML_RecursiveArrayTools.jl-670/log.txt # Conflicts: # test/Core/partitions_test.jl
Merged the default branch into this PR to resolve conflicts (head 06ce2e1). Re-review the resolution before merging. Conflict resolution reportSTATUS: RESOLVED Merge commit: 06ce2e1 "Merge master into perf/ap-broadcast-instantiate-homog" (prepared message and trailers kept, no follow-up commits). Conflicting files
Files that auto-merged with no conflict (master changes kept as-is): No conflict markers remain in Tests (Julia 1.12.7)
Not run: the full 🤖 Posted by an AI agent — harness: Devin CLI 3000.11.3 (merge), Claude Code 2.1.285 (posting) · model: fusion-claude-opus-5-5-high-sidekick-swe-2-medium |
Changes since round 1 review
A Cursor review (round 1) found a blocking bug:
_homog_ap(::ArrayPartition{T,S})called
fieldtype(S, 1)without guardingfieldcount(S) == 0, so an in-placebroadcast on the 0-partition
ArrayPartition()threwBoundsError(Tuple{}, 1)where master succeeds. Fixed by guarding the empty case
(
fieldcount(S) == 0 || S <: NTuple{...}), confirmed it still constant-foldsto
return truefor the empty case viacode_typed, added a Core test(
"Broadcast on 0-partition ArrayPartition") covering in-place broadcast,copy, OOP broadcast, andcopyto!(::ArrayPartition, ::ArrayPartition)onArrayPartition(), and confirmed it fails on the pre-fix code(
BoundsError) and passes after. Also corrected the AP2 row below, whichhad a copy-paste error (both columns showed the same number; the real
"before" is PR1-only ~92.5 ns, "after" is ~41.0 ns on this run — not
"unchanged" as originally written, per the review's non-blocking finding),
and trimmed a changelog-style comment on
_homog_apto a plain "why" note.Numbers, bitwise identity, and the earlier regression test are unchanged and
re-verified below. Re-ran Runic, typos, Core and QA on Julia 1.12.4 and
1.10.12: QA on 1.10.12 fails with 1 Aqua method-ambiguity
(
similar(::AbstractVectorOfArray, ...)vs.Base.similar), confirmedpre-existing on unmodified
origin/masterunder Julia 1.10 (same failure,same ambiguity, via
Test.detect_ambiguitieson a clean worktree) andunrelated to
vector_of_array.jl, which this PR does not touch; 1.12.4 QAis unaffected (
20 Pass) since this ambiguity apparently doesn't triggerunder 1.12's
similarsignature.Stacked on #666
This PR's branch (
perf/ap-broadcast-instantiate-homog) is branched offperf/ap-unpack-args-inline, the branch behind#666, so the diff below
includes both commits: #666's
@inlinefix and this PR's own commit. GitHubdoes not let a cross-fork PR's base be a branch that only exists on the fork,
so this is opened against
masterrather than against #666's branch directly-- please review only the second commit (
Instantiate per-partition Broadcasted...); the first is #666.This PR needs #666: applying only this PR's change (instantiate + type-level
homogeneity check) without the
@inlinefix makes the mixed-partition-eltypecase strictly worse than master, because
Broadcast.instantiateadds workon a path that still runtime-dispatches. Measured directly (Julia 1.12.4, 3000
samples), instantiate-only vs. master:
b+c*db+c*dWith PR1 applied first (this branch), the same cases improve (see below).
What and why
unpack(bc, i)drops aBroadcasted's axes "because it is easier torecompute" (
src/array_partition.jl:558comment), but the per-partitioncopyto!/copyinArrayPartition's own broadcast machinery(
src/array_partition.jl:503,516,521before this change) never recomputedthem — it handed each partition a
Broadcasted{Style, Nothing, ...}. OnJulia 1.12 that forces the partition's own broadcast implementation onto a
slower, non-SIMD path. This PR wraps each per-partition
unpackinBroadcast.instantiatebeforecopy/copyto!.It also replaces the runtime homogeneity check
all(x isa typeof(first(dest.x)) for x in dest.x)— which only inspectsdest(ignoring the broadcast's otherArrayPartitionarguments), andallocates an iterator on every call — with a type-level check
(
S <: NTuple{fieldcount(S), fieldtype(S,1)}) overdestand everyArrayPartitionargument of theBroadcastedtree, so it constant-foldsinstead of running at runtime.
Before/after (reused from the audit's
scratch/a1_bcfix2.jlandscratch/a1_ode.jl)Julia 1.12.4,
@benchmark3000 samples, in-place broadcast on this branch(PR1 + PR2) vs. PR1 alone:
b+c*d4argb+c*db+c*dJulia 1.10.12, same workload:
b+c*db+c*dTsit5
ArrayPartitionIIP ODE solve (scratch/a1_ode.jl,OrdinaryDiffEqTsit5),Julia 1.12.4:
step!(warmed)step!(warmed)solvesolveJulia 1.10.12:
step!step!Results are bitwise identical before/after. Checked with a seeded script
(
correctness2.jl) comparingcollect(a)for every broadcast case above and(sol.t, collect(sol.u[end]))for both Tsit5 ODE workloads, serialized fromthe unpatched tree and compared against this branch:
Failing-before / passing-after
Neither
@allocatednorJET.@report_optdiscriminates this fix: allocationsand runtime-dispatch-report counts for the homogeneous large-
ArrayPartitioncase are 0 both before and after (verified directly) — the regression this PR
fixes is a lost-SIMD timing effect, not an allocation or dispatch effect, so
those techniques used for PR1 don't apply here.
Instead, added a deterministic structural test,
test/Core/partitions_test.jl::"ArrayPartition broadcast instantiates partition Broadcasted", using a custom partition array type that records whether theBroadcastedit receives in its owncopyto!hasaxes !== nothing(i.e.was instantiated). This directly pins the mechanism (not a timing threshold,
so it isn't flaky in CI):
Before this fix (
git stashthesrc/array_partition.jlchange):After:
0-partition
ArrayPartition(the bug found in round-1 review): addedtest/Core/partitions_test.jl::"Broadcast on 0-partition ArrayPartition",covering in-place broadcast,
copy, OOP broadcast, andcopyto!(::ArrayPartition, ::ArrayPartition)onArrayPartition().Before this fix (
git stashthe_homog_apone-line change):After:
Also confirmed with
code_typedthat_homog_ap(ArrayPartition())stillconstant-folds to
return true(not a runtime branch), and spot-checked_homog_apon a 1-partition and a mixed-eltypeArrayPartition(trueandfalserespectively, as expected).Test results
GROUP=Core: all 12 Core testsets pass (Partitions Tests: 135 Pass, 0 Fail,includes this PR's two tests and PR1's).
GROUP=QA:QA: 20 Pass, 0 Fail— required adding:instantiatetotest/QA/qa.jl'sall_qualified_accesses_are_publicignore list next to theother legitimately-qualified
Base.Broadcastnames already there(
:Broadcasted,:flatten,:result_style, etc.), sinceBroadcast.instantiateis not itself declared public by Base.
Both re-run on Julia 1.12.4 and 1.10.12.
What was not verified
AllocCheck.jl-level static allocation checking was not used;@allocatedand
JET.@report_optwere, and neither showed a difference (expected, seeabove).
copy(vs.copyto!) on anArrayPartitionbroadcast with mixed partitiontypes was exercised by the existing test suite and the correctness script,
but no dedicated microbenchmark of mixed-type
copywas run (only theOOProws in the in-place-broadcast benchmark table above, which do exercisecopy).What a reviewer should push back on
The 0-partition case is now covered (see above); worth independently
re-checking that no other empty-
ArrayPartitionpath regressed beyond thefour covered by the new test (
copyto!(dest::AbstractArray, A::ArrayPartition)and
getindex/setindex!on an emptyArrayPartitiondon't go through_homog_apat all, so they were never at risk, but a second pair of eyes onthat claim is welcome).
Please ignore until reviewed by @ChrisRackauckas.
Risk assessment
src/array_partition.jl,Base.copy/Base.copyto!forArrayPartitionbroadcasts and the internal homogeneity check; no signatureor public API change. Affects every
ArrayPartitionbroadcast (homogeneousand mixed).
correctness check across 10 workloads including a Tsit5 ODE solve; Core and
QA test groups pass; a deterministic structural regression test.
ArrayPartitionbroadcast dispatch, so it should not auto-merge even if CIis green.
🤖 Generated with Claude Code
https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv
Risk assessment (independent review, Grok Build grok-4.7, reviewed head 44dfcc7)
Risk: medium
Blast radius: Every
ArrayPartitionbroadcast, in place and copying, now callsBroadcast.instantiateper partition and decides the homogeneous fast path with_homog_apover the destination and everyArrayPartitionin the broadcast tree. OrdinaryDiffEq steppers broadcastArrayPartitions on this path. Partition values and result types matched an independent per-partition broadcast on the cases below. EmptyTuple{}partitions still broadcast.copyto!still returnsdest.Project.tomlstays at 4.5.4. No new dependency. FastBroadcast's extension special-casesAbstractVectorOfArrayonly, so thesecopy/copyto!methods are the Base path.Evidence: Head
44dfcc7fb3459f1130128c06a677a67c38c47d56, rechecked unchanged. Reviewable diff against master issrc/array_partition.jlandtest/Core/partitions_test.jl(+67/−5). https://github.com/SciML/RecursiveArrayTools.jl/pull/670/files . The@inlineonunpack_argsis already on master via Add @inline to unpack_args(i, ::Tuple{Any}) in ArrayPartition broadcast #666 (726bfc39); it is not in this diff.test/QA/qa.jlalready lists:instantiateinall_qualified_accesses_are_public; this diff does not touch it. The PR body still says the branch is stacked on Add @inline to unpack_args(i, ::Tuple{Any}) in ArrayPartition broadcast #666 and that reviewers should look only at the second commit. That text is stale. The conflict-resolution merge06ce2e19is Devin CLI; the behavior change is the Claude Code commits.Julia 1.13.0, master and this head, oracle
a.x[i] .+ b.x[i](and.*) againsta .+ band in-place broadcast: homogeneous+, homogeneous+/*, scalar, in-place, mixedFloat64/Float32, mixed in-place, andSVectorpartitions all matched values, partition values, and types. EmptyArrayPartitionin-place,copy, and out-of-place broadcast succeeded on both. Allocations of those small broadcasts were 0 on both.A recording array whose
copyto!storesbc.axes !== nothing: master in-place andcopyrecorded(false, false)with data[2.0, 3.0]/[2.0, 3.0, 4.0]and copy[3.0, 4.0]/[3.0, 4.0, 5.0]. This head recorded(true, true)with the same data.code_typed(_homog_ap)on this head constant-folds empty, one-part, and homogeneous two-part partitions toreturn true, and a mixed partition toreturn false. The emptyfieldtype(Tuple{}, 1)inference hazard is gone.Bench, 2000 iterations after warmup,
dest .= b .+ c .* d, Julia 1.13.0: homogeneous lengths 1000 and 2000, master 3200.8 ns/call, this head 1309.7 ns/call, 0 allocations both. Mixed length-4 partition: master 99.7 ns/call, this head 104.3 ns/call, inside noise. The body's mixed-partition speedup was not reproduced; the homogeneous speedup is in the same direction as the body's table. Fulltest/Core/partitions_test.jlwas not run. Offset-axis partitions and a downstream ODE were not run.CI: 22/22 pending. Tests: https://github.com/SciML/RecursiveArrayTools.jl/actions/runs/38872109721 . Runic: https://github.com/SciML/RecursiveArrayTools.jl/actions/runs/38872109735 . No failed job. Master's SciMLSensitivity Core4 failure on https://github.com/SciML/RecursiveArrayTools.jl/actions/runs/38819764786 is pre-existing if it reappears. No test skip,
@test_broken, loosened tolerance, allow-failure change, or new dependency.Independent review: Grok Build grok-4.7 rated it medium, high confidence: partition-wise results match an independent broadcast and the instantiate flag flips from false to true, on the broadcast path ODE steppers use.
Merge: needs human review — the results checked out, and the change still rewrites the hot
ArrayPartitionbroadcast path. Not an auto-merge candidate. The author's own risk note says the same.🤖 Posted by an AI agent — harness: Grok Build · model: grok-4.7 (independent reviewer; appended by Claude Code 2.1.301 · claude-opus-5-5)
Conversation: https://claude.ai/code/session_01VZjwf7Ux9YtTzwHGHMqnsq
Risk assessment (independent review, Codex gpt-6.1-sol, head 44dfcc7)
git diff origin/HEAD...HEAD(two files, +67/-5), verified HEAD, and used scratch-only archives of head and origin/HEAD (214fb8d). No linked issue appears in the PR metadata; also read the linked prerequisite PR Add @inline to unpack_args(i, ::Tuple{Any}) in ArrayPartition broadcast #666. All local runs used Julia 1.12.4, JULIA_NUM_THREADS=4, the requested depot overlay, sequential Julia invocations, and timeout 3600. With S denoting this run's scratch directory, commands and observed results were:timeout 3600 julia --startup-file=no --compiled-modules=existing --project="$S/env" "$S/targeted.jl" "$S/head": Partitions Tests 135/135, Partitions and StaticArrays Tests 6/6.regression.jl: head 2/2 structural tests and 4/4 empty-partition tests pass. With--project="$S/env-base", the identical structural test has 2 failures, confirming the intended test discriminates; execution stops before the empty-partition testset on base.nested-regression.jl: base 2/2 pass, head 2 errors, coveringa .+ band directcopyto!(dest, Broadcast.broadcasted(+, a, b)). Logs and the standalone test are in scratch.oracle.jlon each environment: 14/14 pass against independent leaf-wise arithmetic for homogeneous, mixed Float32/Float64, StaticArrays, empty vectors, and matrix partitions.cmp "$S/head-results.bin" "$S/base-results.bin"exits 0. Warmed median timings over 15 batches of 10,000 calls: homogeneous 1000+2000 elements 6999.95 ns base → 1277.10 ns head; mixed three-partition case 58.80 ns → 43.60 ns; both have 0 allocations.AssertionError: mi2 !== nothingin Enzymeparallelrules.jl:233,ensembles.jl:19, EnsembleThreads. This matching failure signature predates the PR; different resolved environments prevent claiming a causal bisect. The separate scratch investigation records the comparison.git diff --check origin/HEAD...HEADpasses, and the original checkout remains clean.🤖 Posted by an AI agent — harness: Codex CLI 0.162.1 · model: gpt-6.1-sol
Conversation: local headless review run (no public session URL); transcript kept by the fleet master.