Improve loading time

[Tokazama]

This supersedes #62 with a better approach to decreasing invalidations. Instead of removing methods for converting StaticInt to Int this subtypes Number. This lets us keep a lot of the basic math methods at the loss of some generic index like behavior, (which we were losing anyway when removing natural conversion to Int).

This gets rid of ALL invalidations on v1.7 and leaves the following on v1.8 and v1.9

SnoopCompile output

 inserting UnitRange{T}(start::Static.StaticNumber, stop) where T<:Real in Static at /Users/zchristensen/projects/Static-proposals/Static.jl/src/Static.jl:300 invalidated:
   mt_backedges: 1: signature Tuple{Type{UnitRange{_A}} where _A, Any, Any} triggered MethodInstance for UnitRange{T}(::Integer, ::Integer) where T<:Real (0 children)
                 2: signature Tuple{Type{UnitRange{_A}} where _A, Any, Any} triggered MethodInstance for UnitRange{T}(::Any, ::Any) where T<:Real (3 children)
   35 mt_cache

 inserting ifelse(::False, x, y) in Static at /Users/zchristensen/projects/Static-proposals/Static.jl/src/Static.jl:85 invalidated:
   mt_backedges: 1: signature Tuple{typeof(ifelse), Any, Real, Real} triggered MethodInstance for min(::T, ::T) where T<:Real (0 children)
                 2: signature Tuple{typeof(ifelse), Any, AbstractFloat, AbstractFloat} triggered MethodInstance for Base._fast(::typeof(max), ::AbstractFloat, ::AbstractFloat) (0 children)
                 3: signature Tuple{typeof(ifelse), Any, AbstractFloat, AbstractFloat} triggered MethodInstance for Base._fast(::typeof(min), ::AbstractFloat, ::AbstractFloat) (0 children)
                 4: signature Tuple{typeof(ifelse), Any, Int64, Int64} triggered MethodInstance for filter!(::Function, ::Vector{Union{Nothing, Base.UUID}}) (0 children)
                 5: signature Tuple{typeof(ifelse), Any, Int64, Int64} triggered MethodInstance for filter!(::Function, ::Vector{Base.UUID}) (0 children)
                 6: signature Tuple{typeof(ifelse), Any, Int64, Int64} triggered MethodInstance for filter!(::Function, ::Vector{Symbol}) (0 children)
                 7: signature Tuple{typeof(ifelse), Any, Int64, Int64} triggered MethodInstance for filter!(::Base.var"#37#39", ::Vector{Any}) (8 children)
   3 mt_cache

I've already made a PR where I tracked down the source of these in to poor inference when filtering when running Pkg. (JuliaLang/julia#45452)

EDIT: PR accepted

Here are the load times:

• v1.7
  • pr: 0.028802 seconds (42.68 k allocations: 2.577 MiB, 19.91% compilation time)
  • master: 0.049850 seconds (89.03 k allocations: 5.158 MiB, 9.87% compilation time)
• v1.8
  • pr: 0.029359 seconds (46.13 k allocations: 2.806 MiB, 17.48% compilation time)
    • still has invalidations now fixed in master
  • master: 0.058451 seconds (100.29 k allocations: 5.878 MiB, 8.20% compilation time)
• v1.9
  • pr: 0.033590 seconds (46.10 k allocations: 2.806 MiB)
  • master: 0.032208 seconds (89.56 k allocations: 5.784 MiB)

These are updated after a70c8e3.
I'm not working on a computer that's great for benchmarking and the time in seconds isn't very accurate, so it's probably more useful to compare allocations between this PR and master

[codecov]

Codecov Report

Merging #64 (277c687) into master (95c4687) will increase coverage by 0.46%.
The diff coverage is 99.25%.

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
+ Coverage   98.82%   99.28%   +0.46%     
==========================================
  Files           8        1       -7     
  Lines         425      420       -5     
==========================================
- Hits          420      417       -3     
+ Misses          5        3       -2     

Impacted Files	Coverage Δ
src/Static.jl	99.28% <99.25%> (+1.50%)	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[ChrisRackauckas]

This does seem like a pretty good solution.

[ChrisRackauckas]

Can we put downstream tests on here?

[Tokazama]

I'll see if I can preemptively reduce some of these errors in ArrayInterface

[chriselrod]

Mind bumping the minor version, to indicate this is a breaking release?
A lot of packages will need to update to say <:Union{Integer,StaticInt}.

[Tokazama]

There are some things I still don't have a solution to here. For example, passing StaticInt to CartesianIndex requires a new method but that creates over 100 invalidations.

[chriselrod]

Which function is invalidated?
Is it deep enough (i.e. not something like eachindex) that we can just define StaticCartesianIndex as an alternative?

[Tokazama]

I came across the issue with this line. It's because AbstractCloseOpen returns a StaticInt for first. It's probably easy to fix in that one package. I'm not sure if it will keep coming up though.

[Tokazama]

@cscherrer it might be good to get at least a preliminary look at this to make sure it gels with what you're doing

[cscherrer]

Thanks @Tokazama. Getting rid of the invalidations is great! I guess this will now have some of the same challenges as Symbolics, for example

julia> @variables i::Int
1-element Vector{Num}:
 i

julia> i isa Integer
false

The biggest concern here is that people get very used to writing methods like f(i::Integer). With Symbolics and now Static going in this direction, there needs to be a more flexible way to write things like this.

I'd normally be worried about this, but having the precedent in place makes it much less of a concern.

[Tokazama]

Can you add a bunch of downstream tests? If those pass, then I think we're ready to rip off the bandaid.

Right now I know that LoopVectorization will fail because it has structs with fields that specify an Integer subtype. I'm trying to do a quick sweep of that stuff and then I'll add some downstream tests

[chriselrod]

There are some things I still don't have a solution to here. For example, passing StaticInt to CartesianIndex requires a new method but that creates over 100 invalidations.

Hmm. It is important that this
https://github.com/JuliaSIMD/LoopVectorization.jl/blob/cbb9f2324ec3cb2c4aa5523d8c4fb98147aa7855/test/offsetarrays.jl#L99
still passes its static size information to LoopVectorization.
It makes a big difference to both compile and runtimes.

Unfortunately, the test only makes sure that the code runs, and not that the passing of static information actually succeeded.
Perhaps I should add a test that CartesianIndices returns an object with static size information.

[Tokazama]

Perhaps I should add a test that CartesianIndices returns an object with static size information.

Do you mean the value returned from iterating CartesianIndices needs to be able to contain static info?

[chriselrod]

Perhaps I should add a test that CartesianIndices returns an object with static size information.

Do you mean the value returned from iterating CartesianIndices needs to be able to contain static info?

Yes, but IIRC CartesianIndices are more flexible than CartesianIndex.

[Tokazama]

CartesianIndices are only supposed to return CartesianIndex. It's just not strictly enforced. I've been playing around with an Indices type that acts more like Dex's index set types and carries bounds checking information. This particular issue might be a good reason to seriously consider implementing it (in ArrayInterface that is)

[ChrisRackauckas]

Revert non-breaking version release seeing the results.

[cscherrer]

It seems to me Irrational is pretty close to the way we'd want StaticFloat64 to be set up:

julia> Irrational{:π} <: Real
true

I don't know of any problems this leads to (are there any?). Could this design could be adapted for StaticFloat64, so we could keep the current subtyping?

[Tokazama]

It seems to me Irrational is pretty close to the way we'd want StaticFloat64 to be set up:

julia> Irrational{:π} <: Real
true

I don't know of any problems this leads to (are there any?). Could this design could be adapted for StaticFloat64, so we could keep the current subtyping?

I'm not sure I understand. Are you suggesting that we assign a symbol as the static value and then use dispatch to derive the float value, like...

struct StaticFloat{sym} <: AbstractFloat end

Base.float(::StaticFloat{:float_one}) = 1.0

[cscherrer]

Are you suggesting that we assign a symbol as the static value and then use dispatch to derive the float value

No, I think the type parameter should be set up to map easily to the "dynamic" value, so in this case just a Float64.

I could be missing something, but to me this seems like evidence the problem can be solved without sacrificing the type hierarchy.

[Tokazama]

Are you suggesting that we assign a symbol as the static value and then use dispatch to derive the float value

No, I think the type parameter should be set up to map easily to the "dynamic" value, so in this case just a Float64.

I could be missing something, but to me this seems like evidence the problem can be solved without sacrificing the type hierarchy.

The subtyping is what causes invalidations, not where we store the values.

[cscherrer]

The subtyping is what causes invalidations, not where we store the values.

I see, so it's not type instability but invalidation, and irrational avoid this problem because they're in Base. Then the design is irrelevant to the problem, and the "just put it in Base" solution is (sadly) not getting much traction. I think I see now, thank you.

[Tokazama]

The subtyping is what causes invalidations, not where we store the values.

I see, so it's not type instability but invalidation, and irrational avoid this problem because they're in Base. Then the design is irrelevant to the problem, and the "just put it in Base" solution is (sadly) not getting much traction. I think I see now, thank you.

Exactly. If you trace back what the original source is for most of these invalidations it poorly inferred methods produced when running Pkg.jl. So I figured that the safest way to avoid invalidations was to use a subtype as far away from anything the package manager would use but still has more functionality than no type hierarchy. So I chose Number.

[chriselrod]

Many packages probably still need updating, but the minor version bump means that's okay and it hopefully won't be too much work to see them update.