More thorough aliasing detection in nonscalar copy, broadcast and assignment #25890

mbauman · 2018-02-05T15:22:49Z

This commit puts together the beginnings of an "interface" for detecting and avoiding aliasing between arrays before performing some mutating operation on one of them.

A′ = unalias(dest, A) is the main entry point; it checks to see if dest and A might alias one another, and then either returns A or a copy of A. When it returns a copy of A, it absolutely must preserve the same type. As such, this function ~~is designed to be specialized on A, typically like:~~ calls an internal ~~copypreservingtype~~ unaliascopy function, which defaults to copy but ensures it returns the same type.

mightalias(A, B) is a quick and ~~rough~~ conservative check to see if two arrays alias eachother. Instead of requiring every array to know about every other array type, it simply asks both arrays for their dataids and then looks for an empty intersection.

Finally, dataids(A) returns a tuple of ~~ranges~~ UInts that describe something about the region of memory the array occupies. ~~It could be simply (objectid(A):objectid(A),)~~ It defaults to simply (objectid(A),), or for an array with multiple fields, (dataids(A.a)..., dataids(A.b)..., ...).

Fixes #21693, fixes #15209, and a pre-req to get tests passing for #24368 (since we had spot checks for Array aliasing, but moving to broadcast means we need a slightly more generic solution). I haven't exported any of these functions since I'd like us to use them a bit more before we officially document and support them for all custom arrays.

nalimilan · 2018-02-05T15:26:28Z

base/abstractarray.jl

+"""
+    unalias(dest, A)
+
+Return either A or a copy of A, in a rough effort to prevent modifications to `dest` from


Missing backticks around A.

nalimilan · 2018-02-05T15:26:38Z

base/abstractarray.jl

+
+This function must return an object of exactly the same type as A for performance and type-stability.
+
+See also `mightalias` and `dataids`.


Use @ref.

nalimilan · 2018-02-05T15:27:26Z

base/abstractarray.jl

+"""
+unalias(dest, A) = A
+unalias(dest, A::Array) = mightalias(dest, A) ? copy(A) : A
+@inline unalias(dest, As::Tuple) = (unalias(dest, As[1]), unalias(dest, tail(As))...)


Why not use varargs?

Why not? I liked this symmetry in the calling conventions:

function setindex(dest, source, I...) source′ = unalias(dest, source) I′ = unalias(dest, I)

It's also nice in that the tuple case mostly fits the explanation above without needing to call it out — it'll either return exactly the tuple or a (partial) copy.

Yeah, I guess you need to choose between symmetry and consistency with setindex.

nalimilan · 2018-02-05T15:29:25Z

base/abstractarray.jl

+
+Perform a rudimentary test to check if arrays A and B might share the same memory.
+
+Defaults to false unless `dataids` is specialized for both `A` and `B`.


When in doubt, wouldn't it be better to return true? More generally, shouldn't the functions require/guarantee that any aliasing is detected and that in doubt a copy will be made? The use of terms like "rough" and "rudimentary" doesn't inspire confidence. ;-) We don't want silent corruption to happen by default...

Or is it frequently impossible to determine whether arrays alias?

Indeed, I don't want to inspire confidence. That's why this isn't exported. I just want to do incrementally better than we had been doing before for the builtin array types. There's an asymmetry in costs here — returning true by default would lead to lots of unnecessary copies. But we also need to perform this check quite frequently, so I don't want the check itself to be expensive. This is the tradeoff I came to.

I initially had default fallbacks for strided arrays based on pointer(A, firstindex(A)) and pointer(A, lastindex(A)) and all other arrays based on objectid(A) — that led to #25858. We could try to extend this more generically later.

mbauman · 2018-02-05T15:53:03Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2018-02-05T21:39:14Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

mbauman · 2018-02-06T18:21:25Z

Thanks Nanosoldier. It had flagged bonus copies coming from broadcast!(conj, A, A). This is indeed safe — we only make one pass through the array… but it's only safe because the destination === the source.

mbauman · 2018-02-07T17:20:07Z

Any further thoughts here? I think this infrastructure gives us a nice place to expand this functionality in the future. The biggest question I had in implementing it is if we should try harder to detect aliasing in the generic case. I had initially had these definitions:

unalias(dest, A::AbstractArray) = mightalias(dest, A) ? deepcopy(A) : A
dataids(A::AbstractArray) = A === parent(A) ? (objectid(A):objectid(A),) : dataids(parent(A))
_increasingrange(a, b) = min(a,b):max(a,b)
dataids(A::StridedArray) = (_increasingrange(UInt(pointer(A, firstindex(A))), UInt(pointer(A, lastindex(A)))),)

But we could add these at any point. In fact, we could eventually lean on the GC or more private struct construction internals to recursively identify and dealias fields if we wished.

JeffBezanson · 2018-02-08T22:49:28Z

base/abstractarray.jl

+"""
+unalias(dest, A) = A
+unalias(dest, A::Array) = mightalias(dest, A) ? copy(A) : A
+@inline unalias(dest, As::Tuple) = (unalias(dest, As[1]), unalias(dest, tail(As))...)


I think this would make more sense as a varargs function; otherwise it seems like you're unaliasing the tuple itself (which is somewhat array-like, so could be confusing).

Yeah, that was Milan's comment, too. I can make that change. #25890 (comment)

mbauman · 2018-02-08T22:49:59Z

~~I'll plan to merge this tomorrow unless I hear any objections.~~

vtjnash · 2018-02-08T23:02:59Z

base/array.jl

@@ -694,22 +694,18 @@ function setindex! end
 # These are redundant with the abstract fallbacks but needed for bootstrap
 function setindex!(A::Array, x, I::AbstractVector{Int})
    @_propagate_inbounds_meta
-    A === I && (I = copy(I))
-    for i in I
+    J = unalias(A, I)


Since we were just calling copy before, maybe we should just keep doing that, and just improve the A === I test. We won't really need the unalias function then (which looks rather complex to implement), and this would just be written mayalias(A, I) && (I = copy(I))

I assume that in cases, the code might also just instead branch to an alternative implementation, so the copy isn't always necessary, just a defensive algorithm?

This is a slightly more special case, since we know that A::Array, so if A === I then I::Array, too. And so we know that copy(I)::typeof(I). Note that unalias(dest, A::Array) is exactly this definition — it just calls copy(A).

In the general case, a branch with copy(I) or I will be type-unstable, necessitating loop switching or a function barrier or some such. That's why custom arrays need to implement it on their own.

Sure, you'd be free to call mightalias instead of unalias if you know about an alternative.

nalimilan · 2018-02-09T09:08:32Z

I still (see #25890 (comment)) find it unsatisfying that might_alias does even guarantee that there will be no false negatives, meaning that this general mechanism may sometimes fail to prevent corruption. I'd rather make a copy or raise an error in that case by default (as for sparse matrices, #21693). But if that's just me and there's no efficient solution to that, go ahead...

mbauman · 2018-02-13T05:21:35Z

I've pushed a refactoring of the API to just use map instead of having a built-in mapper method, but I've not done thorough perf testing on it yet. Let's see if Nanosoldier's up for some overnight work:

@nanosoldier runbenchmarks(ALL, vs=":master")

mbauman · 2018-02-13T20:43:33Z

So as a compromise I tried implementing the slightly better fallback methods I described in #25890 (comment), but I ran into performance issues since dataids(A::AbstractArray) = A === parent(A) ? (objectid(A):objectid(A),) : dataids(parent(A)) cannot inline and isn't type-stable. I also noticed that deepcopy inlines at least a bit of its work into the caller, which adds quite a bit of unnecessary code weight — but that has an easier solution.

One reason not to do the dataids(::Any) = (UInt(0):typemax(UInt),) is because that'd necessitate us documenting, exporting, and standing by this interface. Doing it incrementally like this means that we can test it in base and then roll it out to all custom arrays as a new feature in 1.x.

mbauman · 2018-02-14T00:36:17Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2018-02-14T05:46:20Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

mbauman · 2018-02-14T16:44:03Z

The performance regressions in lucompletepivSub are actually bug fixes. That code attempts to swap columns and rows with an idiom like: A[1:end,[1,2]] = @view A[1:end,[2,1]]. This isn't safe without this patch.

laplace_iter_sub similarly does clever assignments into a view from views of the same array. I think that this one is done in a way that happens to be safe, but it does accumulate eps-level differences with more than 14 iterations when I add a copy around the RHS of the .=. Not sure what's happening there, but this perf change is not surprising.

euro_option_vec is doing S .*= exp.(t2 .* randn!(R) .+ t1), which is effectively broadcast!(f, S, S, t2, randn!(R), t1), so this, too, fails the aliasing check. We're just being a little too strict here — had it been broadcast!(f, S, S', …) (or some other such re-arranging view), then it would have been a bug fix. nbody is doing the same thing. I think we should be able to extend my one-argument perf patch (94262bd) to any arg.

repeat is a slightly different problem — it's "tiling" the repeated region from the top-left corner throughout the array. I should be able to fix this for StridedArrays by adding a specialization for StridedSubArrays that looks at the pointers of the restricted region.

…ignment. This commit puts together the beginnings of an "interface" for detecting and avoiding aliasing between arrays before performing some mutating operation on one of them. `A′ = unalias(dest, A)` is the main entry point; it checks to see if `dest` and `A` might alias one another, and then either returns `A` or a copy of `A`. When it returns a copy of `A`, it absolutely must preserve the same type. As such, this function is designed to be specialized on `A`, typically like: Base.unalias(dest, A::T) = mightalias(dest, A) ? typeof(A)(unalias(dest, A.a), unalias(dest, A.b), ...) : A `mightalias(A, B)` is a quick and rough check to see if two arrays alias eachother. Instead of requiring every array to know about every other array type, it simply asks both arrays for their `dataids` and then looks for an empty intersection between every pair. Finally, `dataids(A)` returns a tuple of ranges that describe something about the region of memory the array occupies. It could be simply `(objectid(A):objectid(A),)` or for an array with multiple fields, `(dataids(A.a)..., dataids(A.b)..., ...)`.

And fixup the assumption that `broadcast!(f, C, A)` is safe

a builtin mapper method. Using varargs would require de-structuring the one arg case: `(a,) = unalias(dest, a)`.

…rray... to only look at the linear data segment of its parent that it can reach

mbauman · 2018-02-14T19:39:08Z

Ok, I've added two commits that try to ameliorate those performance concerns (2254dc5 and d97d0c3 — GitHub is showing them out of order due to a rebase).

Unfortunately, d97d0c3 isn't sufficient to fix the performance regression in repeat, for two reasons: first, it's using the A[I] = B syntax, which doesn't consider indices in its computation — but that's slated to change to broadcast, which resolves this part of the issue. The bigger problem is that I'm only looking at linear segments of memory, and while doing an operation like an N-dimensional tile is strided, I'm only looking at the linear maximal extents of the memory.

It feels rather unsatisfactory, but I think I'll need to change repeat to copy the prototype to a temporary buffer and then tile that throughout the output array. Edit: actually, I think I'm going to spitball a StridedRange type that represents this memory layout and does more careful intersection-checking.

StefanKarpinski · 2018-02-14T20:20:07Z

Can we just have a way of expressing "just trust me, don't worry about aliasing here"?

mbauman · 2018-02-21T18:17:06Z

Alright, I'd like to merge this guy tonight or tomorrow so I can then merge #24368.

vtjnash · 2018-02-21T20:50:04Z

base/abstractarray.jl

+unalias(dest, A) = mightalias(dest, A) ? copypreservingtype(A) : A
+
+copypreservingtype(A::Array) = copy(A)
+copypreservingtype(A::AbstractArray) = (@_noinline_meta; deepcopy(A)::typeof(A))


I'm not sure deepcopy is ever correct. Maybe better just to give a method error?

Ah, you're right, I didn't consider how this would change the identities of the values that get assigned into the array. Perhaps we should just fall back to copy(A). It'll be type-unstable in some cases, but this function exists to allow you to fix it.

This will do what we want in almost all cases. We only hit this method if `A` is aliasing a mutable array... which means that `A` is mutable as well, and thus in many cases it has defined an appropriate `similar(A)` method that returns the same type.

vchuravy · 2018-02-22T16:53:13Z

@nanosoldier runbenchmarks(ALL, vs=":master")

mbauman · 2018-02-22T17:19:19Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2018-02-22T22:33:50Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

StefanKarpinski · 2018-02-22T22:41:56Z

How fast do you want a garbage answer?

mbauman · 2018-02-22T22:43:59Z

I should be able to fix this without too much trouble. I had it working just fine a few commits ago, and the current design is even simpler.

mbauman · 2018-02-23T17:42:53Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2018-02-23T18:14:11Z

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @ararslan

mbauman · 2018-02-23T19:52:11Z

Maybe I'll just try again?

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2018-02-23T21:01:50Z

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @ararslan

mbauman · 2018-02-23T21:03:45Z

Same error — this is very strange.

ERROR: LoadError: MethodError: no method matching sqrt!(::Float64)
Closest candidates are:
  sqrt!(!Matched::AbstractArray) at statistics.jl:263
Stacktrace:
 [1] _std(::Array{Float64,1}, ::Bool, ::Nothing, ::Colon) at ./statistics.jl:298
 [2] #std#703 at ./statistics.jl:287 [inlined]
 [3] std at ./statistics.jl:287 [inlined]
 [4] perf_micro_randmatstat(::Int64) at /home/nanosoldier/.julia/v0.7/BaseBenchmarks/src/micro/methods.jl:109

I don't touch sqrt! here...

KristofferC · 2018-02-23T21:09:40Z

But you do touch broadcasting I guess, which is used in the stackframe above?

mbauman · 2018-02-23T22:45:39Z

Turns out std(rand(10)) was broken on master. Whoops. #26186

alyst · 2018-02-24T10:45:21Z

base/abstractarray.jl

+unalias(dest, A::AbstractRange) = A
+unalias(dest, A) = A
+
+copypreservingtype(A::Array) = copy(A)


This sounds like "copy-preserving type" and too similar to eltype(), keytype() etc.
Maybe something along sametypecopy()/keeptypecopy()/unaliasingcopy() (my fav)?

Fair point. I also like unaliasingcopy for now — it's likely that we'll try to separate the two meanings of copy in the future. By naming this more narrowly it gives us space for that design process.

mbauman · 2018-02-24T16:56:14Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2018-02-24T22:46:34Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

mbauman · 2018-02-26T19:15:07Z

Ok, the other few regressions here are all due to taking views of complicated non-Array types. I want to ensure that modifying the parent array won't change any indices when we construct the view, but this one hurts because — unlike most other unalias calls — this one precedes an O(1) operation. It's slow because we don't (yet) have specialized codegen for objectid. It's fast for Array (and wrappers thereof) because we do have that specialized codegen support for pointer. I'd advocate for merging this as it stands and dealing with the codegen of objectid separately — particularly since I won't be the one best suited to solve that issue.

I pushed a final (I hope) commit here that renames copypreservingtype to unaliascopy and updated all the docstrings.

nalimilan reviewed Feb 5, 2018

View reviewed changes

mbauman added the domain:arrays [a, r, r, a, y, s] label Feb 5, 2018

maleadt mentioned this pull request Feb 5, 2018

Support for loading values through the texture cache (__ldg) JuliaGPU/CUDAnative.jl#158

Merged

mbauman force-pushed the mb/aliaasing branch from bf1f55d to 2301129 Compare February 6, 2018 22:36

JeffBezanson reviewed Feb 8, 2018

View reviewed changes

vtjnash reviewed Feb 8, 2018

View reviewed changes

mbauman force-pushed the mb/aliaasing branch from 99a0697 to 6bbaa50 Compare February 14, 2018 00:35

mbauman added 9 commits February 14, 2018 12:34

Skip aliasing checks in one-argument broadcast!

1e8ac8f

Fixup docstrings from review

901514b

Add support for dealiasing Adjoint and Transpose

534bf91

And fixup the assumption that `broadcast!(f, C, A)` is safe

Refactor API to use map(x->unalias(dest, x), xs) instead of...

0fde137

a builtin mapper method. Using varargs would require de-structuring the one arg case: `(a,) = unalias(dest, a)`.

fixup! missing sparse transition to new API

fa809cb

Add support for BitArray

eeebbe7

Extend broadcast's === aliasing exemption to varargs

2254dc5

Add default StridedArray dataids definition and constrain StridedSubA…

d97d0c3

…rray... to only look at the linear data segment of its parent that it can reach

mbauman force-pushed the mb/aliaasing branch from 6bbaa50 to d97d0c3 Compare February 14, 2018 19:21

Remove hyphen in type-stability

2ba5688

vtjnash reviewed Feb 21, 2018

View reviewed changes

vtjnash approved these changes Feb 21, 2018

View reviewed changes

Avoid comparing dataids in some obvious cases

5caac48

alyst reviewed Feb 24, 2018

View reviewed changes

Rename copypreservingtype to unaliascopy and update docs

1ffc135

mbauman merged commit f38a734 into master Feb 27, 2018

mbauman deleted the mb/aliaasing branch February 27, 2018 19:16

KristofferC mentioned this pull request Jul 11, 2018

Benchmarks vs 0.6 in prep for 0.7 release [do not merge] #27030

Closed

KristofferC mentioned this pull request Jul 19, 2018

AbstractArrays in broadcast falls back to slow objectid when checking mightalias #28178

Open

mbauman mentioned this pull request Oct 16, 2018

Improve aliasing detection of wrappers of SubArrays #29546

Open

pablosanjose mentioned this pull request Jan 14, 2022

Bug in map! for sparse matrices with aliasing JuliaSparse/SparseArrays.jl#26

Open

mkitti mentioned this pull request Apr 13, 2023

assert that input and output buffers passed to transcode! are independent JuliaIO/TranscodingStreams.jl#140

Merged


		This function must return an object of exactly the same type as A for performance and type-stability.

		See also `mightalias` and `dataids`.


		Perform a rudimentary test to check if arrays A and B might share the same memory.

		Defaults to false unless `dataids` is specialized for both `A` and `B`.

More thorough aliasing detection in nonscalar copy, broadcast and assignment #25890

More thorough aliasing detection in nonscalar copy, broadcast and assignment #25890

Conversation

mbauman commented Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbauman commented Feb 5, 2018

nanosoldier commented Feb 5, 2018

mbauman commented Feb 6, 2018

mbauman commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbauman commented Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Feb 9, 2018

mbauman commented Feb 13, 2018

mbauman commented Feb 13, 2018

mbauman commented Feb 14, 2018

nanosoldier commented Feb 14, 2018

mbauman commented Feb 14, 2018

mbauman commented Feb 14, 2018 • edited Loading

StefanKarpinski commented Feb 14, 2018

mbauman commented Feb 21, 2018

Choose a reason for hiding this comment

mbauman Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

vchuravy commented Feb 22, 2018

mbauman commented Feb 22, 2018

nanosoldier commented Feb 22, 2018

StefanKarpinski commented Feb 22, 2018

mbauman commented Feb 22, 2018

mbauman commented Feb 23, 2018

nanosoldier commented Feb 23, 2018

mbauman commented Feb 23, 2018

nanosoldier commented Feb 23, 2018

mbauman commented Feb 23, 2018

KristofferC commented Feb 23, 2018 • edited Loading

mbauman commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbauman commented Feb 24, 2018

nanosoldier commented Feb 24, 2018

mbauman commented Feb 26, 2018

mbauman commented Feb 5, 2018 •

edited

Loading

nalimilan Feb 5, 2018 •

edited

Loading

mbauman commented Feb 8, 2018 •

edited

Loading

mbauman commented Feb 14, 2018 •

edited

Loading

mbauman Feb 21, 2018 •

edited

Loading

KristofferC commented Feb 23, 2018 •

edited

Loading