Delete unused arguments in subarray code #30789
Conversation
base/subarray.jl
Outdated
| _indices_sub(S::SubArray) = () | ||
| _indices_sub(S::SubArray, ::Real, I...) = (@_inline_meta; _indices_sub(S, I...)) | ||
| function _indices_sub(S::SubArray, i1::AbstractArray, I...) | ||
| _indices_sub(@nospecialize(S::SubArray)) = () |
There was a problem hiding this comment.
Maybe this argument can just be removed completely? I don't see it used anywhere.
There was a problem hiding this comment.
The first argument to reindex also seems to be unused?
There was a problem hiding this comment.
I think it's there in case anyone wants to extend this for another array type? CC @mbauman.
There was a problem hiding this comment.
I think it's just historical cruft in all these cases.
I think when I was writing these methods I started out thinking I'd need it and then never culled things back down.
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
|
Yes, this will probably break GPU code. It would be nice to be able to disable |
|
Let's just remove all these un-used arguments? They weren't intended to provide space for external specialization, but I'm not sure if anyone has actually done it. |
base/subarray.jl
Outdated
| (@_propagate_inbounds_meta; (idxs[1][subidxs[1], subidxs[2]], reindex(V, tail(idxs), tail(tail(subidxs)))...)) | ||
|
|
||
| # In general, we index N-dimensional parent arrays with N indices | ||
| @generated function reindex(V, idxs::Tuple{AbstractArray{T,N}, Vararg{Any}}, subidxs::Tuple{Vararg{Any}}) where {T,N} | ||
| @generated function reindex(@nospecialize(V), idxs::Tuple{AbstractArray{T,N}, Vararg{Any}}, subidxs::Tuple{Vararg{Any}}) where {T,N} |
There was a problem hiding this comment.
I guess in this case we do use V to throw an error message — but it's an error message that should never occur because it should get caught at construction time. Edit: so I think it'd be just fine to throw a slightly less helpful error. Heck, the message even implores for a bug report which I've never seen.
| @@ -164,6 +164,10 @@ function promote_eltype_op end | |||
|
|
|||
| # @deprecate one(i::CartesianIndex) oneunit(i) | |||
| # @deprecate one(::Type{I}) where I<:CartesianIndex oneunit(I) | |||
|
|
|||
| @deprecate reindex(V, idxs, subidxs) reindex(idxs, subidxs) | |||
There was a problem hiding this comment.
I think these should be commented out like the lines above it, since we can't introduce new deprecations until 2.0, if I'm not mistaken.
There was a problem hiding this comment.
These are internal functions, so they don't need deprecations at all unless we're aware that people are using them and we want to be nice.
There was a problem hiding this comment.
I'm not aware of any users, but this seemed harmless and better than throwing a MethodError.
There was a problem hiding this comment.
BTW _indices_sub is not deprecatable, so this doesn't try.
There was a problem hiding this comment.
Friendly reminder to use a trailing false in cases like this to prevent unexported things from being exported. :)
|
@nanosoldier |
timholy
changed the title
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
|
I can't replicate the performance regressions locally (and the worst of them don't use any of the code I changed). The test failures look unrelated. @maleadt, this version shouldn't impact GPU code. |
It was inadvertently exported by #30789. It's tailored to `SubArray` and doesn't check for preconditions that are enforced by the `SubArray` constructor — so while we could export it, I'd like to do so intentionally and add friendlier errors for violations of these preconditions.
* Don't export `reindex` It was inadvertently exported by #30789. It's tailored to `SubArray` and doesn't check for preconditions that are enforced by the `SubArray` constructor — so while we could export it, I'd like to do so intentionally and add friendlier errors for violations of these preconditions. * Same deal for `substrides`
On my machine, this drops the time needed for the subarray tests from 300s to 185s. However, do not merge unless there isn't a performance impact. Also, @maleadt, please check to see if this breaks stuff for you (this is basically the opposite direction of #30666).
@nanosoldier
runbenchmarks(ALL, vs = ":master")