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

Passing through NamedTuple throws away known type info from @kwdef constructor #36322

Closed
timholy opened this issue Jun 17, 2020 · 2 comments
Closed
Labels
compiler:inference Type inference compiler:latency Compiler latency

Comments

@timholy
Copy link
Member

timholy commented Jun 17, 2020

Some new convert method definitions cause substantial invalidation due to usage of convert in constructors. A good example is Pkg.Operations.load_direct_deps:

julia> using Pkg, Cthulhu

julia> descend(Pkg.Operations.load_direct_deps, (Pkg.Types.Context,))

From here, you can navigate to the "body method" where you get to the core issue:

%47 = uuid::Base.UUID%48 = name::String%49 = Base.getproperty(entry::Pkg.Types.PackageEntry, :path)::Union{Nothing, String}%50 = Base.getproperty(entry::Pkg.Types.PackageEntry, :repo)::Pkg.Types.GitRepo%51 = Base.getproperty(entry::Pkg.Types.PackageEntry, :pinned)::Bool%52 = Base.getproperty(entry::Pkg.Types.PackageEntry, :tree_hash)::Union{Nothing, Base.SHA1}%53 = Base.getproperty(entry::Pkg.Types.PackageEntry, :version)::Union{Nothing, VersionNumber}%54 = Pkg.Operations.isfixed(entry::Pkg.Types.PackageEntry)::Bool%55 = Pkg.Operations.load_version(%53, %54, preserve)::Union{Nothing, VersionNumber, Pkg.Types.VersionSpec}%56 = Core.tuple(%47, %48, %49, %50, %51, %52, %55)::Tuple{Base.UUID,String,Union{Nothing, String},Pkg.Types.GitRepo,Bool,Union{Nothing, Base.SHA1},Union{Nothing, VersionNumber, Pkg.Types.VersionSpec}}%57 = (%46)(%56)::NamedTuple{(:uuid, :name, :path, :repo, :pinned, :tree_hash, :version),_A} where _A<:Tuple%58 = Core.kwfunc(Pkg.Operations.PackageSpec)::Core.Compiler.Const(Core.var"#Type##kw"(), false)
└───       (@_13 = (%58)(%57, Pkg.Operations.PackageSpec))

Here you can see that most of the inputs for creating the NamedTuple are already known to have the types needed for fields of PackageSpec. However, presumably due to specialization heuristics, that type info is lost with the constructor for NamedTuple.

I don't think this mode of invalidation is outrageously common, but I know I've seen it before, which is why I decided to report it as a systematic issue. Perhaps the NamedTuple constructor should get an extra Union-bonus in the specialization heuristics? Or is it one of the worst offenders desperately in need of limits?

There is a way to circumvent this: create a specialized PackageSpec(::PackageEntry, uuid, ver, pinned, mode) constructor. But I don't see a great way to avoid duplicating the field default specifications, which would be a bit of a code smell.

@timholy timholy added the compiler:latency Compiler latency label Jun 17, 2020
@JeffBezanson
Copy link
Member

Dup of #32699 I think.

@JeffBezanson JeffBezanson added the compiler:inference Type inference label Jun 17, 2020
@JeffBezanson
Copy link
Member

I have a fix, and this is a duplicate, but please see if my change actually fixes the invalidations here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference compiler:latency Compiler latency
Projects
None yet
Development

No branches or pull requests

2 participants