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

fix invalidations in REPL LineEdit.jl from Static.jl #46548

Merged
merged 2 commits into from
Aug 31, 2022

Conversation

ranocha
Copy link
Member

@ranocha ranocha commented Aug 30, 2022

This is based on the following code:

julia> using Pkg; Pkg.activate(temp=true); Pkg.add("Static")

julia> using SnoopCompileCore; invalidations = @snoopr(using Static); using SnoopCompile

julia> trees = invalidation_trees(invalidations)
inserting !(::False) in Static at ~/.julia/packages/Static/sVI3g/src/Static.jl:427 invalidated:
...
                 75: signature Tuple{typeof(!), Any} triggered MethodInstance for REPL.LineEdit.var"#add_nested_key!#24"(::Any, ::typeof(REPL.LineEdit.add_nested_key!), ::Dict, ::Union{Char, String}, ::Any) (60 children)
...

I suggest the labels latency and backport-1.8.

@KristofferC KristofferC added compiler:latency Compiler latency backport 1.8 Change should be backported to release-1.8 labels Aug 30, 2022
@@ -1560,7 +1560,7 @@ function add_nested_key!(keymap::Dict, key::Union{String, Char}, value; override
if y === nothing
keymap[c] = value
break
elseif !(c in keys(keymap) && isa(keymap[c], Dict))
elseif !((c in keys(keymap))::Bool && isa(keymap[c], Dict))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not at all familiar with the invalidation business, but judging from the recurrent patterns: the negation acts on the two expressions whose final value is passed to !, so are you sure you don't need

Suggested change
elseif !((c in keys(keymap))::Bool && isa(keymap[c], Dict))
elseif !((c in keys(keymap) && isa(keymap[c], Dict))::Bool)

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked with Cthulu and the problem was that c in keys(keymap) was inferred as Any.

@JeffBezanson
Copy link
Member

Looks like there is an opportunity to improve inference here:

julia> code_typed(Base.KeySet, (Dict,))
1-element Vector{Any}:
 CodeInfo(
1 ─ %1 = $(Expr(:static_parameter, 1))::Any
│   %2 = $(Expr(:static_parameter, 2))::Type{T} where T<:Dict
│   %3 = Core.apply_type(Base.KeySet, %1, %2)::Type{Base.KeySet{_A, _B}} where {_A, _B}
│   %4 = (%3)(dict)::Base.KeySet
└──      return %4
) => Base.KeySet

If that returned KeySet{<:Any, <:Dict} we would know the exact in method and it would get inferred.

@KristofferC KristofferC merged commit 99d8c7b into JuliaLang:master Aug 31, 2022
@ranocha ranocha deleted the patch-5 branch August 31, 2022 10:49
KristofferC pushed a commit that referenced this pull request Aug 31, 2022
* fix invalidations in REPL LineEdit.jl from Static.jl

(cherry picked from commit 99d8c7b)
@KristofferC KristofferC removed backport 1.8 Change should be backported to release-1.8 compiler:latency Compiler latency labels Sep 7, 2022
@ranocha ranocha added compiler:latency Compiler latency backport 1.8 Change should be backported to release-1.8 labels Sep 14, 2022
@ranocha ranocha mentioned this pull request Sep 14, 2022
28 tasks
@ranocha ranocha removed the backport 1.8 Change should be backported to release-1.8 label Sep 14, 2022
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.

4 participants