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 sort! from Static.jl #46491

Merged
merged 5 commits into from
Aug 29, 2022
Merged

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:
...
                 160: signature Tuple{typeof(!), Any} triggered MethodInstance for sort!(::Vector{String}, ::Int64, ::Int64, ::Base.Sort.InsertionSortAlg, ::Base.Order.Ordering) (127 children)

Since !lt(...) is used here in an if stament - and if statements only accept Bools -
it appears to be sane to assert that lt returns a Bool here.

See also #46481, #46490

@Tokazama
Copy link
Contributor

Another instance where it might be easier in the long run to annotate the function definition itself?

lt(::Left, x::T, y::T)::Bool where {T<:Floats} = slt_int(y, x)
lt(::Right, x::T, y::T)::Bool where {T<:Floats} = slt_int(x, y)

@ranocha
Copy link
Member Author

ranocha commented Aug 26, 2022

I'm not sure whether it works and fixes the invalidations but since you seem to be sure I did what you suggested in bbf1bde

@Tokazama
Copy link
Contributor

Tokazama commented Aug 26, 2022

You can ensure that these fix invalidations in master by building from you branch of Julia and looking at the invalidations again. I didn't see invalidations when I added a Bool assertion on these, but depending on compiler work this may not be reflective of what happens for 1.8.

@ranocha
Copy link
Member Author

ranocha commented Aug 29, 2022

There do not seem to be invalidations anymore on recent master with this PR on top (after moving the type assertions to the definitions of lt as suggested by @Tokazama). I'm building Julia again from the release-1.8 branch with this PR on top of it to see how it looks there.

@giordano giordano added the backport 1.8 Change should be backported to release-1.8 label Aug 29, 2022
This reverts commit 6b218f8.
The new form did not fix invalidations in Julia 1.8.
@ranocha
Copy link
Member Author

ranocha commented Aug 29, 2022

The version proposed by @Tokazama does not fix invalidations from a clean build of Julia from the release-1.8 branch with this PR on top of it (on my system). However, the version I implemented at first did fix the invalidations. Thus, I reverted the change proposed by @Tokazama.

@timholy
Copy link
Member

timholy commented Aug 29, 2022

@Tokazama, inference stops checking whether all possible callees return Bool if the number of methods exceeds 3. Thus in many cases, just adding method annotations does nothing to fix invalidations.

@Tokazama
Copy link
Contributor

@Tokazama, inference stops checking whether all possible callees return Bool if the number of methods exceeds 3. Thus in many cases, just adding method annotations does nothing to fix invalidations.

That's really interesting because one of the ideas being thrown around to resolve invalidations more proactively by Stefan was to have some way of declaring all method variants inherit a type assertion (e.g., function isequal::Bool end would then require all methods return a Bool or error even if they don't have the type assert).

@timholy
Copy link
Member

timholy commented Aug 29, 2022

#42372, #36463

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

Since this calls isless, this seems perfectly safe.

@timholy timholy added compiler:latency Compiler latency merge me PR is reviewed. Merge when all tests are passing labels Aug 29, 2022
@KristofferC KristofferC merged commit 1fae1b9 into JuliaLang:master Aug 29, 2022
@oscardssmith oscardssmith removed the merge me PR is reviewed. Merge when all tests are passing label Aug 29, 2022
@ranocha ranocha deleted the patch-8 branch August 30, 2022 04:45
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
@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