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 some invalidations when loading Static.jl #46553

Merged
merged 4 commits into from
Sep 6, 2022

Conversation

ranocha
Copy link
Member

@ranocha ranocha commented Aug 30, 2022

This improvement prevents some invalidations in abstractarray.jl when loading Static.jl.

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:
...
                 51: signature Tuple{typeof(!), Any} triggered MethodInstance for Base.mightalias(::Vector, ::Vector{Any}) (10 children)
                 52: signature Tuple{typeof(!), Any} triggered MethodInstance for Base.mightalias(::Vector, ::Vector{String}) (11 children)
                 57: signature Tuple{typeof(!), Any} triggered MethodInstance for Base.mightalias(::Vector, ::Vector{Pkg.Types.PackageSpec}) (16 children)
...

I suggest the labels latency and backport-1.8.

…ray)`

This improvement prevents some invalidations in abstractarray.jl when loading Static.jl.
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

The compiler probably should be able to figure this out

@ranocha
Copy link
Member Author

ranocha commented Aug 31, 2022

But it looks like it does not at the moment, at least not on Julia v1.8. Is it already fixed on master? If not, I think it would be nice to get this merged so that many people can benefit from reduced latency.
(Well, and since you requested changes: I do not think I can fix this in the compiler in a reasonable amount of time.)

@timholy
Copy link
Member

timholy commented Aug 31, 2022

I would try to fix this at a higher level; presumably this fails to infer only because the array type is being inferred abstractly.

@ranocha
Copy link
Member Author

ranocha commented Aug 31, 2022

I would try to fix this at a higher level; presumably this fails to infer only because the array type is being inferred abstractly.

Yes, the first argument of mightalias is only inferred as Vector in these cases. I will try to see whether I can fix it at a higher level.

This prevents some invalidations in `mightalias(A::AbstractArray, B::AbstractArray)` in abstractarray.jl when loading Static.jl.
@ranocha ranocha changed the title improve type stability of mightalias(A::AbstractArray, B::AbstractArray) fix some invalidations when loading Static.jl Aug 31, 2022
@ranocha
Copy link
Member Author

ranocha commented Aug 31, 2022

The last two cases reported above come from findall(testf::Function, A::AbstractArray), which used broadcasting. I changed it to map, removing most invalidations.

The first case (with 10 children) is reduced to 7 children:

                 134: signature Tuple{typeof(!), Any} triggered MethodInstance for Base.mightalias(::Vector, ::Vector{Any}) (7 children)

I could not really find a good place where the type instability happens there since ascend ends already with copyto!(::Vector, ...):

julia> ascend(root)
Choose a call for analysis (q to quit):
     mightalias(::Vector, ::Vector{Any})
       unalias(::Vector, ::Vector{Any})
         broadcast_unalias(::Vector, ::Vector{Any})
           preprocess(::Vector, ::Vector{Any})
             preprocess_args(::Vector, ::Tuple{Vector{Any}})
               preprocess_args(::Vector, ::Tuple{Base.RefValue{String}, Vector{Any}})
                 preprocess(::Vector, ::Base.Broadcast.Broadcasted{Nothing, Tuple{Base.OneTo{Int64}}, Pkg.API.var
 >                 copyto!(::Vector, ::Base.Broadcast.Broadcasted{Nothing, Tuple{Base.OneTo{Int64}}, Pkg.API.var"

base/array.jl Outdated
@@ -2325,7 +2325,7 @@ findall(testf::Function, A) = collect(first(p) for p in pairs(A) if testf(last(p

# Broadcasting is much faster for small testf, and computing
# integer indices from logical index using findall has a negligible cost
findall(testf::Function, A::AbstractArray) = findall(testf.(A))
findall(testf::Function, A::AbstractArray) = findall(map(testf, A))
Copy link
Member

Choose a reason for hiding this comment

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

This would increase the allocation size as map return a Array{Bool} by default while broadcast return a BitArray.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, didn't know that. Thanks for noticing!

This prevents some invalidations in `mightalias(A::AbstractArray, B::AbstractArray)` in abstractarray.jl when loading Static.jl.
Here we specialize on the function instead of using map since broadcasting returns a BitArray, map a Vector{Bool}.
@@ -2325,7 +2325,7 @@ findall(testf::Function, A) = collect(first(p) for p in pairs(A) if testf(last(p

# Broadcasting is much faster for small testf, and computing
# integer indices from logical index using findall has a negligible cost
findall(testf::Function, A::AbstractArray) = findall(testf.(A))
findall(testf::F, A::AbstractArray) where {F<:Function} = findall(testf.(A))
Copy link
Member Author

@ranocha ranocha Aug 31, 2022

Choose a reason for hiding this comment

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

This is another possibility to fix the invalidations coming from this by specializing on testf since map has also disadvantages, see #46553 (comment). However, always specializing could also have disadvantages in terms of latency, of course. I don't know what's best here 🤷
There are basically four options I see

  • My first suggestion adding a type assertion in mightalias
  • The version with map here, increasing allocations
  • The version with specialization here, possibly increasing compiler workload
  • Just do nothing for now and hope that the compiler will be improved later

@vtjnash
Copy link
Member

vtjnash commented Aug 31, 2022

@aviatesk Could we get Conditional to refine this Type{<:AbstractArray} to DataType because of the explicit isa test? I suspect that would generally reflect better what the user is intending to do with it (though it will mean the tmerge at the phi after the if branch will widen again to just Type)

@timholy
Copy link
Member

timholy commented Aug 31, 2022

I suspect this could deserve a long-ish conversation. I've often wondered about making this change (mostly in map but here in filter too), and wondered what the tradeoff might be. Not having the function be inferrable causes lots of headaches, as you've discovered here. But as you say, specialization contributes to latency.

Previously, my inclination was to wait until precompilation got better, because good precompilation circumvents the latency. But we may have basically arrived there, or will soon once we can cache native code. So maybe the time is ripe for this conversation. There are other issues where we might consider changing our verdict, e.g., #36454.

So I guess the key question is, are there reasonable benchmarks we could use to help make this decision?

@ranocha
Copy link
Member Author

ranocha commented Sep 5, 2022

So I guess the key question is, are there reasonable benchmarks we could use to help make this decision?

I don't think I have really good benchmarks for this. I could of course provide some biased benchmarks from our packages such as Trixi.jl, but we will need more diverse input, I think. In the end, there is no single best benchmark for everybody but we should look at something reasonable for most people.

@vtjnash vtjnash merged commit 31d4c22 into JuliaLang:master Sep 6, 2022
@ranocha ranocha deleted the patch-8 branch September 6, 2022 19:00
aviatesk added a commit that referenced this pull request Sep 12, 2022
Currently our inference isn't able to propagate `isa`-based type
constraint for cases like `isa(Type{<:...}, DataType)` since
`typeintersect` may return `Type` object itself when taking `Type`
object and `iskindtype`-object.

This case happens in the following kind of situation (motivated by the
discussion at <#46553 (comment)>):
```julia
julia> function isa_kindtype(T::Type{<:AbstractVector})
           if isa(T, DataType)
               # `T` here should be inferred as `DataType` rather than `Type{<:AbstractVector}`
               return T.name.name # should be inferred as ::Symbol
           end
           return nothing
       end
isa_kindtype (generic function with 1 method)

julia> only(code_typed(isa_kindtype; optimize=false))
CodeInfo(
1 ─ %1 = (T isa Main.DataType)::Bool
└──      goto #3 if not %1
2 ─ %3 = Base.getproperty(T, :name)::Any
│   %4 = Base.getproperty(%3, :name)::Any
└──      return %4
3 ─      return Main.nothing
) => Any
```

This commit improves the situation by adding a special casing for
abstract interpretation, rather than changing the behavior of
`typeintersect`.
aviatesk added a commit that referenced this pull request Sep 12, 2022
Currently our inference isn't able to propagate `isa`-based type
constraint for cases like `isa(Type{<:...}, DataType)` since
`typeintersect` may return `Type` object itself when taking `Type`
object and `iskindtype`-object.

This case happens in the following kind of situation (motivated by the
discussion at <#46553 (comment)>):
```julia
julia> function isa_kindtype(T::Type{<:AbstractVector})
           if isa(T, DataType)
               # `T` here should be inferred as `DataType` rather than `Type{<:AbstractVector}`
               return T.name.name # should be inferred as ::Symbol
           end
           return nothing
       end
isa_kindtype (generic function with 1 method)

julia> only(code_typed(isa_kindtype; optimize=false))
CodeInfo(
1 ─ %1 = (T isa Main.DataType)::Bool
└──      goto #3 if not %1
2 ─ %3 = Base.getproperty(T, :name)::Any
│   %4 = Base.getproperty(%3, :name)::Any
└──      return %4
3 ─      return Main.nothing
) => Any
```

This commit improves the situation by adding a special casing for
abstract interpretation, rather than changing the behavior of
`typeintersect`.
aviatesk added a commit that referenced this pull request Sep 13, 2022
…cts (#46712)

Currently our inference isn't able to propagate `isa`-based type
constraint for cases like `isa(Type{<:...}, DataType)` since
`typeintersect` may return `Type` object itself when taking `Type`
object and `iskindtype`-object.

This case happens in the following kind of situation (motivated by the
discussion at <#46553 (comment)>):
```julia
julia> function isa_kindtype(T::Type{<:AbstractVector})
           if isa(T, DataType)
               # `T` here should be inferred as `DataType` rather than `Type{<:AbstractVector}`
               return T.name.name # should be inferred as ::Symbol
           end
           return nothing
       end
isa_kindtype (generic function with 1 method)

julia> only(code_typed(isa_kindtype; optimize=false))
CodeInfo(
1 ─ %1 = (T isa Main.DataType)::Bool
└──      goto #3 if not %1
2 ─ %3 = Base.getproperty(T, :name)::Any
│   %4 = Base.getproperty(%3, :name)::Any
└──      return %4
3 ─      return Main.nothing
) => Any
```

This commit improves the situation by adding a special casing for
abstract interpretation, rather than changing the behavior of
`typeintersect`.
ranocha added a commit to ranocha/julia that referenced this pull request Sep 14, 2022
…46553)

This prevents some invalidations in `mightalias(A::AbstractArray, B::AbstractArray)`
in abstractarray.jl when loading Static.jl.

Here we specialize on the function instead of using map since
`broadcasting` returns a BitArray, `map` returns a Vector{Bool}.
@ranocha ranocha mentioned this pull request Sep 14, 2022
28 tasks
@ranocha ranocha added backport 1.8 Change should be backported to release-1.8 compiler:latency Compiler latency labels Sep 14, 2022
KristofferC pushed a commit that referenced this pull request Sep 16, 2022
This prevents some invalidations in `mightalias(A::AbstractArray, B::AbstractArray)`
in abstractarray.jl when loading Static.jl.

Here we specialize on the function instead of using map since
`broadcasting` returns a BitArray, `map` returns a Vector{Bool}.

(cherry picked from commit 31d4c22)
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Sep 30, 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.

5 participants