-
Notifications
You must be signed in to change notification settings - Fork 148
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
prevent some unnecessary uses of @pure
in traits.jl
#1177
Conversation
Please check if this holds true with x-ref: #1156 Furthermore |
Now I checked with both |
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.
OK, this should be fine I think. Could you just bump the patch version?
There's no reason for a `Base.@pure` annotation in cases where Julia successfully infers `:consistent`, `:effect_free` and `:terminates_globally` without the annotation. Effect inference tested with Julia v1.9.2, the latest release. While the effect inference is not favorable for `_Length(::Int...)`, as the consistency is not inferred, `_Length` actually seems to be an internal function meant as merely an implementation detail for `Length`, whose effect inference is perfect for the relevant method. The script for testing the effect inference is appended. `:consistent` (`+c`), `:effect_free` (`+e`) and `:terminates_globally` (`+t`) are successfully inferred in each case: ```julia using StaticArrays const d = StaticArraysCore.Dynamic() const test_sizes = ( (1,2,3,4,5,6,7,8,9,1,2,3), (7,8,9,1,2,3,1,2,3,4,5,6), (2,3,4,2,3,4,2,3,4,2,3,4), (1,2,3,4,5,6,7,8,9,1,2,d), (7,8,9,1,2,3,1,2,3,4,5,d), (2,3,4,2,3,4,2,3,4,2,3,d), (d,2,3,4,5,6,7,8,9,1,2,3), (d,8,9,1,2,3,1,2,3,4,5,6), (d,3,4,2,3,4,2,3,4,2,3,4), (1,2,3,4,d,6,7,8,d,1,2,3), (7,8,9,1,d,3,1,2,d,4,5,6), (2,3,4,2,d,4,2,3,d,2,3,4), ) const test_lengths = (0, 1, 2, 3, d) const test_size_types = map((s -> Size{s}), test_sizes) const test_length_types = map((l -> Length{l}), test_lengths) const test_tuple_int_types = ( Tuple{}, Tuple{Int}, Tuple{Int,Int}, Tuple{Int,Int,Int,Int,Int,Int,Int,Int,Int,Int,Int}, Tuple{Int,Int,Int,Int,Int,Int,Int,Int,Int,Int,Int,Int}, Tuple{Int,Int,Int,Int,Int,Int,Int,Int,Int,Int,Int,Int,Int}, ) println("Length(::Size)") for S ∈ test_size_types display(Base.infer_effects(Length, (S,))) end println() println("(::Type{Tuple})(::Size)") for S ∈ test_size_types display(Base.infer_effects(Tuple, (S,))) end println() println("length(::Size)") for S ∈ test_size_types display(Base.infer_effects(length, (S,))) end println() println("length_val(::Size)") for S ∈ test_size_types display(Base.infer_effects(StaticArrays.length_val, (S,))) end println() println("==(::Size, ::Tuple{Vararg{Int}})") for S ∈ test_size_types, T ∈ test_tuple_int_types display(Base.infer_effects(==, (S, T))) end println() println("==(::Tuple{Vararg{Int}}, ::Size)") for S ∈ test_size_types, T ∈ test_tuple_int_types display(Base.infer_effects(==, (T, S))) end println() println("prod(::Size)") for S ∈ test_size_types display(Base.infer_effects(prod, (S,))) end println() println("size_tuple(::Size)") for S ∈ test_size_types display(Base.infer_effects(StaticArrays.size_tuple, (S,))) end println() println("==(::Length, ::Int)") for L ∈ test_length_types display(Base.infer_effects(==, (L, Int))) end println() println("==(::Int, ::Length)") for L ∈ test_length_types display(Base.infer_effects(==, (Int, L))) end println() println("sizematch(::Size, ::Size)") for S1 ∈ test_size_types, S2 ∈ test_size_types display(Base.infer_effects(StaticArrays.sizematch, (S1, S2))) end println() println("diagsize(::Size)") for S ∈ test_size_types display(Base.infer_effects(StaticArrays.diagsize, (S,))) end println() ```
TBH I'm not so sure that this change is a good idea after vchuravy pointed out that LTS Julia v1.6 is still supported, and this change could very well negatively impact performance for that old release of Julia. Maybe it would be better to wait until v1.6 is not supported any more? |
These methods look simple enough that Julia 1.6 should handle them through constant propagation. In the worst case we can revert this change. But I will leave this PR open for a couple of days in case someone wants to comment. |
Should this be merged, as there hasn't been any comment in six months? |
Yes, thank you, I forgot about this PR. |
There's no reason for a
Base.@pure
annotation in cases where Julia successfully infers:consistent
,:effect_free
and:terminates_globally
without the annotation.Effect inference tested with Julia v1.9.2, the latest release.
While the effect inference is not favorable for
_Length(::Int...)
, as the consistency is not inferred,_Length
actually seems to be an internal function meant as merely an implementation detail forLength
, whose effect inference is perfect for the relevant method.The script for testing the effect inference is appended.
:consistent
(+c
),:effect_free
(+e
) and:terminates_globally
(+t
) are successfully inferred in each case: