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

prevent some unnecessary uses of @pure in traits.jl #1177

Merged
merged 2 commits into from
Jan 25, 2024
Merged

prevent some unnecessary uses of @pure in traits.jl #1177

merged 2 commits into from
Jan 25, 2024

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Jul 13, 2023

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:

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("diagize(::Size)")
for S  test_size_types
  display(Base.infer_effects(StaticArrays.diagsize, (S,)))
end
println()

@vchuravy
Copy link
Contributor

vchuravy commented Jul 13, 2023

Please check if this holds true with julia --check-bounds=true. @pure still applies in that case.

x-ref: #1156

Furthermore @pure is backwards compatible to 1.6

@nsajko
Copy link
Contributor Author

nsajko commented Jul 13, 2023

Now I checked with both --check-bounds=no and --check-bounds=yes. In all cases we have +c, +e and +t. Can't check for Julia v1.6, because it doesn't have Base.infer_effects.

Copy link
Collaborator

@mateuszbaran mateuszbaran left a 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()
```
@nsajko
Copy link
Contributor Author

nsajko commented Jul 17, 2023

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?

@mateuszbaran
Copy link
Collaborator

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.

@jishnub
Copy link
Member

jishnub commented Jan 25, 2024

Should this be merged, as there hasn't been any comment in six months?

@mateuszbaran
Copy link
Collaborator

Yes, thank you, I forgot about this PR.

@mateuszbaran mateuszbaran merged commit 5cceb14 into JuliaArrays:master Jan 25, 2024
20 of 29 checks passed
@nsajko nsajko deleted the pure branch January 25, 2024 11:09
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

Successfully merging this pull request may close these issues.

4 participants