[PyTorch] Remove the List/Dict move operations (pytorch#69370)

Summary: Pull Request resolved: pytorch#69370 These operations are likely slower than copying because they perform a heap allocation and reference count bump, whereas copying is just a reference count bump. This diff is up to see 1) if anything breaks 2) if we can measure any improvements. ghstack-source-id: 146468907 Test Plan: Ran //sigrid/lib/features/tests:pytorch_feature_conversion_benchmark before/after ``` swolchok@devbig032 ~/f/fbcode> for x in (seq 5); sudo scripts/bertrand/noise/denoise.sh /tmp/pytorch_feature_conversion_benchmark.Dec7Stable ; end ============================================================================ sigrid/lib/features/tests/PyTorchFeatureConversionBenchmark.cpprelative time/iter iters/s ============================================================================ PyTorchFeatureConversionDenseBenchmark 2.43us 410.68K PyTorchFeatureConversionIdListBenchmark 3.74us 267.65K PyTorchFeatureConversionIdScoreListBenchmark 4.98us 200.81K ============================================================================ ============================================================================ sigrid/lib/features/tests/PyTorchFeatureConversionBenchmark.cpprelative time/iter iters/s ============================================================================ PyTorchFeatureConversionDenseBenchmark 2.43us 410.75K PyTorchFeatureConversionIdListBenchmark 3.75us 266.92K PyTorchFeatureConversionIdScoreListBenchmark 4.98us 200.97K ============================================================================ ============================================================================ sigrid/lib/features/tests/PyTorchFeatureConversionBenchmark.cpprelative time/iter iters/s ============================================================================ PyTorchFeatureConversionDenseBenchmark 2.44us 410.43K PyTorchFeatureConversionIdListBenchmark 3.75us 266.75K PyTorchFeatureConversionIdScoreListBenchmark 5.04us 198.23K ============================================================================ ============================================================================ sigrid/lib/features/tests/PyTorchFeatureConversionBenchmark.cpprelative time/iter iters/s ============================================================================ PyTorchFeatureConversionDenseBenchmark 2.43us 411.17K PyTorchFeatureConversionIdListBenchmark 3.74us 267.60K PyTorchFeatureConversionIdScoreListBenchmark 5.00us 199.84K ============================================================================ ============================================================================ sigrid/lib/features/tests/PyTorchFeatureConversionBenchmark.cpprelative time/iter iters/s ============================================================================ PyTorchFeatureConversionDenseBenchmark 2.44us 410.19K PyTorchFeatureConversionIdListBenchmark 3.73us 267.89K PyTorchFeatureConversionIdScoreListBenchmark 4.96us 201.46K ============================================================================ swolchok@devbig032 ~/f/fbcode> for x in (seq 5); sudo scripts/bertrand/noise/denoise.sh /tmp/pytorch_feature_conversion_benchmark.Dec8RemoveListAndDictMove ; end ============================================================================ sigrid/lib/features/tests/PyTorchFeatureConversionBenchmark.cpprelative time/iter iters/s ============================================================================ PyTorchFeatureConversionDenseBenchmark 2.47us 405.12K PyTorchFeatureConversionIdListBenchmark 3.60us 278.07K PyTorchFeatureConversionIdScoreListBenchmark 4.87us 205.44K ============================================================================ ============================================================================ sigrid/lib/features/tests/PyTorchFeatureConversionBenchmark.cpprelative time/iter iters/s ============================================================================ PyTorchFeatureConversionDenseBenchmark 2.45us 407.39K PyTorchFeatureConversionIdListBenchmark 3.63us 275.56K PyTorchFeatureConversionIdScoreListBenchmark 4.95us 202.17K ============================================================================ ============================================================================ sigrid/lib/features/tests/PyTorchFeatureConversionBenchmark.cpprelative time/iter iters/s ============================================================================ PyTorchFeatureConversionDenseBenchmark 2.47us 405.49K PyTorchFeatureConversionIdListBenchmark 3.63us 275.58K PyTorchFeatureConversionIdScoreListBenchmark 4.88us 205.05K ============================================================================ ============================================================================ sigrid/lib/features/tests/PyTorchFeatureConversionBenchmark.cpprelative time/iter iters/s ============================================================================ PyTorchFeatureConversionDenseBenchmark 2.52us 396.13K PyTorchFeatureConversionIdListBenchmark 3.59us 278.29K PyTorchFeatureConversionIdScoreListBenchmark 4.88us 204.94K ============================================================================ ============================================================================ sigrid/lib/features/tests/PyTorchFeatureConversionBenchmark.cpprelative time/iter iters/s ============================================================================ PyTorchFeatureConversionDenseBenchmark 2.46us 406.77K PyTorchFeatureConversionIdListBenchmark 3.62us 276.17K PyTorchFeatureConversionIdScoreListBenchmark 4.92us 203.07K ============================================================================ ``` Reviewed By: suo, hlu1 Differential Revision: D32836701 fbshipit-source-id: 6e1c3d81f1b4ee13156320263dac17f5256c1462
yanbing-j · Jan 5, 2022 · a5bc444 · a5bc444
1 parent b283b1d
commit a5bc444
Show file tree

Hide file tree

Showing 6 changed files with 24 additions and 46 deletions.
diff --git a/aten/src/ATen/core/Dict.h b/aten/src/ATen/core/Dict.h
@@ -239,8 +239,6 @@ class Dict final {
 
   Dict(const Dict&) = default;
   Dict& operator=(const Dict&) = default;
-  Dict(Dict&&) noexcept;
-  Dict& operator=(Dict&&) noexcept;
 
   /**
    * Create a new Dict pointing to a deep copy of the same data.

diff --git a/aten/src/ATen/core/Dict_inl.h b/aten/src/ATen/core/Dict_inl.h
@@ -82,21 +82,9 @@ Dict<Key, Value>::Dict(TypePtr keyType, TypePtr valueType)
   static_assert(std::is_same<Value, IValue>::value, "This constructor is only valid for c10::impl::GenericDict.");
 }
 
-template<class Key, class Value>
-Dict<Key, Value>::Dict(Dict&& rhs) noexcept: impl_(std::move(rhs.impl_)) {
-  rhs.impl_ = make_intrusive<detail::DictImpl>(detail::DictImpl::dict_map_type(), impl_->elementTypes);
-}
-
 template<class Key, class Value>
 Dict<Key, Value>::Dict(c10::intrusive_ptr<detail::DictImpl>&& impl): impl_(std::move(impl)) {}
 
-template<class Key, class Value>
-Dict<Key, Value>& Dict<Key, Value>::operator=(Dict&& rhs) noexcept {
-  impl_ = std::move(rhs.impl_);
-  rhs.impl_ = make_intrusive<detail::DictImpl>(detail::DictImpl::dict_map_type(), impl_->elementTypes);
-  return *this;
-}
-
 template<class Key, class Value>
 Dict<Key, Value> Dict<Key, Value>::copy() const {
   return Dict<Key, Value>(impl_->copy());

diff --git a/aten/src/ATen/core/List.h b/aten/src/ATen/core/List.h
@@ -259,8 +259,6 @@ class List final {
 
   List(const List&) = default;
   List& operator=(const List&) = default;
-  List(List&&) noexcept;
-  List& operator=(List&&) noexcept;
 
   /**
    * Create a new List pointing to a deep copy of the same data.

diff --git a/aten/src/ATen/core/List_inl.h b/aten/src/ATen/core/List_inl.h
@@ -78,18 +78,6 @@ impl::GenericList toList(const List<T>& list) {
 }
 }
 
-template<class T>
-List<T>::List(List&& rhs) noexcept: impl_(std::move(rhs.impl_)) {
-  rhs.impl_ = make_intrusive<c10::detail::ListImpl>(std::vector<IValue>{}, impl_->elementType);
-}
-
-template<class T>
-List<T>& List<T>::operator=(List&& rhs) noexcept {
-  impl_ = std::move(rhs.impl_);
-  rhs.impl_ = make_intrusive<c10::detail::ListImpl>(std::vector<IValue>{}, impl_->elementType);
-  return *this;
-}
-
 template<class T>
 List<T> List<T>::copy() const {
   return List<T>(impl_->copy());

diff --git a/aten/src/ATen/core/List_test.cpp b/aten/src/ATen/core/List_test.cpp
@@ -341,21 +341,23 @@ TEST(ListTest_IValueBasedList, whenMoveAssigningList_thenNewIsCorrect) {
   EXPECT_EQ("4", list2.get(1));
 }
 
-TEST(ListTest_IValueBasedList, whenMoveConstructingList_thenOldIsEmpty) {
+TEST(ListTest_IValueBasedList, whenMoveConstructingList_thenOldIsUnchanged) {
   List<string> list1({"3", "4"});
 
   List<string> list2(std::move(list1));
-  // NOLINTNEXTLINE(bugprone-use-after-move)
-  EXPECT_TRUE(list1.empty());
+  EXPECT_EQ(2, list1.size());
+  EXPECT_EQ("3", list1.get(0));
+  EXPECT_EQ("4", list1.get(1));
 }
 
-TEST(ListTest_IValueBasedList, whenMoveAssigningList_thenOldIsEmpty) {
+TEST(ListTest_IValueBasedList, whenMoveAssigningList_thenOldIsUnchanged) {
   List<string> list1({"3", "4"});
 
   List<string> list2;
   list2 = std::move(list1);
-  // NOLINTNEXTLINE(bugprone-use-after-move)
-  EXPECT_TRUE(list1.empty());
+  EXPECT_EQ(2, list1.size());
+  EXPECT_EQ("3", list1.get(0));
+  EXPECT_EQ("4", list1.get(1));
 }
 
 TEST(ListTest_IValueBasedList, givenIterator_whenPostfixIncrementing_thenMovesToNextAndReturnsOldPosition) {
@@ -887,21 +889,23 @@ TEST(ListTest_NonIValueBasedList, whenMoveAssigningList_thenNewIsCorrect) {
   EXPECT_EQ(4, list2.get(1));
 }
 
-TEST(ListTest_NonIValueBasedList, whenMoveConstructingList_thenOldIsEmpty) {
+TEST(ListTest_NonIValueBasedList, whenMoveConstructingList_thenOldIsUnchanged) {
   List<int64_t> list1({3, 4});
 
   List<int64_t> list2(std::move(list1));
-  // NOLINTNEXTLINE(bugprone-use-after-move)
-  EXPECT_TRUE(list1.empty());
+  EXPECT_EQ(2, list1.size());
+  EXPECT_EQ(3, list1.get(0));
+  EXPECT_EQ(4, list1.get(1));
 }
 
-TEST(ListTest_NonIValueBasedList, whenMoveAssigningList_thenOldIsEmpty) {
+TEST(ListTest_NonIValueBasedList, whenMoveAssigningList_thenOldIsUnchanged) {
   List<int64_t> list1({3, 4});
 
   List<int64_t> list2;
   list2 = std::move(list1);
-  // NOLINTNEXTLINE(bugprone-use-after-move)
-  EXPECT_TRUE(list1.empty());
+  EXPECT_EQ(2, list1.size());
+  EXPECT_EQ(3, list1.get(0));
+  EXPECT_EQ(4, list1.get(1));
 }
 
 TEST(ListTest_NonIValueBasedList, givenIterator_whenPostfixIncrementing_thenMovesToNextAndReturnsOldPosition) {

diff --git a/aten/src/ATen/test/Dict_test.cpp b/aten/src/ATen/test/Dict_test.cpp
@@ -354,25 +354,27 @@ TEST(DictTest, whenMoveAssigningDict_thenNewIsCorrect) {
   EXPECT_EQ("4", dict2.at(4));
 }
 
-TEST(DictTest, whenMoveConstructingDict_thenOldIsEmpty) {
+TEST(DictTest, whenMoveConstructingDict_thenOldIsUnchanged) {
   Dict<int64_t, string> dict1;
   dict1.insert(3, "3");
   dict1.insert(4, "4");
 
   Dict<int64_t, string> dict2(std::move(dict1));
-  // NOLINTNEXTLINE(bugprone-use-after-move)
-  EXPECT_TRUE(dict1.empty());
+  EXPECT_EQ(2, dict1.size());
+  EXPECT_EQ("3", dict1.at(3));
+  EXPECT_EQ("4", dict1.at(4));
 }
 
-TEST(DictTest, whenMoveAssigningDict_thenOldIsEmpty) {
+TEST(DictTest, whenMoveAssigningDict_thenOldIsUnchanged) {
   Dict<int64_t, string> dict1;
   dict1.insert(3, "3");
   dict1.insert(4, "4");
 
   Dict<int64_t, string> dict2;
   dict2 = std::move(dict1);
-  // NOLINTNEXTLINE(bugprone-use-after-move)
-  EXPECT_TRUE(dict1.empty());
+  EXPECT_EQ(2, dict1.size());
+  EXPECT_EQ("3", dict1.at(3));
+  EXPECT_EQ("4", dict1.at(4));
 }
 
 TEST(DictTest, givenIterator_whenPostfixIncrementing_thenMovesToNextAndReturnsOldPosition) {