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

Invalidations due to conversion to OrderedDict #90

Open
sethaxen opened this issue Sep 2, 2022 · 0 comments
Open

Invalidations due to conversion to OrderedDict #90

sethaxen opened this issue Sep 2, 2022 · 0 comments

Comments

@sethaxen
Copy link

sethaxen commented Sep 2, 2022

There are several invalidations in this package that impact downstream dependants:

julia> using SnoopCompileCore

julia> invalidations = @snoopr using OrderedCollections;

julia> using SnoopCompile

julia> trees = invalidation_trees(invalidations)
3-element Vector{SnoopCompile.MethodInvalidations}:
 inserting in(key, v::Base.KeySet{K, T}) where {K, T<:(OrderedDict{K})} in OrderedCollections at /home/sethaxen/.julia/packages/OrderedCollections/PRayh/src/ordered_dict.jl:394 invalidated:
   backedges: 1: superseding in(k, v::Base.KeySet) in Base at abstractdict.jl:71 with MethodInstance for in(::Char, ::Base.KeySet) (2 children)

 inserting (::Base.var"#sort!##kw")(::Any, ::typeof(sort!), d::OrderedDict) in OrderedCollections at /home/sethaxen/.julia/packages/OrderedCollections/PRayh/src/dict_sorting.jl:4 invalidated:
   mt_backedges: 1: signature Tuple{Base.var"#sort!##kw", NamedTuple{(:by,), Tuple{typeof(identity)}}, typeof(sort!), Any} triggered MethodInstance for TOML.Internals.Printer.var"#_print#10"(::Int64, ::Bool, ::Bool, ::typeof(identity), ::typeof(TOML.Internals.Printer._print), ::Nothing, ::IOStream, ::AbstractDict, ::Vector{String}) (46 children)

 inserting convert(::Type{OrderedDict{K, V}}, d::OrderedDict{K, V}) where {K, V} in OrderedCollections at /home/sethaxen/.julia/packages/OrderedCollections/PRayh/src/ordered_dict.jl:110 invalidated:
   backedges: 1: superseding convert(::Type{T}, x::AbstractDict) where T<:AbstractDict in Base at abstractdict.jl:561 with MethodInstance for convert(::Type, ::AbstractDict) (242 children)
   3 mt_cache

The final one is the most severe with 242 children. This has some effect on load time:

current:

julia> @time using OrderedCollections
  0.011149 seconds (16.78 k allocations: 1.136 MiB)

removing the convert methods:

julia> @time using OrderedCollections
  0.008151 seconds (9.70 k allocations: 771.891 KiB)

The methods in question are:

function convert(::Type{OrderedDict{K,V}}, d::AbstractDict) where {K,V}
if !isordered(typeof(d))
Base.depwarn("Conversion to OrderedDict is deprecated for unordered associative containers (in this case, $(typeof(d))). Use an ordered or sorted associative type, such as SortedDict and OrderedDict.", :convert)
end
h = OrderedDict{K,V}()
for (k,v) in d
ck = convert(K,k)
if !haskey(h,ck)
h[ck] = convert(V,v)
else
error("key collision during dictionary conversion")
end
end
return h
end
convert(::Type{OrderedDict{K,V}},d::OrderedDict{K,V}) where {K,V} = d

The default method in Base forwards to the constructor:

function convert(::Type{T}, x::AbstractDict) where T<:AbstractDict
    h = T(x)
    if length(h) != length(x)
        error("key collision during dictionary conversion")
    end
    return h
end

But for OrderedDict, convert and the constructor don't do the same thing. e.g. OrderedDict{K,V}(::OrderedDictr{K,V}) copies the keys and values, while convert does not. Likewise, OrderedDict{K,V}(::AbstractDict) doesn't raise a deprecation warning, but convert(OrderedDict{K,V}, ::AbstractDict) does.

Is it possible to remove both of these methods? If they are removed, the test suite still passes.

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 a pull request may close this issue.

1 participant