Skip to content

Stop the cache-less JVP from overwriting the caller's x - #231

Merged
ChrisRackauckas merged 1 commit into
JuliaDiff:masterfrom
ChrisRackauckas-Claude:fix/jvp-cacheless-aliasing
Aug 8, 2026
Merged

Stop the cache-less JVP from overwriting the caller's x#231
ChrisRackauckas merged 1 commit into
JuliaDiff:masterfrom
ChrisRackauckas-Claude:fix/jvp-cacheless-aliasing

Conversation

@ChrisRackauckas-Claude

Copy link
Copy Markdown

Please ignore until reviewed by @ChrisRackauckas.

What changed and why

The cache-less finite_difference_jvp! built its internal cache with the non-allocating JVPCache(x, fx, fdtype) constructor, so cache.x1 === x; the in-place perturbation @. x1 = x + ϵ*v then overwrote the caller's input array. With fdtype = Val(:central) and f_in supplied it was worse: the caller's f_in was also clobbered (it was handed over as the cache's fx1 scratch), and the returned JVP was half the correct value, because after x1 = x - ϵv the "second" evaluation x1 = x + ϵv landed back on x rather than x + ϵv.

The cache-less path now perturbs a copy of x, and treats f_in as read-only. No public API changes; the cached methods and the JVPCache constructors are untouched.

Reproducer (on master, 58ad147)

using FiniteDiff, LinearAlgebra
f!(fx, x) = (fx[1] = x[1]^2 + x[2]; fx[2] = x[1]*x[2]; nothing)
x0 = [1.0, 2.0]; v = [1.0, 0.5]
truth = [2x0[1] 1.0; x0[2] x0[1]] * v          # [2.5, 2.5]
for (lbl, args) in (("central, no f_in", (Val(:central),)),
                    ("central, f_in",    (Val(:central), f!(zeros(2), x0))),
                    ("forward, no f_in", (Val(:forward),)),
                    ("forward, f_in",    (Val(:forward), f!(zeros(2), x0))))
    x = copy(x0); jvp = zeros(2)
    FiniteDiff.finite_difference_jvp!(jvp, f!, x, v, args...)
    println(rpad(lbl,20), " jvp=", jvp, " relerr=", norm(jvp-truth)/norm(truth), " xchanged=", x != x0)
end

Before:

truth = [2.5, 2.5]
central, no f_in     jvp=[2.500000000003882, 2.499999999994715] relerr=1.8547363684788055e-12 xchanged=false
central, f_in        jvp=[1.2499939445336223, 1.249996972263198] relerr=0.5000018166410026 xchanged=false
forward, no f_in     jvp=[2.5000000298023224, 2.500000014901161] relerr=9.424321830774484e-9 xchanged=true
forward, f_in        jvp=[2.5000000298023224, 2.500000014901161] relerr=9.424321830774484e-9 xchanged=true

After:

truth = [2.5, 2.5]
central, no f_in     jvp=[2.500000000003882, 2.499999999994715] relerr=1.8547363684788055e-12 xchanged=false
central, f_in        jvp=[2.500000000003882, 2.499999999994715] relerr=1.8547363684788055e-12 xchanged=false
forward, no f_in     jvp=[2.5000000298023224, 2.500000014901161] relerr=9.424321830774484e-9 xchanged=false
forward, f_in        jvp=[2.5000000298023224, 2.500000014901161] relerr=9.424321830774484e-9 xchanged=false

(central, f_in shows xchanged=false only because (x - ϵv) + ϵv happened to round back to x for these values; that is not guaranteed, and the halved answer is the real damage. The f_in array is observably clobbered — see the failing test output below.)

Affected methods, established by direct test rather than inspection:

method aliased? symptom
cache-less finite_difference_jvp!, :forward, no f_in yes caller's x overwritten
cache-less finite_difference_jvp!, :forward, with f_in yes caller's x overwritten
cache-less finite_difference_jvp!, :central, with f_in yes caller's f_in overwritten, result halved
cache-less finite_difference_jvp!, :central, no f_in no already used the allocating constructor
cache-less finite_difference_jvp (out-of-place) benign builds an aliased cache, but the OOP path never writes cache.x1 (it rebinds x1 = @. x + ϵ*v) — left untouched
finite_difference_jacobian!/gradient/hessian cache-less no verified x unchanged; JacobianCache(x, fx, …) and HessianCache(x, …) already copy

Pre-existing: identical code in v2.32.1, so this predates #230.

Design tradeoff

Three options were on the table:

  1. Copy x into the cache (chosen).
  2. Save/restore the perturbed entries after the fact — preserves allocations, but is not exception-safe without a try/finally, and floating-point (x - ϵv) + ϵv is not bit-exact in general, so it silently returns a slightly different x than it was given.
  3. Document that x may be mutated.

(3) was ruled out on evidence: the cache-less method's docstring says only "Cache-less." and documents no mutation; docs/src/jvp.md documents none; the mutation contract that does exist is on the non-allocating JVPCache(x, fx1, fdtype) constructor ("The arrays x and fx1 will be modified during JVP computations") — i.e. mutation is the documented behavior of the opt-in path, which this PR leaves exactly as-is. Nothing in the test suite or docs asserts that the cache-less path is non-allocating; there is no @allocated test anywhere near JVP. (test/jvp_accuracy_tests.jl defensively passes copy(x_big) everywhere, which is itself a symptom of this bug.)

Allocation cost, measured with @allocated at n = 1000 (one Vector{Float64} = 8000 bytes):

case before after
:forward, no f_in 8072 B 16144 B
:forward, with f_in 64 B 8136 B
:central, no f_in 16208 B 16240 B
:central, with f_in 64 B 16144 B

The cache-less path was never allocation-free except in the f_in cases, and it was allocation-free there precisely because it was writing into the caller's arrays. Callers who want zero allocation already have the supported route: build a JVPCache once and call the cached method. Silently corrupting caller data is not a performance feature.

Failing before / passing after

git stash push src/jvp.jl (test present, fix reverted), then include("test/cache_reuse_tests.jl"):

  cache-less JVP does not mutate the caller's x      |    6     4     10   2.7s
    Val{:forward}(), f_in=false                      |    1     1      2   2.2s
    Val{:forward}(), f_in=true                       |    2     1      3   0.0s
    Val{:central}(), f_in=false                      |    2            2   0.3s
    Val{:central}(), f_in=true                       |    1     2      3   0.0s

Val{:forward}(), f_in=false: Test Failed at test/cache_reuse_tests.jl:181
  Expression: x == x_ref
   Evaluated: [1.0000000298023224, 2.000000014901161] == [1.0, 2.0]

Val{:forward}(), f_in=true: Test Failed at test/cache_reuse_tests.jl:181
  Expression: x == x_ref
   Evaluated: [1.0000000298023224, 2.000000014901161] == [1.0, 2.0]

Val{:central}(), f_in=true: Test Failed at test/cache_reuse_tests.jl:182
  Expression: fin == f_in
   Evaluated: [2.9999697228744124, 1.9999697228010753] == [3.0, 2.0]

Val{:central}(), f_in=true: Test Failed at test/cache_reuse_tests.jl:183
  Expression: ≈(jvp, jvp_ref, atol = 1.0e-6)
   Evaluated: [1.2499939445336223, 1.249996972263198] ≈ [2.5, 2.5] (atol=1.0e-6)

ERROR: LoadError: Some tests did not pass: 33 passed, 4 failed, 0 errored, 0 broken.

git stash pop, same command:

Test Summary:                   | Pass  Total  Time
Cache reuse safety (issue #213) |   37     37  9.2s

Existing suite

GROUP=Core julia +1.12 --project -e 'using Pkg; Pkg.test()' on this branch:

Test Summary:    | Pass  Total  Time
Public API Tests |   30     30  0.5s
Test Summary:             | Pass  Broken  Total   Time
FiniteDiff Standard Tests |  219       1    220  48.7s
Test Summary:               | Pass  Total     Time
Color Differentiation Tests |   39     39  4m33.9s
Test Summary:      | Pass  Total  Time
JVP Accuracy Tests |   17     17  2.7s
Test Summary:      | Pass  Total   Time
Out of Place Tests |   28     28  44.4s
Test Summary:            | Pass  Total  Time
Cache Reuse Safety Tests |   37     37  7.1s
     Testing FiniteDiff tests passed

Nothing broke. The single Broken is the pre-existing @test_broken at test/finitedifftests.jl:277 (a forward-mode gradient tolerance), present on master and unrelated. No test encoded the aliasing behavior.

Relationship to #230

#230 merged (58ad147) and was released as v2.33.0 before this branch was cut, so this branches from a master that already contains it and there is no conflict — the diff does not touch jvp_epsilon or any step-size code.

Versioning

Patch: 2.33.0 → 2.33.1. 2.33.0 is already in the General registry, so master's version is taken. This adds no public API and removes none; the behavior change is the removal of undocumented data corruption.

Not verified

  • GROUP=Downstream (OrdinaryDiffEq) — not run; it exceeded my time budget while instantiating. This changes allocation counts on a code path OrdinaryDiffEq's matrix-free JVPs may use, so that job is worth watching in CI.
  • No formatter was run: there is still no .JuliaFormatter.toml and no format/typos CI in this repo. Current Runic wants ~420 lines of unrelated changes to src/jvp.jl on master alone, so running it here would be a mechanical sweep that belongs in its own PR. typos over the touched files is clean.
  • Non-Array inputs (StaticArrays, GPU arrays) were not exercised for the JVP path; the fix relies only on copy/zero.

For a reviewer to push back on

  • Separately found and not fixed here, to keep the diff focused: cache-less finite_difference_jvp! with :central and no f_in builds JVPCache(x, fdtype), whose fx1 is copy(x) — i.e. it assumes a square Jacobian. For a 2→3 map it throws BoundsError, for 3→2 DimensionMismatch. That fails loudly rather than silently, so it is a separate issue; the :central-with-f_in branch added here happens to be immune because zero(f_in) has the right size.

finite_difference_jvp! built its internal cache with the non-allocating
JVPCache(x, fx, fdtype) constructor, so cache.x1 === x and the in-place
perturbation overwrote the caller's input. With fdtype = Val(:central) and
f_in supplied it also overwrote f_in and returned half the correct JVP,
because the restored x1 put the second evaluation back at x.

The cache-less path now perturbs a copy of x and never writes into f_in.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
@ChrisRackauckas
ChrisRackauckas marked this pull request as ready for review August 8, 2026 13:09
@ChrisRackauckas
ChrisRackauckas merged commit ccb2c6c into JuliaDiff:master Aug 8, 2026
5 of 6 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