From 96e5d3af18d8868f39af94c58248f3d9d5d8f073 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sat, 20 Feb 2021 09:42:22 +0100 Subject: [PATCH 01/35] add pool sharing to PooledArrays --- src/PooledArrays.jl | 88 ++++++++++++++++++++++++++++++--------------- 1 file changed, 59 insertions(+), 29 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index fa4b9d0..14cc6cb 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -24,22 +24,28 @@ function _invert(d::Dict{K,V}) where {K,V} for (k, v) in d d1[v] = k end - d1 + return d1 end mutable struct PooledArray{T, R<:Integer, N, RA} <: AbstractArray{T, N} refs::RA pool::Vector{T} invpool::Dict{T,R} - - function PooledArray(rs::RefArray{RA}, - invpool::Dict{T, R}, - pool=_invert(invpool)) where {T,R,N,RA<:AbstractArray{R, N}} + cow::Bool + lock::Ref{Threads.ReentrantLock} + + function PooledArray(rs::RefArray{RA}, invpool::Dict{T, R}, pool::Vector{T}, cow::Bool, + lock::Ref{Threads.ReentrantLock}) where {T,R,N,RA<:AbstractArray{R, N}} + # this is a quick but incomplete consistency check + if length(pool) != length(invpool) + throw(ArgumentError("inconsistent pool and invpool")) + end # refs mustn't overflow pool - if length(rs.a) > 0 && maximum(rs.a) > length(invpool) + minref, maxref = extrema(rs.a) + if length(rs.a) > 0 && (minref < 1 || maxref > length(invpool)) throw(ArgumentError("Reference array points beyond the end of the pool")) end - new{T,R,N,RA}(rs.a,pool,invpool) + new{T,R,N,RA}(rs.a,pool,invpool, cow, lock) end end const PooledVector{T,R} = PooledArray{T,R,1} @@ -60,12 +66,17 @@ const PooledMatrix{T,R} = PooledArray{T,R,2} ############################################################################## # Echo inner constructor as an outer constructor -function PooledArray(refs::RefArray{R}, invpool::Dict{T,R}, pool=_invert(invpool)) where {T,R} - PooledArray{T,eltype(R),ndims(R),R}(refs, invpool, pool) +PooledArray(refs::RefArray{R}, invpool::Dict{T,R}, pool::Vector{T}, cow::Bool, + lock::Ref{Threads.ReentrantLock}) where {T,R} = + PooledArray{T,eltype(R),ndims(R),R}(refs, invpool, pool, cow, lock) + +function PooledArray(d::PooledArray{T,R}) where {T,R} + Threads.lock(d.lock) + d.cow = true + Threads.unlock(d.lock) + return PooledArray(RefArray(RefArray(copy(d.refs.a)), d.invpool, d.pool, true) end -PooledArray(d::PooledArray) = copy(d) - function _label(xs::AbstractArray, ::Type{T}=eltype(xs), ::Type{I}=DEFAULT_POOLED_REF_TYPE, @@ -132,19 +143,19 @@ function PooledArray{T}(d::AbstractArray, r::Type{R}) where {T,R<:Integer} end # Assertions are needed since _label is not type stable - PooledArray(RefArray(refs::Vector{R}), invpool::Dict{T,R}, pool) + return PooledArray(RefArray(refs::Vector{R}), invpool::Dict{T,R}, pool, false, + Ref{Threads.ReentrantLock()}) end function PooledArray{T}(d::AbstractArray; signed::Bool=false, compress::Bool=false) where {T} R = signed ? (compress ? Int8 : DEFAULT_SIGNED_REF_TYPE) : (compress ? UInt8 : DEFAULT_POOLED_REF_TYPE) refs, invpool, pool = _label(d, T, R) - PooledArray(RefArray(refs), invpool, pool) + return PooledArray(RefArray(refs), invpool, pool, false, Ref{Threads.ReentrantLock()}) end PooledArray(d::AbstractArray{T}, r::Type) where {T} = PooledArray{T}(d, r) -function PooledArray(d::AbstractArray{T}; signed::Bool=false, compress::Bool=false) where {T} +PooledArray(d::AbstractArray{T}; signed::Bool=false, compress::Bool=false) where {T} = PooledArray{T}(d, signed=signed, compress=compress) -end # Construct an empty PooledVector of a specific type PooledArray(t::Type) = PooledArray(Array(t,0)) @@ -165,31 +176,37 @@ Base.size(pa::PooledArray) = size(pa.refs) Base.length(pa::PooledArray) = length(pa.refs) Base.lastindex(pa::PooledArray) = lastindex(pa.refs) -Base.copy(pa::PooledArray) = PooledArray(RefArray(copy(pa.refs)), copy(pa.invpool)) -# TODO: Implement copy_to() +Base.copy(pa::PooledArray) = + return PooledArray(RefArray(copy(pa.refs)), pa.invpool, pa.pool, true, pa.lock) + +# TODO: Implement copy! and copyto! taking into account when pool sharing should happen function Base.resize!(pa::PooledArray{T,R,1}, n::Integer) where {T,R} oldn = length(pa.refs) resize!(pa.refs, n) pa.refs[oldn+1:n] .= zero(R) - pa + return pa end -Base.reverse(x::PooledArray) = PooledArray(RefArray(reverse(x.refs)), x.invpool) +function Base.reverse(x::PooledArray) + Threads.lock(x.lock) + x.cow = true + Threads.unlock(x.lock) + PooledArray(RefArray(reverse(x.refs)), x.invpool, x.pool, true, x.lock) +end function Base.permute!!(x::PooledArray, p::AbstractVector{T}) where T<:Integer Base.permute!!(x.refs, p) - x + return x end function Base.invpermute!!(x::PooledArray, p::AbstractVector{T}) where T<:Integer Base.invpermute!!(x.refs, p) - x + return x end -function Base.similar(pa::PooledArray{T,R}, S::Type, dims::Dims) where {T,R} +Base.similar(pa::PooledArray{T,R}, S::Type, dims::Dims) where {T,R} = PooledArray(RefArray(zeros(R, dims)), Dict{S,R}()) -end Base.findall(pdv::PooledVector{Bool}) = findall(convert(Vector{Bool}, pdv)) @@ -224,7 +241,8 @@ function Base.map(f, x::PooledArray{T,R}) where {T,R<:Integer} newinvpool = Dict(zip(map(f, ks), vs)) refarray = copy(x.refs) end - PooledArray(RefArray(refarray), newinvpool) + return PooledArray(RefArray(refarray), newinvpool, _invert(newinvpool), false, + Ref(Threads.ReentrantLock())) end ############################################################################## @@ -288,6 +306,8 @@ Base.sort(pa::PooledArray; kw...) = pa[sortperm(pa; kw...)] ## ############################################################################## +# TODO: fix conversions to correctly handle cow and lock + Base.convert(::Type{PooledArray{S,R1,N}}, pa::PooledArray{T,R2,N}) where {S,T,R1<:Integer,R2<:Integer,N} = PooledArray(RefArray(convert(Array{R1,N}, pa.refs)), convert(Dict{S,R1}, pa.invpool)) Base.convert(::Type{PooledArray{S,R,N}}, pa::PooledArray{T,R,N}) where {S,T,R<:Integer,N} = @@ -368,7 +388,7 @@ function getpoolidx(pa::PooledArray{T,R}, val::Any) where {T,R} end function unsafe_pool_push!(pa::PooledArray{T,R}, val) where {T,R} - _pool_idx = length(pa.pool)+1 + _pool_idx = length(pa.pool) + 1 if _pool_idx > typemax(R) throw(ErrorException(string( "You're using a PooledArray with ref type $R, which can only hold $(Int(typemax(R))) values,\n", @@ -377,6 +397,15 @@ function unsafe_pool_push!(pa::PooledArray{T,R}, val) where {T,R} ))) end pool_idx = convert(R, _pool_idx) + if pa.cow + l = pa.lock + Threads.lock(l) + pa.invpool = copy(pa.invpool) + pa.pool = copy(pa.pool) + pa.cow = false + pa.lock = Threads.ReentrantLock() + Threads.unlock(l) + end pa.invpool[val] = pool_idx push!(pa.pool, val) pool_idx @@ -420,20 +449,21 @@ Base.empty!(pv::PooledVector) = (empty!(pv.refs); pv) Base.deleteat!(pv::PooledVector, inds) = (deleteat!(pv.refs, inds); pv) -function _vcat!(c,a,b) +function _vcat!(c, a, b) copyto!(c, 1, a, 1, length(a)) - copyto!(c, length(a)+1, b, 1, length(b)) + return copyto!(c, length(a)+1, b, 1, length(b)) end +# TODO: rethink pool sharing in vcat function Base.vcat(a::PooledArray{<:Any, <:Integer, 1}, b::AbstractArray{<:Any, 1}) output = similar(b, promote_type(eltype(a), eltype(b)), length(b) + length(a)) - _vcat!(output, a, b) + return _vcat!(output, a, b) end function Base.vcat(a::AbstractArray{<:Any, 1}, b::PooledArray{<:Any, <:Integer, 1}) output = similar(a, promote_type(eltype(a), eltype(b)), length(b) + length(a)) - _vcat!(output, a, b) + return _vcat!(output, a, b) end function Base.vcat(a::PooledArray{T, <:Integer, 1}, b::PooledArray{S, <:Integer, 1}) where {T, S} From dd77169dbb585768109f68ad4364e8a6df60736c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sat, 20 Feb 2021 09:54:14 +0100 Subject: [PATCH 02/35] further code review --- src/PooledArrays.jl | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 14cc6cb..2489610 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -4,6 +4,10 @@ import DataAPI export PooledArray, PooledVector, PooledMatrix +# TODO: +# 1. review whole code because we changed constructor +# 2. implement compress! function that in place replaces refarray, invpool and pool dropping unused levels + ############################################################################## ## ## PooledArray type definition @@ -180,6 +184,8 @@ Base.copy(pa::PooledArray) = return PooledArray(RefArray(copy(pa.refs)), pa.invpool, pa.pool, true, pa.lock) # TODO: Implement copy! and copyto! taking into account when pool sharing should happen +# the idea is that if the target is PooledArray and it has an empty pool +# instead of creating the pool from scratch we can do pool sharing function Base.resize!(pa::PooledArray{T,R,1}, n::Integer) where {T,R} oldn = length(pa.refs) @@ -362,15 +368,27 @@ Base.@propagate_inbounds function Base.isassigned(pa::PooledArray, I::Int...) end # Vector case -Base.@propagate_inbounds function Base.getindex(A::PooledArray, I::Union{Real,AbstractVector}...) - PooledArray(RefArray(getindex(A.refs, I...)), copy(A.invpool)) +function Base.@propagate_inbounds Base.getindex(A::PooledArray, I::Union{Real,AbstractVector}...) + Threads.lock(A.lock) + A.cow = true + Threads.lock(A.unlock) + return PooledArray(RefArray(getindex(A.refs, I...)), A.invpool, A.pool, true, A.lock) end # Dispatch our implementation for these cases instead of Base -Base.@propagate_inbounds Base.getindex(A::PooledArray, I::AbstractVector) = - PooledArray(RefArray(getindex(A.refs, I)), copy(A.invpool)) -Base.@propagate_inbounds Base.getindex(A::PooledArray, I::AbstractArray) = - PooledArray(RefArray(getindex(A.refs, I)), copy(A.invpool)) +function Base.@propagate_inbounds Base.getindex(A::PooledArray, I::AbstractVector) + Threads.lock(A.lock) + A.cow = true + Threads.lock(A.unlock) + return PooledArray(RefArray(getindex(A.refs, I)), A.invpool, A.pool, true, A.lock) +end + +function Base.@propagate_inbounds Base.getindex(A::PooledArray, I::AbstractArray) + Threads.lock(A.lock) + A.cow = true + Threads.lock(A.unlock) + return PooledArray(RefArray(getindex(A.refs, I)), A.invpool, A.pool, true, A.lock) +end ############################################################################## ## @@ -454,8 +472,6 @@ function _vcat!(c, a, b) return copyto!(c, length(a)+1, b, 1, length(b)) end -# TODO: rethink pool sharing in vcat - function Base.vcat(a::PooledArray{<:Any, <:Integer, 1}, b::AbstractArray{<:Any, 1}) output = similar(b, promote_type(eltype(a), eltype(b)), length(b) + length(a)) return _vcat!(output, a, b) @@ -466,6 +482,9 @@ function Base.vcat(a::AbstractArray{<:Any, 1}, b::PooledArray{<:Any, <:Integer, return _vcat!(output, a, b) end +# TODO: rethink if this cannot be made more efficient in some cases when we can just copy +# invpool and pool of the longer array instead of re-creating them + function Base.vcat(a::PooledArray{T, <:Integer, 1}, b::PooledArray{S, <:Integer, 1}) where {T, S} ap = a.invpool bp = b.invpool From 79321215106d8c8b6adfba1bfa89c2f72787c704 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sat, 20 Feb 2021 18:20:54 +0100 Subject: [PATCH 03/35] use Atomic{Int} and implement copyto! --- src/PooledArrays.jl | 130 +++++++++++++++++++++++++++----------------- 1 file changed, 81 insertions(+), 49 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 2489610..4447555 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -4,9 +4,7 @@ import DataAPI export PooledArray, PooledVector, PooledMatrix -# TODO: -# 1. review whole code because we changed constructor -# 2. implement compress! function that in place replaces refarray, invpool and pool dropping unused levels +# TODO: implement compresspool! and compresspool functions that compresses pool of PooledArray ############################################################################## ## @@ -35,11 +33,11 @@ mutable struct PooledArray{T, R<:Integer, N, RA} <: AbstractArray{T, N} refs::RA pool::Vector{T} invpool::Dict{T,R} - cow::Bool - lock::Ref{Threads.ReentrantLock} + refcount::Threads.Atomic{Int} - function PooledArray(rs::RefArray{RA}, invpool::Dict{T, R}, pool::Vector{T}, cow::Bool, - lock::Ref{Threads.ReentrantLock}) where {T,R,N,RA<:AbstractArray{R, N}} + function PooledArray(rs::RefArray{RA}, invpool::Dict{T, R}, + pool::Vector{T}=_invert(invpool), + refcount::Threads.Atomic{Int}=Threads.Atomic()) where {T,R,N,RA<:AbstractArray{R, N}} # this is a quick but incomplete consistency check if length(pool) != length(invpool) throw(ArgumentError("inconsistent pool and invpool")) @@ -49,7 +47,9 @@ mutable struct PooledArray{T, R<:Integer, N, RA} <: AbstractArray{T, N} if length(rs.a) > 0 && (minref < 1 || maxref > length(invpool)) throw(ArgumentError("Reference array points beyond the end of the pool")) end - new{T,R,N,RA}(rs.a,pool,invpool, cow, lock) + pa = new{T,R,N,RA}(rs.a, pool, invpool, refcount) + finalizer(x -> Threads.atomic_sub!(x.refcount, 1), pa) + return pa end end const PooledVector{T,R} = PooledArray{T,R,1} @@ -70,15 +70,13 @@ const PooledMatrix{T,R} = PooledArray{T,R,2} ############################################################################## # Echo inner constructor as an outer constructor -PooledArray(refs::RefArray{R}, invpool::Dict{T,R}, pool::Vector{T}, cow::Bool, - lock::Ref{Threads.ReentrantLock}) where {T,R} = - PooledArray{T,eltype(R),ndims(R),R}(refs, invpool, pool, cow, lock) +PooledArray(refs::RefArray{R}, invpool::Dict{T,R}, pool::Vector{T}=_invert(invpool), + refcount::Threads.Atomic{Int}=Threads.Atomic()) where {T,R} = + PooledArray{T,eltype(R),ndims(R),R}(refs, invpool, pool, refcount) function PooledArray(d::PooledArray{T,R}) where {T,R} - Threads.lock(d.lock) - d.cow = true - Threads.unlock(d.lock) - return PooledArray(RefArray(RefArray(copy(d.refs.a)), d.invpool, d.pool, true) + Threads.atomic_add!(d.refcount, 1) + return PooledArray(RefArray(RefArray(copy(d.refs.a)), d.invpool, d.pool, d.refcount) end function _label(xs::AbstractArray, @@ -136,6 +134,13 @@ If `array` is not a `PooledArray` then the order of elements in `refpool` in the Note that if you hold mutable objects in `PooledArray` it is not allowed to modify them after they are stored in it. + +In order to improve performance of `getindex` and `copyto!` operations `PooledArray`s +may share `pool` and `invpool` fields. This sharing is automatically handled +and is removed for any array sharing common pool if new levels are added to it. + +It is not thread safe to use add new levels to `PooledArray` (both for the single +`PooledArray` and in case of several `PooledArrays` sharing a common pool described above). """ PooledArray @@ -181,11 +186,34 @@ Base.length(pa::PooledArray) = length(pa.refs) Base.lastindex(pa::PooledArray) = lastindex(pa.refs) Base.copy(pa::PooledArray) = - return PooledArray(RefArray(copy(pa.refs)), pa.invpool, pa.pool, true, pa.lock) + return PooledArray(RefArray(copy(pa.refs)), pa.invpool, pa.pool, true, pa.refcount) + +function copyto!(dest::PooledArray{T, R, N, RA}, doffs::Union{Signed, Unsigned, + src::PooledArray{T, R, N, RA}, soffs::Union{Signed, Unsigned, + n::Union{Signed, Unsigned) where {T, R, N, RA} + n == 0 && return dest + n > 0 || Base._throw_argerror() + if soffs < 1 || doffs < 1 || soffs+n-1 > length(src) || doffs+n-1 > length(dest) + throw(BoundsError()) + end + + if length(dest.pool) == 0 + @assert length(dest.invpool) == 0 + Threads.atomic_add!(src.refcount, 1) + dest.pool = src.pool + dest.invpool = src.invpool + Threads.atomic_sub!(dest.refcount, 1) + copyto!(dest.refs, doffs, src.refs, soffs, n) + elseif dest.pool === src.pool && dest.invpool === src.invpool + copyto!(dest.refs, doffs, src.refs, soffs, n) + else + @inbounds for i in 0:n-1 + dest[dstart+i] = src[sstart+i] + end + end + return dest +end -# TODO: Implement copy! and copyto! taking into account when pool sharing should happen -# the idea is that if the target is PooledArray and it has an empty pool -# instead of creating the pool from scratch we can do pool sharing function Base.resize!(pa::PooledArray{T,R,1}, n::Integer) where {T,R} oldn = length(pa.refs) @@ -195,10 +223,8 @@ function Base.resize!(pa::PooledArray{T,R,1}, n::Integer) where {T,R} end function Base.reverse(x::PooledArray) - Threads.lock(x.lock) - x.cow = true - Threads.unlock(x.lock) - PooledArray(RefArray(reverse(x.refs)), x.invpool, x.pool, true, x.lock) + Threads.atomic_add!(x.refcount, 1) + PooledArray(RefArray(reverse(x.refs)), x.invpool, x.pool, x.refcount) end function Base.permute!!(x::PooledArray, p::AbstractVector{T}) where T<:Integer @@ -312,12 +338,28 @@ Base.sort(pa::PooledArray; kw...) = pa[sortperm(pa; kw...)] ## ############################################################################## -# TODO: fix conversions to correctly handle cow and lock +function Base.convert(::Type{PooledArray{S,R1,N}}, pa::PooledArray{T,R2,N}) where {S,T,R1<:Integer,R2<:Integer,N} + if S === R && R1 === R2 + return pa + else + refs_conv = convert(Array{R1,N}, pa.refs) + @assert refs_conv !== pa.refs + invpool_conv = convert(Dict{S,R}, pa.invpool) + @assert invpool_conv !== pa.invpool + return PooledArray(RefArray(refs_conv), invpool_conv) + end +end + +function Base.convert(::Type{PooledArray{S,R,N}}, pa::PooledArray{T,R,N}) where {S,T,R<:Integer,N} + if S === R + return pa + else + invpool_conv = convert(Dict{S,R}, pa.invpool) + @assert invpool_conv !== pa.invpool + return PooledArray(RefArray(copy(pa.refs)), invpool_conv) + end +end -Base.convert(::Type{PooledArray{S,R1,N}}, pa::PooledArray{T,R2,N}) where {S,T,R1<:Integer,R2<:Integer,N} = - PooledArray(RefArray(convert(Array{R1,N}, pa.refs)), convert(Dict{S,R1}, pa.invpool)) -Base.convert(::Type{PooledArray{S,R,N}}, pa::PooledArray{T,R,N}) where {S,T,R<:Integer,N} = - PooledArray(RefArray(copy(pa.refs)), convert(Dict{S,R}, pa.invpool)) Base.convert(::Type{PooledArray{T,R,N}}, pa::PooledArray{T,R,N}) where {T,R<:Integer,N} = pa Base.convert(::Type{PooledArray{S,R1}}, pa::PooledArray{T,R2,N}) where {S,T,R1<:Integer,R2<:Integer,N} = convert(PooledArray{S,R1,N}, pa) @@ -369,25 +411,20 @@ end # Vector case function Base.@propagate_inbounds Base.getindex(A::PooledArray, I::Union{Real,AbstractVector}...) - Threads.lock(A.lock) - A.cow = true - Threads.lock(A.unlock) - return PooledArray(RefArray(getindex(A.refs, I...)), A.invpool, A.pool, true, A.lock) + Threads.atomic_add!(A.refcount, 1) + return PooledArray(RefArray(getindex(A.refs, I...)), A.invpool, A.pool, A.refcount) end # Dispatch our implementation for these cases instead of Base function Base.@propagate_inbounds Base.getindex(A::PooledArray, I::AbstractVector) - Threads.lock(A.lock) - A.cow = true - Threads.lock(A.unlock) - return PooledArray(RefArray(getindex(A.refs, I)), A.invpool, A.pool, true, A.lock) + Threads.atomic_add!(A.refcount, 1) + return PooledArray(RefArray(getindex(A.refs, I)), A.invpool, A.pool, A.refcount) end function Base.@propagate_inbounds Base.getindex(A::PooledArray, I::AbstractArray) - Threads.lock(A.lock) - A.cow = true - Threads.lock(A.unlock) - return PooledArray(RefArray(getindex(A.refs, I)), A.invpool, A.pool, true, A.lock) + Threads.atomic_add!(A.refcount, 1) + + return PooledArray(RefArray(getindex(A.refs, I)), A.invpool, A.pool, A.refcount) end ############################################################################## @@ -406,6 +443,7 @@ function getpoolidx(pa::PooledArray{T,R}, val::Any) where {T,R} end function unsafe_pool_push!(pa::PooledArray{T,R}, val) where {T,R} + # Warning - unsafe_pool_push! may not be used in any multithreaded context _pool_idx = length(pa.pool) + 1 if _pool_idx > typemax(R) throw(ErrorException(string( @@ -415,14 +453,11 @@ function unsafe_pool_push!(pa::PooledArray{T,R}, val) where {T,R} ))) end pool_idx = convert(R, _pool_idx) - if pa.cow - l = pa.lock - Threads.lock(l) + if pa.refcount[] > 0 pa.invpool = copy(pa.invpool) pa.pool = copy(pa.pool) - pa.cow = false - pa.lock = Threads.ReentrantLock() - Threads.unlock(l) + Threads.atomic_sub!(pa.refcount, 1) + pa.refcount = Threads.Atomic() end pa.invpool[val] = pool_idx push!(pa.pool, val) @@ -482,9 +517,6 @@ function Base.vcat(a::AbstractArray{<:Any, 1}, b::PooledArray{<:Any, <:Integer, return _vcat!(output, a, b) end -# TODO: rethink if this cannot be made more efficient in some cases when we can just copy -# invpool and pool of the longer array instead of re-creating them - function Base.vcat(a::PooledArray{T, <:Integer, 1}, b::PooledArray{S, <:Integer, 1}) where {T, S} ap = a.invpool bp = b.invpool From 8dbee32cae5a727e9fa4d3422ae6626c0c0217e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sat, 20 Feb 2021 18:23:40 +0100 Subject: [PATCH 04/35] fix copy --- src/PooledArrays.jl | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 4447555..ac8a110 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -74,7 +74,7 @@ PooledArray(refs::RefArray{R}, invpool::Dict{T,R}, pool::Vector{T}=_invert(invpo refcount::Threads.Atomic{Int}=Threads.Atomic()) where {T,R} = PooledArray{T,eltype(R),ndims(R),R}(refs, invpool, pool, refcount) -function PooledArray(d::PooledArray{T,R}) where {T,R} +function PooledArray(d::PooledArray) Threads.atomic_add!(d.refcount, 1) return PooledArray(RefArray(RefArray(copy(d.refs.a)), d.invpool, d.pool, d.refcount) end @@ -185,8 +185,7 @@ Base.size(pa::PooledArray) = size(pa.refs) Base.length(pa::PooledArray) = length(pa.refs) Base.lastindex(pa::PooledArray) = lastindex(pa.refs) -Base.copy(pa::PooledArray) = - return PooledArray(RefArray(copy(pa.refs)), pa.invpool, pa.pool, true, pa.refcount) +Base.copy(pa::PooledArray) = PooledArray(pa) function copyto!(dest::PooledArray{T, R, N, RA}, doffs::Union{Signed, Unsigned, src::PooledArray{T, R, N, RA}, soffs::Union{Signed, Unsigned, From 92850e50c24827d522a18351e5848ca8b0d3b00a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sat, 20 Feb 2021 18:24:44 +0100 Subject: [PATCH 05/35] fix typo --- src/PooledArrays.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index ac8a110..36b87f6 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -76,7 +76,7 @@ PooledArray(refs::RefArray{R}, invpool::Dict{T,R}, pool::Vector{T}=_invert(invpo function PooledArray(d::PooledArray) Threads.atomic_add!(d.refcount, 1) - return PooledArray(RefArray(RefArray(copy(d.refs.a)), d.invpool, d.pool, d.refcount) + return PooledArray(RefArray(copy(d.refs.a), d.invpool, d.pool, d.refcount) end function _label(xs::AbstractArray, From 995b3cd61e783517baacfc40270c6444af837905 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sat, 20 Feb 2021 18:28:04 +0100 Subject: [PATCH 06/35] fix leftover code --- src/PooledArrays.jl | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 36b87f6..6bf91cf 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -152,14 +152,13 @@ function PooledArray{T}(d::AbstractArray, r::Type{R}) where {T,R<:Integer} end # Assertions are needed since _label is not type stable - return PooledArray(RefArray(refs::Vector{R}), invpool::Dict{T,R}, pool, false, - Ref{Threads.ReentrantLock()}) + return PooledArray(RefArray(refs::Vector{R}), invpool::Dict{T,R}, pool, Threads.Atomic()) end function PooledArray{T}(d::AbstractArray; signed::Bool=false, compress::Bool=false) where {T} R = signed ? (compress ? Int8 : DEFAULT_SIGNED_REF_TYPE) : (compress ? UInt8 : DEFAULT_POOLED_REF_TYPE) refs, invpool, pool = _label(d, T, R) - return PooledArray(RefArray(refs), invpool, pool, false, Ref{Threads.ReentrantLock()}) + return PooledArray(RefArray(refs), invpool, pool, Threads.Atomic()) end PooledArray(d::AbstractArray{T}, r::Type) where {T} = PooledArray{T}(d, r) @@ -272,8 +271,7 @@ function Base.map(f, x::PooledArray{T,R}) where {T,R<:Integer} newinvpool = Dict(zip(map(f, ks), vs)) refarray = copy(x.refs) end - return PooledArray(RefArray(refarray), newinvpool, _invert(newinvpool), false, - Ref(Threads.ReentrantLock())) + return PooledArray(RefArray(refarray), newinvpool, _invert(newinvpool), Threads.Atomic()) end ############################################################################## From 4c407f30efdd3ed9a227a4db9f27832b7181405c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sat, 20 Feb 2021 18:30:07 +0100 Subject: [PATCH 07/35] add missing ) --- src/PooledArrays.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 6bf91cf..d4d1668 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -76,7 +76,7 @@ PooledArray(refs::RefArray{R}, invpool::Dict{T,R}, pool::Vector{T}=_invert(invpo function PooledArray(d::PooledArray) Threads.atomic_add!(d.refcount, 1) - return PooledArray(RefArray(copy(d.refs.a), d.invpool, d.pool, d.refcount) + return PooledArray(RefArray(copy(d.refs.a)), d.invpool, d.pool, d.refcount) end function _label(xs::AbstractArray, From 00af820c396dd4957f35f9cfa6e26d81ebc1db9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sat, 20 Feb 2021 19:24:55 +0100 Subject: [PATCH 08/35] fix small issues --- src/PooledArrays.jl | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index d4d1668..4247acc 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -44,7 +44,8 @@ mutable struct PooledArray{T, R<:Integer, N, RA} <: AbstractArray{T, N} end # refs mustn't overflow pool minref, maxref = extrema(rs.a) - if length(rs.a) > 0 && (minref < 1 || maxref > length(invpool)) + # 0 indicates #undef + if length(rs.a) > 0 && (minref < 0 || maxref > length(invpool)) throw(ArgumentError("Reference array points beyond the end of the pool")) end pa = new{T,R,N,RA}(rs.a, pool, invpool, refcount) @@ -76,7 +77,7 @@ PooledArray(refs::RefArray{R}, invpool::Dict{T,R}, pool::Vector{T}=_invert(invpo function PooledArray(d::PooledArray) Threads.atomic_add!(d.refcount, 1) - return PooledArray(RefArray(copy(d.refs.a)), d.invpool, d.pool, d.refcount) + return PooledArray(RefArray(copy(d.refs)), d.invpool, d.pool, d.refcount) end function _label(xs::AbstractArray, @@ -186,9 +187,9 @@ Base.lastindex(pa::PooledArray) = lastindex(pa.refs) Base.copy(pa::PooledArray) = PooledArray(pa) -function copyto!(dest::PooledArray{T, R, N, RA}, doffs::Union{Signed, Unsigned, - src::PooledArray{T, R, N, RA}, soffs::Union{Signed, Unsigned, - n::Union{Signed, Unsigned) where {T, R, N, RA} +function Base.copyto!(dest::PooledArray{T, R, N, RA}, doffs::Union{Signed, Unsigned}, + src::PooledArray{T, R, N, RA}, soffs::Union{Signed, Unsigned}, + n::Union{Signed, Unsigned}) where {T, R, N, RA} n == 0 && return dest n > 0 || Base._throw_argerror() if soffs < 1 || doffs < 1 || soffs+n-1 > length(src) || doffs+n-1 > length(dest) @@ -407,18 +408,18 @@ Base.@propagate_inbounds function Base.isassigned(pa::PooledArray, I::Int...) end # Vector case -function Base.@propagate_inbounds Base.getindex(A::PooledArray, I::Union{Real,AbstractVector}...) +Base.@propagate_inbounds function Base.getindex(A::PooledArray, I::Union{Real,AbstractVector}...) Threads.atomic_add!(A.refcount, 1) return PooledArray(RefArray(getindex(A.refs, I...)), A.invpool, A.pool, A.refcount) end # Dispatch our implementation for these cases instead of Base -function Base.@propagate_inbounds Base.getindex(A::PooledArray, I::AbstractVector) +Base.@propagate_inbounds function Base.getindex(A::PooledArray, I::AbstractVector) Threads.atomic_add!(A.refcount, 1) return PooledArray(RefArray(getindex(A.refs, I)), A.invpool, A.pool, A.refcount) end -function Base.@propagate_inbounds Base.getindex(A::PooledArray, I::AbstractArray) +Base.@propagate_inbounds function Base.getindex(A::PooledArray, I::AbstractArray) Threads.atomic_add!(A.refcount, 1) return PooledArray(RefArray(getindex(A.refs, I)), A.invpool, A.pool, A.refcount) From 45a33a922cf1b34a24db286c1e9070a335efe1b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sat, 20 Feb 2021 19:29:08 +0100 Subject: [PATCH 09/35] add missing method --- src/PooledArrays.jl | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 4247acc..ecc1ed5 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -187,6 +187,10 @@ Base.lastindex(pa::PooledArray) = lastindex(pa.refs) Base.copy(pa::PooledArray) = PooledArray(pa) +Base.copyto!(dest::PooledArray{T, R, N, RA}, + src::PooledArray{T, R, N, RA}) where {T, R, N, RA} = + copyto!(dest, 1, src, 1, lenth(src)) + function Base.copyto!(dest::PooledArray{T, R, N, RA}, doffs::Union{Signed, Unsigned}, src::PooledArray{T, R, N, RA}, soffs::Union{Signed, Unsigned}, n::Union{Signed, Unsigned}) where {T, R, N, RA} From da13879c240da4c6145466dbae729cb6ec5e28b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sat, 20 Feb 2021 19:32:16 +0100 Subject: [PATCH 10/35] another missing method --- src/PooledArrays.jl | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index ecc1ed5..562ae8b 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -189,7 +189,17 @@ Base.copy(pa::PooledArray) = PooledArray(pa) Base.copyto!(dest::PooledArray{T, R, N, RA}, src::PooledArray{T, R, N, RA}) where {T, R, N, RA} = - copyto!(dest, 1, src, 1, lenth(src)) + copyto!(dest, 1, src, 1, length(src)) + +function Base.copy!(dest::PooledArray{T, R, N, RA}, + src::PooledArray{T, R, N, RA}) where {T, R, N, RA} + copy!(dest.refs, src.refs) + Threads.atomic_add!(src.refcount, 1) + dest.pool = src.pool + dest.invpool = src.invpool + dest.refcount = src.refcount + return dest +end function Base.copyto!(dest::PooledArray{T, R, N, RA}, doffs::Union{Signed, Unsigned}, src::PooledArray{T, R, N, RA}, soffs::Union{Signed, Unsigned}, From 665bf9a02cb5dd2dcb49688006ab1202ab10143c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sat, 20 Feb 2021 19:53:57 +0100 Subject: [PATCH 11/35] fix various lurking problems (and bugs) in old code --- src/PooledArrays.jl | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 562ae8b..dec5aaa 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -35,7 +35,7 @@ mutable struct PooledArray{T, R<:Integer, N, RA} <: AbstractArray{T, N} invpool::Dict{T,R} refcount::Threads.Atomic{Int} - function PooledArray(rs::RefArray{RA}, invpool::Dict{T, R}, + function PooledArray{T,R,N,RA}(rs::RefArray{RA}, invpool::Dict{T, R}, pool::Vector{T}=_invert(invpool), refcount::Threads.Atomic{Int}=Threads.Atomic()) where {T,R,N,RA<:AbstractArray{R, N}} # this is a quick but incomplete consistency check @@ -71,9 +71,9 @@ const PooledMatrix{T,R} = PooledArray{T,R,2} ############################################################################## # Echo inner constructor as an outer constructor -PooledArray(refs::RefArray{R}, invpool::Dict{T,R}, pool::Vector{T}=_invert(invpool), - refcount::Threads.Atomic{Int}=Threads.Atomic()) where {T,R} = - PooledArray{T,eltype(R),ndims(R),R}(refs, invpool, pool, refcount) +PooledArray(refs::RefArray{RA}, invpool::Dict{T,R}, pool::Vector{T}=_invert(invpool), + refcount::Threads.Atomic{Int}=Threads.Atomic()) where {T,R,RA<:AbstractArray{R}} = + PooledArray{T,R,ndims(RA),RA}(refs, invpool, pool, refcount) function PooledArray(d::PooledArray) Threads.atomic_add!(d.refcount, 1) @@ -421,22 +421,19 @@ Base.@propagate_inbounds function Base.isassigned(pa::PooledArray, I::Int...) !iszero(pa.refs[I...]) end -# Vector case -Base.@propagate_inbounds function Base.getindex(A::PooledArray, I::Union{Real,AbstractVector}...) - Threads.atomic_add!(A.refcount, 1) - return PooledArray(RefArray(getindex(A.refs, I...)), A.invpool, A.pool, A.refcount) -end - -# Dispatch our implementation for these cases instead of Base -Base.@propagate_inbounds function Base.getindex(A::PooledArray, I::AbstractVector) - Threads.atomic_add!(A.refcount, 1) - return PooledArray(RefArray(getindex(A.refs, I)), A.invpool, A.pool, A.refcount) -end +# Other cases case +Base.@propagate_inbounds function Base.getindex(A::PooledArray, I...) + new_refs = getindex(A.refs, I...) -Base.@propagate_inbounds function Base.getindex(A::PooledArray, I::AbstractArray) + if new_refs isa AbstractArray Threads.atomic_add!(A.refcount, 1) - - return PooledArray(RefArray(getindex(A.refs, I)), A.invpool, A.pool, A.refcount) + return PooledArray(RefArray(getindex(A.refs, I...)), A.invpool, A.pool, A.refcount) + else + @assert typeof(new_refs) === eltype(A.refs) + # scalar produced + iszero(new_refs) && throw(UndefRefError()) + return @inbounds A.pool[new_refs] + end end ############################################################################## From cb1308c9f0673470a060cbd5ff147735215ceea6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sun, 21 Feb 2021 08:27:48 +0100 Subject: [PATCH 12/35] Apply suggestions from code review Co-authored-by: Milan Bouchet-Valat --- src/PooledArrays.jl | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index dec5aaa..ef95b46 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -140,8 +140,10 @@ In order to improve performance of `getindex` and `copyto!` operations `PooledAr may share `pool` and `invpool` fields. This sharing is automatically handled and is removed for any array sharing common pool if new levels are added to it. -It is not thread safe to use add new levels to `PooledArray` (both for the single -`PooledArray` and in case of several `PooledArrays` sharing a common pool described above). +It is not safe to assign values that are not already present in a `PooledArray`'s pool +from one thread while either reading or writing to the same array from another thread +(even if pools are not shared). However, reading and writing from different threads is safe +if all values already exist in the pool. """ PooledArray @@ -153,13 +155,13 @@ function PooledArray{T}(d::AbstractArray, r::Type{R}) where {T,R<:Integer} end # Assertions are needed since _label is not type stable - return PooledArray(RefArray(refs::Vector{R}), invpool::Dict{T,R}, pool, Threads.Atomic()) + return PooledArray(RefArray(refs::Vector{R}), invpool::Dict{T,R}, pool) end function PooledArray{T}(d::AbstractArray; signed::Bool=false, compress::Bool=false) where {T} R = signed ? (compress ? Int8 : DEFAULT_SIGNED_REF_TYPE) : (compress ? UInt8 : DEFAULT_POOLED_REF_TYPE) refs, invpool, pool = _label(d, T, R) - return PooledArray(RefArray(refs), invpool, pool, Threads.Atomic()) + return PooledArray(RefArray(refs), invpool, pool) end PooledArray(d::AbstractArray{T}, r::Type) where {T} = PooledArray{T}(d, r) @@ -286,7 +288,7 @@ function Base.map(f, x::PooledArray{T,R}) where {T,R<:Integer} newinvpool = Dict(zip(map(f, ks), vs)) refarray = copy(x.refs) end - return PooledArray(RefArray(refarray), newinvpool, _invert(newinvpool), Threads.Atomic()) + return PooledArray(RefArray(refarray), newinvpool) end ############################################################################## @@ -421,7 +423,7 @@ Base.@propagate_inbounds function Base.isassigned(pa::PooledArray, I::Int...) !iszero(pa.refs[I...]) end -# Other cases case +# Other cases Base.@propagate_inbounds function Base.getindex(A::PooledArray, I...) new_refs = getindex(A.refs, I...) From be314123c67e757b6676da298f6235d4356fd881 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sun, 21 Feb 2021 08:47:10 +0100 Subject: [PATCH 13/35] apply comments from the review --- .github/workflows/ci.yml | 2 ++ src/PooledArrays.jl | 24 +++++++++--------------- test/runtests.jl | 8 +++++++- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e883daf..28eb529 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,6 +44,8 @@ jobs: ${{ runner.os }}- - uses: julia-actions/julia-buildpkg@v1 - uses: julia-actions/julia-runtest@v1 + env: + JULIA_NUM_THREADS: 4 - uses: julia-actions/julia-processcoverage@v1 - uses: codecov/codecov-action@v1 with: diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index ef95b46..c036c17 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -4,8 +4,6 @@ import DataAPI export PooledArray, PooledVector, PooledMatrix -# TODO: implement compresspool! and compresspool functions that compresses pool of PooledArray - ############################################################################## ## ## PooledArray type definition @@ -33,6 +31,7 @@ mutable struct PooledArray{T, R<:Integer, N, RA} <: AbstractArray{T, N} refs::RA pool::Vector{T} invpool::Dict{T,R} + # refcount[] is 0 if only one PooledArray holds a reference to pool and invpool refcount::Threads.Atomic{Int} function PooledArray{T,R,N,RA}(rs::RefArray{RA}, invpool::Dict{T, R}, @@ -137,8 +136,7 @@ Note that if you hold mutable objects in `PooledArray` it is not allowed to modi after they are stored in it. In order to improve performance of `getindex` and `copyto!` operations `PooledArray`s -may share `pool` and `invpool` fields. This sharing is automatically handled -and is removed for any array sharing common pool if new levels are added to it. +may share pools. It is not safe to assign values that are not already present in a `PooledArray`'s pool from one thread while either reading or writing to the same array from another thread @@ -356,22 +354,18 @@ function Base.convert(::Type{PooledArray{S,R1,N}}, pa::PooledArray{T,R2,N}) wher if S === R && R1 === R2 return pa else - refs_conv = convert(Array{R1,N}, pa.refs) - @assert refs_conv !== pa.refs - invpool_conv = convert(Dict{S,R}, pa.invpool) + invpool_conv = convert(Dict{S,R1}, pa.invpool) @assert invpool_conv !== pa.invpool - return PooledArray(RefArray(refs_conv), invpool_conv) end -end -function Base.convert(::Type{PooledArray{S,R,N}}, pa::PooledArray{T,R,N}) where {S,T,R<:Integer,N} - if S === R - return pa + if R1 === R2 + refs_conv = pa.refs else - invpool_conv = convert(Dict{S,R}, pa.invpool) - @assert invpool_conv !== pa.invpool - return PooledArray(RefArray(copy(pa.refs)), invpool_conv) + refs_conv = convert(Array{R1,N}, pa.refs) + @assert refs_conv !== pa.refs end + + return PooledArray(RefArray(refs_conv), invpool_conv) end Base.convert(::Type{PooledArray{T,R,N}}, pa::PooledArray{T,R,N}) where {T,R<:Integer,N} = pa diff --git a/test/runtests.jl b/test/runtests.jl index 60c99b7..1f4685e 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -2,6 +2,12 @@ using Test using PooledArrays using DataAPI: refarray, refvalue, refpool, invrefpool +if Threads.nthreads() < 2 + @warn("Running with only one thread: correctness of parallel operations is not tested") +else + @show Threads.nthreads() +end + @testset "PooledArrays" begin a = rand(10) b = rand(10,10) @@ -88,7 +94,7 @@ using DataAPI: refarray, refvalue, refpool, invrefpool end @test refpool(s) == ["a", "b"] @test invrefpool(s) == Dict("a" => 1, "b" => 2) - + @testset "push!" begin xs = PooledArray([10, 20, 30]) @test xs === push!(xs, -100) From b41e9a18fc3cd17521df075c2d0b7391b3a12069 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sun, 21 Feb 2021 08:58:07 +0100 Subject: [PATCH 14/35] improve getindex --- src/PooledArrays.jl | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index c036c17..d1d5f33 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -417,19 +417,13 @@ Base.@propagate_inbounds function Base.isassigned(pa::PooledArray, I::Int...) !iszero(pa.refs[I...]) end -# Other cases -Base.@propagate_inbounds function Base.getindex(A::PooledArray, I...) - new_refs = getindex(A.refs, I...) - - if new_refs isa AbstractArray - Threads.atomic_add!(A.refcount, 1) - return PooledArray(RefArray(getindex(A.refs, I...)), A.invpool, A.pool, A.refcount) - else - @assert typeof(new_refs) === eltype(A.refs) - # scalar produced - iszero(new_refs) && throw(UndefRefError()) - return @inbounds A.pool[new_refs] - end +# Other cases; we rely on the fact that non-standard indexing will fall back to this eventually +Base.@propagate_inbounds function Base.getindex(A::PooledArray, I::Union{Real, AbstractVector}...) + # make sure we do not increase A.refcount in case creation of newrefs fails + newrefs = getindex(A.refs, I...) + @assert newrefs isa AbstractArray + Threads.atomic_add!(A.refcount, 1) + return PooledArray(RefArray(newrefs), A.invpool, A.pool, A.refcount) end ############################################################################## From 07e7b9c340954eb107c2f8f6e809150273fff28b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sun, 21 Feb 2021 12:02:47 +0100 Subject: [PATCH 15/35] add view --- Project.toml | 2 +- src/PooledArrays.jl | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Project.toml b/Project.toml index b3036bb..942d0ca 100644 --- a/Project.toml +++ b/Project.toml @@ -1,6 +1,6 @@ name = "PooledArrays" uuid = "2dfb63ee-cc39-5dd5-95bd-886bf059d720" -version = "1.1.0" +version = "2.0.0" [deps] DataAPI = "9a962f9c-6df0-11e9-0e5d-c546b8b5ee8a" diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index d1d5f33..4a21e53 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -426,6 +426,11 @@ Base.@propagate_inbounds function Base.getindex(A::PooledArray, I::Union{Real, A return PooledArray(RefArray(newrefs), A.invpool, A.pool, A.refcount) end +# views + +Base.view(A::PooledArray, I...) = + PooledArray(RefArray(view(A.refs, I...)), A.invpool, A.pool, A.refcount) + ############################################################################## ## ## setindex!() definitions From 14192837c65d4ffc8a6cdf8af785319fb3562edf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sun, 21 Feb 2021 22:45:33 +0100 Subject: [PATCH 16/35] Apply suggestions from code review Co-authored-by: Milan Bouchet-Valat --- src/PooledArrays.jl | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 4a21e53..4c0e756 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -35,8 +35,8 @@ mutable struct PooledArray{T, R<:Integer, N, RA} <: AbstractArray{T, N} refcount::Threads.Atomic{Int} function PooledArray{T,R,N,RA}(rs::RefArray{RA}, invpool::Dict{T, R}, - pool::Vector{T}=_invert(invpool), - refcount::Threads.Atomic{Int}=Threads.Atomic()) where {T,R,N,RA<:AbstractArray{R, N}} + pool::Vector{T}=_invert(invpool), + refcount::Threads.Atomic{Int}=Threads.Atomic()) where {T,R,N,RA<:AbstractArray{R, N}} # this is a quick but incomplete consistency check if length(pool) != length(invpool) throw(ArgumentError("inconsistent pool and invpool")) @@ -136,7 +136,8 @@ Note that if you hold mutable objects in `PooledArray` it is not allowed to modi after they are stored in it. In order to improve performance of `getindex` and `copyto!` operations `PooledArray`s -may share pools. +may share pools. This sharing is automatically undone by copying a shared pool before +adding new values to it. It is not safe to assign values that are not already present in a `PooledArray`'s pool from one thread while either reading or writing to the same array from another thread From 0cb0021400a707a0807f2b70bd3681d3e67583d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sun, 21 Feb 2021 23:26:58 +0100 Subject: [PATCH 17/35] add SubArray handling --- src/PooledArrays.jl | 68 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 59 insertions(+), 9 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index aed8f73..6357788 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -31,12 +31,12 @@ mutable struct PooledArray{T, R<:Integer, N, RA} <: AbstractArray{T, N} refs::RA pool::Vector{T} invpool::Dict{T,R} - # refcount[] is 0 if only one PooledArray holds a reference to pool and invpool + # refcount[] is 1 if only one PooledArray holds a reference to pool and invpool refcount::Threads.Atomic{Int} function PooledArray{T,R,N,RA}(rs::RefArray{RA}, invpool::Dict{T, R}, pool::Vector{T}=_invert(invpool), - refcount::Threads.Atomic{Int}=Threads.Atomic()) where {T,R,N,RA<:AbstractArray{R, N}} + refcount::Threads.Atomic{Int}=Threads.Atomic{Int}(1)) where {T,R,N,RA<:AbstractArray{R, N}} # this is a quick but incomplete consistency check if length(pool) != length(invpool) throw(ArgumentError("inconsistent pool and invpool")) @@ -71,7 +71,7 @@ const PooledMatrix{T,R} = PooledArray{T,R,2} # Echo inner constructor as an outer constructor PooledArray(refs::RefArray{RA}, invpool::Dict{T,R}, pool::Vector{T}=_invert(invpool), - refcount::Threads.Atomic{Int}=Threads.Atomic()) where {T,R,RA<:AbstractArray{R}} = + refcount::Threads.Atomic{Int}=Threads.Atomic{Int}(1)) where {T,R,RA<:AbstractArray{R}} = PooledArray{T,R,ndims(RA),RA}(refs, invpool, pool, refcount) function PooledArray(d::PooledArray) @@ -192,6 +192,12 @@ Base.copyto!(dest::PooledArray{T, R, N, RA}, src::PooledArray{T, R, N, RA}) where {T, R, N, RA} = copyto!(dest, 1, src, 1, length(src)) +# TODO: handle case when dest is SubArray + +Base.copyto!(dest::PooledArray{T, R, N, RA}, + src::SubArray{T, N, PooledArray{T, R, M, RA}}) where {T, R, N, M, RA} = + copyto!(dest, 1, src, 1, length(src)) + function Base.copy!(dest::PooledArray{T, R, N, RA}, src::PooledArray{T, R, N, RA}) where {T, R, N, RA} copy!(dest.refs, src.refs) @@ -202,6 +208,16 @@ function Base.copy!(dest::PooledArray{T, R, N, RA}, return dest end +function Base.copy!(dest::PooledArray{T, R, N, RA}, + src::SubArray{T, N, PooledArray{T, R, M, RA}}) where {T, R, N, M, RA} + copy!(dest.refs, view(parent(src).refs, src.indices)) + Threads.atomic_add!(src.refcount, 1) + dest.pool = src.pool + dest.invpool = src.invpool + dest.refcount = src.refcount + return dest +end + function Base.copyto!(dest::PooledArray{T, R, N, RA}, doffs::Union{Signed, Unsigned}, src::PooledArray{T, R, N, RA}, soffs::Union{Signed, Unsigned}, n::Union{Signed, Unsigned}) where {T, R, N, RA} @@ -217,6 +233,7 @@ function Base.copyto!(dest::PooledArray{T, R, N, RA}, doffs::Union{Signed, Unsig dest.pool = src.pool dest.invpool = src.invpool Threads.atomic_sub!(dest.refcount, 1) + dest.refcount = src.refcount copyto!(dest.refs, doffs, src.refs, soffs, n) elseif dest.pool === src.pool && dest.invpool === src.invpool copyto!(dest.refs, doffs, src.refs, soffs, n) @@ -228,6 +245,36 @@ function Base.copyto!(dest::PooledArray{T, R, N, RA}, doffs::Union{Signed, Unsig return dest end +# TODO: handle case when dest is SubArray + +function Base.copyto!(dest::PooledArray{T, R, N, RA}, doffs::Union{Signed, Unsigned}, + src::SubArray{T, N, PooledArray{T, R, M, RA}}, soffs::Union{Signed, Unsigned}, + n::Union{Signed, Unsigned}) where {T, R, N, M, RA} + n == 0 && return dest + n > 0 || Base._throw_argerror() + if soffs < 1 || doffs < 1 || soffs+n-1 > length(src) || doffs+n-1 > length(dest) + throw(BoundsError()) + end + + src_parent = parent(src) + + if length(dest.pool) == 0 + @assert length(dest.invpool) == 0 + Threads.atomic_add!(src_parent.refcount, 1) + dest.pool = src_parent.pool + dest.invpool = src_parent.invpool + Threads.atomic_sub!(dest.refcount, 1) + dest.refcount = src_parent.refcount + copyto!(dest.refs, doffs, view(src_parent.refs, src.indices), soffs, n) + elseif dest.pool === src.pool && dest.invpool === src.invpool + copyto!(dest.refs, doffs, view(src_parent.refs, src.indices), soffs, n) + else + @inbounds for i in 0:n-1 + dest[dstart+i] = src[sstart+i] + end + end + return dest +end function Base.resize!(pa::PooledArray{T,R,1}, n::Integer) where {T,R} oldn = length(pa.refs) @@ -427,10 +474,13 @@ Base.@propagate_inbounds function Base.getindex(A::PooledArray, I::Union{Real, A return PooledArray(RefArray(newrefs), A.invpool, A.pool, A.refcount) end -# views - -Base.view(A::PooledArray, I...) = - PooledArray(RefArray(view(A.refs, I...)), A.invpool, A.pool, A.refcount) +Base.@propagate_inbounds function Base.getindex(A::SubArray{T,N,<:PooledArray}, I::Union{Real, AbstractVector}...) + # make sure we do not increase A.refcount in case creation of newrefs fails + newrefs = getindex(view(parent(A).refs, A.indices), I...) + @assert newrefs isa AbstractArray + Threads.atomic_add!(A.refcount, 1) + return PooledArray(RefArray(newrefs), A.invpool, A.pool, A.refcount) +end ############################################################################## ## @@ -458,11 +508,11 @@ function unsafe_pool_push!(pa::PooledArray{T,R}, val) where {T,R} ))) end pool_idx = convert(R, _pool_idx) - if pa.refcount[] > 0 + if pa.refcount[] > 1 pa.invpool = copy(pa.invpool) pa.pool = copy(pa.pool) Threads.atomic_sub!(pa.refcount, 1) - pa.refcount = Threads.Atomic() + pa.refcount = Threads.Atomic{Int}(1) end pa.invpool[val] = pool_idx push!(pa.pool, val) From be09d78ee2dc880805901a9a327fa1e54df802a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Tue, 23 Feb 2021 21:21:20 +0100 Subject: [PATCH 18/35] Apply suggestions from code review Co-authored-by: Jacob Quinn --- src/PooledArrays.jl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 6357788..e5746e9 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -223,7 +223,7 @@ function Base.copyto!(dest::PooledArray{T, R, N, RA}, doffs::Union{Signed, Unsig n::Union{Signed, Unsigned}) where {T, R, N, RA} n == 0 && return dest n > 0 || Base._throw_argerror() - if soffs < 1 || doffs < 1 || soffs+n-1 > length(src) || doffs+n-1 > length(dest) + if soffs < 1 || doffs < 1 || soffs + n - 1 > length(src) || doffs + n - 1 > length(dest) throw(BoundsError()) end @@ -252,7 +252,7 @@ function Base.copyto!(dest::PooledArray{T, R, N, RA}, doffs::Union{Signed, Unsig n::Union{Signed, Unsigned}) where {T, R, N, M, RA} n == 0 && return dest n > 0 || Base._throw_argerror() - if soffs < 1 || doffs < 1 || soffs+n-1 > length(src) || doffs+n-1 > length(dest) + if soffs < 1 || doffs < 1 || soffs + n - 1 > length(src) || doffs + n - 1 > length(dest) throw(BoundsError()) end From 7f5f7e2e90794d25a9c0ea5e2fb5e61f0bc72030 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Tue, 23 Feb 2021 21:29:29 +0100 Subject: [PATCH 19/35] Update src/PooledArrays.jl --- src/PooledArrays.jl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index e5746e9..bbdb4ac 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -227,6 +227,9 @@ function Base.copyto!(dest::PooledArray{T, R, N, RA}, doffs::Union{Signed, Unsig throw(BoundsError()) end + # if dest.pool is empty we can safely replace it as we are sure it holds + # no information; having this path is useful because then we can efficiently + # `copyto!` into a fresh `PooledArray` created using the `similar` function if length(dest.pool) == 0 @assert length(dest.invpool) == 0 Threads.atomic_add!(src.refcount, 1) From 61bcbdc1a2385a084a8a3a20cbcf22ae340bb260 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Wed, 24 Feb 2021 09:37:59 +0100 Subject: [PATCH 20/35] add PooledArrOrSub --- src/PooledArrays.jl | 118 +++++++++++++++++--------------------------- 1 file changed, 45 insertions(+), 73 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index bbdb4ac..38af771 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -55,6 +55,9 @@ end const PooledVector{T,R} = PooledArray{T,R,1} const PooledMatrix{T,R} = PooledArray{T,R,2} +const PooledArrOrSub{T, N, R} = Union{PooledArray{T, R, N}, + SubArray{T, N, <:PooledArray{T, R}}} where {T, R, N} + ############################################################################## ## ## PooledArray constructors @@ -181,6 +184,13 @@ DataAPI.refarray(pa::PooledArray) = pa.refs DataAPI.refvalue(pa::PooledArray, i::Integer) = pa.pool[i] DataAPI.refpool(pa::PooledArray) = pa.pool DataAPI.invrefpool(pa::PooledArray) = pa.invpool +refcount(pa::PooledArray) = pa.refcount + +DataAPI.refarray(pav::SubArray{<:Any, <:Any, <:PooledArray}) = view(parent(pav).refs, pav.indices) +DataAPI.refvalue(pav::SubArray{<:Any, <:Any, <:PooledArray}, i::Integer) = parent(pav).pool[i] +DataAPI.refpool(pav::SubArray{<:Any, <:Any, <:PooledArray}) = parent(pav).pool +DataAPI.invrefpool(pav::SubArray{<:Any, <:Any, <:PooledArray}) = parent(pav).invpool +refcount(pav::SubArray{<:Any, <:Any, <:PooledArray}) = parent(pav).refcount Base.size(pa::PooledArray) = size(pa.refs) Base.length(pa::PooledArray) = length(pa.refs) @@ -188,89 +198,50 @@ Base.lastindex(pa::PooledArray) = lastindex(pa.refs) Base.copy(pa::PooledArray) = PooledArray(pa) -Base.copyto!(dest::PooledArray{T, R, N, RA}, - src::PooledArray{T, R, N, RA}) where {T, R, N, RA} = - copyto!(dest, 1, src, 1, length(src)) - -# TODO: handle case when dest is SubArray - -Base.copyto!(dest::PooledArray{T, R, N, RA}, - src::SubArray{T, N, PooledArray{T, R, M, RA}}) where {T, R, N, M, RA} = - copyto!(dest, 1, src, 1, length(src)) - -function Base.copy!(dest::PooledArray{T, R, N, RA}, - src::PooledArray{T, R, N, RA}) where {T, R, N, RA} - copy!(dest.refs, src.refs) - Threads.atomic_add!(src.refcount, 1) - dest.pool = src.pool - dest.invpool = src.invpool - dest.refcount = src.refcount - return dest -end - -function Base.copy!(dest::PooledArray{T, R, N, RA}, - src::SubArray{T, N, PooledArray{T, R, M, RA}}) where {T, R, N, M, RA} - copy!(dest.refs, view(parent(src).refs, src.indices)) - Threads.atomic_add!(src.refcount, 1) - dest.pool = src.pool - dest.invpool = src.invpool - dest.refcount = src.refcount +if isdefined(Base, :copy!) + import Base.copy! +else + import Future: copy! +end + +# here we do not allow dest to be SubArray as copy! is intended to replace whole arrays +# slow path will be used for SubArray +function copy!(dest::PooledArray{T, R, N}, + src::PooledArrOrSub{T, N, R}) where {T, N, R} + copy!(dest.refs, DataAPI.refarray(src)) + src_refcount = refcount(src) + Threads.atomic_add!(src_refcount, 1) + dest.pool = DataAPI.refpool(src) + dest.invpool = DataAPI.invrefpool(src) + dest.refcount = src_refcount return dest end -function Base.copyto!(dest::PooledArray{T, R, N, RA}, doffs::Union{Signed, Unsigned}, - src::PooledArray{T, R, N, RA}, soffs::Union{Signed, Unsigned}, - n::Union{Signed, Unsigned}) where {T, R, N, RA} +function Base.copyto!(dest::PooledArrOrSub{T, N, R}, doffs::Union{Signed, Unsigned}, + src::PooledArrOrSub{T, N, R}, soffs::Union{Signed, Unsigned}, + n::Union{Signed, Unsigned}) where {T, N, R} n == 0 && return dest n > 0 || Base._throw_argerror() if soffs < 1 || doffs < 1 || soffs + n - 1 > length(src) || doffs + n - 1 > length(dest) throw(BoundsError()) end - # if dest.pool is empty we can safely replace it as we are sure it holds + dest_pa = dest isa PooledArray ? dest : parent(dest) + src_refcount = refcount(src) + + # if dest_pa.pool is empty we can safely replace it as we are sure it holds # no information; having this path is useful because then we can efficiently # `copyto!` into a fresh `PooledArray` created using the `similar` function - if length(dest.pool) == 0 - @assert length(dest.invpool) == 0 - Threads.atomic_add!(src.refcount, 1) - dest.pool = src.pool - dest.invpool = src.invpool - Threads.atomic_sub!(dest.refcount, 1) - dest.refcount = src.refcount - copyto!(dest.refs, doffs, src.refs, soffs, n) - elseif dest.pool === src.pool && dest.invpool === src.invpool - copyto!(dest.refs, doffs, src.refs, soffs, n) - else - @inbounds for i in 0:n-1 - dest[dstart+i] = src[sstart+i] - end - end - return dest -end - -# TODO: handle case when dest is SubArray - -function Base.copyto!(dest::PooledArray{T, R, N, RA}, doffs::Union{Signed, Unsigned}, - src::SubArray{T, N, PooledArray{T, R, M, RA}}, soffs::Union{Signed, Unsigned}, - n::Union{Signed, Unsigned}) where {T, R, N, M, RA} - n == 0 && return dest - n > 0 || Base._throw_argerror() - if soffs < 1 || doffs < 1 || soffs + n - 1 > length(src) || doffs + n - 1 > length(dest) - throw(BoundsError()) - end - - src_parent = parent(src) - - if length(dest.pool) == 0 - @assert length(dest.invpool) == 0 - Threads.atomic_add!(src_parent.refcount, 1) - dest.pool = src_parent.pool - dest.invpool = src_parent.invpool - Threads.atomic_sub!(dest.refcount, 1) - dest.refcount = src_parent.refcount - copyto!(dest.refs, doffs, view(src_parent.refs, src.indices), soffs, n) - elseif dest.pool === src.pool && dest.invpool === src.invpool - copyto!(dest.refs, doffs, view(src_parent.refs, src.indices), soffs, n) + if length(dest_pa.pool) == 0 + @assert length(dest_pa.invpool) == 0 + Threads.atomic_add!(src_refcount, 1) + dest_pa.pool = DataAPI.refpool(src) + dest_pa.invpool = DataAPI.invpool(src) + Threads.atomic_sub!(dest_pa.refcount, 1) + dest_pa.refcount = src_refcount + copyto!(DataAPI.refarray(dest), doffs, DataAPI.refarray(src), soffs, n) + elseif dest.pool === DataAPI.refpool(src) && dest.invpool === DataAPI.invpool(src) + copyto!(DataAPI.refarray(dest), doffs, DataAPIR.refarray(src), soffs, n) else @inbounds for i in 0:n-1 dest[dstart+i] = src[sstart+i] @@ -477,7 +448,8 @@ Base.@propagate_inbounds function Base.getindex(A::PooledArray, I::Union{Real, A return PooledArray(RefArray(newrefs), A.invpool, A.pool, A.refcount) end -Base.@propagate_inbounds function Base.getindex(A::SubArray{T,N,<:PooledArray}, I::Union{Real, AbstractVector}...) +Base.@propagate_inbounds function Base.getindex(A::SubArray{<:Any,<:Any,<:PooledArray}, + I::Union{Real, AbstractVector}...) # make sure we do not increase A.refcount in case creation of newrefs fails newrefs = getindex(view(parent(A).refs, A.indices), I...) @assert newrefs isa AbstractArray From 8ec385465f659090f589f8600e5dd6ea2e00c24b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Wed, 24 Feb 2021 11:32:43 +0100 Subject: [PATCH 21/35] start adding tests --- Project.toml | 2 +- src/PooledArrays.jl | 39 ++++++++++++---- test/runtests.jl | 105 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 137 insertions(+), 9 deletions(-) diff --git a/Project.toml b/Project.toml index 942d0ca..a85b6bf 100644 --- a/Project.toml +++ b/Project.toml @@ -1,6 +1,6 @@ name = "PooledArrays" uuid = "2dfb63ee-cc39-5dd5-95bd-886bf059d720" -version = "2.0.0" +version = "1.2.0" [deps] DataAPI = "9a962f9c-6df0-11e9-0e5d-c546b8b5ee8a" diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 38af771..09a4167 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -77,9 +77,19 @@ PooledArray(refs::RefArray{RA}, invpool::Dict{T,R}, pool::Vector{T}=_invert(invp refcount::Threads.Atomic{Int}=Threads.Atomic{Int}(1)) where {T,R,RA<:AbstractArray{R}} = PooledArray{T,R,ndims(RA),RA}(refs, invpool, pool, refcount) -function PooledArray(d::PooledArray) - Threads.atomic_add!(d.refcount, 1) - return PooledArray(RefArray(copy(d.refs)), d.invpool, d.pool, d.refcount) +# workaround https://github.com/JuliaLang/julia/pull/39809 +_our_copy(x) = copy(x) + +function _our_copy(x::SubArray{<:Any, 0}) + y = similar(x) + y[] = x[] + return y +end + +function PooledArray(d::PooledArrOrSub) + Threads.atomic_add!(refcount(d), 1) + return PooledArray(RefArray(_our_copy(DataAPI.refarray(d))), + DataAPI.invrefpool(d), DataAPI.refpool(d), refcount(d)) end function _label(xs::AbstractArray, @@ -186,7 +196,7 @@ DataAPI.refpool(pa::PooledArray) = pa.pool DataAPI.invrefpool(pa::PooledArray) = pa.invpool refcount(pa::PooledArray) = pa.refcount -DataAPI.refarray(pav::SubArray{<:Any, <:Any, <:PooledArray}) = view(parent(pav).refs, pav.indices) +DataAPI.refarray(pav::SubArray{<:Any, <:Any, <:PooledArray}) = view(parent(pav).refs, pav.indices...) DataAPI.refvalue(pav::SubArray{<:Any, <:Any, <:PooledArray}, i::Integer) = parent(pav).pool[i] DataAPI.refpool(pav::SubArray{<:Any, <:Any, <:PooledArray}) = parent(pav).pool DataAPI.invrefpool(pav::SubArray{<:Any, <:Any, <:PooledArray}) = parent(pav).invpool @@ -196,7 +206,7 @@ Base.size(pa::PooledArray) = size(pa.refs) Base.length(pa::PooledArray) = length(pa.refs) Base.lastindex(pa::PooledArray) = lastindex(pa.refs) -Base.copy(pa::PooledArray) = PooledArray(pa) +Base.copy(pa::PooledArrOrSub) = PooledArray(pa) if isdefined(Base, :copy!) import Base.copy! @@ -236,11 +246,11 @@ function Base.copyto!(dest::PooledArrOrSub{T, N, R}, doffs::Union{Signed, Unsign @assert length(dest_pa.invpool) == 0 Threads.atomic_add!(src_refcount, 1) dest_pa.pool = DataAPI.refpool(src) - dest_pa.invpool = DataAPI.invpool(src) + dest_pa.invpool = DataAPI.invrefpool(src) Threads.atomic_sub!(dest_pa.refcount, 1) dest_pa.refcount = src_refcount copyto!(DataAPI.refarray(dest), doffs, DataAPI.refarray(src), soffs, n) - elseif dest.pool === DataAPI.refpool(src) && dest.invpool === DataAPI.invpool(src) + elseif dest.pool === DataAPI.refpool(src) && dest.invpool === DataAPI.invrefpool(src) copyto!(DataAPI.refarray(dest), doffs, DataAPIR.refarray(src), soffs, n) else @inbounds for i in 0:n-1 @@ -435,6 +445,19 @@ Base.@propagate_inbounds function Base.getindex(pa::PooledArray, I::Integer...) return @inbounds pa.pool[idx] end +Base.@propagate_inbounds function Base.getindex(pav::SubArray{<:Any,<:Any,<:PooledArray}, I::Integer...) + idx = DataAPI.refarray(pav)[I...] + iszero(idx) && throw(UndefRefError()) + return @inbounds DataAPI.refpool(pav)[idx] +end + +# this is needed due to dispatch ambiguities +Base.@propagate_inbounds function Base.getindex(pav::SubArray{T,N,<:PooledArray}, I::Vararg{Int,N}) where {T,N} + idx = DataAPI.refarray(pav)[I...] + iszero(idx) && throw(UndefRefError()) + return @inbounds DataAPI.refpool(pav)[idx] +end + Base.@propagate_inbounds function Base.isassigned(pa::PooledArray, I::Int...) !iszero(pa.refs[I...]) end @@ -451,7 +474,7 @@ end Base.@propagate_inbounds function Base.getindex(A::SubArray{<:Any,<:Any,<:PooledArray}, I::Union{Real, AbstractVector}...) # make sure we do not increase A.refcount in case creation of newrefs fails - newrefs = getindex(view(parent(A).refs, A.indices), I...) + newrefs = getindex(DataAPI.refarray(A), I...) @assert newrefs isa AbstractArray Threads.atomic_add!(A.refcount, 1) return PooledArray(RefArray(newrefs), A.invpool, A.pool, A.refcount) diff --git a/test/runtests.jl b/test/runtests.jl index 1f4685e..eeb435d 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -2,6 +2,10 @@ using Test using PooledArrays using DataAPI: refarray, refvalue, refpool, invrefpool +if !isdefined(Base, :copy!) + using Future: copy! +end + if Threads.nthreads() < 2 @warn("Running with only one thread: correctness of parallel operations is not tested") else @@ -123,3 +127,104 @@ end @test isbitstype(eltype(PooledArrays.fast_sortable(v3))) Base.Order.Perm(Base.Order.Forward, v3).data == PooledArray([1, 3, 2, 4]) end + +@testset "pool non-copying constructor and copy tests" begin + pa = PooledArray([1, 2, 3]) + @test pa.refcount[] == 1 + pa2 = PooledArray(pa) + @test pa.refcount[] == 2 + @test pa.refs == pa2.refs + @test pa.refs !== pa2.refs + @test pa.pool === pa2.pool + @test pa.invpool === pa2.invpool + @test pa.refcount === pa2.refcount + + pav = @view pa[[3, 1]] + + @test pav == [3, 1] + @test DataAPI.refarray(pav) isa SubArray{UInt32,1,Array{UInt32,1},Tuple{Array{Int,1}},false} + @test DataAPI.refpool(pav) === pa.pool + @test DataAPI.invrefpool(pav) === pa.invpool + @test_throws BoundsError DataAPI.refvalue(pav, 0) + @test DataAPI.refvalue(pav, 1) === 1 + @test DataAPI.refvalue(pav, 2) === 2 + @test DataAPI.refvalue(pav, 3) === 3 + @test_throws BoundsError DataAPI.refvalue(pav, 4) + + @test pa.refcount[] == 2 + pa3 = PooledArray(pav) + @test pa.refcount[] == 3 + @test pa.refs[[3, 1]] == pa3.refs + @test pa.refs !== pa3.refs + @test pa.pool === pa3.pool + @test pa.invpool === pa3.invpool + @test pa.refcount === pa3.refcount + pa2 = pa3 + # try to force GC to check finalizer + GC.gc(); GC.gc(); GC.gc(); GC.gc() + @test pa.refcount[] == 2 + + pa = PooledArray([1, 2, 3]) + @test pa.refcount[] == 1 + pa2 = copy(pa) + @test pa.refcount[] == 2 + @test pa.refs == pa2.refs + @test pa.refs !== pa2.refs + @test pa.pool === pa2.pool + @test pa.invpool === pa2.invpool + @test pa.refcount === pa2.refcount + + pav = @view pa[1] + pa3 = copy(pav) + @test pa.refcount[] == 3 + @test DataAPI.refarray(pav) == pa3.refs + @test pa.refs !== pa3.refs + @test pa.pool === pa3.pool + @test pa.invpool === pa3.invpool + @test pa.refcount === pa3.refcount +end + +@testset "test de-referencing on setindex!" begin + pa = PooledArray([1, 2, 3]) + @test pa.refcount[] == 1 + pa2 = copy(pa) + @test pa.refcount[] == 2 + old_pool = pa.pool + old_invpool = pa.invpool + old_refcount = pa.refcount + + # within pool + pa2[1] = 3 + @test pa == 1:3 + @test pa.pool === old_pool + @test pa.invpool === old_invpool + @test pa.refcount === old_refcount + @test pa.refcount[] == 2 + @test pa2 == [3, 2, 3] + @test pa2.pool === old_pool + @test pa2.invpool === old_invpool + @test pa2.refcount === old_refcount + + # new value + pa2[1] = 4 + @test pa == 1:3 + @test pa.pool == old_pool + @test pa.invpool == old_invpool + @test pa.refcount == old_refcount + @test pa.refcount[] == 1 + @test pa2 == [4, 2, 3] + @test pa2.pool !== old_pool + @test pa2.invpool !== old_invpool + @test pa2.refcount !== old_refcount + @test pa2.refcount[] == 1 +end + +@testset "copy! tests" begin + pa = PooledArray([1 2; 3 4]) + pav = @view pa[1:2] + pac = @view pa[1:2] + + pa2 = PooledArray([1]) + pa3 = PooledArray([1 2 3; 4 5 6]) + +end From cd20319e1a090f5d99fa00a3b9d246475ceccdf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Wed, 24 Feb 2021 11:35:25 +0100 Subject: [PATCH 22/35] Apply suggestions from code review Co-authored-by: Milan Bouchet-Valat --- src/PooledArrays.jl | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 38af771..b420266 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -218,8 +218,8 @@ function copy!(dest::PooledArray{T, R, N}, end function Base.copyto!(dest::PooledArrOrSub{T, N, R}, doffs::Union{Signed, Unsigned}, - src::PooledArrOrSub{T, N, R}, soffs::Union{Signed, Unsigned}, - n::Union{Signed, Unsigned}) where {T, N, R} + src::PooledArrOrSub{T, N, R}, soffs::Union{Signed, Unsigned}, + n::Union{Signed, Unsigned}) where {T, N, R} n == 0 && return dest n > 0 || Base._throw_argerror() if soffs < 1 || doffs < 1 || soffs + n - 1 > length(src) || doffs + n - 1 > length(dest) @@ -240,8 +240,9 @@ function Base.copyto!(dest::PooledArrOrSub{T, N, R}, doffs::Union{Signed, Unsign Threads.atomic_sub!(dest_pa.refcount, 1) dest_pa.refcount = src_refcount copyto!(DataAPI.refarray(dest), doffs, DataAPI.refarray(src), soffs, n) - elseif dest.pool === DataAPI.refpool(src) && dest.invpool === DataAPI.invpool(src) - copyto!(DataAPI.refarray(dest), doffs, DataAPIR.refarray(src), soffs, n) + elseif dest.pool === DataAPI.refpool(src) + @assert dest.invpool === DataAPI.invpool(src) + copyto!(DataAPI.refarray(dest), doffs, DataAPI.refarray(src), soffs, n) else @inbounds for i in 0:n-1 dest[dstart+i] = src[sstart+i] From 02b002ba49ec00f4d192498ea0de825433779cc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Wed, 24 Feb 2021 11:57:01 +0100 Subject: [PATCH 23/35] merge methods (currently fails but I first need to understand if it is my or Julia Base bug) --- src/PooledArrays.jl | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 5b41269..10d22a5 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -440,20 +440,14 @@ Base.convert(::Type{Array}, pa::PooledArray{T, R, N}) where {T, R, N} = convert( ############################################################################## # Scalar case -Base.@propagate_inbounds function Base.getindex(pa::PooledArray, I::Integer...) - idx = pa.refs[I...] - iszero(idx) && throw(UndefRefError()) - return @inbounds pa.pool[idx] -end - -Base.@propagate_inbounds function Base.getindex(pav::SubArray{<:Any,<:Any,<:PooledArray}, I::Integer...) +Base.@propagate_inbounds function Base.getindex(pav::PooledArrOrSub, I::Integer...) idx = DataAPI.refarray(pav)[I...] iszero(idx) && throw(UndefRefError()) return @inbounds DataAPI.refpool(pav)[idx] end # this is needed due to dispatch ambiguities -Base.@propagate_inbounds function Base.getindex(pav::SubArray{T,N,<:PooledArray}, I::Vararg{Int,N}) where {T,N} +Base.@propagate_inbounds function Base.getindex(pav::PooledArrOrSub{T,N}, I::Vararg{Int,N}) where {T,N} idx = DataAPI.refarray(pav)[I...] iszero(idx) && throw(UndefRefError()) return @inbounds DataAPI.refpool(pav)[idx] From 21b33543520f05ebd8282da256b59a2f12ef97c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Wed, 24 Feb 2021 12:24:32 +0100 Subject: [PATCH 24/35] enough to remove where --- src/PooledArrays.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 10d22a5..1bf0fa1 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -56,7 +56,7 @@ const PooledVector{T,R} = PooledArray{T,R,1} const PooledMatrix{T,R} = PooledArray{T,R,2} const PooledArrOrSub{T, N, R} = Union{PooledArray{T, R, N}, - SubArray{T, N, <:PooledArray{T, R}}} where {T, R, N} + SubArray{T, N, <:PooledArray{T, R}}} ############################################################################## ## From ec0f7108442418236aa15048b7ef051ec67725d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Wed, 24 Feb 2021 13:52:58 +0100 Subject: [PATCH 25/35] simplify definition --- src/PooledArrays.jl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 1bf0fa1..becd1c5 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -55,8 +55,8 @@ end const PooledVector{T,R} = PooledArray{T,R,1} const PooledMatrix{T,R} = PooledArray{T,R,2} -const PooledArrOrSub{T, N, R} = Union{PooledArray{T, R, N}, - SubArray{T, N, <:PooledArray{T, R}}} +const PooledArrOrSub = Union{SubArray{T, N, <:PooledArray{T, R}}, + PooledArray{T, R, N}} where {T, N, R} ############################################################################## ## From 35430e0bf125bc9359b615054a9ee251452bdfa0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Wed, 24 Feb 2021 14:27:15 +0100 Subject: [PATCH 26/35] update getindex --- src/PooledArrays.jl | 42 ++++++++++++++++++++---------------------- test/runtests.jl | 1 + 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index becd1c5..9eb6c1c 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -439,15 +439,31 @@ Base.convert(::Type{Array}, pa::PooledArray{T, R, N}) where {T, R, N} = convert( ## ############################################################################## -# Scalar case -Base.@propagate_inbounds function Base.getindex(pav::PooledArrOrSub, I::Integer...) +# We need separate functions due to dispatch ambiguities + +for T in (PooledArray, SubArray{<:Any, <:Any, <:PooledArray}) + @eval Base.@propagate_inbounds function Base.getindex(pav::$T, I::Integer...) + idx = DataAPI.refarray(pav)[I...] + iszero(idx) && throw(UndefRefError()) + return @inbounds DataAPI.refpool(pav)[idx] + end + + @eval Base.@propagate_inbounds function Base.getindex(A::$T, I::Union{Real, AbstractVector}...) + # make sure we do not increase A.refcount in case creation of newrefs fails + newrefs = getindex(A.refs, I...) + @assert newrefs isa AbstractArray + Threads.atomic_add!(A.refcount, 1) + return PooledArray(RefArray(newrefs), A.invpool, A.pool, A.refcount) + end +end + +Base.@propagate_inbounds function Base.getindex(pav::PooledArray{<:Any, <:Integer, N}, I::Vararg{Int,N}) where {T,N} idx = DataAPI.refarray(pav)[I...] iszero(idx) && throw(UndefRefError()) return @inbounds DataAPI.refpool(pav)[idx] end -# this is needed due to dispatch ambiguities -Base.@propagate_inbounds function Base.getindex(pav::PooledArrOrSub{T,N}, I::Vararg{Int,N}) where {T,N} +Base.@propagate_inbounds function Base.getindex(pav::SubArray{<:Any, N, <:PooledArray}, I::Vararg{Int,N}) where {T,N} idx = DataAPI.refarray(pav)[I...] iszero(idx) && throw(UndefRefError()) return @inbounds DataAPI.refpool(pav)[idx] @@ -457,24 +473,6 @@ Base.@propagate_inbounds function Base.isassigned(pa::PooledArray, I::Int...) !iszero(pa.refs[I...]) end -# Other cases; we rely on the fact that non-standard indexing will fall back to this eventually -Base.@propagate_inbounds function Base.getindex(A::PooledArray, I::Union{Real, AbstractVector}...) - # make sure we do not increase A.refcount in case creation of newrefs fails - newrefs = getindex(A.refs, I...) - @assert newrefs isa AbstractArray - Threads.atomic_add!(A.refcount, 1) - return PooledArray(RefArray(newrefs), A.invpool, A.pool, A.refcount) -end - -Base.@propagate_inbounds function Base.getindex(A::SubArray{<:Any,<:Any,<:PooledArray}, - I::Union{Real, AbstractVector}...) - # make sure we do not increase A.refcount in case creation of newrefs fails - newrefs = getindex(DataAPI.refarray(A), I...) - @assert newrefs isa AbstractArray - Threads.atomic_add!(A.refcount, 1) - return PooledArray(RefArray(newrefs), A.invpool, A.pool, A.refcount) -end - ############################################################################## ## ## setindex!() definitions diff --git a/test/runtests.jl b/test/runtests.jl index eeb435d..9e0dab2 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -176,6 +176,7 @@ end pav = @view pa[1] pa3 = copy(pav) + @test pa3 isa PooledArray{Int, UInt32, 0} @test pa.refcount[] == 3 @test DataAPI.refarray(pav) == pa3.refs @test pa.refs !== pa3.refs From d3fb0a3574577564c210c084df2f9f7b324efc5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Wed, 24 Feb 2021 17:50:50 +0100 Subject: [PATCH 27/35] continue adding tests --- src/PooledArrays.jl | 15 ++++++++--- test/runtests.jl | 63 ++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 68 insertions(+), 10 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 9eb6c1c..34e9f5b 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -220,10 +220,17 @@ function copy!(dest::PooledArray{T, R, N}, src::PooledArrOrSub{T, N, R}) where {T, N, R} copy!(dest.refs, DataAPI.refarray(src)) src_refcount = refcount(src) - Threads.atomic_add!(src_refcount, 1) - dest.pool = DataAPI.refpool(src) - dest.invpool = DataAPI.invrefpool(src) - dest.refcount = src_refcount + + if dest.pool !== DataAPI.refpool(src) + Threads.atomic_sub!(dest.refcount, 1) + Threads.atomic_add!(src_refcount, 1) + dest.pool = DataAPI.refpool(src) + dest.invpool = DataAPI.invrefpool(src) + dest.refcount = src_refcount + else + @assert dest.invpool === DataAPI.invrefpool(src) + @assert dest.refcount === src_refcount + end return dest end diff --git a/test/runtests.jl b/test/runtests.jl index 9e0dab2..9c93328 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -1,6 +1,6 @@ using Test using PooledArrays -using DataAPI: refarray, refvalue, refpool, invrefpool +using DataAPI: refarray, refvalue, refpool, invrefpool, refcount if !isdefined(Base, :copy!) using Future: copy! @@ -221,11 +221,62 @@ end end @testset "copy! tests" begin - pa = PooledArray([1 2; 3 4]) - pav = @view pa[1:2] - pac = @view pa[1:2] + pa = PooledArray(1:4) + pa1 = pa[1:2] + pav1 = @view pa[2:3] + pa2 = PooledArray(fill(2)) + pav2 = @view pa[3] + + pat1 = PooledArray([0, 0]) + copy!(pat1, pa1) + @test pat1 == pa1 + @test DataAPI.refpool(pat1) == DataAPI.refpool(pa1) + @test DataAPI.invrefpool(pat1) == DataAPI.invrefpool(pa1) + @test refcount(pat1) == refcount(pa1) + @test refcount(pat1)[] == 3 + + copy!(pat1, pav1) + @test pat1 == pav1 + @test DataAPI.refpool(pat1) == DataAPI.refpool(pav1) + @test DataAPI.invrefpool(pat1) == DataAPI.invrefpool(pav1) + @test refcount(pat1) == refcount(pav1) + @test refcount(pat1)[] == 3 + + pat2 = PooledArray(fill(0)) + copy!(pat2, pa2) + @test pat2 == pa2 + @test DataAPI.refpool(pat2) == DataAPI.refpool(pa2) + @test DataAPI.invrefpool(pat2) == DataAPI.invrefpool(pa2) + @test refcount(pat2) == refcount(pa2) + @test refcount(pat2)[] == 2 + + copy!(pat2, pav1) + @test pat2 == pav2 + @test DataAPI.refpool(pat2) == DataAPI.refpool(pav2) + @test DataAPI.invrefpool(pat2) == DataAPI.invrefpool(pav2) + @test refcount(pat2) == refcount(pav2) + @test refcount(pat2)[] == 1 + @test refcount(pa)[] == 4 - pa2 = PooledArray([1]) - pa3 = PooledArray([1 2 3; 4 5 6]) +end +@testset "correct refcount when treading" begin + pa = PooledArray([1 2; 3 4]) + x = Vector{Any}(undef, 120) + Threads.@threads for i in 1:120 + x[i] = copy(pa) + end + @test pa.refcount[] == 121 + Threads.@threads for i in 1:61 + @test x[i].refcount === pa.refcount + x[i][1] = 2 + @test x[i].refcount === pa.refcount + x[i][1] = 5 + @test x[i].refcount[] == 1 + end + @test pa.refcount[] == 60 + x = nothing + # try to force GC to check finalizer + GC.gc(); GC.gc(); GC.gc(); GC.gc() + @test pa.refcount[] == 1 end From b6e6c853f08403ba8afeb2fb396921481959a424 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Wed, 24 Feb 2021 19:34:13 +0100 Subject: [PATCH 28/35] fix current tests --- test/runtests.jl | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/test/runtests.jl b/test/runtests.jl index 9c93328..b6effbb 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -232,14 +232,14 @@ end @test pat1 == pa1 @test DataAPI.refpool(pat1) == DataAPI.refpool(pa1) @test DataAPI.invrefpool(pat1) == DataAPI.invrefpool(pa1) - @test refcount(pat1) == refcount(pa1) + @test refcount(pat1) === refcount(pa1) @test refcount(pat1)[] == 3 copy!(pat1, pav1) @test pat1 == pav1 @test DataAPI.refpool(pat1) == DataAPI.refpool(pav1) @test DataAPI.invrefpool(pat1) == DataAPI.invrefpool(pav1) - @test refcount(pat1) == refcount(pav1) + @test refcount(pat1) === refcount(pav1) @test refcount(pat1)[] == 3 pat2 = PooledArray(fill(0)) @@ -247,17 +247,16 @@ end @test pat2 == pa2 @test DataAPI.refpool(pat2) == DataAPI.refpool(pa2) @test DataAPI.invrefpool(pat2) == DataAPI.invrefpool(pa2) - @test refcount(pat2) == refcount(pa2) + @test refcount(pat2) === refcount(pa2) @test refcount(pat2)[] == 2 - copy!(pat2, pav1) + copy!(pat2, pav2) @test pat2 == pav2 @test DataAPI.refpool(pat2) == DataAPI.refpool(pav2) @test DataAPI.invrefpool(pat2) == DataAPI.invrefpool(pav2) - @test refcount(pat2) == refcount(pav2) - @test refcount(pat2)[] == 1 - @test refcount(pa)[] == 4 - + @test refcount(pat2) === refcount(pav2) + @test refcount(pat2)[] == 4 + @test refcount(pa2)[] == 1 end @testset "correct refcount when treading" begin From 2af32caa1359fc5bcbc836e5a4a8e63f856c72ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Wed, 24 Feb 2021 21:22:54 +0100 Subject: [PATCH 29/35] finalize tests --- src/PooledArrays.jl | 7 ++- test/runtests.jl | 110 ++++++++++++++++++++++++++++++++++++-------- 2 files changed, 98 insertions(+), 19 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 34e9f5b..4cae5f7 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -234,6 +234,10 @@ function copy!(dest::PooledArray{T, R, N}, return dest end +# this is needed as Julia Base uses a special path for this case we want to avoid +Base.copyto!(dest::PooledArrOrSub{T, N, R}, src::PooledArrOrSub{T, N, R}) where {T, N, R} = + copyto!(dest, 1, src, 1, length(src)) + function Base.copyto!(dest::PooledArrOrSub{T, N, R}, doffs::Union{Signed, Unsigned}, src::PooledArrOrSub{T, N, R}, soffs::Union{Signed, Unsigned}, n::Union{Signed, Unsigned}) where {T, N, R} @@ -251,6 +255,7 @@ function Base.copyto!(dest::PooledArrOrSub{T, N, R}, doffs::Union{Signed, Unsign # `copyto!` into a fresh `PooledArray` created using the `similar` function if DataAPI.refpool(dest) === DataAPI.refpool(src) @assert DataAPI.invrefpool(dest) === DataAPI.invrefpool(src) + @assert refcount(dest) === refcount(src) copyto!(DataAPI.refarray(dest), doffs, DataAPI.refarray(src), soffs, n) elseif length(dest_pa.pool) == 0 @assert length(dest_pa.invpool) == 0 @@ -262,7 +267,7 @@ function Base.copyto!(dest::PooledArrOrSub{T, N, R}, doffs::Union{Signed, Unsign copyto!(DataAPI.refarray(dest), doffs, DataAPI.refarray(src), soffs, n) else @inbounds for i in 0:n-1 - dest[dstart+i] = src[sstart+i] + dest[doffs+i] = src[soffs+i] end end return dest diff --git a/test/runtests.jl b/test/runtests.jl index b6effbb..02d66ed 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -1,6 +1,7 @@ using Test using PooledArrays -using DataAPI: refarray, refvalue, refpool, invrefpool, refcount +using DataAPI: refarray, refvalue, refpool, invrefpool +using PooledArrays: refcount if !isdefined(Base, :copy!) using Future: copy! @@ -142,14 +143,14 @@ end pav = @view pa[[3, 1]] @test pav == [3, 1] - @test DataAPI.refarray(pav) isa SubArray{UInt32,1,Array{UInt32,1},Tuple{Array{Int,1}},false} - @test DataAPI.refpool(pav) === pa.pool - @test DataAPI.invrefpool(pav) === pa.invpool - @test_throws BoundsError DataAPI.refvalue(pav, 0) - @test DataAPI.refvalue(pav, 1) === 1 - @test DataAPI.refvalue(pav, 2) === 2 - @test DataAPI.refvalue(pav, 3) === 3 - @test_throws BoundsError DataAPI.refvalue(pav, 4) + @test refarray(pav) isa SubArray{UInt32,1,Array{UInt32,1},Tuple{Array{Int,1}},false} + @test refpool(pav) === pa.pool + @test invrefpool(pav) === pa.invpool + @test_throws BoundsError refvalue(pav, 0) + @test refvalue(pav, 1) === 1 + @test refvalue(pav, 2) === 2 + @test refvalue(pav, 3) === 3 + @test_throws BoundsError refvalue(pav, 4) @test pa.refcount[] == 2 pa3 = PooledArray(pav) @@ -178,7 +179,7 @@ end pa3 = copy(pav) @test pa3 isa PooledArray{Int, UInt32, 0} @test pa.refcount[] == 3 - @test DataAPI.refarray(pav) == pa3.refs + @test refarray(pav) == pa3.refs @test pa.refs !== pa3.refs @test pa.pool === pa3.pool @test pa.invpool === pa3.invpool @@ -230,30 +231,30 @@ end pat1 = PooledArray([0, 0]) copy!(pat1, pa1) @test pat1 == pa1 - @test DataAPI.refpool(pat1) == DataAPI.refpool(pa1) - @test DataAPI.invrefpool(pat1) == DataAPI.invrefpool(pa1) + @test refpool(pat1) === refpool(pa1) + @test invrefpool(pat1) === invrefpool(pa1) @test refcount(pat1) === refcount(pa1) @test refcount(pat1)[] == 3 copy!(pat1, pav1) @test pat1 == pav1 - @test DataAPI.refpool(pat1) == DataAPI.refpool(pav1) - @test DataAPI.invrefpool(pat1) == DataAPI.invrefpool(pav1) + @test refpool(pat1) === refpool(pav1) + @test invrefpool(pat1) === invrefpool(pav1) @test refcount(pat1) === refcount(pav1) @test refcount(pat1)[] == 3 pat2 = PooledArray(fill(0)) copy!(pat2, pa2) @test pat2 == pa2 - @test DataAPI.refpool(pat2) == DataAPI.refpool(pa2) - @test DataAPI.invrefpool(pat2) == DataAPI.invrefpool(pa2) + @test refpool(pat2) === refpool(pa2) + @test invrefpool(pat2) === invrefpool(pa2) @test refcount(pat2) === refcount(pa2) @test refcount(pat2)[] == 2 copy!(pat2, pav2) @test pat2 == pav2 - @test DataAPI.refpool(pat2) == DataAPI.refpool(pav2) - @test DataAPI.invrefpool(pat2) == DataAPI.invrefpool(pav2) + @test refpool(pat2) === refpool(pav2) + @test invrefpool(pat2) === invrefpool(pav2) @test refcount(pat2) === refcount(pav2) @test refcount(pat2)[] == 4 @test refcount(pa2)[] == 1 @@ -279,3 +280,76 @@ end GC.gc(); GC.gc(); GC.gc(); GC.gc() @test pa.refcount[] == 1 end + +@testset "copyto! tests" begin + pa1 = PooledArray([1, 2, 3]) + pa2 = similar(pa1, 4) + @test_throws BoundsError copyto!(pa1, pa2) + copyto!(pa2, pa1) + @test refpool(pa2) === refpool(pa1) + @test invrefpool(pa2) === invrefpool(pa1) + @test refcount(pa2) === refcount(pa1) + @test refcount(pa2)[] == 2 + + pa1 = view(PooledArray([1, 2, 3]), :) + pa2 = similar(pa1, 4) + @test_throws BoundsError copyto!(pa1, pa2) + copyto!(pa2, pa1) + @test refpool(pa2) === refpool(pa1) + @test invrefpool(pa2) === invrefpool(pa1) + @test refcount(pa2) === refcount(pa1) + @test refcount(pa2)[] == 2 + + pa1 = PooledArray([1, 2, 3]) + pa2 = view(similar(pa1, 4), :) + @test_throws BoundsError copyto!(pa1, pa2) + copyto!(pa2, pa1) + @test refpool(pa2) === refpool(pa1) + @test invrefpool(pa2) === invrefpool(pa1) + @test refcount(pa2) === refcount(pa1) + @test refcount(pa2)[] == 2 + + pa1 = view(PooledArray([1, 2, 3]), :) + pa2 = view(similar(pa1, 4), :) + @test_throws BoundsError copyto!(pa1, pa2) + copyto!(pa2, pa1) + @test refpool(pa2) === refpool(pa1) + @test invrefpool(pa2) === invrefpool(pa1) + @test refcount(pa2) === refcount(pa1) + @test refcount(pa2)[] == 2 + + pa1 = PooledArray([1, 2, 3]) + pa2 = similar(pa1, 4) + copyto!(pa2, 1, view(pa1, [1, 1]), 1, 2) + @test refpool(pa2) === refpool(pa1) + @test invrefpool(pa2) === invrefpool(pa1) + @test refcount(pa2) === refcount(pa1) + @test refcount(pa2)[] == 2 + copyto!(pa2, 3, view(pa1, [1, 1]), 1, 2) + @test refpool(pa2) === refpool(pa1) + @test invrefpool(pa2) === invrefpool(pa1) + @test refcount(pa2) === refcount(pa1) + @test refcount(pa2)[] == 2 + @test pa2 == [1, 1, 1, 1] + + pa1 = PooledArray([1, 2, 3]) + pa2 = similar(pa1, Float64, 3) + copyto!(pa2, 1, pa1, 1, 3) + @test refpool(pa2) !== refpool(pa1) + @test invrefpool(pa2) !== invrefpool(pa1) + @test refcount(pa2) !== refcount(pa1) + @test refcount(pa1)[] == 1 + @test refcount(pa2)[] == 1 + @test pa2 == [1, 2, 3] + + pa1 = PooledArray([1, 2, 3]) + pa2 = similar(pa1, 3) + pa2 .= 1 + copyto!(pa2, 1, pa1, 1, 3) + @test refpool(pa2) !== refpool(pa1) + @test invrefpool(pa2) !== invrefpool(pa1) + @test refcount(pa2) !== refcount(pa1) + @test refcount(pa1)[] == 1 + @test refcount(pa2)[] == 1 + @test pa2 == [1, 2, 3] +end From c0333af1eefb7588febe4e8f665125646e671bdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Wed, 24 Feb 2021 22:01:41 +0100 Subject: [PATCH 30/35] hopefully final fixes --- src/PooledArrays.jl | 36 +++++++++++++++------------ test/runtests.jl | 59 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 16 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 4cae5f7..3ac9cf9 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -396,7 +396,7 @@ Base.sort(pa::PooledArray; kw...) = pa[sortperm(pa; kw...)] ############################################################################## function Base.convert(::Type{PooledArray{S,R1,N}}, pa::PooledArray{T,R2,N}) where {S,T,R1<:Integer,R2<:Integer,N} - if S === R && R1 === R2 + if S === T && R1 === R2 return pa else invpool_conv = convert(Dict{S,R1}, pa.invpool) @@ -454,35 +454,35 @@ Base.convert(::Type{Array}, pa::PooledArray{T, R, N}) where {T, R, N} = convert( # We need separate functions due to dispatch ambiguities for T in (PooledArray, SubArray{<:Any, <:Any, <:PooledArray}) - @eval Base.@propagate_inbounds function Base.getindex(pav::$T, I::Integer...) - idx = DataAPI.refarray(pav)[I...] + @eval Base.@propagate_inbounds function Base.getindex(A::$T, I::Integer...) + idx = DataAPI.refarray(A)[I...] iszero(idx) && throw(UndefRefError()) - return @inbounds DataAPI.refpool(pav)[idx] + return @inbounds DataAPI.refpool(A)[idx] end @eval Base.@propagate_inbounds function Base.getindex(A::$T, I::Union{Real, AbstractVector}...) # make sure we do not increase A.refcount in case creation of newrefs fails - newrefs = getindex(A.refs, I...) + newrefs = getindex(DataAPI.refarray(A), I...) @assert newrefs isa AbstractArray - Threads.atomic_add!(A.refcount, 1) - return PooledArray(RefArray(newrefs), A.invpool, A.pool, A.refcount) + Threads.atomic_add!(refcount(A), 1) + return PooledArray(RefArray(newrefs), DataAPI.invrefpool(A), DataAPI.refpool(A), refcount(A)) end end -Base.@propagate_inbounds function Base.getindex(pav::PooledArray{<:Any, <:Integer, N}, I::Vararg{Int,N}) where {T,N} - idx = DataAPI.refarray(pav)[I...] +Base.@propagate_inbounds function Base.getindex(A::PooledArray{<:Any, <:Integer, N}, I::Vararg{Int,N}) where {T,N} + idx = DataAPI.refarray(A)[I...] iszero(idx) && throw(UndefRefError()) - return @inbounds DataAPI.refpool(pav)[idx] + return @inbounds DataAPI.refpool(A)[idx] end -Base.@propagate_inbounds function Base.getindex(pav::SubArray{<:Any, N, <:PooledArray}, I::Vararg{Int,N}) where {T,N} - idx = DataAPI.refarray(pav)[I...] +Base.@propagate_inbounds function Base.getindex(A::SubArray{<:Any, N, <:PooledArray}, I::Vararg{Int,N}) where {T,N} + idx = DataAPI.refarray(A)[I...] iszero(idx) && throw(UndefRefError()) - return @inbounds DataAPI.refpool(pav)[idx] + return @inbounds DataAPI.refpool(A)[idx] end -Base.@propagate_inbounds function Base.isassigned(pa::PooledArray, I::Int...) - !iszero(pa.refs[I...]) +Base.@propagate_inbounds function Base.isassigned(pa::PooledArrOrSub, I::Int...) + !iszero(DataAPI.refarray(pa)[I...]) end ############################################################################## @@ -522,7 +522,11 @@ function unsafe_pool_push!(pa::PooledArray{T,R}, val) where {T,R} pool_idx end -Base.@propagate_inbounds function Base.setindex!(x::PooledArray, val, ind::Integer) +# assume PooledArray is only used with Arrays as this is what _label does +# this simplifies code below +Base.IndexStyle(::Type{<:PooledArray}) = IndexLinear() + +Base.@propagate_inbounds function Base.setindex!(x::PooledArray, val, ind::Int) x.refs[ind] = getpoolidx(x, val) return x end diff --git a/test/runtests.jl b/test/runtests.jl index 02d66ed..8311ce9 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -353,3 +353,62 @@ end @test refcount(pa2)[] == 1 @test pa2 == [1, 2, 3] end + +@testset "reverse" begin + pa1 = PooledArray([1, 2, 3]) + pa2 = reverse(pa1) + pa3 = reverse(pa2) + @test pa2 == [3, 2, 1] + @test pa3 == pa1 + @test refpool(pa1) === refpool(pa2) === refpool(pa3) + @test invrefpool(pa1) === invrefpool(pa2) === invrefpool(pa3) + @test refcount(pa1) === refcount(pa2) === refcount(pa3) + @test refcount(pa1)[] == 3 +end + +@testset "convert" begin + pa1 = PooledArray([1, 2, 3]) + @test convert(PooledArray, pa1) === pa1 + @test eltype(convert(PooledArray{Float64}, pa1)) === Float64 + @test convert(PooledArray{Int, UInt64, 1}, pa1) isa PooledArray{Int,UInt64,1,Array{UInt64,1}} +end + +@testset "indexing" begin + pa = PooledArray([1 2; 3 4]) + @test pa[2, 2, 1] == 4 + @test pa[2, 2] == 4 + @test pa[big(2), 2] == 4 + pav = view(pa, :, :) + @test pav[2, 2, 1] == 4 + @test pav[2, 2] == 4 + @test pav[big(2), 2] == 4 + + @test refcount(pa)[] == 1 + pa2 = pa[[true, false, false, true]] + @test pa2 == [1, 4] + @test refpool(pa) === refpool(pa2) + @test invrefpool(pa) === invrefpool(pa2) + @test refcount(pa) === refcount(pa2) + @test refcount(pa)[] == 2 + pa3 = pav[[true, false, false, true]] + @test pa3 == [1, 4] + @test refpool(pa) === refpool(pa3) + @test invrefpool(pa) === invrefpool(pa3) + @test refcount(pa) === refcount(pa3) + @test refcount(pa)[] == 3 +end + +@testset "isassigned" begin + pa1 = PooledArray(["a"]) + pa2 = similar(pa1, 2) + pa2v = view(pa2, 1) + @test !isassigned(pa2, 1) + @test !isassigned(pa2v) +end + +@testset "setindex!" begin + pa = PooledArray([1 2; 3 4]) + pa[1, 1] = 10 + @test pa == [10 2; 3 4] + @test [pa pa] == [10 2 10 2; 3 4 3 4] +end From 46d971cd5b8ac809a2fd2489db3a4f403168423a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Thu, 25 Feb 2021 00:41:04 +0100 Subject: [PATCH 31/35] add Julia 1.0.5 support --- Project.toml | 1 + src/PooledArrays.jl | 18 ++++++++++++------ test/runtests.jl | 13 +++++++------ 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/Project.toml b/Project.toml index a85b6bf..1adacfe 100644 --- a/Project.toml +++ b/Project.toml @@ -4,6 +4,7 @@ version = "1.2.0" [deps] DataAPI = "9a962f9c-6df0-11e9-0e5d-c546b8b5ee8a" +Future = "9fa8497b-333b-5362-9e8d-4d0656e87820" [compat] DataAPI = "1.5" diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 3ac9cf9..f47f075 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -1,6 +1,7 @@ module PooledArrays import DataAPI +import Future.copy! export PooledArray, PooledVector, PooledMatrix @@ -208,12 +209,6 @@ Base.lastindex(pa::PooledArray) = lastindex(pa.refs) Base.copy(pa::PooledArrOrSub) = PooledArray(pa) -if isdefined(Base, :copy!) - import Base.copy! -else - import Future: copy! -end - # here we do not allow dest to be SubArray as copy! is intended to replace whole arrays # slow path will be used for SubArray function copy!(dest::PooledArray{T, R, N}, @@ -469,6 +464,17 @@ for T in (PooledArray, SubArray{<:Any, <:Any, <:PooledArray}) end end +if VERSION < v"1.1" + Base.@propagate_inbounds function Base.getindex(A::SubArray{T,D,P,I,true} , + i::Int) where {I<:Tuple{Union{Base.Slice, + AbstractUnitRange}, + Vararg{Any}}, P<:PooledArray, T, D} + idx = DataAPI.refarray(A)[i] + iszero(idx) && throw(UndefRefError()) + return @inbounds DataAPI.refpool(A)[idx] + end +end + Base.@propagate_inbounds function Base.getindex(A::PooledArray{<:Any, <:Integer, N}, I::Vararg{Int,N}) where {T,N} idx = DataAPI.refarray(A)[I...] iszero(idx) && throw(UndefRefError()) diff --git a/test/runtests.jl b/test/runtests.jl index 8311ce9..903b16a 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -2,10 +2,7 @@ using Test using PooledArrays using DataAPI: refarray, refvalue, refpool, invrefpool using PooledArrays: refcount - -if !isdefined(Base, :copy!) - using Future: copy! -end +import Future.copy! if Threads.nthreads() < 2 @warn("Running with only one thread: correctness of parallel operations is not tested") @@ -163,7 +160,9 @@ end pa2 = pa3 # try to force GC to check finalizer GC.gc(); GC.gc(); GC.gc(); GC.gc() - @test pa.refcount[] == 2 + if pa.refcount[] != 2 + @warn "finalizer of PooledArray not triggered; excess refs: $(pa.refcount[] - 2)" + end pa = PooledArray([1, 2, 3]) @test pa.refcount[] == 1 @@ -278,7 +277,9 @@ end x = nothing # try to force GC to check finalizer GC.gc(); GC.gc(); GC.gc(); GC.gc() - @test pa.refcount[] == 1 + if pa.refcount[] != 1 + @warn "finalizer of PooledArray not triggered; excess refs: $(pa.refcount[] - 1)" + end end @testset "copyto! tests" begin From c55b8e95f23dc3f246ef8d40a02e406f8042f780 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Thu, 25 Feb 2021 09:49:59 +0100 Subject: [PATCH 32/35] Update src/PooledArrays.jl --- src/PooledArrays.jl | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index f47f075..66f3ede 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -391,12 +391,8 @@ Base.sort(pa::PooledArray; kw...) = pa[sortperm(pa; kw...)] ############################################################################## function Base.convert(::Type{PooledArray{S,R1,N}}, pa::PooledArray{T,R2,N}) where {S,T,R1<:Integer,R2<:Integer,N} - if S === T && R1 === R2 - return pa - else - invpool_conv = convert(Dict{S,R1}, pa.invpool) - @assert invpool_conv !== pa.invpool - end + invpool_conv = convert(Dict{S,R1}, pa.invpool) + @assert invpool_conv !== pa.invpool if R1 === R2 refs_conv = pa.refs From 03a3221c12d589145e5fbe7d343748bdf6bd997b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sat, 27 Feb 2021 23:07:54 +0100 Subject: [PATCH 33/35] fixes after code review --- src/PooledArrays.jl | 6 ------ test/runtests.jl | 41 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index f47f075..ff0e263 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -475,12 +475,6 @@ if VERSION < v"1.1" end end -Base.@propagate_inbounds function Base.getindex(A::PooledArray{<:Any, <:Integer, N}, I::Vararg{Int,N}) where {T,N} - idx = DataAPI.refarray(A)[I...] - iszero(idx) && throw(UndefRefError()) - return @inbounds DataAPI.refpool(A)[idx] -end - Base.@propagate_inbounds function Base.getindex(A::SubArray{<:Any, N, <:PooledArray}, I::Vararg{Int,N}) where {T,N} idx = DataAPI.refarray(A)[I...] iszero(idx) && throw(UndefRefError()) diff --git a/test/runtests.jl b/test/runtests.jl index 903b16a..9903f5d 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -371,7 +371,10 @@ end pa1 = PooledArray([1, 2, 3]) @test convert(PooledArray, pa1) === pa1 @test eltype(convert(PooledArray{Float64}, pa1)) === Float64 - @test convert(PooledArray{Int, UInt64, 1}, pa1) isa PooledArray{Int,UInt64,1,Array{UInt64,1}} + pa1c = convert(PooledArray{Int, UInt64, 1}, pa1) + @test pa1c isa PooledArray{Int,UInt64,1,Array{UInt64,1}} + @test pa1c == pa1 + @test !(pa1c isa typeof(pa1)) end @testset "indexing" begin @@ -397,6 +400,32 @@ end @test invrefpool(pa) === invrefpool(pa3) @test refcount(pa) === refcount(pa3) @test refcount(pa)[] == 3 + + # these checks are mostly needed to check for dispatch ambiguities + @test pa[1] == 1 + @test pa[1, 1] == 1 + @test pa[1, 1, 1] == 1 + @test pa[:] == [1, 3, 2, 4] + @test pa[1:4] == [1, 3, 2, 4] + @test pa[collect(1:4)] == [1, 3, 2, 4] + @test pa[1, 1:2] == [1, 2] + @test pa[1, [1, 2]] == [1, 2] + @test pa[1:1, 1:2] == [1 2] + @test pa[1:1, [1, 2]] == [1 2] + @test pa[[1], 1:2] == [1 2] + @test pa[[1], [1, 2]] == [1 2] + @test pav[1] == 1 + @test pav[1, 1] == 1 + @test pav[1, 1, 1] == 1 + @test pav[:] == [1, 3, 2, 4] + @test pav[1:4] == [1, 3, 2, 4] + @test pav[collect(1:4)] == [1, 3, 2, 4] + @test pav[1, 1:2] == [1, 2] + @test pav[1, [1, 2]] == [1, 2] + @test pav[1:1, 1:2] == [1 2] + @test pav[1:1, [1, 2]] == [1 2] + @test pav[[1], 1:2] == [1 2] + @test pav[[1], [1, 2]] == [1 2] end @testset "isassigned" begin @@ -412,4 +441,14 @@ end pa[1, 1] = 10 @test pa == [10 2; 3 4] @test [pa pa] == [10 2 10 2; 3 4 3 4] + pa[2] = 1000 + @test pa == [10 2; 1000 4] + pa[1, :] = [11, 12] + @test pa == [11 12; 1000 4] + pa[1:2, 1:1] = [111, 222] + @test pa == [111 12; 222 4] + pa[1, 1, 1] = 0 + @test pa == [0 12; 222 4] + pa[:] = [1 2; 3 4] + @test pa == [1 2; 3 4] end From 3a92892d35cb9c14924e02d83ba7beb91a120575 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sat, 27 Feb 2021 23:09:10 +0100 Subject: [PATCH 34/35] Update src/PooledArrays.jl Co-authored-by: Milan Bouchet-Valat --- src/PooledArrays.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index 66f3ede..2b6a900 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -453,7 +453,7 @@ for T in (PooledArray, SubArray{<:Any, <:Any, <:PooledArray}) @eval Base.@propagate_inbounds function Base.getindex(A::$T, I::Union{Real, AbstractVector}...) # make sure we do not increase A.refcount in case creation of newrefs fails - newrefs = getindex(DataAPI.refarray(A), I...) + newrefs = DataAPI.refarray(A)[I...] @assert newrefs isa AbstractArray Threads.atomic_add!(refcount(A), 1) return PooledArray(RefArray(newrefs), DataAPI.invrefpool(A), DataAPI.refpool(A), refcount(A)) From 3eb65e7101c1d7c2406c9aa631992eb1e3b01efb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Sun, 28 Feb 2021 23:53:41 +0100 Subject: [PATCH 35/35] Update src/PooledArrays.jl Co-authored-by: Milan Bouchet-Valat --- src/PooledArrays.jl | 1 + 1 file changed, 1 insertion(+) diff --git a/src/PooledArrays.jl b/src/PooledArrays.jl index f4919c0..5ddd0bd 100644 --- a/src/PooledArrays.jl +++ b/src/PooledArrays.jl @@ -471,6 +471,7 @@ if VERSION < v"1.1" end end +# Defined to avoid ambiguities with Base Base.@propagate_inbounds function Base.getindex(A::SubArray{<:Any, N, <:PooledArray}, I::Vararg{Int,N}) where {T,N} idx = DataAPI.refarray(A)[I...] iszero(idx) && throw(UndefRefError())