Skip to content

Commit

Permalink
make Window memory safe
Browse files Browse the repository at this point in the history
fixes #2

Surprisingly the compiler can optimize away the parent array reference
in `Window` already and we do not need to wait for Julia 1.5.
  • Loading branch information
stev47 committed Apr 21, 2020
1 parent 5bd1e71 commit b884686
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 51 deletions.
2 changes: 0 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@ sum(k, a)

## User Notes

- using/storing the window beyond the kernel operation itself is considered
unsafe, see #2
- for best performance you should annotate kernel functions with `@inline` and
`@inbounds`
- the package is aimed at small kernels, use different algorithms for larger
Expand Down
24 changes: 12 additions & 12 deletions src/extension.jl
Original file line number Diff line number Diff line change
Expand Up @@ -29,33 +29,33 @@ Depending on the extension a call to this function may be a no-op.

@inline function getindex_extension(w, wi, ext::ExtensionReplicate)
pi = position(w) + wi
pimod = CartesianIndex(clamp.(Tuple(pi), ntuple(_->1, Val(length(wi))), w.parent_size))
return getindex_parent(w, pimod)
pimod = CartesianIndex(clamp.(Tuple(pi), ntuple(_->1, Val(length(wi))), size(parent(w))))
return parent(w)[pimod]
end
@inline function setindex_extension!(w, x, wi, ext::ExtensionReplicate)
pi = position(w) + wi
pimod = CartesianIndex(clamp.(Tuple(pi), ntuple(_->1, Val(length(wi))), w.parent_size))
return setindex_parent!(w, x, pimod)
pimod = CartesianIndex(clamp.(Tuple(pi), ntuple(_->1, Val(length(wi))), size(parent(w))))
return parent(w)[pimod] = x
end

@inline function getindex_extension(w, wi, ext::ExtensionCircular)
pi = position(w) + wi
pimod = CartesianIndex(mod1.(Tuple(pi), w.parent_size))
return getindex_parent(w, pimod)
pimod = CartesianIndex(mod1.(Tuple(pi), size(parent(w))))
return parent(w)[pimod]
end
@inline function setindex_extension!(w, x, wi, ext::ExtensionCircular)
pi = position(w) + wi
pimod = CartesianIndex(mod1.(Tuple(pi), w.parent_size))
return setindex_parent!(w, x, pimod)
pimod = CartesianIndex(mod1.(Tuple(pi), size(parent(w))))
return parent(w)[pimod] = x
end

@inline function getindex_extension(w, wi, ext::ExtensionSymmetric)
pi = position(w) - wi
pimod = CartesianIndex(mod1.(Tuple(pi), w.parent_size))
return getindex_parent(w, pimod)
pimod = CartesianIndex(mod1.(Tuple(pi), size(parent(w))))
return parent(w)[pimod]
end
@inline function setindex_extension!(w, x, wi, ext::ExtensionSymmetric)
pi = position(w) - wi
pimod = CartesianIndex(mod1.(Tuple(pi), w.parent_size))
return setindex_parent!(w, x, pimod)
pimod = CartesianIndex(mod1.(Tuple(pi), size(parent(w))))
return parent(w)[pimod] = x
end
5 changes: 2 additions & 3 deletions src/types.jl
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,13 @@ A stack-allocated array view fit for a kernel type `K` with indexing on axes
#struct Window{T,N,K} <: AbstractArray{T,N}
struct Window{T,N,X,K}
position::CartesianIndex{N}
parent_ptr::Ptr{T}
parent_size::NTuple{N,Int}
parent::Array{T,N}
kernel::K

# TODO: relax to StridedArrays
function Window{X}(k::Kernel, a::DenseArray{T,N}, pos::CartesianIndex{N}) where {T,N,X}
# because we use Base._sub2ind
require_one_based_indexing(a)
return new{T,N,X,typeof(k)}(pos, pointer(a), size(a), k)
return new{T,N,X,typeof(k)}(pos, a, k)
end
end
37 changes: 3 additions & 34 deletions src/window.jl
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@ Create a stack-allocated view on `a` with axes `X` and cartesian indexing
relative to `pos` in the parent array.
The window axes span at most the axes of kernel type `K` and behaviour when
indexing outside them is determined by the extension specified by `K`.
The user is responsible for ensuring that the parent array outlives this object
by using e.g. `GC.@preserve`.
"""
function Window end

Expand All @@ -25,7 +22,7 @@ Base.size(w::Window) = length.(axes(w))
# we rely on the compiler to constant propagate this check away
checkbounds(Bool, w, wi) || return getindex_extension(w, wi, extension(w.kernel))

return getindex_parent(w, position(w) + wi)
return parent(w)[position(w) + wi]
end

@propagate_inbounds Base.setindex!(w::Window, x, wi::Int...) = setindex!(w, x, CartesianIndex(wi))
Expand All @@ -34,7 +31,7 @@ end
# we rely on the compiler to constant propagate this check away
checkbounds(Bool, w, wi) || return setindex_extension!(w, x, wi, extension(w.kernel))

return setindex_parent!(w, x, position(w) + wi)
return parent(w)[position(w) + wi] = x
end

# Window interface
Expand All @@ -60,40 +57,12 @@ NOTE: this doesn't check bounds and thus assumes the window was properly
return ntuple(f, Val(prod(size(w))))
end

"""
getindex_parent(w::Window, i::CartesianIndex)
Equivalent to `parent(w)[i]` but non-allocating.
"""
@propagate_inbounds @inline function getindex_parent(w::Window{<:Any,N}, pi::CartesianIndex{N}) where N
return unsafe_load(w.parent_ptr, _cart2lin(w, pi))
end

"""
setindex_parent!(w::Window, x, i::CartesianIndex)
Equivalent to `parent(w)[i] = x` but non-allocating.
"""
@propagate_inbounds @inline function setindex_parent!(w::Window{<:Any,N}, x, pi::CartesianIndex{N}) where N
return unsafe_store!(w.parent_ptr, x, _cart2lin(w, pi))
end

@inline function _cart2lin(w::Window, pi)
@boundscheck checkbounds(Bool, CartesianIndices(w.parent_size), pi) ||
throw(BoundsError(parent(w), (pi,)))

# TODO: would like to use LinearIndices here, but it creates extra
# instructions, fix upstream?
# return LinearIndices(w.parent_size)[pi]
return _sub2ind(w.parent_size, Tuple(pi)...)
end

"""
parent(w::Window)
Return reference to parent array. This method may allocate.
"""
Base.parent(w::Window) = unsafe_wrap(Array, w.parent_ptr, w.parent_size)
Base.parent(w::Window) = w.parent

# TODO: we don't want a fully fledged OffsetArray, but having similar() and
# copy() work would be nice
Expand Down

0 comments on commit b884686

Please sign in to comment.