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 REPLCompletions from Static.jl #46494

Merged
merged 4 commits into from
Aug 30, 2022

Conversation

ranocha
Copy link
Member

@ranocha ranocha commented Aug 26, 2022

This should hopefully fix quite some invalidations coming from Static.jl.

Here is the code:
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.8.0 (2022-08-17)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

(@v1.8) pkg> activate --temp

(jl_PDrBpd) pkg> add Static
    Updating registry at `~/.julia/registries/General.toml`
   Resolving package versions...
    Updating `/tmp/jl_PDrBpd/Project.toml`
  [aedffcd0] + Static v0.7.6
    Updating `/tmp/jl_PDrBpd/Manifest.toml`
  [615f187c] + IfElse v0.1.1
  [aedffcd0] + Static v0.7.6

julia> using SnoopCompileCore

julia> invalidations = @snoopr using Static

julia> using SnoopCompile

julia> trees = invalidation_trees(invalidations)
7-element Vector{SnoopCompile.MethodInvalidations}:
...
 inserting !(::False) in Static at ~/.julia/packages/Static/sVI3g/src/Static.jl:427 invalidated:
...
                 150: signature Tuple{typeof(!), Any} triggered MethodInstance for REPL.REPLCompletions.get_value(::Expr, ::Module) (23 children)

As far as I can tell, get_value will always return a Bool as second argument.

@Tokazama
Copy link
Contributor

As far as I can tell, get_value will always return a Bool as second argument.

Should we just do this then?

function get_value(sym::Symbol, fn)::Tuple{Any,Bool}
    isdefined(fn, sym) ? (getfield(fn, sym), true) : (nothing, false)
end
get_value(sym::QuoteNode, fn)::Tuple{Any,Bool} = (sym.value, true)
get_value(sym::GlobalRef, fn)::Tuple{Any,Bool} = get_value(sym.name, sym.mod)
get_value(sym, fn)::Tuple{Any,Bool} = (sym, true)

@ranocha
Copy link
Member Author

ranocha commented Aug 26, 2022

I don't know. Does it fix the invalidation? Or do we still need the type assertion?

@Tokazama
Copy link
Contributor

I don't know. Does it fix the invalidation? Or do we still need the type assertion?

I'm pretty sure that works if every method definition available asserts that, but I don't have enough memory available right now to explicitly test this.

Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

This type assertion seems reasonable, as the type of ex there is only known at runtime but there are 5 possible matching methods for the get_value. As an alternative is to define get_value(args...) = _get_value(args...)::Tuple{Any,Bool}.

stdlib/REPL/src/REPLCompletions.jl Outdated Show resolved Hide resolved
stdlib/REPL/src/REPLCompletions.jl Outdated Show resolved Hide resolved
@giordano giordano added the backport 1.8 Change should be backported to release-1.8 label Aug 29, 2022
@timholy
Copy link
Member

timholy commented Aug 29, 2022

A more elegant fix might be

ex, found = get_value(ex, fn)::Tuple{Any, Bool}

@ranocha
Copy link
Member Author

ranocha commented Aug 30, 2022

A more elegant fix might be

ex, found = get_value(ex, fn)::Tuple{Any, Bool}

Thanks! I changed the code according to your suggestion.

@KristofferC KristofferC merged commit 99340fe into JuliaLang:master Aug 30, 2022
@ranocha ranocha deleted the patch-10 branch August 30, 2022 09:54
ranocha added a commit to ranocha/julia that referenced this pull request Aug 30, 2022
KristofferC pushed a commit that referenced this pull request Aug 30, 2022
@giordano giordano added the compiler:latency Compiler latency label Aug 31, 2022
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Sep 7, 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.

6 participants