Skip to content

Commit

Permalink
Change i=1:length(A) to i in eachindex(A)
Browse files Browse the repository at this point in the history 
This fixes performance problems for many SubArray operations
  • Loading branch information
@timholy
timholy committed Apr 19, 2015
1 parent fe0b3ab commit 4297135
Show file tree
Hide file tree
Showing 9 changed files with 52 additions and 34 deletions.
19 changes: 15 additions & 4 deletions base/abstractarray.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,11 @@ linearindexing{T<:AbstractArray}(::Type{T}) = LinearSlow()
linearindexing{T<:Array}(::Type{T}) = LinearFast()
linearindexing{T<:Range}(::Type{T}) = LinearFast()

*(::LinearFast, ::LinearFast) = LinearFast()

This comment has been minimized.

Sign in to view

Copy link
@JeffBezanson

JeffBezanson Jun 4, 2015

Member

This is kind of weird. Could we use a different function for this?

This comment has been minimized.

Sign in to view

Copy link
@mbauman

mbauman Jun 4, 2015

Member

I think I'd like to try revamping the eachindex interface to always return the same number of iterators as arguments. That is, something effectively like eachindex(A,B,C) = zip(map(eachindex, (A, B, C))...). That way you can commingle different iterator types together without trouble. (It also paves the way towards a broadcasting eachindex/Cartesian implementation...)

This comment has been minimized.

Sign in to view

Copy link
@timholy

timholy Jun 4, 2015

Author Member

Would & be better, or just a separate function like promote_linear?

I'm not sure I agree with eachindex(A,B) returning two iterators. You can already do the zip thing manually. What's otherwise lacking is an easy way to generate the "least common denominator" iterator for multiple objects. I don't remember my benchmarks on this, but I seem to remember that incrementing two rather than one is a noticeable performance hit for a tight loop.

This comment has been minimized.

Sign in to view

Copy link
@StefanKarpinski

StefanKarpinski Jun 4, 2015

Member

This seems very much to be operator punning (as does &). @timholy, can you spell out what this is doing here (on the premise that explaining it will suggest good names)?

This comment has been minimized.

Sign in to view

Copy link
@mbauman

mbauman Jun 4, 2015

Member

(On the premise that someone who didn't code it may have a different vocabulary, I'll give it a shot): The idea is that when indexing through two arrays at the same time, you want to use the fastest indexing method available to both of them. This implements that choice — it's like a 'fastest common denominator' or 'common supported functionality.' Hm. Those are really long. I don't think I'd call this 'promote,' but maybe 'combine' or 'merge.'

This comment has been minimized.

Sign in to view

Copy link
@StefanKarpinski

StefanKarpinski Jun 4, 2015

Member

This seems like it may be worthy of its own vocabulary – generic array iteration is fairly fresh territory.

This comment has been minimized.

Sign in to view

Copy link
@mbauman

mbauman Jun 4, 2015

Member

Actually, intersect seems like it represents the operation fairly well in my mind. Still a little bit of a pun, but you're intersecting the fast indexing capabilities?

This comment has been minimized.

Sign in to view

Copy link
@nalimilan

nalimilan Jun 4, 2015

Member

intersect sounds logical. But this might deserve more thought, if you consider it more broadly in terms of what could become traits in the future. If traits could inherit from one another (just like types currently do), this would be more like finding the common ancestor to two traits. I.e., LinearFast would be a trait that some types implement, and LinearSlow would just be a fallback used for all other types, or for combinations where LinearFast is not a common ancestor to both types.

This comment has been minimized.

Sign in to view

Copy link
@JeffBezanson

JeffBezanson Jun 4, 2015

Member

It strikes me as most similar to type promotion, though maybe not identical.

This comment has been minimized.

Sign in to view

Copy link
@andreasnoack

andreasnoack Jun 4, 2015

Member

It's exactly the rules of boolean & with LinearFast=true so I don't think & is that weird. You could say the same with ({0,1},*) and LinearFast=1 so I don't think * is that weird either.

This comment has been minimized.

Sign in to view

Copy link
@timholy

timholy Jun 4, 2015

Author Member

What @andreasnoack said. But I suggested the analogy to promotion because it's also similar (Float64+Float64=Float64, Float64+Int=Float64, Int+Int=Int, so Int=true and Float64=false).

This comment has been minimized.

Sign in to view

Copy link
@StefanKarpinski

StefanKarpinski Jun 4, 2015

Member

Correct me if I'm misunderstanding here. This is an protocol which people can add methods to in order to hook into the generic efficient indexing framework, right? I.e. this is more like promote_rule – a hook point into a system – than it is an operation that people directly use. In that case it's not exactly "private" – naming does matter – but it's also not appropriate to coopt a normal operator like * or & or intersect, regardless of if the implementation or behavior
happens to be similar. How about calling this index_rule or something like that to mirror promote_rule?

This comment has been minimized.

Sign in to view

Copy link
@ScottPJones

ScottPJones Jun 4, 2015

Contributor

@StefanKarpinski 's argument makes sense... index_rule maybe?

This comment has been minimized.

Sign in to view

Copy link
@timholy

timholy Jun 4, 2015

Author Member

linearindexing(A) is a protocol/hook, but this * method has exactly one client, eachindex, and is not something users are expected to touch. Basically the rule is "use a linear index if we can get away with it (if all arrays support efficient linear indexing), otherwise use a CartesianIndex". This function just implements the AND logic.

This comment has been minimized.

Sign in to view

Copy link
@StefanKarpinski

StefanKarpinski Jun 4, 2015

Member

Ok, then this definitely shouldn't overload a public function.

This comment has been minimized.

Sign in to view

Copy link
@timholy

timholy Jun 4, 2015

Author Member

Yeah, I shouldn't have done that. Sorry, folks. Fix coming soon.

This comment has been minimized.

Sign in to view

Copy link
@timholy

timholy Jun 4, 2015

Author Member

Hmm, we can just re-use linearindexing and have a 2-argument (or n-argument) version.

linearindexing isn't currently exported, but perhaps it should be? It's used only to set a trait for new AbstractArray types.

This comment has been minimized.

Sign in to view

Copy link
@mbauman

mbauman Jun 4, 2015

Member

Ah, I like that. And, yes, it's not exported. I documented it but kept it unexported, with the thinking that you need to explicitly import it anyways and almost all user use-cases will be simply defining the method.
*(::LinearSlow, ::LinearFast) = LinearSlow()
*(::LinearFast, ::LinearSlow) = LinearSlow()
*(::LinearSlow, ::LinearSlow) = LinearSlow()

## Bounds checking ##
checkbounds(sz::Int, ::Colon) = nothing
checkbounds(sz::Int, i::Int) = 1 <= i <= sz || throw(BoundsError())
Expand Down Expand Up @@ -350,9 +355,15 @@ next(A::AbstractArray,i) = (@_inline_meta(); (idx, s) = next(i[1], i[2]); (A[idx
done(A::AbstractArray,i) = done(i[1], i[2])

# eachindex iterates over all indices. LinearSlow definitions are later.
eachindex(A::AbstractArray) = (@_inline_meta; eachindex(linearindexing(A), A))
eachindex(A::AbstractArray) = (@_inline_meta(); eachindex(linearindexing(A), A))
eachindex(::LinearFast, A::AbstractArray) = 1:length(A)

function eachindex(A::AbstractArray, B::AbstractArray)
@_inline_meta
eachindex(linearindexing(A)*linearindexing(B), A, B)
end
eachindex(::LinearFast, A::AbstractArray, B::AbstractArray) = 1:max(length(A),length(B))

isempty(a::AbstractArray) = (length(a) == 0)

## Conversions ##
Expand Down Expand Up @@ -431,7 +442,7 @@ getindex(t::AbstractArray, i::Real) = error("indexing not defined for ", typeof(
# linear indexing with a single multi-dimensional index
function getindex(A::AbstractArray, I::AbstractArray)
x = similar(A, size(I))
for i=1:length(I)
for i in eachindex(I)
x[i] = A[I[i]]
end
return x
Expand Down Expand Up @@ -856,7 +867,7 @@ function isequal(A::AbstractArray, B::AbstractArray)
if isa(A,Range) != isa(B,Range)
return false
end
for i = 1:length(A)
for i in eachindex(A)
if !isequal(A[i], B[i])
return false
end
Expand All @@ -880,7 +891,7 @@ function (==)(A::AbstractArray, B::AbstractArray)
if isa(A,Range) != isa(B,Range)
return false
end
for i = 1:length(A)
for i in eachindex(A)
if !(A[i]==B[i])
return false
end
Expand Down
28 changes: 14 additions & 14 deletions base/array.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ function fill!{T<:Union(Integer,FloatingPoint)}(a::Array{T}, x)
ccall(:memset, Ptr{Void}, (Ptr{Void}, Cint, Csize_t),
a, 0, length(a)*sizeof(T))
else
for i = 1:length(a)
for i in eachindex(a)
@inbounds a[i] = xT
end
end
Expand Down Expand Up @@ -703,7 +703,7 @@ end
## Unary operators ##

function conj!{T<:Number}(A::AbstractArray{T})
for i=1:length(A)
for i in eachindex(A)
A[i] = conj(A[i])
end
return A
Expand All @@ -713,15 +713,15 @@ for f in (:-, :~, :conj, :sign)
@eval begin
function ($f)(A::StridedArray)
F = similar(A)
for i=1:length(A)
for i in eachindex(A)
F[i] = ($f)(A[i])
end
return F
end
end
end

(-)(A::StridedArray{Bool}) = reshape([ -A[i] for i=1:length(A) ], size(A))
(-)(A::StridedArray{Bool}) = reshape([ -A[i] for i in eachindex(A) ], size(A))

real(A::StridedArray) = reshape([ real(x) for x in A ], size(A))
imag(A::StridedArray) = reshape([ imag(x) for x in A ], size(A))
Expand All @@ -730,7 +730,7 @@ imag{T<:Real}(x::StridedArray{T}) = zero(x)

function !(A::StridedArray{Bool})
F = similar(A)
for i=1:length(A)
for i in eachindex(A)
F[i] = !A[i]
end
return F
Expand Down Expand Up @@ -789,7 +789,7 @@ for f in (:+, :-, :div, :mod, :&, :|, :$)
end
function ($f){S,T}(A::AbstractArray{S}, B::AbstractArray{T})
F = similar(A, promote_type(S,T), promote_shape(size(A),size(B)))
for i=1:length(A)
for i in eachindex(A,B)
@inbounds F[i] = ($f)(A[i], B[i])
end
return F
Expand All @@ -800,14 +800,14 @@ for f in (:.+, :.-, :.*, :.%, :.<<, :.>>, :div, :mod, :rem, :&, :|, :$)
@eval begin
function ($f){T}(A::Number, B::AbstractArray{T})
F = similar(B, promote_array_type(typeof(A),T))
for i=1:length(B)
for i in eachindex(B)
@inbounds F[i] = ($f)(A, B[i])
end
return F
end
function ($f){T}(A::AbstractArray{T}, B::Number)
F = similar(A, promote_array_type(typeof(B),T))
for i=1:length(A)
for i in eachindex(A)
@inbounds F[i] = ($f)(A[i], B)
end
return F
Expand All @@ -830,14 +830,14 @@ for f in (:.+, :.-)
@eval begin
function ($f)(A::Bool, B::StridedArray{Bool})
F = similar(B, Int, size(B))
for i=1:length(B)
for i in eachindex(B)
@inbounds F[i] = ($f)(A, B[i])
end
return F
end
function ($f)(A::StridedArray{Bool}, B::Bool)
F = similar(A, Int, size(A))
for i=1:length(A)
for i in eachindex(A)
@inbounds F[i] = ($f)(A[i], B)
end
return F
Expand All @@ -848,7 +848,7 @@ for f in (:+, :-)
@eval begin
function ($f)(A::StridedArray{Bool}, B::StridedArray{Bool})
F = similar(A, Int, promote_shape(size(A), size(B)))
for i=1:length(A)
for i in eachindex(A,B)
@inbounds F[i] = ($f)(A[i], B[i])
end
return F
Expand All @@ -861,23 +861,23 @@ end
function complex{S<:Real,T<:Real}(A::Array{S}, B::Array{T})
if size(A) != size(B); throw(DimensionMismatch()); end
F = similar(A, typeof(complex(zero(S),zero(T))))
for i=1:length(A)
for i in eachindex(A)
@inbounds F[i] = complex(A[i], B[i])
end
return F
end

function complex{T<:Real}(A::Real, B::Array{T})
F = similar(B, typeof(complex(A,zero(T))))
for i=1:length(B)
for i in eachindex(B)
@inbounds F[i] = complex(A, B[i])
end
return F
end

function complex{T<:Real}(A::Array{T}, B::Real)
F = similar(A, typeof(complex(zero(T),B)))
for i=1:length(A)
for i in eachindex(A)
@inbounds F[i] = complex(A[i], B)
end
return F
Expand Down
6 changes: 3 additions & 3 deletions base/floatfuncs.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ for f in (:trunc,:floor,:ceil,:round)
[ ($f)(T, x[i,j])::T for i = 1:size(x,1), j = 1:size(x,2) ]
end
function ($f){T}(::Type{T}, x::AbstractArray)
reshape([ ($f)(T, x[i])::T for i = 1:length(x) ], size(x))
reshape([ ($f)(T, x[i])::T for i in eachindex(x) ], size(x))
end
end
end
Expand All @@ -85,7 +85,7 @@ function round{R}(x::AbstractArray{R,2}, r::RoundingMode)
[ round(x[i,j], r) for i = 1:size(x,1), j = 1:size(x,2) ]
end
function round(x::AbstractArray, r::RoundingMode)
reshape([ round(x[i], r) for i = 1:length(x) ], size(x))
reshape([ round(x[i], r) for i in eachindex(x) ], size(x))
end

function round{T,R}(::Type{T}, x::AbstractArray{R,1}, r::RoundingMode)
Expand All @@ -95,7 +95,7 @@ function round{T,R}(::Type{T}, x::AbstractArray{R,2}, r::RoundingMode)
[ round(T, x[i,j], r)::T for i = 1:size(x,1), j = 1:size(x,2) ]
end
function round{T}(::Type{T}, x::AbstractArray, r::RoundingMode)
reshape([ round(T, x[i], r)::T for i = 1:length(x) ], size(x))
reshape([ round(T, x[i], r)::T for i in eachindex(x) ], size(x))
end

# adapted from Matlab File Exchange roundsd: http://www.mathworks.com/matlabcentral/fileexchange/26212
Expand Down
4 changes: 2 additions & 2 deletions base/io.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ write(s::IO, x::Float64) = write(s, reinterpret(Int64,x))

function write(s::IO, a::AbstractArray)
nb = 0
for i = 1:length(a)
for i in eachindex(a)
nb += write(s, a[i])
end
nb
Expand Down Expand Up @@ -125,7 +125,7 @@ read{T}(s::IO, t::Type{T}, d1::Integer, dims::Integer...) =
read{T}(s::IO, ::Type{T}, dims::Dims) = read!(s, Array(T, dims))

function read!{T}(s::IO, a::Array{T})
for i = 1:length(a)
for i in eachindex(a)
a[i] = read(s, T)
end
return a
Expand Down
2 changes: 1 addition & 1 deletion base/math.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ end
function frexp{T<:FloatingPoint}(A::Array{T})
f = similar(A)
e = Array(Int, size(A))
for i = 1:length(A)
for i in eachindex(A)
f[i], e[i] = frexp(A[i])
end
return (f, e)
Expand Down
8 changes: 8 additions & 0 deletions base/multidimensional.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,14 @@ stagedfunction eachindex{T,N}(::LinearSlow, A::AbstractArray{T,N})
:($meta; CartesianRange(CartesianIndex{$N}($(startargs...)), CartesianIndex{$N}($(stopargs...))))
end

stagedfunction eachindex{S,T,M,N}(::LinearSlow, A::AbstractArray{S,M}, B::AbstractArray{T,N})
K = max(M,N)
startargs = fill(1, K)
stopargs = [:(max(size(A,$i),size(B,$i))) for i=1:K]
meta = Expr(:meta, :inline)
:($meta; CartesianRange(CartesianIndex{$K}($(startargs...)), CartesianIndex{$K}($(stopargs...))))
end

eltype{I}(::Type{CartesianRange{I}}) = I
eltype{I}(::CartesianRange{I}) = I

Expand Down
8 changes: 4 additions & 4 deletions base/operators.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -358,21 +358,21 @@ macro vectorize_1arg(S,f)
($f){$T<:$S}(x::AbstractArray{$T,2}) =
[ ($f)(x[i,j]) for i=1:size(x,1), j=1:size(x,2) ]
($f){$T<:$S}(x::AbstractArray{$T}) =
reshape([ ($f)(x[i]) for i=1:length(x) ], size(x))
reshape([ ($f)(x[i]) for i in eachindex(x) ], size(x))
end
end

macro vectorize_2arg(S,f)
S = esc(S); f = esc(f); T1 = esc(:T1); T2 = esc(:T2)
quote
($f){$T1<:$S, $T2<:$S}(x::($T1), y::AbstractArray{$T2}) =
reshape([ ($f)(x, y[i]) for i=1:length(y) ], size(y))
reshape([ ($f)(x, y[i]) for i in eachindex(y) ], size(y))
($f){$T1<:$S, $T2<:$S}(x::AbstractArray{$T1}, y::($T2)) =
reshape([ ($f)(x[i], y) for i=1:length(x) ], size(x))
reshape([ ($f)(x[i], y) for i in eachindex(x) ], size(x))

function ($f){$T1<:$S, $T2<:$S}(x::AbstractArray{$T1}, y::AbstractArray{$T2})
shp = promote_shape(size(x),size(y))
reshape([ ($f)(x[i], y[i]) for i=1:length(x) ], shp)
reshape([ ($f)(x[i], y[i]) for i in eachindex(x,y) ], shp)
end
end
end
Expand Down
8 changes: 4 additions & 4 deletions base/random.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ rand(r::AbstractRNG, T::Type, d1::Integer, dims::Integer...) = rand(r, T, tuple(
# moreover, a call like rand(r, NotImplementedType()) would be an infinite loop

function rand!{T}(r::AbstractRNG, A::AbstractArray{T})
for i = 1:length(A)
for i in eachindex(A)
@inbounds A[i] = rand(r, T)
end
A
Expand Down Expand Up @@ -356,7 +356,7 @@ end
function rand!{T<:Union(Float16, Float32)}(r::MersenneTwister, A::Array{T}, ::Type{CloseOpen})
rand!(r, A, Close1Open2)
I32 = one(Float32)
for i in 1:length(A)
for i in eachindex(A)
@inbounds A[i] = T(Float32(A[i])-I32) # faster than "A[i] -= one(T)" for T==Float16
end
A
Expand Down Expand Up @@ -1104,7 +1104,7 @@ function randn_unlikely(rng, idx, rabs, x)
end

function randn!(rng::AbstractRNG, A::AbstractArray{Float64})
for i = 1:length(A)
for i in eachindex(A)
@inbounds A[i] = randn(rng)
end
A
Expand Down Expand Up @@ -1137,7 +1137,7 @@ function randexp_unlikely(rng, idx, x)
end

function randexp!(rng::AbstractRNG, A::Array{Float64})
for i = 1:length(A)
for i in eachindex(A)
@inbounds A[i] = randexp(rng)
end
A
Expand Down
3 changes: 1 addition & 2 deletions base/statistics.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ end
##### standard deviation #####

function sqrt!(A::AbstractArray)
for i = 1:length(A)
for i in eachindex(A)
@inbounds A[i] = sqrt(A[i])
end
A
Expand Down Expand Up @@ -657,4 +657,3 @@ hist2d(v::AbstractMatrix, n1::Integer, n2::Integer) =
hist2d(v, histrange(sub(v,:,1),n1), histrange(sub(v,:,2),n2))
hist2d(v::AbstractMatrix, n::Integer) = hist2d(v, n, n)
hist2d(v::AbstractMatrix) = hist2d(v, sturges(size(v,1)))

2 comments on commit 4297135

@ScottPJones
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as in the discussion about Int^N, and @prcastro 's suggestion, could you have both? A nice short form, and a spelled out function, to make it easy to find documentation, etc.?

@timholy
Copy link
Member Author

@timholy timholy commented on 4297135 Jun 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only an internal interface (not exported), so I don't personally care whether it's short or long. Jeff, if you like my suggestion of promote_linear, I'm fine with that. Or should this just be promote_type?

Please sign in to comment.