RFC: Change Array(T, dims) to Array{T}(dims) #17583
Conversation
|
This was seen to have performance implications if not done carefully. In particular, it's best to add the N parameter also so that it becomes a leaf type. (The two in the deprecation set don't need to change, esp. since the N isn't available for them) |
|
Sorry, I wasn't aware of the performance regressions! I'll go through and add the |
|
Is it the array changes here that broke everything? I can't really tell. |
|
I think the answer there is "yes." I'll leave stuff like this to the experts. |
| _array_for(T, itr, ::HasLength) = Array(T, Int(length(itr)::Integer)) | ||
| _array_for(T, itr, ::HasShape) = Array(T, convert(Dims,size(itr))) | ||
| _array_for(T, itr, ::HasLength) = Array{T,1}(Int(length(itr)::Integer)) | ||
| _array_for(T, itr, ::HasShape) = Array{T,1}(convert(Dims,size(itr))) |
Carry on – you don't become an expert without trying stuff like this! |
| _array_for(T, itr, ::HasLength) = Array(T, Int(length(itr)::Integer)) | ||
| _array_for(T, itr, ::HasShape) = Array(T, convert(Dims,size(itr))) | ||
| _array_for(T, itr, ::HasLength) = Array{T,ndims(itr)}(Int(length(itr)::Integer)) | ||
| _array_for(T, itr, ::HasShape) = Array{T,ndims(itr)}(convert(Dims,size(itr))) | ||
|
|
There was a problem hiding this comment.
I'd leave the first one as Array{T,1} (it is the only Array{T,N} that can have a single Int as argument). The second one is probably better just as Array{T} since the ndims(itr) will make it non inferable.
There was a problem hiding this comment.
I had no idea that the ndims(itr) would ruin the type inference. @vtjnash Is there a way to specify the N in this case that preserves inferability?
There was a problem hiding this comment.
I might have expressed a bit wrong. The problem is that since ndims(itr) is not inferable the compiler cannot inline the constructor where it might help speed something.
But don't worry. I believe this got you covered here: https://github.com/JuliaLang/julia/pull/16851/files#diff-4cbc8eb916240b8a07540daa4316d1c5R319
There was a problem hiding this comment.
Oh, I see. I changed it to Array{Int} per your suggestion and so far the CI hasn't failed. Thanks for your help! 👍
|
Woo, thanks to @pabloferz things are passing! Since @tkelman and @vtjnash cited performance concerns, should we check for performance regressions here using nanosoldier? |
|
go team |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
|
That |
|
What did I change that would affect string joins? |
|
perhaps |
|
Ah, okay. Thanks for all of your help here, everyone. And thanks for the encouragement, @StefanKarpinski. |
In the documentation for
Arrayin base/docs/helpdb/Base.jl, it's claimed that the syntaxArray(T, dims)is available but deprecated. (Though there's no actual deprecation warning or anything for it.) I figured that if we're claiming thatArray(T, dims)shouldn't be used, then we shouldn't be using it ourselves. 😉