Update on "[PyTorch] RFC: ExclusivelyOwned"

swolchok · swolchok · commit 770a4feeca14 · 2021-06-14T11:40:11.000-07:00
This introduces ExclusivelyOwned, which allows isolated pieces of code that can make ownership guarantees to opt out of reference counting operations on `intrusive_ptr` and `Tensor` entirely. To elaborate, if you know you are the exclusive owner of an `intrusive_ptr` or `Tensor`, moving it into an `ExclusivelyOwned` will avoid performing atomic reference counting operations at destruction time. The documentation comment should provide sufficient explanation; please request changes if not. Differential Revision: [D28885314](https://our.internmc.facebook.com/intern/diff/D28885314/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D28885314/)! [ghstack-poisoned]
diff --git a/aten/src/ATen/templates/TensorBody.h b/aten/src/ATen/templates/TensorBody.h
@@ -1029,8 +1029,6 @@ struct ExclusivelyOwnedTraits<at::Tensor> {
   using const_pointer_type = const at::Tensor*;
 
   static repr_type nullRepr() {
-    // REVIEW: do we need to be able to distinguish an empty
-    // ExclusivelyOwned<Tensor> from an undefined Tensor?
     return at::Tensor();
   }
 
diff --git a/c10/util/ExclusivelyOwned.h b/c10/util/ExclusivelyOwned.h
@@ -4,14 +4,37 @@
 
 namespace c10 {
 
-// TODO: document ExclusivelyOwnedTraits and settle on names (and
-// signatures?) of operations. In the meantime, see examples in
-// TensorBody.h and intrusive_ptr.h.
-// REVIEW: would anyone like to argue that it doesn't make sense to
-// have this traits class, and instead we should just have two
-// explicit specializations of ExclusivelyOwned? There would be some
-// small amount of code duplication, but perhaps it would be easier to
-// understand?
+// See example implementations in TensorBody.h and intrusive_ptr.h.
+// Synopsis:
+//
+// repr_type -- type to use to store an owned T in ExclusivelyOwned.
+//
+// pointer_type -- pointer-esque type to return from
+// ExclusivelyOwned's get() and operator*() methods.
+//
+// const_pointer_type -- similar to pointer_type, used for the const methods.
+//
+// static repr_type nullRepr() -- return a null instance of repr_type.
+//
+// template <class... Args>
+// static repr_type createInPlace(Args&&... args) -- used by the in-place
+// ExclusivelyOwned constructor.
+//
+// static repr_type moveToRepr(T&& x) -- move the given x into an
+// instance of repr_type. used by the ExclusivelyOwned(T&&)
+// constructor.
+//
+// static void destroyOwned(repr_type x) -- free memory for a
+// known-exclusively-owned instance of x. Replaces calling repr_type's
+// destructor. Being able to implement this more efficiently than
+// repr_type's destructor is the main reason to use ExclusivelyOwned
+// for a type.
+//
+// static T take(repr_type&) -- move out of the given repr_type into an owned T.
+//
+// static pointer_type getImpl(const repr_type&) -- return a pointer
+// to the given repr_type. May take repr_type by value if that is more
+// efficient.
 template <typename T>
 struct ExclusivelyOwnedTraits;
 
@@ -54,6 +77,8 @@ class ExclusivelyOwned {
     rhs.repr_ = EOT::nullRepr();
   }
 
+  ExclusivelyOwned& operator=(const ExclusivelyOwned&) = delete;
+
   ExclusivelyOwned& operator=(ExclusivelyOwned&& rhs) noexcept {
     EOT::destroyOwned(repr_);
     repr_ = std::move(rhs.repr_);
