Skip to content
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

Adding a convert(::Type{T}, ...) where {T} is a source of a large number of invalidations #144

Open
KristofferC opened this issue Sep 30, 2020 · 6 comments

Comments

@KristofferC
Copy link
Contributor

KristofferC commented Sep 30, 2020

The method definition here:

# Allows an interval to be converted to a scalar when the set contained by the interval only
# contains a single element.
function Base.convert(::Type{T}, interval::AnchoredInterval{P,T}) where {P,T}
if isclosed(interval) && (sign(P) == 0 || first(interval) == last(interval))
return first(interval)
else
# Remove deprecation in version 2.0.0
depwarn(
"`convert(T, interval::AnchoredInterval{P,T})` is deprecated for " *
"intervals which are not closed with coinciding endpoints. " *
"Use `anchor(interval)` instead.",
:convert,
)
return anchor(interval)
# TODO: For when deprecation is removed
# throw(DomainError(interval, "The interval is not closed with coinciding endpoints"))
end
end

is quite bad for latency because it invalidates the convert(::Type{Any}, x) method in Base which a large number of methods are inferred to use. This is a similar issue to JuliaData/CategoricalArrays.jl#177. The invalidations can be seen via:

julia> using SnoopCompileCore

julia> invalidations = @snoopr using Intervals

julia> using SnoopCompile

julia> trees = invalidation_trees(invalidations);

julia> trees[end]

....
inserting convert(::Type{T}, interval::AnchoredInterval{P, T, L, R} where R<:Intervals.Bounded where L<:Intervals.Bounded) where {P, T} in Intervals at /home/kc/.julia/packages/Intervals/T6hJb/src/anchoredinterval.jl:177 invalidated:
....
                 358: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for Base.ImmutableDict{Symbol, Any}(::Base.ImmutableDict{Symbol, Any}, ::Any, ::Any) (101 children)
                 359: signature Tuple{typeof(convert), Type{Symbol}, Any} triggered MethodInstance for merge(::NamedTuple{(), Tuple{}}, ::Base.Iterators.Pairs) (394 children)
                 360: signature Tuple{typeof(convert), Type{Union{Dict{String, Any}, Base.TOML.ParserError}}, Any} triggered MethodInstance for recurse_dict!(::Base.TOML.Parser, ::Dict{String, Any}, ::SubArray{String, 1, Vector{String}, Tuple{UnitRange{Int64}}, true}, ::Bool) (536 children)
   backedges: 1: superseding convert(::Type{Any}, x) in Base at essentials.jl:204 with MethodInstance for convert(::Type{Any}, ::Any) (1242 children)

These two are also pretty bad:

Base.:(==)(a, b::Endpoint) = a == b.endpoint && isclosed(b)
Base.:(==)(a::Endpoint, b) = b == a

@omus
Copy link
Collaborator

omus commented Sep 30, 2020

An attempt to address this: #143

@KristofferC
Copy link
Contributor Author

Thanks, hadn't seen that one.

@omus
Copy link
Collaborator

omus commented Oct 1, 2020

I've been making some PRs to Julia to use more specific function calls which avoids triggering invalidations in base when using Intervals.jl. These changes won't address invalidations that occur with including other packages but fixing the Base invalidations should still help. So far I've managed to address the +, - and isless invalidations Intervals.jl triggers.

@oxinabox
Copy link
Member

oxinabox commented Mar 3, 2021

I wonder if this shouldn't actually be an overload of Base.only.
We are converting a interval that contains exactly 1 point to be exactly just that point.
It feels weird to me to be able to use an interval with 1 element where you can use the element.
e.g. push(::Vector{T}, ::AnchoredInterval{T}) doesn't seem like it should ever work.
but i think it does because of this convert

Do we actually use this method anywhere in practice?

@pabloferz
Copy link
Contributor

@oxinabox see #145

@simeonschaub
Copy link

This would be good to fix before Julia 1.8 is released, since JuliaLang/julia#43671 will add a convert(Any, x) step for any assignment to a global, therefore adding a bunch more invalidation risks for this method.

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

No branches or pull requests

5 participants