Avoid reshape allocation in extract_jacobian! for Matrix results#797
Conversation
src/jacobian.jl
Outdated
| function extract_jacobian!(::Type{T}, result::Matrix, ydual::AbstractVector, n) where {T} | ||
| for j in 1:n | ||
| for i in eachindex(ydual) | ||
| result[i, j] = partials(T, ydual[i], j) |
There was a problem hiding this comment.
There's no guarantee that result has the correct dimensions? Additionally, i might not be a valid index for result.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #797 +/- ##
==========================================
- Coverage 90.75% 90.69% -0.06%
==========================================
Files 11 11
Lines 1071 1075 +4
==========================================
+ Hits 972 975 +3
- Misses 99 100 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
test/AllocationsTest.jl
Outdated
| T = ForwardDiff.Tag{Nothing, Float64} | ||
| N = 3 | ||
| ydual = ForwardDiff.Dual{T}.( | ||
| [1.0, 2.0, 3.0], | ||
| [ForwardDiff.Partials((1.0, 2.0, 3.0)), | ||
| ForwardDiff.Partials((4.0, 5.0, 6.0)), | ||
| ForwardDiff.Partials((7.0, 8.0, 9.0))] | ||
| ) | ||
| result = zeros(3, 3) | ||
|
|
||
| allocs_extract!() = @allocated ForwardDiff.extract_jacobian!(T, result, ydual, N) |
There was a problem hiding this comment.
Move these definitions into the function body to avoid closing over these variables?
cb850f0 to
526aecd
Compare
`extract_jacobian!` called `reshape(result, length(ydual), n)` which allocates a 48-byte ReshapedArray wrapper. Under `--check-bounds=yes` (used by Pkg.test), this allocation cannot be elided by the compiler, causing 48 bytes per jacobian! call. For implicit ODE/SDE solvers that call jacobian! multiple times per step, this adds up (e.g. 144 bytes/step for SKenCarp with 3 NL solver iterations). Add `_maybe_reshape` that returns the array as-is when it already has the target shape, avoiding the wrapper allocation. Falls back to `reshape` when dimensions don't match. Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
526aecd to
84b9290
Compare
|
@devmotion this should be simpler? |
| if size(result) == (m, n) | ||
| return result | ||
| else | ||
| return reshape(result, m, n) | ||
| end |
There was a problem hiding this comment.
This is not type-stable in general?
From searching through the codebase, my impression is that in quite a few cases we call extract_jacobian! internally with a matrix of the correct dimensions constructed in the previous line. In these cases, we don't even need this check and can always operate with result.
AFAICT the only case where we might have to reshape is when users provide an output array.
Maybe we could reshape user-provided arrays to matrices higher up in the call stack, maybe even in the user-facing function directly, and then never reshape in this internal function and only accept matrices.
The reshaping of the user-provided arrays could eg be done unconditionally for non-Matrix input (and other types for which we don't know whether the function in this draft would be type-stable) and only for Matrix (and other known to be type-stable types) we would use the conditional reshaping.
Summary
extract_jacobian!callsreshape(result, length(ydual), n)which allocates a 48-byteReshapedArraywrapper on every call. Under--check-bounds=yes(whichPkg.test()always uses), this allocation cannot be elided by the compiler.For implicit ODE/SDE solvers that call
jacobian!multiple times per step via the NL solver, this adds up. For example,SKenCarpin StochasticDiffEq.jl callsnlsolve!3 times per step, each triggering a Jacobian computation, resulting in 3 × 48 = 144 bytes/step of unnecessary allocations.Fix: Add a specialized
extract_jacobian!method forMatrixresult withAbstractVectorydual that uses direct loop indexing instead ofreshape+broadcast. This is zero-alloc under--check-bounds=yesand produces identical results.MWE
Test plan
test/AllocationsTest.jl(passes under--check-bounds=yes)--check-bounds=yes)🤖 Generated with Claude Code