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

Making StaticInt an Integer again? #73

Open
oschulz opened this issue Jun 23, 2022 · 19 comments
Open

Making StaticInt an Integer again? #73

oschulz opened this issue Jun 23, 2022 · 19 comments

Comments

@oschulz
Copy link

oschulz commented Jun 23, 2022

Since Static v0.7, StaticInt is not a subtype of Integer anymore. Would it be possible to change that again? Many constructions that used StaticInt in places that require an Integer just aren't possible anymore now. For example, Base.OneTo(static(1)) doesn't work anymore and I'm sure there are lots of other uses cases that will find it hard (or impossible) to adapt their code to v0.7).

CC @cscherrer

@chriselrod
Copy link
Collaborator

Base.oneto(static(N)) should return static(1):static(N). I'd be fine with having Base.OneTo also do this, but some people dislike having "constructors" return objects of different types.

The issue with making StaticInt an integer is that it causes a tremendous amount of invalidations.

@cscherrer
Copy link
Contributor

Is there any update on the suggestion to get StaticInt into Base? This seems like the obvious solution. There are concrete problems that arise from not doing this, are there any that would be caused by doing it? Is there somewhere we can help argue the case?

Also... Are there also invalidations caused by making StaticFloat64 <: AbstractFloat?

@oschulz
Copy link
Author

oschulz commented Jun 23, 2022

Base.oneto(static(N)) should return static(1):static(N).

We can add that, right?

@Tokazama
Copy link
Collaborator

That's a bit tricky because we'd have to add the optionally static range types and then we are moving into ArrayInterface territory. The separation is far from perfect but there are certain components here that just don't need to be in ArrayInterface

@Tokazama
Copy link
Collaborator

Can we change it back to to a subtype of Integer, yes. However, it would be difficult to accomplish because it causes a ridiculous amount of invalidations. This varies between Julia releases because there's on ongoing effort to reduce ambiguities that cause invalidations in base; but integer is used so widely that they pop up unexpectedly all the time.

@Tokazama
Copy link
Collaborator

Here's the issue you can follow to voice opinions on its addition to base JuliaLang/julia#44538 (comment).

@oschulz
Copy link
Author

oschulz commented Jun 23, 2022

Thanks for the detailed explanation @Tokazama - large numbers of invalidations are indeed definitely not great.

@cscherrer , do you think we can use something like static(1):static(n) directly?

@Tokazama
Copy link
Collaborator

I'm open to PRs and suggestions, but I went through at least 3 rewrites of this package trying to find a solution that worked at all. At one point I started speaking in tongues (well, binary). So be careful going into any rewrites for your own sanity's well being

@oschulz
Copy link
Author

oschulz commented Jun 23, 2022

Oh, sorry - I meant work around StaticInt not being an Integer anymore in downsteam packages.

@Tokazama
Copy link
Collaborator

@cscherrer, if you're interested in separating out StataticFloat64 as a subtype of AbstractFloat I'd be willing to review a PR. I would caution you to look out for how much that proportionally increases load time though.

@oschulz oschulz changed the title Making StaticInt and Integer again? Making StaticInt an Integer again? Jun 24, 2022
@oschulz
Copy link
Author

oschulz commented Nov 1, 2022

I've been trying to find a range type that works with the new static - static(1):static(5) doesn't work, the only thing that seems to work is static(1):static(1):static(5), which is really cumbersome and hard to fit into generic code that naturally would deal with UnitRanges.

Do the invalidations of "static being an integer" actually affect Static.jl itself or packages that use it?

@Tokazama
Copy link
Collaborator

Tokazama commented Nov 2, 2022

IIRC invalidations explode when we define conversion to Int.

@Tokazama
Copy link
Collaborator

Tokazama commented Nov 2, 2022

Here's the big one we get when making a new subtype of Integer and converting it to Int

 inserting (::Type{T})(x::StaticInts.StaticInt) where T<:Integer in StaticInts at /Users/zchristensen/projects/Static.jl/libs/StaticInts/src/StaticInts.jl:27 invalidated:
   mt_backedges:  1: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for convert(::Type{Int64}, ::Integer) (0 children)
                  2: signature Tuple{Type{UInt64}, Integer} triggered MethodInstance for convert(::Type{UInt64}, ::Integer) (0 children)
                  3: signature Tuple{Type{UInt32}, Integer} triggered MethodInstance for VersionNumber(::Integer, ::Int64, ::Int64, ::Tuple{}, ::Tuple{}) (1 children)
                  4: signature Tuple{Type{UInt64}, Integer} triggered MethodInstance for UInt64(::Enum) (1 children)
                  5: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for Int64(::Enum) (1 children)
                  6: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for insert!(::BitVector, ::Integer, ::Any) (2 children)
                  7: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for findprev(::InteractiveUtils.var"#34#39", ::AbstractString, ::Integer) (3 children)
                  8: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for Base._array_for(::Type{Union{Int64, Symbol}}, ::Base.HasLength, ::Integer) (6 children)
                  9: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for Base._array_for(::Type{Base.PkgId}, ::Base.HasLength, ::Integer) (6 children)
                 10: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for findnext(::Pkg.Types.var"#27#28"{String}, ::AbstractString, ::Integer) (38 children)
                 11: signature Tuple{Type{Int64}, Integer} triggered MethodInstance for Base.to_shape(::Integer) (470 children)

@Tokazama
Copy link
Collaborator

Tokazama commented Nov 2, 2022

This is all you need to reproduce

module StaticInts

struct StaticInt{N} <: Integer
    StaticInt{N}() where {N} = new{N::Int}()
end

Base.Int(::StaticInt{N}) where {N} = N

end

@oschulz
Copy link
Author

oschulz commented Nov 30, 2022

@Tokazama , following up on the discussion on #87, I tried just changing StaticInteger{N} <: Number to StaticInteger{N} <: Integer as in initial test and counted invalidations via

using SnoopCompileCore
invalidations = @snoopr begin
    using Static
end
using SnoopCompile 
length(uinvalidated(invalidations))

and load time with @time_imports (20 measurements each time).

  • With StaticInteger{N} <: Number I get 470 invalidations and a load time of (40 ± 2) ms

  • With StaticInteger{N} <: Integer I get 801 invalidations and a load time of (46 ± 3) ms.

Given that, could we pursue making StaticInteger and Integer (which will re-enable statically sized Fill arrays and so on), or is that number of invalidations unacceptable?

Would StaticInteger{N} <: Integer allow us to remove OptionallyStaticUnitRange and OptionallyStaticStepRange? That might bring invalidations down again:

  • With StaticInteger{N} <: Integer , no "ranges.jl": 475 invalidations, load time 475 (30 ± 1) ms.

@chriselrod
Copy link
Collaborator

Would StaticInteger{N} <: Integer allow us to remove OptionallyStaticUnitRange and OptionallyStaticStepRange?

No.
I suggest looking at the invalidations and PRing to remove the type instabilities that let them happen.

@Tokazama
Copy link
Collaborator

IRRC, a lot of the current invalidations are due to things with StaticBool and have improved on Julia's master branch.

@oschulz
Copy link
Author

oschulz commented Nov 30, 2022

Ok so there no hard blockers? Then I'd try my hand at a StaticInteger{N} <: Integer PR.

@Tokazama
Copy link
Collaborator

I think the range invalidation s are mostly due to similar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants