From b884686e27a0b5ca20a5b47b727e34faf9d8e3a8 Mon Sep 17 00:00:00 2001 From: Stephan Hilb Date: Tue, 21 Apr 2020 22:16:54 +0200 Subject: [PATCH] make `Window` memory safe 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. --- README.md | 2 -- src/extension.jl | 24 ++++++++++++------------ src/types.jl | 5 ++--- src/window.jl | 37 +++---------------------------------- 4 files changed, 17 insertions(+), 51 deletions(-) diff --git a/README.md b/README.md index c869884..6e8a792 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/src/extension.jl b/src/extension.jl index bf68d13..9ae297b 100644 --- a/src/extension.jl +++ b/src/extension.jl @@ -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 diff --git a/src/types.jl b/src/types.jl index 11673e4..ea8d184 100644 --- a/src/types.jl +++ b/src/types.jl @@ -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 diff --git a/src/window.jl b/src/window.jl index 9104683..24a8aa1 100644 --- a/src/window.jl +++ b/src/window.jl @@ -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 @@ -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)) @@ -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 @@ -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