Remove caffe2::Tensor copy constructor (#15416)

Summary:
Based on offline discussion it should be less surprising to the users of existing code. Thus caffe2::Tensor is now a move-only class (as it used to be), explicit calls to UnsafeSharedInstance() are necessary to get shared_ptr behavior.

This change also identified a few places that misused the copy constructor - those are fixed

Pull Request resolved: https://github.com/pytorch/pytorch/pull/15416

Reviewed By: Yangqing

Differential Revision: D13524598

fbshipit-source-id: aea12d6dff77342606fa88ce4ddddbff266245a7

diff --git a/caffe2/core/blob.h b/caffe2/core/blob.h
index 02851fd..0310b73 100644 (file)
--- a/caffe2/core/blob.h
+++ b/caffe2/core/blob.h
@@ -24,15 +24,15 @@ inline bool BlobIsTensorType(const Blob& blob, DeviceType device_type) {
   return tensor && *tensor && tensor->GetDeviceType() == device_type;
 }
 
-inline Tensor* BlobSetTensor(Blob* blob, const Tensor& tensor) {
-  return blob->Reset<Tensor>(new Tensor(tensor));
+inline Tensor* BlobSetTensor(Blob* blob, Tensor&& tensor) {
+  return blob->Reset<Tensor>(new Tensor(std::move(tensor)));
 }
 
 inline Tensor GetSizedTensorWithOptions(
-    const Tensor& t,
+    Tensor&& previous_tensor,
     at::IntList dims,
     at::TensorOptions options) {
-  Tensor tensor = t;
+  Tensor tensor = std::move(previous_tensor);
   if (tensor.GetDevice() == options.device() ||
       (!tensor.GetDevice().has_index() &&
        tensor.GetDeviceType() == options.device().type())) {
@@ -84,7 +84,7 @@ BlobGetMutableTensor(Blob* blob, at::IntList dims, at::TensorOptions options) {
 
 inline Tensor
 XBlobGetMutableTensor(Blob* blob, at::IntList dims, at::TensorOptions options) {
-  return *BlobGetMutableTensor(blob, dims, options);
+  return BlobGetMutableTensor(blob, dims, options)->UnsafeSharedInstance();
 }
 
 inline Tensor* BlobGetMutableTensor(Blob* blob, DeviceType device_type) {

diff --git a/caffe2/core/operator.h b/caffe2/core/operator.h
index 5e1a9ab..ca781a5 100644 (file)
--- a/caffe2/core/operator.h
+++ b/caffe2/core/operator.h
@@ -193,8 +193,9 @@ class CAFFE2_API OperatorBase : public Observable<OperatorBase> {
     CAFFE_ENFORCE(
         ival->isTensor(),
         "Output(int, DeviceType) is only available for IValues that store Tensors");
-    Tensor tensor = caffe2::Tensor(ival->toTensor());
-    tensor = GetSizedTensorWithOptions(tensor, dims, options);
+    Tensor tensor = GetSizedTensorWithOptions(
+        caffe2::Tensor(ival->toTensor()), dims, options);
+    // assign it back in case it changed
     auto at_tensor = at::Tensor(std::move(tensor.getIntrusivePtr()));
     *ival = IValue(at_tensor);
 

diff --git a/caffe2/core/tensor.h b/caffe2/core/tensor.h
index 0f92806..4c64b1b 100644 (file)
--- a/caffe2/core/tensor.h
+++ b/caffe2/core/tensor.h
@@ -23,6 +23,10 @@ using at::UndefinedTensorImpl;
  * NB: See TensorImpl for documentation on these methods.
  */
 class CAFFE2_API Tensor final {
+ private:
+  enum Unsafe { IDoWantAliasing };
+  Tensor(const Tensor& other, Unsafe _) : impl_(other.getIntrusivePtr()) {}
+
  protected:
   using TensorImplPtr = c10::intrusive_ptr<TensorImpl, UndefinedTensorImpl>;
   TensorImplPtr impl_;
@@ -36,6 +40,21 @@ class CAFFE2_API Tensor final {
     }
   }
 
+  // caffe2::Tensor is explicitly marked as moveable-only because before
+  // the refactoring the class used to be a value type and a lot of user code
+  // is written this way. With PyTorch unification, caffe2::Tensor actually
+  // has semantics of a shared_ptr now (via intrusive_ptr). However, to prevent
+  // accidental mistakes when changing legacy code we keep caffe2::Tensor
+  // to have movable semantics.
+  //
+  // If you need to get a pointer to the same Tensor instance (not to be
+  // confused with shared storage), `UnsafeSharedInstance` can be used. It has
+  // the same behavior as `at::Tensor a = b`.
+  Tensor(const Tensor&) = delete;
+  Tensor& operator=(const Tensor&) = delete;
+  Tensor(Tensor&&) = default;
+  Tensor& operator=(Tensor&&) = default;
+
   operator bool() const {
     return impl_.defined();
   }
@@ -44,6 +63,10 @@ class CAFFE2_API Tensor final {
     return impl_.get();
   }
 
+  Tensor UnsafeSharedInstance() const {
+    return Tensor(*this, IDoWantAliasing);
+  }
+
   /**
    * @brief Creates a tensor of the given device type.
    *

diff --git a/caffe2/operators/slice_op.h b/caffe2/operators/slice_op.h
index 641c19e..536df39 100644 (file)
--- a/caffe2/operators/slice_op.h
+++ b/caffe2/operators/slice_op.h
@@ -242,7 +242,7 @@ class SliceOp : public Operator<Context> {
       }
     }
 
-    auto data = Input(0);
+    const auto& data = Input(0);
     auto output = Output(0);
 
     return SliceImpl<SIndex, Context>(

diff --git a/caffe2/predictor/predictor.cc b/caffe2/predictor/predictor.cc
index 4b7cbc4..615d871 100644 (file)
--- a/caffe2/predictor/predictor.cc
+++ b/caffe2/predictor/predictor.cc
@@ -57,9 +57,10 @@ bool Predictor::operator()(const TensorList& inputs, TensorList* outputs) {
       inputs.size() <=
       static_cast<unsigned>(config_.predict_net->external_input_size()));
   for (size_t i = 0; i < inputs.size(); ++i) {
+    // This is evil and shares the same underlying tensor
     BlobSetTensor(
         getBlob(config_.ws.get(), config_.predict_net->external_input(i)),
-        inputs[i]);
+        inputs[i].UnsafeSharedInstance());
   }
 
   if (!config_.ws->RunNet(config_.predict_net->name())) {
@@ -67,8 +68,9 @@ bool Predictor::operator()(const TensorList& inputs, TensorList* outputs) {
   }
   outputs->clear();
   for (size_t i = 0; i < config_.predict_net->external_output_size(); ++i) {
-    outputs->push_back(
-        getTensor(config_.ws.get(), config_.predict_net->external_output(i)));
+    outputs->emplace_back(
+        getTensor(config_.ws.get(), config_.predict_net->external_output(i))
+            .UnsafeSharedInstance());
   }
   return true;
 }
@@ -85,7 +87,10 @@ bool Predictor::run_map_workspace(const TensorMap& inputs) {
           "Input can't be found: ",
           input.first);
     }
-    BlobSetTensor(getBlob(config_.ws.get(), input.first), input.second);
+    // This is evil and shares the same underlying tensor
+    BlobSetTensor(
+        getBlob(config_.ws.get(), input.first),
+        input.second.UnsafeSharedInstance());
   }
 
   return config_.ws->RunNet(config_.predict_net->name());
@@ -98,7 +103,8 @@ bool Predictor::operator()(const TensorMap& inputs, TensorList* outputs) {
   outputs->clear();
   for (size_t i = 0; i < config_.predict_net->external_output_size(); ++i) {
     outputs->push_back(
-        getTensor(config_.ws.get(), config_.predict_net->external_output(i)));
+        getTensor(config_.ws.get(), config_.predict_net->external_output(i))
+            .UnsafeSharedInstance());
   }
   return true;
 }
@@ -109,7 +115,9 @@ bool Predictor::operator()(const TensorMap& inputs, TensorMap* outputs) {
   }
 
   for (const std::string& outputName : output_names()) {
-    outputs->emplace(outputName, getTensor(config_.ws.get(), outputName));
+    outputs->emplace(
+        outputName,
+        getTensor(config_.ws.get(), outputName).UnsafeSharedInstance());
   }
   return true;
 }

diff --git a/caffe2/python/pybind_state.h b/caffe2/python/pybind_state.h
index ea1a831..3415de7 100644 (file)
--- a/caffe2/python/pybind_state.h
+++ b/caffe2/python/pybind_state.h
@@ -178,10 +178,17 @@ class TensorFetcher : public BlobFetcherBase {
 template <class Context>
 class TensorFeeder : public BlobFeederBase {
  public:
-  Tensor FeedTensor(
+  Tensor FeedTensor(const DeviceOption& option, PyArrayObject* original_array) {
+    Tensor out;
+    FeedTensor(option, original_array, &out, false);
+    return out;
+  }
+
+  void FeedTensor(
       const DeviceOption& option,
       PyArrayObject* original_array,
-      Tensor* out = nullptr) {
+      Tensor* out,
+      bool in_place) {
 #ifdef USE_NUMPY
     PyArrayObject* array = PyArray_GETCONTIGUOUS(original_array);
     auto g = MakeGuard([&]() { Py_XDECREF(array); });
@@ -203,11 +210,9 @@ class TensorFeeder : public BlobFeederBase {
       dims.push_back(npy_dims[i]);
     }
 
-    Tensor tensor;
-    bool in_place = out != nullptr;
+    Tensor& tensor = *out;
     if (in_place) {
-      out->Resize(dims);
-      tensor = *out;
+      tensor.Resize(dims);
     }
     // Now, copy the data to the tensor.
     switch (npy_type) {
@@ -262,10 +267,8 @@ class TensorFeeder : public BlobFeederBase {
             tensor.raw_mutable_data());
     }
     context.FinishDeviceComputation();
-    return tensor;
 #else
     CAFFE_THROW("Caffe2 compiled without NumPy support.");
-    return caffe2::Tensor(); // will not reach here
 #endif // USE_NUMPY
   }
 
@@ -278,7 +281,8 @@ class TensorFeeder : public BlobFeederBase {
       FeedTensor(
           option,
           original_array,
-          BlobGetMutableTensor(blob, OptionToDevice(option).type()));
+          BlobGetMutableTensor(blob, OptionToDevice(option).type()),
+          true);
     } else {
       blob->Reset<Tensor>(new Tensor(FeedTensor(option, original_array)));
     }

diff --git a/caffe2/python/pybind_state_ideep.cc b/caffe2/python/pybind_state_ideep.cc
index 38df5e1..f829622 100644 (file)
--- a/caffe2/python/pybind_state_ideep.cc
+++ b/caffe2/python/pybind_state_ideep.cc
@@ -180,7 +180,8 @@ public:
           cpu_tensor_feeder.FeedTensor(
               option,
               original_array,
-              BlobGetMutableTensor(blob, OptionToDevice(option).type()));
+              BlobGetMutableTensor(blob, OptionToDevice(option).type()),
+              true);
         } else {
           blob->Reset<Tensor>(new Tensor(
                                   cpu_tensor_feeder.FeedTensor(cpu_option, original_array)));