-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Artifacts: Change Dict{AbstractPlatform} to Dict{Platform} to reduce Pkg invalidations #54073
Artifacts: Change Dict{AbstractPlatform} to Dict{Platform} to reduce Pkg invalidations #54073
Conversation
Before this pull request: julia> using SnoopCompileCore
julia> invalidations = @snoopr begin
using Pkg
end
julia> using SnoopCompile
julia> invalidation_trees(invalidations)
3-element Vector{SnoopCompile.MethodInvalidations}:
inserting ==(a::Union{Pkg.BinaryPlatforms.FreeBSD, Pkg.BinaryPlatforms.Linux, Pkg.BinaryPlatforms.MacOS, Pkg.BinaryPlatforms.Windows}, b::Base.BinaryPlatforms.AbstractPlatform) @ Pkg.BinaryPlatforms ~/.julia/juliaup/julia-nightly/share/julia/stdlib/v1.12/Pkg/src/BinaryPlatforms_compat.jl:89 invalidated:
backedges: 1: superseding ==(x, y) @ Base Base.jl:208 with MethodInstance for ==(::Base.BinaryPlatforms.AbstractPlatform, ::Base.BinaryPlatforms.AbstractPlatform) (4 children)
inserting tags(::Pkg.BinaryPlatforms.UnknownPlatform) @ Pkg.BinaryPlatforms ~/.julia/juliaup/julia-nightly/share/julia/stdlib/v1.12/Pkg/src/BinaryPlatforms_compat.jl:17 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.os_version(::Base.BinaryPlatforms.AbstractPlatform) (1 children)
3: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.libgfortran_version(::Base.BinaryPlatforms.AbstractPlatform) (1 children)
4: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.cxxstring_abi(::Base.BinaryPlatforms.AbstractPlatform) (1 children)
5: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.libstdcxx_version(::Base.BinaryPlatforms.AbstractPlatform) (1 children)
6: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.arch(::Base.BinaryPlatforms.AbstractPlatform) (2 children)
7: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.os(::Base.BinaryPlatforms.AbstractPlatform) (2 children)
8: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.libc(::Base.BinaryPlatforms.AbstractPlatform) (2 children)
9: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.call_abi(::Base.BinaryPlatforms.AbstractPlatform) (2 children)
10: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for Base.BinaryPlatforms.platforms_match(::Base.BinaryPlatforms.AbstractPlatform, ::Base.BinaryPlatforms.Platform) (13 children)
11: signature Tuple{typeof(Base.BinaryPlatforms.tags), Base.BinaryPlatforms.AbstractPlatform} triggered MethodInstance for (::Base.BinaryPlatforms.var"#match_loss#50")(::Base.BinaryPlatforms.AbstractPlatform, ::Base.BinaryPlatforms.Platform) (30 children)
inserting print(io::Pkg.UnstableIO, arg::Union{SubString{String}, String}) @ Pkg ~/.julia/juliaup/julia-nightly/share/julia/stdlib/v1.12/Pkg/src/Pkg.jl:48 invalidated:
backedges: 1: superseding print(xs...) @ Base coreio.jl:3 with MethodInstance for print(::Any, ::String) (7 children)
2: superseding print(io::IO, s::Union{SubString{String}, String}) @ Base strings/io.jl:250 with MethodInstance for print(::IO, ::String) (374 children)
1 mt_cache After this pull request: julia> using SnoopCompileCore
julia> invalidations = @snoopr begin
using Pkg
end
julia> using SnoopCompile
julia> invalidation_trees(invalidations)
1-element Vector{SnoopCompile.MethodInvalidations}:
inserting print(io::Pkg.UnstableIO, arg::Union{SubString{String}, String}) @ Pkg ~/src/Pkg.jl/src/Pkg.jl:48 invalidated:
backedges: 1: superseding print(xs...) @ Base coreio.jl:3 with MethodInstance for print(::Any, ::String) (7 children)
2: superseding print(io::IO, s::Union{SubString{String}, String}) @ Base strings/io.jl:250 with MethodInstance for print(::IO, ::String) (435 children)
1 mt_cache |
Presumably, the use of |
Yes. The main use of I have proposed normalizing After that Pkg.jl PR, we can add the following in Pkg.BinaryPlatforms:
I did some extended exploration of the ecosystem here: JuliaLang/Pkg.jl#3736 (comment) AppBundler.jl also consumes the Pkg.BinaryPlatforms API. @ararslan also has some uses in scripts, apparently. As in this pull request, there can be a default conversion to |
|
The generic conversion in this pull request works for
|
Is this a problem?
|
I don't think |
Conceptually, I agree, but from a pragmatic standpoint it would help type stability if we could avoid use of I think with some small modifications we could make a |
I haven't looked into this at all (thanks for doing so, @mkitti), but in case you're not aware of it, I've often found it useful to define a |
I'm starting to think about a
I need to think about this further if that would actually help. In some ways, the implementation of Note that we have also have an IO related invalidation issue that we are also working on with regard to Pkg.jl. |
The current invalidation issue originates with a Maybe we need to do some inference magic here to force dynamic dispatch so that there is nothing to invalidate? |
Yeah, sprinkling a few |
Just so we are clear about the invalidation problem, here is the minimum working example.
|
Have you experimented with the following? module BinaryPlatforms
Base.Experimental.@max_methods 1
# rest of module definition
end |
This does not quite work because there there is only one subtype of
|
Another motivation I found is that some folks like to dispatch on on |
Can you call |
BackgroundThe definition of struct Platform <: AbstractPlatform
tags::Dict{String,String}
# The "compare strategy" allows selective overriding on how a tag is compared
compare_strategies::Dict{String,Function}
# ...
end
tags(p::Platform) = p.tags
arch(p::AbstractPlatform) = get(tags(p), "arch", nothing)
os(p::AbstractPlatform) = get(tags(p), "os", nothing)
libc(p::AbstractPlatform) = get(tags(p), "libc", nothing)
call_abi(p::AbstractPlatform) = get(tags(p), "call_abi", nothing)
libgfortran_version(p::AbstractPlatform) = VNorNothing(tags(p), "libgfortran_version")
libstdcxx_version(p::AbstractPlatform) = VNorNothing(tags(p), "libstdcxx_version")
cxxstring_abi(p::AbstractPlatform) = get(tags(p), "cxxstring_abi", nothing)
os_version(p::AbstractPlatform) = VNorNothing(tags(p), "os_version") OptionsI see two options from here.
Forcing dynamic dispatch via
|
base/binaryplatforms.jl
Outdated
@@ -171,6 +178,7 @@ end | |||
|
|||
# Allow us to easily serialize Platform objects | |||
function Base.show(io::IO, p::Platform) | |||
os(p) == "any" && return print(io, "any") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think AnyPlatform needs special treatment? I don't think Pkg ever deals directly with platforms defined elsewhere. AnyPlatform is only used internally in BinaryBuilder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not talking about Pkg here. This is about Base and Artifacts as a standard library built into the system image.
Pkg, like BinaryBuilderBase, introduces new AbstractPlatform
subtypes causing invalidations due to the use of Dict{AbstractPlatform}
in Artifacts.jl.
As you pointed out, BinaryBuilderBase.jl does call artifacts_meta
in Artifacts, possibly passing in AnyPlatform()
which this PR would convert to Platform("any", "any")
.
Showing Platform("any", "any")
does throw an error though which is what I'm trying to handle here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you pointed out, BinaryBuilderBase.jl does call
artifacts_meta
in Artifacts, possibly passing inAnyPlatform()
which this PR would convert to Platform("any", "any").
No, we pass a concrete target platform, which is a Platform
object, never AnyPlatform
. I still don't think AnyPlatform
needs special treatment. My only point in #54073 (comment) was not to remove AbstractPlatform
.
I'm have minimized the diff. We do not need any method signature changes or additions. All we need is conversion to |
I'm minimizing the diff. Also I've submitted to a similar pull request to Pkg.jl which makes it harder to invalidate methods there when loading a package (such as BinaryBuilderBase) that declares new methods on a new I have also included here some a revision to julia> using BinaryBuilderBase
julia> convert(Platform, AnyPlatform())
any any |
@JanisErdmanis and @ararslan , is this pull request compatbile with your usage? |
It would work for me. Perhaps, looking at the changes, I would prefer restricting the type signature of the methods to I see that all the methods in
If you could add a corresponding |
Sure, but that would be changing the interface of a standard library package. The current approach at least allows for some reasonable expectation of backwards compatibility. The only case I have seen so far is
Thank you for taking a look. |
@staticfloat Do you have an opinion on this change? Basically, within Artifacts, I can convert all |
Unfortunately, the CrossPlatform in BinaryBuilderPlatformExtensions is not a Platform and needs to work with Artifacts. |
I think the main problem here is It is unclear to me when |
…ng, ::AbstractPlatform)
This reverts commit a6c9841.
5b31fc1
to
fe94768
Compare
…rm}, ::AbstractPlatform)
The change here is basically a single line. Changing The key of the dictionary comes from julia/stdlib/Artifacts/src/Artifacts.jl Lines 404 to 412 in 3c2198b
julia> using Artifacts
julia> Base.return_types(Artifacts.unpack_platform, Tuple{Dict{String, Any}, String, String})
1-element Vector{Any}:
Union{Nothing, Base.BinaryPlatforms.Platform} julia> using SnoopCompileCore
julia> invalidations = @snoopr begin
using Pkg
end;
julia> using SnoopCompile
julia> trees = invalidation_trees(invalidations)
1-element Vector{SnoopCompile.MethodInvalidations}:
inserting typeinfo_implicit(::Type{Dates.Day}) @ Dates ~/src/julia/usr/share/julia/stdlib/v1.12/Dates/src/periods.jl:23 invalidated:
backedges: 1: superseding typeinfo_implicit(T) @ Base arrayshow.jl:550 with MethodInstance for Base.typeinfo_implicit(::Any) (22 children)
1 mt_cache |
…Pkg invalidations (#54073) This pull request now only changes `dl_dict = Dict{AbstractPlatform,Dict{String,Any}}()` to `dl_dict = Dict{Platform,Dict{String,Any}}()` in `artifact_meta`. This is possible since the other possible types for the key are either `Platform` or `nothing` unless someone overrides `Artifacts.unpack_platform`. (cherry picked from commit a14cc38)
This pull request now only changes
dl_dict = Dict{AbstractPlatform,Dict{String,Any}}()
todl_dict = Dict{Platform,Dict{String,Any}}()
inartifact_meta
.This is possible since the other possible types for the key are either
Platform
ornothing
unless someone overridesArtifacts.unpack_platform
.The Artifacts standard library currently makes use ofBase.BinaryPlatforms.AbstractPlatform
for several keywords.This is problematic because these methods become susceptible to invalidation.
In particular,
Pkg.BinaryPlatforms
causes many invalidations of methods in the standard system image because of this.To alleviate this, all methods receiving
AbstractPlatform
will now attempt to convert theAbstractPlatform
keywordarguments to
Platform
. Also, use ofDict{AbstractPlatform}
is removed.To assist with this conversion, a generic conversion of
AbstractPlatform
toPlatform
is added toBase.BinaryPlatforms
.See #54073 (comment) to see the reduction in invalidations.