Improve aliasing detection of wrappers of SubArrays

[tkf]

It fixes #29545, i.e., it makes the following snippet work

buffer = zeros(UInt8, 4 * sizeof(Int))
mid = length(buffer) ÷ 2
x1 = reinterpret(Int, @view buffer[1:mid])
x2 = reinterpret(Int, @view buffer[mid+1:end])
x1 .= x2

However, this PR still has some problems. For example, it does not support mightalias(::ReinterpretArray, ::SubArray). But defining mightalias(::ReinterpretArray, ::AbstractArray) and mightalias(::AbstractArray, ::ReinterpretArray) would cause the ambiguity problem (and not DRY).

A general question is: How one can ensure that mightalias is symmetric (i.e., mightalias(a, b) == mightalias(b, a)) and easily overloadable? One possibility is to define a function (say)

symmetric_mightalias(a, b) = mightalias(a, b) && mightalias(b, a)

and document that the first argument of mightalias has to specify the type user defined (to reduce ambiguity).

Note that symmetric_mightalias has to be exposed as a public (non-overloadable) API so that it can be called from user's mightalias:

mightalias(a::ReinterpretArray, b::AbstractArray) = symmetric_mightalias(parent(a), b)

Is it a sane approach? I'm pretty sure this is a generic problem solved in Julia community ages ago.

[tkf]

Another problem is that defining this for all "child array types" such as ReshapedArray is not DRY. It would be nice if there is something like AbstractViewArray (say) which is inherited by SubArray, ReshapedArray, ReinterpretArray, etc. so that one can just define

mightalias(a::AbstractViewArray, b::AbstractArray) = symmetric_mightalias(parent(a), b)

[martinholters]

Isn't it dataids that should delegate to the parent here? That would automatically give symmetry.

[tkf]

Yes. That's why I said "symmetric and easily overloadable." There is a special aliasing analysis in mightalias(::SubArray, ::SubArray). Relying on dataids makes it hard to call mightalias on .parent and enforce symmetry, I think.

[StefanKarpinski]

@mbauman is on vacation at the moment so it might take a while before he can take a look at this. He’s not the only one who could review, of course, but he is the primary suspect :)

[tkf]

Thanks! Good to be informed.

[martinholters]

This certainly isn't wrong, but as noted, it doesn't scale. Not sure how many of those specialized mightaliases we want to add, or whether there is a viable better option.

[tkf]

it doesn't scale

How about defining AbstractViewArray as I commented above #29546 (comment)? You don't need to add method definition for each type this way. Also, I don't think my approach would reduce scalability since dataids-based analysis would still work. It just adds another point that users can hook into.

[mbauman]

Right, the whole key to the mightalias/dataids system is that it allows us to generically define symmetric relationships between chunks of memory without getting into a maze of ambiguities and asymmetries. It's intentionally a rudimentary and non-exact heuristic, geared to preserve performance in the by-far-most-common case where two arrays definitively do not alias while simultaneously being tractable to extend.

SubArrays are indeed special. They're really the only array view we have that subsets into memory blocks. That's why it's the only builtin specialization for mightalias. Note, too, that tracking subsetted memory locations through different views is exceedingly difficult and likely to be too expensive to be worthwhile in all but the most trivial of cases. Defining mightalias(::StridedArray, ::StridedArray) could make sense (it was the first thing I tried) but it's still tricky in cases like SubArray where the indices themselves can alias.

The proper fix for #29545 is for us to define unaliascopy as the error message suggests. Yes, that will pessimistically create some copies in cases where it needn't, but there are often workarounds if you find it's a problem via profiling.

[tkf]

Note, too, that tracking subsetted memory locations through different views is exceedingly difficult and likely to be too expensive to be worthwhile in all but the most trivial of cases.

I don't think what I'm suggesting would introduce more complex analysis. It's all about delegating to existing dedicated analysis and making use of it more.

I suppose that until the buffer type #12447 is introduced, SubArray would be the only non-unsafe way to create a custom memory pool? I think reinterpreted view is a good substitute until then.

Aside: Reading #29481, I started thinking that Julia may not have a standard way to define extensible symmetric/commutative interface. My idea above is somewhat based on Python's NotImplemented and __add__/__radd__ method pairs and I think it can work in other cases like #29481. While it's not possible to fix #29481 in 1.x time-frame, I think it would be nice to establish a standard strategy for achieving symmetry/commutativity before 2.0 and start using it whenever possible. Perhaps even turn it into a trait-based system. If what I presented here was one of the application of the standard pattern, I think the apparent complexity could be considered "amortized."

[mbauman]

I must say that I am very excited that you're excited and hoping to improve the aliasing detection system! This is a new portion of Julia that not many folks have touched and it's great to get a fresh set of eyes on it. You'll probably find the PR where this was introduced interesting — note that I started trying for a much more complicated and exacting design but iteratively simplified to the most basic thing that could work.
#25890 (comment)

The part that becomes intractable is SubArrays of other views.

There are generally two idioms to create a symmetric interface: one is indeed trying it in both directions (see, e.g., promotion). The other way is just to create an (::Any, ::Any) fallback definition that doesn't require specialization — this often just asks each argument for the essential property and then works directly with that… allowing folks to specialize on the essential property.

My thought here has been that mightalias/dataids can serve as our first line of attack: rough, fast, easy to understand, and pessimistic. I intentionally punted on views of SubArrays, favoring a simpler interface and implementation over higher precision. Now it may be possible to layer on a delegation system for mightalias for array wrappers that only make use of one array and use it entirely. One way I might go about tackling that is by introducing a new function like aliasingroot that guarantees those properties and just have mightalias(::Any, ::Any) call that on its arguments.

[tkf]

Thanks a lot for detailed reply. Yes, I should have looked into the pre-existing discussion for why the current implementation is chosen. I'll look into #25890 to catch up.

aliasingroot sounds interesting but how do you get rid of infinite recursion? I suppose you meant something like

mightalias(a::Array, b::Array) = # dataids-based analysis
mightalias(a::AbstractArray, b::AbstractArray) =
    mightalias(aliasingroot(a), aliasingroot(b))  # possible recursion

function aliasingroot(a::AbstractArray)  # maybe use @generated to check .parent field?
    if parent(a) === a
        return a
    else
        return aliasingroot(parent(a))
    end
end

aliasingroot(a::SubArray) = a

But if you define a new "root" array type MyArray (i.e., without .parent) then you need to make sure mightalias(a::MyArray, b::AbstractArray) and mightalias(a::AbstractArray, b::MyArray) are defined to avoid infinite recursion.

One "solution" I can think of is to decouple "calling API" and "overloading API", like:

# Calling API:
mightalias(a::AbstractArray, b::AbstractArray) = __mightalias__(aliasingroot(a), aliasingroot(b))

# Overloading API:
__mightalias__(::AbstractArray, ::AbstractArray) = # dataids-based analysis
__mightalias__(a::SubArray, b::SubArray) = ...

With this, I still need to define Base.__mightalias__(a::MyArray, b::AbstractArray) and Base.__mightalias__(a::AbstractArray, b::MyArray) if I want to make it more efficient but there is no infinite recursion by default.

[mbauman]

Unfortunately parent is not helpful for generic code like that. I know it's so very tempting to use, but it's not defined thoroughly enough to be useful. It's also not type-stable when you recurse like that. I've burned many hours trying to do useful things with parent.

I'd just have a fallback aliasingroot(x) = x definition and then specific subtypes would do something like aliasingroot(x::T) = aliasingroot(x.data). And then, yes, this really only punts the problem as we need an internal __mightalias__ or some such that does the real work. The key, though, is that the number of arrays that need to specialize that is extremely limited (likely only SubArrays) and so it does not need to be public or officially documented.

[martinholters]

Should that be aliasingroots(x) = (x,) with __mightalias__ then being invoked on all pairs of those roots? That could support wrappers with multiple data fields.

[tkf]

Unfortunately parent is not helpful for generic code like that.

@mbauman Right, I saw you mentioning it in the PR you linked. I wrote "maybe use @generated" to indicate that the function definition is for prototyping though I could be more explicit.

specific subtypes would do something like aliasingroot(x::T) = aliasingroot(x.data)

Initially, I thought it would be nice to have a default implementation like

@generated function aliasingroot(a::AbstractArray)
    if :parent in fieldnames(a)
        return :(aliasingroot(a.parent))
    else
        return :(a)
    end
end

so that it works automatically for all "child array types" but maybe manually defining aliasingroot would be safer.

it does not need to be public or officially documented.

I was trapped in thinking that mightalias itself has to be extensible but actually all I need here can be achieved by aliasingroot. Keeping overloadable API minimal sounds like a good plan always.

aliasingroots(x) = (x,)

@martinholters Yes, I think this is the right direction, too. (And I guess that's why too much magic in default aliasingroot is discouraged.)

[tkf]

I implemented aliasingroots. Could you review and merge if appropriate?

[tkf]

Would it be possible for this to be in 1.1?

[StefanKarpinski]

If it gets merged this week, yes. What's blocking progress here?

[mbauman]

Well, let's make sure Nanosoldier is still happy:

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

This is definitely an improvement for wrappers of SubArrays. I'm a little hesitant, though, in that it mixes in a layer of complexity to this interface — now you must think about both aliasingroots and dataids… and it's not clear what the difference is between the two. What happens if your wrapper specialize one but not the other? Right now they both describe the roughly the same information but in a slightly different way… and don't know about one-another. I would love it if we could make the two systems to be either more synchronized or more orthogonal.

For example, it'd be great if aliasingroots could be the "first line" API, with dataids built atop it. That is, I'd want something like dataids(A::AbstractArray) = map(dataid, aliasingroots(A)). Unfortunately, that's a recursive nightmare, because we don't want a singular dataid — each aliasingroot could have also multiple ids. And then I don't want yet another level of indirection.

So that's where I am here — I like it but I'm hesitant.

[mbauman]

I'd even be open to scrapping dataids in favor of aliasingroots, but I intentionally designed dataids to avoid O(N^2) specializations in such a core algorithm. With all the inlining and such, is this even a concern for the compiler team? Should that continue to be a design goal?

[ararslan]

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

[tkf]

I'm a little hesitant, though, in that it mixes in a layer of complexity to this interface — now you must think about both aliasingroots and dataids… and it's not clear what the difference is between the two. What happens if your wrapper specialize one but not the other? Right now they both describe the roughly the same information but in a slightly different way… and don't know about one-another.

I agree that it's more complex than simple dataids. I think library implementers should almost always use aliasingroots (which was the intention when I was writing the docstrings). In theory, dataid can be useful when defining new "root array types"; e.g., when wrapping external C API. But I'm not sure it is worth supporting this scenario.

scrapping dataids in favor of aliasingroots

It looks like dataids has been a public overloading API. For example, it's used in JuliaArrays. https://github.com/search?q=org%3AJuliaArrays+dataids&type=Code. I guess we can't remove it until Julia 2.0?

designed dataids to avoid O(N^2) specializations in such a core algorithm

What is N here? If N is the number of concrete array types, old dataids-based approach was already O(N^2), right? It already had the method signature mightalias(A::AbstractArray, B::AbstractArray).

If N is the number of types passed to each argument of _anymightalias (where I used it via mightalias(A, B) = _anymightalias(aliasingroots(A), aliasingroots(B))) then N is 1 most of the time and almost always very small (though it can be arbitrary large for some cases like LazyArrays.Vcat; but I'd say the user wants to pay specialization cost in such case for the performance).

[nanosoldier]

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

[mbauman]

Yeah, sorry, my memory on the specializations was fuzzy. I was thinking about combinations of types, but it's not the specializations that's the hard part (those all get inlined on-demand in any case) — it was the number of implementations that you have to write. That's what going to an integer id wins you.

As far as dataids being an overloadable API: yes, but if we come up with something significantly better I think we can transition to it without too much consternation. It'd be a "minor change," and we can probably do it gracefully. The number of packages that do this are fairly limited. Let's not let that get in the way of getting the right design — at least not at first.

Perhaps this is indeed the right design (or a good enough one), but I keep coming back to the fact that this addition is entirely dedicated to improving the detection of aliasing between views of two SubArrays with the same parent. It sure feels like we should be able to come up with something simpler — or at least keep the weight of the code out of the hot path in the far more likely situation where that's not the case. But I'm not seeing an obvious alternative, and it is indeed an important special case. (Edit: I suppose this may indeed be a zero-cost abstraction at runtime; Nanosoldier likes it).

[tkf]

— it was the number of implementations that you have to write. That's what going to an integer id wins you.

As far as the number of the interfaces you have to overload, isn't it the same? You have to write aliasingroots(A::MyArray) = (A.parent,) or dataids(A::MyArray) = (dataids(A.parent),).

entirely dedicated to improving the detection of aliasing between views of two SubArrays with the same parent

I wonder external packages like https://github.com/Jutho/Strided.jl needs to use it.

at least keep the weight of the code out of the hot path in the far more likely situation where that's not the case

What would be the weight in my current implementation? Maybe compilation of this recursion?

_anymightalias(as::Tuple, bs::Tuple) =
    any(b -> _mightalias(as[1], b), bs) || _anymightalias(tail(as), bs)

If so, does defining manual specialization _anymightalias(as::Tuple{TA}, bs::Tuple{TB}) help?

[tkf]

@Jutho I see you are building an advanced package (Strided.jl) on top of broadcasting facility. It looks like it is close SubArray so maybe you are interested in aliasing analysis API too?

[Jutho]

I have to say that I found the current (at the time of creating Strided.jl) aliasing detection scheme somewhat strange//unintuitive/hard to understand, or to use for my purpose, so I kinda gave up on it. Probably that was just me not wanting to invest sufficient effort.

So yes, I defined dataids for the views in Strided.jl, but then never check for aliasing in e.g. my broadcasting code. So it's up to the users not to do unsafe things.

[Jutho]

For example, I think I got confused about the point of just returning pointer(A) as the dataids of an Array. Wouldn't you at least want like a range/window in memory where the data of A lives, even if it does not use all the elements in that memory range. That way, you could at least correctly detect e.g.
A = randn(8); Base.mightalias(reshape(view(A,1:4),(2,2)),reshape(view(A,5:8),(2,2)))

I think this is a not totally uncommon use case. Start with a large vector, define new arrays by taking just a part of that vector and reshaping it into a specific multidimensional array. E.g. the vector could be the different blocks of an array with block-sparsity stacked underneath each other. You still want to detect that the different blocks don't alias.

So a slightly more advanced dataids, say, some tuple of StepRange{UInt}s that represent memory locations that might be used by the array, might simplify the mightalias routine and make it much more generally valid than for SubArrays.

[tkf]

Thanks, it's nice that you commented the painful points so that this PR is more convincing to core devs :)

correctly detect e.g.
A = randn(8); Base.mightalias(reshape(view(A,1:4),(2,2)),reshape(view(A,5:8),(2,2)))

That's exactly what this PR would solve, by internally calling mightalias(::SubArray, ::SubArray) from it.

So a slightly more advanced dataids, say, some tuple of StepRange{UInt}s that represent memory locations that might be used by the array

Isn't "represent memory locations" equivalent to what SubArray does? I think you can construct and return a SubArray from aliasingroots(::StridedView) (even though SubArray is not really used elsewhere). In other words, I think SubArray is already a good representation for specifying sub-region of the memory you are using in an Array. (I'm assuming that temporary created SubArray can be completely compiled away. I guess it's possible, e.g., with more @inline?)

But I think this shows that aliasingroots may not be the best name, as returned arrays may not be used elsewhere. A better name could be memoryregions or something?

[mbauman]

@Jutho — yes, thank you for the report. You're exactly right: the existing API explicitly punts on those cases and that's precisely what this change is aiming to address. It punts in favor of simplicity.

My fear, though, is that if the existing system was already strange/unintuitive/hard, then I don't think this change is going to make things better. Perhaps some of that is indeed in the name of aliasingroots. For example, it's not obvious why SubArray wouldn't implement aliasingroots to return the aliasingroots of its parent and indices. Calling it memoryregions would definitely help here — and the insight that we should use SubArrays to represent strided (and other) regions is perfect. The only difficulty is that we don't always have a first-class parent object — it may just be a pointer.

I also wonder if the fact that we currently have a mightalias(::SubArray, ::SubArray) is throwing us down the wrong path here. For example, what about a default mightalias definition that looks like this:

mightalias(A::AbstractArray, B::AbstractArray) = !isbits(A) && !isbits(B) && !_isdisjoint(dataids(A), dataids(B)) && anyoverlaps(memoryregions(A), memoryregions(B))

The advantage is that we only do the more complicated memory overlap checking if we've already proven that there are two components that match. We could completely outline that last branch, passing just A and B, which allows us to then inline everything subsequently, keeping the code weight off of the hot path and out of the cache in the much more common case where there aren't any matching dataids. The only thing I don't like is that a custom array would still need to define both dataids and memoryregions… but it may be that we'd be able to get the memory region checking to be just as fast and could get rid of dataids later. That's a challenge, though.

[Jutho]

The only difficulty is that we don't always have a first-class parent object — it may just be a pointer.

Exactly, currently in UnsafeStridedView (which is modeled after the UnsafeArrays.jl package), I keep both a pointer to the parent and an offset. However, I could get rid of the offset and absorb it into the pointer, by just letting it point towards the first memory location. But then, with a dataids which is just a pointer, your !_isdisjoint(dataids(A), dataids(B)) would fail, i.e. A and B could still alias even if they have different pointers.

[mbauman]

Well, it's unsafe and you need to be very careful to GC.@preserve the actual parent array separate from the object. I understand why you don't want to carry around the first-class parent in the struct, but I hope you can appreciate that this isn't going to be our first priority to accommodate in an aliasing API.

[Jutho]

but I hope you can appreciate that this isn't going to be our first priority to accommodate in an aliasing API.

Sure, I also keep the offset now even though I didn't quite care about aliasing (I should put this in the readme somewhere), so I don't mind keeping it around.

I know you need to GC.@preserve the actual array, that's why that type is only to be used within a @unsafe_strided macro that adds the necessary GC.@preserves.

[mbauman]

Oh, I see I misread your comment. Sorry — we're in agreement here.

My point is that if you have a bare pointer — while you may be able to compute its original dataid — you won't be able to construct the SubArray that represents its particular region into that original memory chunk to do the finer-grained memory overlap detection we're spitballing.

Perhaps @oschultz would be interested in this discussion, too — he implemented mightalias in his UnsafeViews.jl package.

[tkf]

My fear, though, is that if the existing system was already strange/unintuitive/hard, then I don't think this change is going to make things better.

My impression was that it is too simplistic and limited rather than strange/unintuitive/hard. This is just because my first exposure to the system is via the error which led me to this PR. I think current simplicity has been a great choice for the first stages in the development as it captures most useful cases. But I don't think what you are suggesting here is much more complex for API consumers (I'd say it's even simpler, since most of the time you only need to return (myarray.parent,) instead of manually applying dataids to .parent).

We could completely outline that last branch, passing just A and B, which allows us to then inline everything subsequently, keeping the code weight off of the hot path and out of the cache in the much more common case where there aren't any matching dataids.

This sounds like a good idea. So something like

mightalias(A::AbstractArray, B::AbstractArray) =
    !isbits(A) && !isbits(B) && !_isdisjoint(dataids(A), dataids(B)) &&
    anyoverlaps(A, B)

@inline dataids(A::Array) = ...
@inline dataids(A::SubArray) = ...
@inline dataids(A) = tuplejoin(map(dataids, memoryregions(A))...)
# tuplejoin from https://discourse.julialang.org/t/efficient-tuple-concatenation/5398

@noinline anyoverlaps(A, B) = _anyoverlaps(memoryregions(A), memoryregions(B))

@inline _anyoverlaps(::Tuple{}, ::Tuple) = false
@inline _anyoverlaps(as::Tuple, bs::Tuple) =
    any(b -> __anyoverlaps(as[1], b), bs) || _anyoverlaps(tail(as), bs)

@inline __anyoverlaps(::Any, ::Any) = true  # @pure?

@inline __anyoverlaps(A::SubArray, B::SubArray) =
    ...

One small uneasy point is that this runs memoryregions twice (first via dataids and second via anyoverlaps). Maybe this is totally fine since we do more expensive analysis or about to run unaliascopy. For some frequently used arrays, we can even define inlineable shortcuts like @inline anyoverlaps(::Array, ::Array) = true.

The only thing I don't like is that a custom array would still need to define both dataids and memoryregions…

We can define dataids in terms of memoryregions, right? (see code above)

[mbauman]

Yes, that's exactly the kind of design I'd like to find. Let's call your __anyoverlaps just overlaps — it's the kernel that checks two memory locations for an overlap and doesn't care about any anymore. There are a few challenges:

• Unfortunately, we cannot have overlaps(::Any, ::Any) = true — as it is combined with an "any" clause we don't know which of the dataids matched in the preceding clause and this will compare all the arrays again — including ones that don't share dataids. I think that a default implementation of overlaps(A, B) = A === B would work, though.
• Defining a default dataids in terms of memoryregions only works if it gets back to a root node that has itself defined dataids. By default (without any specializations) it'll be a stack overflow error. (There's also Base._splatmap which can be helpful for these kinds of operations, though.)

At the same time, I think walking through memoryregions twice shouldn't be a problem — I'd guess it'll be free at runtime.

[tkf]

• Unfortunately, we cannot have overlaps(::Any, ::Any) = true — as it is combined with an "any" clause we don't know which of the dataids matched in the preceding clause and this will compare all the arrays again — including ones that don't share dataids. I think that a default implementation of overlaps(A, B) = A === B would work, though.

Oops. Good catch! But I think A === B produces false negative with the case like A = [1, 2, 3]; mightalias(A, view(A, 1:2))? One solution would be to define

memoryregions(A::Array) = (view(A, :),)

so that we hit overlaps(::SubArray, ::SubArray) almost always.

• Defining a default dataids in terms of memoryregions only works if it gets back to a root node that has itself defined dataids. By default (without any specializations) it'll be a stack overflow error.

I see. My suggestion above requires implementers to define at least one of dataids and memoryregions while in the current implementation there is no need for that (but still you can opt-in performance gain and the safety against aliasing).

Maybe requiring both dataids and mightalias is not so bad as both of them are rather short in most cases? Alternatively, I guess we can do

dataids(A::Array) = (UInt(pointer(A)),)
dataids(::Any) = nothing

_dataids(A) = _splatmap(x -> something(dataids(x), default_dataids(x)),
                        memoryregions(A))
default_dataids(A::AbstractArray) = (UInt(objectid(A)),)
default_dataids(::Any) = ()

mightalias(A::AbstractArray, B::AbstractArray) =
    !isbits(A) && !isbits(B) && !_isdisjoint(_dataids(A), _dataids(B)) &&
    anyoverlaps(A, B)

but this probably defeats the purpose of this hybrid approach for reducing compiler's work in the hot branch.

There's also Base._splatmap which can be helpful for these kinds of operations, though.

Thanks for the tips!

At the same time, I think walking through memoryregions twice shouldn't be a problem — I'd guess it'll be free at runtime.

Can that happen with @noinline anyoverlaps? I thought that would require inlining. (Still may be negligible, though.)

[dgleich]

Just a quick note that I ran into this issue when I was debugging allocations in my code. My case was equivalent to:

function mycase(x)
  @assert(length(x) > 2)
  y = @view(x[1:end-1])
  z = @view(x[end:end])
  Z = reshape(z, 1, 1)
  @show Base.mightalias(@view(y[1:1]), @view(z[1:1]))
  @show Base.mightalias(@view(y[2:2]), @view(Z[1:1,1]))
  copyto!(@view(y[1:1]), @view(z[1:1]))
  copyto!(@view(y[2:2]), @view(Z[1:1,1]))
  nbytes1 = @allocated copyto!(@view(y[1:1]), @view(z[1:1]))
  nbytes2 = @allocated copyto!(@view(y[2:2]), @view(Z[1:1,1])) # allocates a copy... 
  @show nbytes1, nbytes2
end 
mycase(randn(10)) # expected nbytes1 == nbytes2 == 0 

The expected output was not to have a copy due to the alias detection, but the output shows:

Base.mightalias(#= Untitled-2:6 =# @view(y[1:1]), #= Untitled-2:6 =# @view(z[1:1])) = false
Base.mightalias(#= Untitled-2:7 =# @view(y[2:2]), #= Untitled-2:7 =# @view(Z[1:1, 1])) = true
(nbytes1, nbytes2) = (0, 64)
(0, 64)

The issue is that the alias detection is too conservative in this case as Z has been reshaped from the view.

I fixed it for my code by just writing a version of copyto! that skips alias detection.