Refactor storage types for views and mutable views #148
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously `&[T]` and `&mut [T]` were used as the storage types for tensor views and mutable tensor views respectively. When logically non-overlapping views are created of non-contigous tensors, their underlying storages (ie. the memory ranges) may however overlap. Though it has not caused a practical problem yet, it violates Rust's ownership rules. This refactor replaces the storage types for views with custom types which have the same representation as slices, but do not have the same ownership rules. The goal is to prevent future mistakes caused by these ownership rule violations, and also enable rten to be checked by Miri without disabling the stacked borrows check.
Since mutable views no longer use mutable slices as their backing storage, Miri doesn't complain if the storage ranges overlap. Note that the library still needs to ensure that the logically accessible elements within each view do not overlap.
Update following the changes to tensor storage.
robertknight
force-pushed
the
storage-trait
branch
2 times, most recently
from
62d2b4f
to
65bf5c4
Compare
The element type is part of the storage type, accessible via `Storage::Elem`, so `TensorBase` doesn't need its own generic parameter for this.
Since `Storage` bypasses the Rust restriction on mutable slices not overlapping other slices, the caller has to ensure this instead. Also make these methods default methods in `Storage`, since the logic is the same for each impl.
Replace the existing `TensorBase::{non_contiguous_data, data_mut_ptr}` methods
with `storage` and `storage_mut`, for low-level code that needs to get at the
underlying data pointer. `non_contiguous_data` was unsound in the case where the
view was not contiguous and overlapping mutable views existed.
robertknight
force-pushed
the
storage-trait
branch
from
65bf5c4
to
e5ebdf9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously tensor views used slices as their storage type (the
TensorBase::datafield).When tensor operations create logically separate mutable views whose underlying storage ranges overlap (for example, a mutable iterator over the column axis of a matrix), this lead to violations of Rust's ownership rules. Miri's stacked borrows check complained about this.
This PR refactors tensor storage to address this:
TensorViewandTensorViewMutnow use custom types (ViewData,ViewMutData) instead of slices for their storage. These types have the same representation, but don't promise there is no overlap between a mutable storage and other storage instances. Instead the caller has to ensure that.TensorBasehas changed fromTensorBase<T, S: AsRef<[T]>, L: MutLayout>toTensorBase<S: Storage, L: MutLayout>whereStorageis a trait that is implemented byVec<T>,Cow<[T]>,ViewDataandViewMutData. TheStoragetrait provides access to the underlying raw data. It has methods for getting a reference to the element at a given offset, but unlikeAsRefthese are unsafe, since the caller has to ensure that no mutable references can exist. The type of element in aTensorBaseis now accessed viaS::ElemTensorBase::{non_contiguous_data, data_mut_ptr}have been replaced withTensorBase::{storage, storage_mut}for low-level code that needs to get at the underlying data pointer.non_contiguous_datawas unsound due to the same issues as storage for mutable views.make mirihas been updated to re-enable the stacked borrows check.