-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 from novel Integer conversions #36459
Conversation
One thought (essentially an alternative to #35901 and the suggestion to use runtime dispatch for any non-specialized argument) would be to be able to annotate methods like this: @ifabstract Int ncodeunits(s::AbstractString) = #= implementation=# The idea here is that specific methods can override the return type, but that the supplied type is used if inference is purely abstract, i.e., using just the constraints in the method definition itself. And then inference would use this for methods that called the abstract definition. |
a53e6d5
to
06b86f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I don't like "whack-a-mole" type assertions.:confused:
This is off-topic, but there seems to be room for optimization in integer-->string conversions(e.g. bin
). So I think it's better to tackle the conversion functions on another PR.
Edit:
I submitted PR #36470, which only targets bin()
and so on.
@@ -382,7 +382,7 @@ julia> leading_zeros(Int32(1)) | |||
31 | |||
``` | |||
""" | |||
leading_zeros(x::BitInteger) = ctlz_int(x) % Int | |||
leading_zeros(x::BitInteger) = (ctlz_int(x) % Int)::Int |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
base/intfuncs.jl
Outdated
@inbounds a[i] = (0x30+(x&0x1)::Unsigned)::Unsigned | ||
x = (x >> 1)::Unsigned |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
I'm not a fan either, which is why this PR feels ugly compared to most of the former ones. The issue is that these are code paths that end up getting used by callers with poor inference (sometimes due to |
I think it is perfectly fine to live with a bit of ugliness to get rid of the most egregious invalidations (based on empirical data). When the low hanging fruits are gone, then we are in a better position to evaluate if a more systematic method is needed or if it is better to focus on another angle for attacking invalidations etc. I'm a heavy proponent of not letting perfect be the enemy of good. |
iterate(s::SubstitutionString, i::Integer...) = iterate(s.string, i...) | ||
ncodeunits(s::SubstitutionString) = ncodeunits(s.string)::Int | ||
codeunit(s::SubstitutionString) = codeunit(s.string)::Type{<:Union{UInt8, UInt16, UInt32}} | ||
codeunit(s::SubstitutionString, i::Integer) = codeunit(s.string, i)::Union{UInt8, UInt16, UInt32} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this is true for all string types in practice, but it's not technically required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs say this:
codeunit(s::AbstractString) -> Type{<:Union{UInt8, UInt16, UInt32}}
I've been taking the docs as license to enforce stuff. Should I not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess someone could have a different code unit, but it would be very weird. Not sure all of the logic in base would work correctly with some other code unit.
Defining struct MyInt <: Integer x::Int end (::Type{T})(x::MyInt) where T<:Integer = T(x.x) triggers about 830 unique method invalidations. This fixes the majority of them, but it's a lot of type-annotation.
I've stripped out the part that is now being handled by #36470. If there are no objections, I'll merge this fairly soon. |
Defining
triggers about 830 unique method invalidations. This fixes the majority of them, and most of the rest are nothing to worry about (they are in Core.Compiler, or run in the
normalize_keymap
phase of REPL initialization). One package that adds this kind of method is GeometryTypes, which is used by Plots. This PR, together with JuliaWeb/HTTP.jl#549, fix the last "big" sources of invalidation fromusing Plots
. Altogether there are "only" 440 invalidations left, many of which are probably unconcerning; when I started, my estimate (which is probably ballpark but not entirely comparable due to some changes in how I'm counting) was nearly 10-fold higher (see https://github.com/JuliaLang/www.julialang.org/pull/794/files).EDIT: fun fact, the opposite case (#29166) was handled in #30830.
This PR by itself is not a huge effect, but for the record:
Just the predecessors of this PR (which is better than current
master
), including my customizations of #35877:Adding this PR on top of the previous:
Compared to the timings I posted here for master, this is almost 2s faster, so the cumulative effect is mounting (but also coming to its end, I don't expect much further progress just from fixing invalidations). There's still about 0.8s of precompilation, so getting more to precompile would add a little more benefit.
However, this specific PR should not be merged immediately. For most of my invalidation PRs, I think the improvements in inference can also be justified as improving code-quality. While that applies to some of this PR, a lot of it is just type-annotation, and one might really wish that most of those annotations aren't necessary. Consequently I've marked this RFC because it's worth asking whether there is a more systematic way to achieve the same benefits without all the annotation.