Skip to content

Conversation

@mkitti
Copy link
Contributor

@mkitti mkitti commented Apr 17, 2024

This pull request hardens Pkg.jl from invalidations involving AbstractPlatform. When AbstractPlatform is used as the type of a keyword argument, Julia may not
specialize on the type.

This pull request inserts convert(Platform, platform)::Platform to all functions that take platform::AbstractPlatform as a keyword argument.

@mkitti
Copy link
Contributor Author

mkitti commented Apr 17, 2024

Before this pull request:

julia> using SnoopCompileCore, Pkg

julia> invalidations = @snoopr using BinaryBuilderBase;

julia> using SnoopCompile

julia> trees = invalidation_trees(invalidations)
3-element Vector{SnoopCompile.MethodInvalidations}:
 inserting joinpath(prefix::Prefix, args...) @ BinaryBuilderBase ~/.julia/packages/BinaryBuilderBase/tGUXK/src/Prefix.jl:68 invalidated:
   mt_backedges: 1: signature Tuple{typeof(joinpath), Any, String} triggered MethodInstance for Artifacts.jointail(::Any, ::String) (0 children)

 inserting repr(p::FileProduct) @ BinaryBuilderBase ~/.julia/packages/BinaryBuilderBase/tGUXK/src/Products.jl:463 invalidated:
   backedges: 1: superseding repr(x; context) @ Base strings/io.jl:286 with MethodInstance for repr(::Any) (43 children)

 inserting tags(p::AnyPlatform) @ BinaryBuilderBase ~/.julia/packages/BinaryBuilderBase/tGUXK/src/Platforms.jl:14 invalidated:
   mt_backedges:  1: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.triplet(::Base.BinaryPlatforms.AbstractPlatform) (0 children)
                  2: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.libgfortran_version(::Base.BinaryPlatforms.AbstractPlatform) (1 children)
                  3: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.cxxstring_abi(::Base.BinaryPlatforms.AbstractPlatform) (1 children)
                  4: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.libstdcxx_version(::Base.BinaryPlatforms.AbstractPlatform) (1 children)
                  5: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.os_version(::Base.BinaryPlatforms.AbstractPlatform) (2 children)
                  6: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.call_abi(::Base.BinaryPlatforms.AbstractPlatform) (2 children)
                  7: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.arch(::Base.BinaryPlatforms.AbstractPlatform) (2 children)
                  8: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.os(::Base.BinaryPlatforms.AbstractPlatform) (2 children)
                  9: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.var"#Platform#8"(::Base.Pairs{Symbol, Union{}, Tuple{}, @NamedTuple{}}, ::Type{Platform}, ::Base.BinaryPlatforms.AbstractPlatform) (5 children)
                 10: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.libc(::Base.BinaryPlatforms.AbstractPlatform) (60 children)

After this pull request:

julia> using SnoopCompileCore, Pkg

julia> invalidations = @snoopr using BinaryBuilderBase;

julia> using SnoopCompile

julia> trees = invalidation_trees(invalidations)
2-element Vector{SnoopCompile.MethodInvalidations}:
 inserting joinpath(prefix::Prefix, args...) @ BinaryBuilderBase ~/.julia/packages/BinaryBuilderBase/tGUXK/src/Prefix.jl:68 invalidated:
   mt_backedges: 1: signature Tuple{typeof(joinpath), Any, String} triggered MethodInstance for Artifacts.jointail(::Any, ::String) (0 children)

 inserting repr(p::FileProduct) @ BinaryBuilderBase ~/.julia/packages/BinaryBuilderBase/tGUXK/src/Products.jl:463 invalidated:
   backedges: 1: superseding repr(x; context) @ Base strings/io.jl:286 with MethodInstance for repr(::Any) (43 children)


const PlatformUnion = Union{Linux,MacOS,Windows,FreeBSD}

Base.convert(::Type{Platform}, p::PlatformUnion) = p.p
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm highlighting this change.

Here we convert most of the AbstractPlatform subtypes to Platform by unwrapping them.

The one outlier is UnknownPlatform but there is another pull request to address that.

@IanButterworth
Copy link
Member

@mkitti what's the status of this?

@mkitti
Copy link
Contributor Author

mkitti commented Jul 8, 2024

We might not need it.

@IanButterworth
Copy link
Member

Ok. I also see these:

Can any be closed? Obviously they can be reopened if plans change

#3742
#3872
#3873

@mkitti
Copy link
Contributor Author

mkitti commented Jul 9, 2024

It is not clear to me whether general conversion to Platform would be compatible with BinaryBuilder 2. Thus, I suggest closing.

@mkitti mkitti closed this Jul 9, 2024
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 this pull request may close these issues.

2 participants