Skip to content

Add @inline to unpack_args(i, ::Tuple{Any}) in ArrayPartition broadcast - #666

Merged
ChrisRackauckas merged 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:perf/ap-unpack-args-inline
Oct 10, 2026
Merged

ChrisRackauckas merged 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:perf/ap-unpack-args-inline

Conversation

@ChrisRackauckas-Claude

@ChrisRackauckas-Claude ChrisRackauckas-Claude commented Oct 8, 2026 •

Copy link
Copy Markdown
Member

What and why

unpack_args(i, args::Tuple{Any}) is the base case of the tuple-recursion
that unpacks each ArrayPartition's i-th partition for broadcasting
(src/array_partition.jl:575). It was missing @inline. For a homogeneous
partition the compiler can still fold the one-method recursion away, but as
soon as any two partitions in the broadcast have different element types
(e.g. Float32 vs Float64), the call is not inlined and becomes a runtime
dispatch on every broadcast.

This PR adds @inline to that one method. No behavior change.

Before/after (reused from the audit's scratch/a1_bcfix.jl, re-measured on this branch)

AP3mixed/AP5mixed have mixed Float32/Float64 partitions; AP2 is
homogeneous (unaffected, included as a control).

Julia 1.12.4 (@benchmark, 3000 samples), a .= b + c .* d style in-place broadcasts:

case before after allocs before → after
AP2 (homog, control) 92.5 ns 92.6 ns 0 → 0
AP3mixed b+c*d 5170.0 ns 100.1 ns 21 → 0
AP3mixed 4arg 156.1 ns 149.7 ns 0 → 0
AP3mixed nested 7587.5 ns 205.7 ns 33 → 0
AP5mixed b+c*d 8670.0 ns 203.6 ns 35 → 0
AP5mixed nested 12520.0 ns 486.7 ns 55 → 0

Julia 1.10.12, same workload:

case before after
AP2 (homog, control) 40.4 ns 40.1 ns
AP3mixed b+c*d 5189.8 ns 59.3 ns
AP5mixed b+c*d 8606.7 ns 120.5 ns

Results are bitwise identical before/after (same arithmetic, only inlining changes).

JET @report_opt on the mixed-eltype in-place broadcast (target_modules=(RecursiveArrayTools,)):
runtime-dispatch reports go from 3 to 2 on this branch alone. The 2 remaining
reports are at the Broadcast.Broadcasted(...) constructor calls inside
unpack, not at unpack_args; they are addressed by the separate
Broadcast.instantiate PR (candidate 2/3 in the audit), not by this change.

Failing-before / passing-after

Added test/Core/partitions_test.jl::"Mixed-eltype in-place broadcast does not allocate",
which asserts @allocated f!(a,b,c,d) == 0 for a 3-partition mixed Float32/Float64
ArrayPartition.

Before this fix (git stash the src/array_partition.jl change):

allocated after warmup: 672

After:

allocated after warmup: 0

Test results

GROUP=Core: all 12 Core testsets pass, including the new test (Partitions Tests: 129 Pass, 0 Fail).
GROUP=QA: QA: 20 Pass, 0 Fail.

Both re-run on Julia 1.12.4 and 1.10.12.

What was not verified

  • Downstream, GPU, AD, and NoPre (JET) test groups were not run (out of scope for
    a one-line @inline change with no API surface).
  • The Tsit5 ArrayPartition ODE solve benchmark from scratch/a1_ode.jl was not run
    for this PR specifically (no mixed-eltype RHS in that script); it is included in
    PR 2, which is the change that affects that workload.

What a reviewer should push back on

Whether a one-line @inline warrants its own PR versus being folded into the
Broadcast.instantiate PR (candidate 2/3). They were kept separate because they
fix independent problems (dispatch at the tuple base case vs. lost SIMD from an
uninstantiated Broadcasted) and @inline alone does not require re-measuring
the homogeneity-check path.

Please ignore until reviewed by @ChrisRackauckas.

Risk assessment

  • Risk: low
  • Blast radius: src/array_partition.jl, one method, adds @inline only; no
    signature, semantics, or public API change. Affects any ArrayPartition broadcast
    with mixed partition eltypes.
  • Evidence: before/after allocation and timing numbers above, measured on Julia
    1.12.4 and 1.10.12; Core and QA test groups pass.
  • Independent review: pending
  • Merge: auto-merge candidate, pending independent review

🤖 Generated with Claude Code

https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv

Independent review: Devin Fusion (fusion-claude-opus-5-5-high-sidekick-swe-2-medium) rated it low, verdict MERGE: #666 (comment)

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
@ChrisRackauckas

Copy link
Copy Markdown
Member

🤖 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 low.

Full review

VERDICT: MERGE
RISK: low

PR: #666. The diff is one line, adding @inline to unpack_args(i, ::Tuple{Any}) at src/array_partition.jl:575, plus an 11-line allocation test. It doesn't change any signature, semantics, dependencies or public API. The PR links no issue. The repo root has no AGENTS.md or CLAUDE.md, so I applied the global CLAUDE.md rules.

Blocking findings

None.

Non-blocking findings

  1. The test comment narrates history. Confirmed by reading. At test/Core/partitions_test.jl:421-423 the comment reads "unpack_args(i, ::Tuple{Any}) was missing @inline, forcing a runtime dispatch...". That describes the old code, which CLAUDE.md's "describe what exists, never what it replaced" rule forbids. Change it to a present-tense statement, for example "Mixed partition eltypes take the ntuple path in copyto!; the unpack/unpack_args chain must fully inline there", or delete it. The commit message is the right place for the history.
  2. PR Instantiate per-partition Broadcasted in ArrayPartition copy/copyto!, use type-level homogeneity check #670 overlaps this one. Confirmed with gh pr diff 670. Instantiate per-partition Broadcasted in ArrayPartition copy/copyto!, use type-level homogeneity check #670 is by the same author and contains this exact @inline hunk and the exact same testset, followed by more tests. Merge Add @inline to unpack_args(i, ::Tuple{Any}) in ArrayPartition broadcast #666 first and then rebase Instantiate per-partition Broadcasted in ArrayPartition copy/copyto!, use type-level homogeneity check #670, or close Add @inline to unpack_args(i, ::Tuple{Any}) in ArrayPartition broadcast #666 in favour of Instantiate per-partition Broadcasted in ArrayPartition copy/copyto!, use type-level homogeneity check #670. If both merge as they stand, the identical append at the end of partitions_test.jl will conflict or duplicate.
  3. The NamedArrayPartition path is untouched. Suspected from reading. src/named_array_partition.jl:222-224 loops over a runtime i with no ntuple path, so a mixed-eltype NamedArrayPartition broadcast is presumably still type-unstable. This PR doesn't claim to fix that, and sibling PR Instantiate per-partition Broadcasted in NamedArrayPartition copyto! #677 works on that function. It's out of scope here and I'm noting it only for completeness.
  4. Part of the PR body can't be checked. It cites audit scripts (scratch/a1_bcfix.jl, scratch/a1_ode.jl) that aren't in the repo. The JET claim ("3 → 2 runtime-dispatch reports") has no command output attached, and I didn't rerun it (see below). The allocation, timing and test-count claims did reproduce.
  5. One CI check is red: SciMLSensitivity.jl/Core1. The failure is a Mooncake MethodError: no constructors have been defined for Any at concrete_solve_derivatives.jl:451. The same job also fails on the unrelated PRs Fix two silent memory-safety bugs in recursivecopy!/copyat_or_push! #667, Add O(1) fast path to AbstractVectorOfArray getindex #668 and Instantiate per-partition Broadcasted in ArrayPartition copy/copyto!, use type-level homogeneity check #670, so it is a pre-existing downstream failure and not caused by this PR. All other checks pass, including Core on 1, lts, x86 and pre, plus QA and GPU.

What I ran

All runs used copies of origin/master and of the PR head, made with git archive into scratch/{master,pr}. Each copy was dev'd into its own env, on Julia 1.10.12 and 1.12.7. The two src/ trees differ only at line 575.

  • Probe scratch/probe.jl. Confirmed by running. It measures @allocated for four in-place broadcasts (b+c*d, sin(b), b+c, and a longer b*2+c*d-b/c) on four partition layouts: mixed3 (F64/F32/F64), homog2, mixed5 and mixed F32/F64. It also checks numerical correctness against a scalar reference and checks the out-of-place type.

    • On master (1.10 and 1.12), b+c*d allocates 672 B for mixed3 and mixed-F32 and 1120 B for mixed5. The longer expression allocates 1728 B or 2880 B. Homogeneous layouts allocate 0.
    • On the PR (1.10 and 1.12), every case allocates 0. Out-of-place b .+ c .* d allocations also drop, for example from 1024 B to 384 B on 1.12 for mixed3.
    • Results match the reference (isapprox, rtol 1e-6) on both master and the PR, and the out-of-place result keeps the input type.
  • Fail-before / pass-after. Confirmed by running. I ran the PR's test/Core/partitions_test.jl against master's src on 1.12: Evaluated: 672 == 0, giving 128 passed, 1 failed. Against the PR's src it gives 129 / 129 pass. This matches the PR body ("672", "129 Pass").

  • Timing, scratch/time.jl, 1.12, mixed3 b+c*d. Confirmed by running. The figure is the minimum ns per call:

    • master: 5107 ns at default settings, 4207 ns with --check-bounds=yes
    • PR: 98.5 ns at default settings, 238.7 ns with --check-bounds=yes

    The PR still allocates 0 under --check-bounds=yes, so the test isn't fragile under CI's check-bounds setting. These numbers agree with the PR body's 5170 → 100 ns.

  • Test suites on the PR copy. Confirmed by running.

    • GROUP=Core Pkg.test() on 1.12: all 12 testsets pass, with Partitions Tests at 129/129.
    • GROUP=QA on 1.12: 20/20 pass.
    • GROUP=Core on 1.10: all pass, with Partitions Tests at 129/129.
    • The 2 Broken results in "VecOfArr Indexing Tests" are pre-existing. The diff adds no @test_broken or @test_skip.
  • Assertion strength. Checked by reading and running. The test uses ArrayPartition(rand(10), rand(Float32, 20), rand(5)), which fails the all(x isa typeof(first(dest.x)) ...) check at src/array_partition.jl:514. It therefore takes the ntuple branch, which is the code path the change affects. The test warms up before measuring, so the == 0 assertion is exact and can fail, as the master run showed.

  • I ran git status in the checkout and it is clean. No Julia processes I started are still running.

What I did not verify

  • The JET @report_opt claim (3 → 2 runtime-dispatch reports).
  • The "bitwise identical" claim. I checked only isapprox against a reference. @inline doesn't change floating-point semantics in Julia, so I expect it holds.
  • The PR's 1.10 timing table, and its AP5 and "nested" timings. I measured timing only for mixed3 b+c*d on 1.12.
  • The Downstream, GPU, AD and NoPre test groups locally. I relied on the PR's CI results for those.

🤖 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)

@ChrisRackauckas
ChrisRackauckas marked this pull request as ready for review October 10, 2026 18:13
@ChrisRackauckas
ChrisRackauckas merged commit 1826933 into SciML:master Oct 10, 2026
45 of 46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants