-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Does MutableDiffResult
have to exist at all?
#26
Comments
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. Here's an issue to keep track of these miscreants: |
Yes, there is an ImmutableDiffResult and if you update the package to use that you see resulting heap allocations.
Yes
Yes, there are many SciML examples use it for mutation and ignore the return. |
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: JuliaDiff/ReverseDiff.jl#251 (comment) |
No, those would use the immutable diff results and a different code path.
It causes allocations last time I checked. In certain scenarios it can elide but often it does not. |
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:
DiffResults.jl/src/DiffResults.jl
Lines 19 to 25 in 724a23d
Here's a proof of concept: instead of the current code
DiffResults.jl/src/DiffResults.jl
Lines 156 to 157 in 724a23d
we could instead return a new object every time
The text was updated successfully, but these errors were encountered: