-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
generic size: avoid method static parameters and abstract type assert
#59465
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
Conversation
|
Indeed, this PR is a prerequisite for PR #59442. Otherwise three of these effect inference tests fail: julia/Compiler/test/effects.jl Lines 792 to 797 in cdd4ac5
|
701c909 to
94abb7f
Compare
The generic method `size(::AbstractArray, ::Any)` currently has: * method static parameters * an abstract type assert (`::Integer`) in the method body PR JuliaLang#59442 aims to make this generic method be used for `Array` and `Memory`, by deleting the more specific methods. It is my understanding that method static parameters and abstract type asserts can cause performance issues, when they're not optimized out, which happens when the argument types are not concretely inferred. So I'm putting up this PR, which eliminates the method static parameters and the `typeassert`, to prevent any possible regressions from PR JuliaLang#59442. Another thing that is necessary for preserving good abstract inference is to convert the index to `Int` before using it. This is correct it there can't be more than `typemax(Int)` dimensions anyway. (cherry picked from PR JuliaLang#59465)
The generic method `size(::AbstractArray, ::Any)` currently has: * method static parameters * an abstract type assert (`::Integer`) in the method body PR JuliaLang#59442 aims to make this generic method be used for `Array` and `Memory`, by deleting the more specific methods. It is my understanding that method static parameters and abstract type asserts can cause performance issues, when they're not optimized out, which happens when the argument types are not concretely inferred. So I'm putting up this PR, which eliminates the method static parameters and the `typeassert`, to prevent any possible regressions from PR JuliaLang#59442. Another thing that is necessary for preserving good abstract inference is to convert the index to `Int` before using it. This is correct it there can't be more than `typemax(Int)` dimensions anyway. (cherry picked from PR JuliaLang#59465)
The generic method `size(::AbstractArray, ::Any)` currently has: * method static parameters * an abstract type assert (`::Integer`) in the method body PR JuliaLang#59442 aims to make this generic method be used for `Array` and `Memory`, by deleting the more specific methods. It is my understanding that method static parameters and abstract type asserts can cause performance issues, when they're not optimized out, which happens when the argument types are not concretely inferred. So I'm putting up this PR, which eliminates the method static parameters and the `typeassert`, to prevent any possible regressions from PR JuliaLang#59442. Another thing that is necessary for preserving good abstract inference is to convert the index to `Int` before using it. This is correct it there can't be more than `typemax(Int)` dimensions anyway. (cherry picked from PR JuliaLang#59465)
8302694 to
4f3b8c2
Compare
The generic method `size(::AbstractArray, ::Any)` currently has: * method static parameters * an abstract type assert (`::Integer`) in the method body PR JuliaLang#59442 aims to make this generic method be used for `Array` and `Memory`, by deleting the more specific methods. It is my understanding that method static parameters and abstract type asserts can cause performance issues, when they're not optimized out, which happens when the argument types are not concretely inferred. So I'm putting up this PR, which eliminates the method static parameters and the `typeassert`, to prevent any possible regressions from PR JuliaLang#59442. Another thing that is necessary for preserving good abstract inference is to convert the index to `Int` before using it. This is correct it there can't be more than `typemax(Int)` dimensions anyway. (cherry picked from PR JuliaLang#59465)
The generic method `size(::AbstractArray, ::Any)` currently has: * method static parameters * an abstract type assert (`::Integer`) in the method body PR JuliaLang#59442 aims to make this generic method be used for `Array` and `Memory`, by deleting the more specific methods. It is my understanding that method static parameters and abstract type asserts can cause performance issues, when they're not optimized out, which happens when the argument types are not concretely inferred. So I'm putting up this PR, which eliminates the method static parameters and the `typeassert`, to prevent any possible regressions from PR JuliaLang#59442. Another thing that is necessary for preserving good abstract inference is to convert the index to `Int` before using it. This is correct it there can't be more than `typemax(Int)` dimensions anyway. (cherry picked from PR JuliaLang#59465)
|
bump |
|
ping |
vtjnash
left a comment
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 don't like this. It changes clear code into a mess. Changing to simplified code like this would be okay though:
function size(t::AbstractArray, dim)
d = Int(dim)::Int
s = size(t)
d <= length(s) ? s[d] : 1
end
a4f8310 to
4ae27ee
Compare
|
bump? or merge PR #59512? |
The generic method `size(::AbstractArray, ::Any)` currently has: * method static parameters * an abstract type assert (`::Integer`) in the method body PR JuliaLang#59442 aims to make this generic method be used for `Array` and `Memory`, by deleting the more specific methods. It is my understanding that method static parameters and abstract type asserts can cause performance issues, when they're not optimized out, which happens when the argument types are not concretely inferred. So I'm putting up this PR, which eliminates the method static parameters and the `typeassert`, to prevent any possible regressions from PR JuliaLang#59442. Another thing that is necessary for preserving good abstract inference is to convert the index to `Int` before using it. This is correct it there can't be more than `typemax(Int)` dimensions anyway.
Now this PR changes behavior: non-`Integer` values are allowed for the second argument as long as they are convertible to `Int`.
6655c96 to
91a1978
Compare
|
@vtjnash your suggestion was implemented, however your negative review is still around. Also, there's now an approval. Can you take a look again? |
Changes:
* Eliminate some nongeneric `length` methods:
* `GenericMemory`
* `Slice`
* `IdentityUnitRange`
* `CartesianIndices`
* `LogicalIndex`
* `CodeUnits`
* `UnsafeView` (from the `Random` stdlib)
* Eliminate some nongeneric two-argument `size` methods:
* `GenericMemory`
* `Array`
* `BitVector`
Depends on PR #59465 to prevent abstract inference regressions.
…rt (JuliaLang#59465) The generic method `size(::AbstractArray, ::Any)` currently has: * method static parameters * an abstract type assert (`::Integer`) in the method body PR JuliaLang#59442 aims to make this generic method be used for `Array` and `Memory`, by deleting the more specific methods. It is my understanding that method static parameters and abstract type asserts can cause performance issues, when they're not optimized out, which happens when the argument types are not concretely inferred. So I'm putting up this PR, which eliminates the method static parameters and the `typeassert`, to prevent any possible regressions from PR JuliaLang#59442. Another thing that is necessary for preserving good abstract inference is to convert the index argument to `Int` before using it. This is correct as there can't be more than `typemax(Int)` dimensions anyway (the `N` type parameter to `AbstractArray` is an `Int` value).
…9442) Changes: * Eliminate some nongeneric `length` methods: * `GenericMemory` * `Slice` * `IdentityUnitRange` * `CartesianIndices` * `LogicalIndex` * `CodeUnits` * `UnsafeView` (from the `Random` stdlib) * Eliminate some nongeneric two-argument `size` methods: * `GenericMemory` * `Array` * `BitVector` Depends on PR JuliaLang#59465 to prevent abstract inference regressions.
The generic method
size(::AbstractArray, ::Any)currently has:method static parameters
an abstract type assert (
::Integer) in the method bodyPR #59442 aims to make this generic method be used for
ArrayandMemory, by deleting the more specific methods.It is my understanding that method static parameters and abstract type asserts can cause performance issues, when they're not optimized out, which happens when the argument types are not concretely inferred.
So I'm putting up this PR, which eliminates the method static parameters and the
typeassert, to prevent any possible regressions from PR #59442.Another thing that is necessary for preserving good abstract inference is to convert the index argument to
Intbefore using it. This is correct as there can't be more thantypemax(Int)dimensions anyway (theNtype parameter toAbstractArrayis anIntvalue).