DiffResults.jl icon indicating copy to clipboard operation
DiffResults.jl copied to clipboard

Does `MutableDiffResult` have to exist at all?

Open gdalle opened this issue 1 year ago • 4 comments

Preliminary: DiffResult objects must always be realiased when updated, as mentioned in the docs

https://juliadiff.org/DiffResults.jl/stable/#DiffResults.value!

In most cases it is unnecessary, because the result object itself gets mutated. However, it causes a heap allocation to create the mutable struct defined here:

https://github.com/JuliaDiff/DiffResults.jl/blob/724a23d326353353aa78ae9b9e770685119492dc/src/DiffResults.jl#L19-L25

Here's a proof of concept: instead of the current code

https://github.com/JuliaDiff/DiffResults.jl/blob/724a23d326353353aa78ae9b9e770685119492dc/src/DiffResults.jl#L156-L157

we could instead return a new object every time

value!(r::MutableDiffResult, x::Number) = MutableDiffResult(x, r.derivs)
value!(r::MutableDiffResult, x::AbstractArray) = (copyto!(value(r), x); return r)
  • Would it decrease or increase the performance of the update? Don't know
  • Would it be considered a breaking change? Perhaps (see below)
  • Would it break stuff in practice? Definitely (see below)

gdalle avatar Mar 15 '24 16:03 gdalle

In terms of SemVer, I'm not sure whether the mutable/immutable distinction is part of the API. It is mentioned in a few docstrings but the concrete types themselves have no docstrings

https://juliadiff.org/DiffResults.jl/stable/#DiffResults.DiffResult

I know that some dependencies (like ForwardDiff and ReverseDiff) break if we decide to return a new struct in more cases, because they don't follow the documentation's advice on realiasing. But then again they misuse the interface so it's pretty much their fault, right?

Here's an issue to keep track of these miscreants:

  • https://github.com/JuliaDiff/DiffResults.jl/issues/27

gdalle avatar Mar 15 '24 17:03 gdalle

Would it decrease or increase the performance of the update? Don't know

Yes, there is an ImmutableDiffResult and if you update the package to use that you see resulting heap allocations.

Would it be considered a breaking change? Perhaps (see below)

Yes

Would it break stuff in practice? Definitely (see below)

Yes, there are many SciML examples use it for mutation and ignore the return.

ChrisRackauckas avatar Mar 15 '24 18:03 ChrisRackauckas

Not sure i understand your answer to the performance aspect, I asked whether it would be good or bad and you said yes ^^

The SciML packages that use results without realiasing are already incorrect for StaticArrays, even without changes in DiffResults. Example: https://github.com/JuliaDiff/ReverseDiff.jl/issues/251#issuecomment-2000036388 In this regard, wouldnt it make sense to fix those used anyway?

gdalle avatar Mar 15 '24 19:03 gdalle

No, those would use the immutable diff results and a different code path.

Not sure i understand your answer to the performance aspect, I asked whether it would be good or bad and you said yes ^^

It causes allocations last time I checked. In certain scenarios it can elide but often it does not.

ChrisRackauckas avatar Mar 15 '24 19:03 ChrisRackauckas