Improvements to using Static time
#55
Conversation
Trying to get rid of any load times we can without losing functionality. Most of these changes are in response to the `StaticInt` PR to Base, but there was also some code consolidation that helped reduce load time. This changes master branch results from ``` julia> @time using Static 0.051377 seconds (97.85 k allocations: 5.779 MiB, 9.30% compilation time) ``` To this... ``` julia> @time using Static 0.039661 seconds (76.61 k allocations: 4.347 MiB, 11.46% compilation time) ``` The "precompile.jl" file is a bit of a cherry picked feed out from SnoopCompile, but isn't currently included because it makes `using` slower for some reason.
Codecov Report
@@ Coverage Diff @@
## master #55 +/- ##
==========================================
- Coverage 98.94% 98.80% -0.15%
==========================================
Files 8 8
Lines 476 417 -59
==========================================
- Hits 471 412 -59
Misses 5 5
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
| Base.length(::NDIndex{N}) where {N} = N | ||
| Base.length(::Type{NDIndex{N,I}}) where {N,I} = N | ||
| Base.length(@nospecialize(x::NDIndex))::Int = length(Tuple(x)) | ||
| Base.length(@nospecialize(T::Type{<:NDIndex}))::Int = @inbounds(T.parameters[1]) |
There was a problem hiding this comment.
This change is incorrect. There is no guarantee about the order of parameters in a type that is <:NDIndex (or that they're even DataTypes at all:
julia> const NDIndex2{I<:Tuple{Vararg{Union{StaticInt,Int},2}}} = NDIndex{2, I}
NDIndex{2, I} where I<:Tuple{Union{Int64, StaticInt}, Union{Int64, StaticInt}}
julia> length(ans)
ERROR: type UnionAll has no field parameters
Stacktrace:
[1] getproperty
@ ./Base.jl:32 [inlined]
[2] length(T::Type{<:NDIndex})
@ Static ~/.julia/packages/Static/x0MGi/src/ndindex.jl:68
[3] top-level scope
@ REPL[3]:1
There was a problem hiding this comment.
There is no guarantee about the order of parameters in a type that is <:NDIndex (or that they're even DataTypes at all:
I'm assuming we can still count on the order of parameters once we know it's a DataType thought right? So unwrapping UnionAll would work like:
function Base.length(@nospecialize(T::Type{<:NDIndex}))::Int
if T isa UnionAll
return length(T.body)
else
return @inbounds(T.parameters[1])
end
endThere was a problem hiding this comment.
No, that's broken also. It could at the very least be a Union and even then you're relying on normalization rules that are not guaranteed to be stable. Just use the form it had before. That's the correct way to write it. If there's a performance issue with that, file a base issue.
3 participants
Results
For Julia v1.6 this changes
@time using Staticfrom 0.054736 to 0.043993.For Julia v1.7 this changes
@time using Staticfrom 0.048681 to 0.038870.Implementation
A lot of the improvement here is because redundant method definitions identified in the StaticInt PR to Base (e.g.,
Base.:(*)(::Zero, ::Zero)). As discussed in that PR, these are already known at compile time so explicitly defining them doesn't improve performance.I spent a pretty good amount of time with SnoopCompile and identified and simplified some method influence compile time. I tried generating a precompile file but it made start up time worse, so I'll have to look into that more later. This might be as good of an improvement as we can get while we still have invalidations in Base we can't resolve.