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

Protect cmd_gen against invalidation #48557

Merged
merged 1 commit into from
Feb 6, 2023
Merged

Protect cmd_gen against invalidation #48557

merged 1 commit into from
Feb 6, 2023

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented Feb 6, 2023

This gets used by Base.require, arguably the most painful of all invalidations. CSV is one package that invalidates it.

This gets used by `Base.require`, arguably the most painful of
all invalidations. CSV is one package that invalidates it.
@KristofferC
Copy link
Sponsor Member

@timholy
Copy link
Sponsor Member Author

timholy commented Feb 6, 2023

Hmm, in my analysis it's

julia> tree
inserting convert(::Type{String}, x::WeakRefStrings.WeakRefString) @ WeakRefStrings ~/.julia/packages/WeakRefStrings/31nkb/src/WeakRefStrings.jl:81 invalidated:
   mt_backedges: 1: MethodInstance for setindex!(::Vector{String}, ::Any, ::Int64) at depth 0 with 24 children blocked 0.05790986799999999 inclusive time for 2 nodes


julia> ascend(root)
Choose a call for analysis (q to quit):
 >   setindex!(::Vector{String}, ::Any, ::Int64)
       getindex(::Type{String}, ::Any)
         arg_gen(::AbstractString)
           cmd_gen(::Tuple{Tuple{Cmd}, Tuple{String}, Tuple{String}, Tuple{SubString{String}}, Tuple{String}, Tuple{String}, Tuple{Nothing}, Tuple{String}, Tuple{String}, Tuple{String}, Tu
             link_image_cmd(::Nothing, ::String)
               link_image(::Nothing, ::String, ::IO, ::IO)
                 link_image(::Nothing, ::String)
                   compilecache(::Base.PkgId, ::String, ::IO, ::IO, ::Bool)
                     compilecache(::Base.PkgId, ::String)
v                      _require(::Base.PkgId, ::Nothing)

This is using the analysis in JuliaData/DataFrames.jl#3285 (comment)

@KristofferC KristofferC added compiler:latency Compiler latency backport 1.9 Change should be backported to release-1.9 labels Feb 6, 2023
@KristofferC
Copy link
Sponsor Member

KristofferC commented Feb 6, 2023

Hm, okay but I've seen traces pointing to that method extension elsewhere at least.

@timholy
Copy link
Sponsor Member Author

timholy commented Feb 6, 2023

Me too. I'm not very familiar with FilePathsBase, but I wonder if the conveniences it offers are so nice as to make the invalidations worth it.

@timholy timholy merged commit 5721ae7 into master Feb 6, 2023
@timholy timholy deleted the teh/cmd_gen branch February 6, 2023 18:25
KristofferC pushed a commit that referenced this pull request Feb 7, 2023
This gets used by `Base.require`, arguably the most painful of
all invalidations. CSV is one package that invalidates it.

(cherry picked from commit 5721ae7)
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:latency Compiler latency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants