Narrow convert methods to reduce invalidations
#172
Conversation
This package caused >1000 invalidations for DifferentialEquations.
The main problem is a pair of `convert` methods: one is redundant with
a method in Base (and needlessly invalidates some code),
and another intersected with signatures like
`convert(Base.SHA1, ::Any)` and other crazy types.
The reason is because the `T` in `APL{T}` is completely unconstrained,
and consequently it's possible in principle for it to be a
`REPLTerminal` or a `Module` or whatever. It didn't seem as if that were
the real intent of the convert method.
timholy
force-pushed
the
teh/invalidations
branch
from
040b236
to
d71b833
Compare
| @@ -73,5 +72,6 @@ function Base.convert(S::Type{<:Union{Number, T}}, p::APL{T}) where T | |||
| end | |||
| s | |||
| end | |||
| Base.convert(::Type{T}, p::APL) where T<:Number = scalarize(T, p) | |||
There was a problem hiding this comment.
Does this still cause some invalidations ?
There was a problem hiding this comment.
Nothing serious, if memory serves. The real problem is totally-unconstrained T.
When fixing invalidations, usually I don't like to suggest restrictions on what packages can do (better to fix the code being invalidated). But convert is one of those rare functions that's used in a ton of places with poor inference on the 2nd argument, and it's often used to make inference better. Consequently it's bad to specialize only on the 2nd argument, since so many callers don't know what type their 2nd argument is. It's generally fine if you specialize on the first argument.
You can add more convert methods than this, I just gave one that is needed to pass tests. But keep in mind that the call site could also just call scalarize directly, and that would be the cleanest solution of all (just deleting this method altogether).
There was a problem hiding this comment.
Ok, thanks for the detailed explanation. I try to keep the same behavior independently of whether the coefficient is a number or not as the coefficient is for instance sometimes a JuMP expressions which is not a number. Therefore I might remove this convert and see what's break. Meanwhile I'll merge this PR. Thanks!
This package caused >1000 invalidations for DifferentialEquations.
The main problem is a pair of
convertmethods: one is redundant witha method in Base (and needlessly invalidates some code),
and another intersected with signatures like
convert(Base.SHA1, ::Any)and other crazy types.The reason is because the
TinAPL{T}is completely unconstrained,and consequently it's possible in principle for it to be a
REPLTerminalor aModuleor whatever. It didn't seem as if that werethe real intent of the convert method, but I'm just guessing that this won't break anything.
I have not run the full tests locally but 🤞
CC @ChrisRackauckas
Some background in another package: JuliaData/CategoricalArrays.jl#177