Skip to content

Commit

Permalink
faster copy! between arrays of different types (ref #11004)
Browse files Browse the repository at this point in the history
  • Loading branch information
@JeffBezanson
JeffBezanson committed Apr 26, 2015
1 parent 252bb1b commit 102e840
Showing 1 changed file with 17 additions and 3 deletions.
20 changes: 17 additions & 3 deletions base/abstractarray.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,8 @@ end

squeeze(A::AbstractArray, dim::Integer) = squeeze(A, (Int(dim),))

## from general iterable to any array

function copy!(dest::AbstractArray, src)
i = 1
for x in src
Expand All @@ -220,7 +222,6 @@ function copy!(dest::AbstractArray, src)
return dest
end

# copy with minimal requirements on src
# if src is not an AbstractArray, moving to the offset might be O(n)
function copy!(dest::AbstractArray, doffs::Integer, src)
doffs < 1 && throw(BoundsError())
Expand All @@ -247,7 +248,7 @@ function copy!(dest::AbstractArray, doffs::Integer, src, soffs::Integer)
dn = done(src, st)
dn && throw(BoundsError())
i, dmax = doffs, length(dest)
@inbounds while !dn
@inbounds while !dn
i > dmax && throw(BoundsError())
val, st = next(src, st)
dest[i] = val
Expand Down Expand Up @@ -280,7 +281,20 @@ function copy!(dest::AbstractArray, doffs::Integer, src, soffs::Integer, n::Inte
return dest
end

# if src is an AbstractArray and a source offset is passed, use indexing
## copy between abstract arrays - generally more efficient
## since a single index variable can be used.

function copy!(dest::AbstractArray, src::AbstractArray)
n = length(src)
if n > length(dest)
throw(BoundsError())
end
@inbounds for i = 1:n

This comment has been minimized.

Sign in to view

Copy link
@timholy

timholy Apr 26, 2015

Member

Ahem. Get with the times, dude 😄. #10858

(You need to check that the sizes are the same first, however, and can use this as a fallback if they don't match. If they do, use the eachindex(dest,src) version.)

This comment has been minimized.

Sign in to view

Copy link
@timholy

timholy Apr 26, 2015

Member

You could also add @simd, presumably.

This comment has been minimized.

Sign in to view

Copy link
@JeffBezanson

JeffBezanson Apr 26, 2015

Author Member

Ok, fair point! I notice the copy! in multidimensional.jl requires its arguments' types to match, but that doesn't seem to actually be necessary. It also requires the number of dimensions to match, and I guess that's more relevant, but maybe can be relaxed with a bit more work?

This comment has been minimized.

Sign in to view

Copy link
@JeffBezanson

JeffBezanson Apr 26, 2015

Author Member

Also it looks like the multidimensional code can be improved to share indexes between the arrays as long as they match in all but the last dimension.

This comment has been minimized.

Sign in to view

Copy link
@timholy

timholy Apr 26, 2015

Member

You could relax it this way:

Rsrc, Rdest = eachindex(src), eachindex(dest)
ssrc, sdest = start(Rsrc), start(Rdest)
while !done(Rdest, sdest)
    isrc, ssrc = next(Rsrc, ssrc)
    idest, sdest = next(Rdest, sdest)
    dest[idest] = src[isrc]
end

But when the shapes match you still want to use the single-iterator form, because incrementing two Cartesian iterators is more expensive than incrementing one.

FYI: adding @simd also works around #9080 by splitting out the inner loop, see #9871.

This comment has been minimized.

Sign in to view

Copy link
@timholy

timholy Apr 27, 2015

Member

Yeah, matching in all but the last should be sufficient.

In #9080 Matt reports that zip isn't slow anymore, so mismatched dimensions should be easier now.
dest[i] = src[i]
end
return dest
end

function copy!(dest::AbstractArray, doffs::Integer, src::AbstractArray)
copy!(dest, doffs, src, 1, length(src))
end
Expand Down

0 comments on commit 102e840

Please sign in to comment.