-
Notifications
You must be signed in to change notification settings - Fork 355
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
RFC: Tensor mutability of TensorOps #145
Comments
Thanks for the proposal. I'd like to highlight the pros and cons of having mutable operations. Pros:
Cons:
I have two potential solutions in mind:
Maybe both solutions could be combined in a way that simplifies the decorator backend's analysis of graphs, using explicit clone calls to provide lifetime information. |
I thought about the problem and came up with en even better potential solution!
pub trait TensorOps<B> {
...
fn share<const D: usize>(tensor: &mut B::TensorPrimitive<D> ) -> B::TensorPrimitive<D>;
} Backends can implement this with a simple clone if they want, but they can also change the datastore to a shared one. struct MyTensorPrimitive {
...
storage: MyTensorStorage,
}
enum MyTensorStorage {
Owned(Storage),
Shared(Arc<Storage>),
} The I don't see any drawback to this solution. It does not increase the size of the API in the |
+1 on improving inference performance. |
I came across |
@nathanielsimard You worked on this. Is this ticket complete? |
Yes it's completed. |
with current design of TensorOps, TensorPrivimitive is once initialized, never mutated,
not allowing Buffer of passed TensorPrimitive.
(well, while it's possible with interior mutability that wouldn't be an expected behavior)
some operations may be able to just use same buffer(ex. reshape)
or write result into same buffer(ex. sqrt, relu)
this behavior will prevent allocating new buffer for result, possibly cutting memory usage in half,
but makes user to clone the Tensor before passing if planning to use original value of passed Tensor.
i'm suggesting to support both current behavior and new "append" behavior
by adding both functions to TensorOps trait and making current behavior implementation optional(using trait default behavior)
i'm a newbie so correct me 😓
The text was updated successfully, but these errors were encountered: