Skip to content

Are non-contiguous owned arrays safe? #443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ExpHP opened this issue Apr 28, 2018 · 6 comments
Closed

Are non-contiguous owned arrays safe? #443

ExpHP opened this issue Apr 28, 2018 · 6 comments

Comments

@ExpHP
Copy link
Contributor

ExpHP commented Apr 28, 2018

  • It is currently possible to create discontiguous, owned Arrays with the use of slice_inplace. This seems to me like an incredibly niche use case that might not have been anticipated in other code.
  • I don't see any tests that do this. (though perhaps there are tests that create discontiguous Arrays in other ways)

Basically, I'm just asking: Has this use case been considered, or is this "feature" of slice_inplace accidental?

My, oh, my, the sky is falling! I must run and tell the lion about it - says Chicken Little and begins to run.

@jturner314
Copy link
Member

Yes, this use-case has been considered, and this capability is useful in some cases (see e.g. #425). In addition to .slice_inplace(), other methods for creating discontiguous owned arrays include .slice_move(), .slice_axis_inplace(), .subview_inplace(), .into_subview(), .remove_axis(), and ::from_shape_vec().

Is this safe? Yes. You might be concerned that elements outside the visible area of the array could be uninitialized and get dropped. However, owned arrays store their data in a Vec, and all elements in the Vec (even those outside the visible area of the array) must be initialized. (OwnedRepr<A> is a wrapper for Vec<A>, and OwnedArcRepr<A> is a wrapper for Arc<Vec<A>>.) When an owned array is dropped, the Vec is dropped (unless in the case of an ArcArray the data is shared with another ArcArray instance), and the Vec takes care of dropping its elements. The shape/strides of the array have no influence on the drop behavior. (The only exception to the "no uninitialized elements" rule is an array created with the unsafe .uninitialized() method, which requires the element type to be Copy to prohibit destructors.) Is there something else you're concerned about regarding discontiguous owned arrays?

There are some tests involving discontiguous views, but I don't see any involving discontiguous owned arrays. Would you like to add some? :) I'd be happy for a PR adding more tests.

@ExpHP
Copy link
Contributor Author

ExpHP commented Apr 28, 2018

Is there something else you're concerned about regarding discontiguous owned arrays?

Nope, nothing in particular. When I discovered this, one of the first things I did was search for Drop impls (and was relieved not to find any), but I still wanted to ask just to be certain. The breadth of other methods that can do this is reassuring.

This is a neat property of the design of ndarray in how strides/shape are handled completely orthogonally to data ownership. Earlier this week before deciding to adopt ndarray as a dependency in my project, I was working on a similar type, but mine had a separate struct for owned arrays simply to ensure contiguousness (and to save a few usizes), and I was perhaps stuck thinking in that mindset.

There are some tests involving discontiguous views, but I don't see any involving discontiguous owned arrays. Would you like to add some? :)

Sure! I'll start putting something together.

@jturner314
Copy link
Member

For what it's worth, owned arrays in ndarray are currently designed around Vec, but that's not strictly necessary. We could switch to allocating our own memory so that elements outside of the visible area wouldn't need to be initialized. This would make the shape/strides affect ownership. Personally, though, I like relying on Vec because it simplifies reasoning about safety.

@ExpHP
Copy link
Contributor Author

ExpHP commented Apr 28, 2018

I guess that support for this feature also explains why there's no IntoIterator. (even on contiguous data, writing a Drop-safe/panic-safe into_iter is already no picnic... but discontiguous data makes it even worse by introducing elements that will never be iterated over)

@bluss
Copy link
Member

bluss commented Apr 28, 2018

ndarray itself should handle this correctly, since it's a feature since the start of the library.

@ExpHP
Copy link
Contributor Author

ExpHP commented Apr 30, 2018

I won't wait for #444 to close this, as it is clear that the this has been an integral part of the library's design from day one.

@ExpHP ExpHP closed this as completed Apr 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants