fix some issues in Artifacts code reported by JET #49596
Conversation
| tags::Dict{String,String} | ||
| # The "compare strategy" allows selective overriding on how a tag is compared | ||
| compare_strategies::Dict{String,Function} | ||
| compare_strategies::IdDict{String,Function} |
There was a problem hiding this comment.
Dict should be better here, since the key is a String
| for x in meta | ||
| x::Dict{String} | ||
| dl_dict[unpack_platform(x, name, artifacts_toml)] = x | ||
| y = x::Dict{String} |
There was a problem hiding this comment.
This type assert seems somewhat expensive, since you don't know the key
| meta === nothing && return nothing | ||
| # If it's NOT a dict, complain | ||
| elseif !isa(meta, Dict) | ||
| if !isa(meta, Dict{String}) |
There was a problem hiding this comment.
this makes the isa check more expensive, since we have a fast path for unparameterized types, but it cannot optimize partially parameterized types
vtjnash
left a comment
vtjnash
left a comment
There was a problem hiding this comment.
RSLGTM. Someone else might know this code a lot better
| # @invokelatest here to not get invalidated by new defs of `==(::Function, ::Function)` | ||
| return @invokelatest getindex(download_info, first(ps)) |
There was a problem hiding this comment.
So if I understand the compiler properly, what this does is pays a constant penalty (unable to inline getindex()) on every invocation, in order to avoid paying an infrequent but huge penalty (recompiling all callers of this function upon invalidation)? Where the "best" solution where we pay no penalty (separate compilation) would have the (arguable) downside of not reacting to said invalidations?
There was a problem hiding this comment.
I think that is a fair summary yes.
| function parse_mapping(mapping::Dict, name::String, _::String) | ||
| d = Dict{String, Any}() | ||
| for (k, v) in mapping | ||
| d[k] = parse_mapping(v, name) |
There was a problem hiding this comment.
| d[k] = parse_mapping(v, name) | |
| d[k] = parse_mapping(v, name, _) |
There was a problem hiding this comment.
The fact that this is not caught by the test suite is a little worrying.
| # If this mapping is the empty string, un-override it | ||
| if mapping == "" | ||
| delete!(overrides[:hash], hash) | ||
| overrides_hash = overrides[:hash]::Dict{SHA1,Union{String,SHA1}} |
There was a problem hiding this comment.
I think the idea of having one dictionary keyed by a symbol is just fundamentally sloppy. I should have instead just tracked two separate dicts that can be properly inferred.
| overrides_hash[hash] = mapping | ||
| end | ||
| elseif isa(mapping, Dict) | ||
| elseif isa(mapping, Dict{String}) |
There was a problem hiding this comment.
Same comment here for the slow isa check here.
|
Thanks for the reviews. This PR was a bit "spray and pray". I'll try cut down the changes to the bare minimum to still make the original motivating cause work. |
This avoids invalidating
when loading a JLL that uses platform augmentation after e.g. Symbolics.jl which defines a bunch of core methods (
==,hash, etc) on new types that are<: Function.In general, the Artifacts and BinaryPlatform modules feels quite "under typed". I wonder if it would be worth doing the TOML parsing in one single place and produce type stable outputs from that one place instead of passing around
Dicts to so many functions.