Let Sychronized move-constructor not lock the source

Summary: [Folly] Let `Sychronized` move-constructor not lock the source object, since it is an rvalue-reference and the move-constructor code may therefore be assumed to have the only live reference to the source. Same with the move-assignment operator.

Reviewed By: aary

Differential Revision: D8219883

fbshipit-source-id: f62ff87197ac4b9ceed290a73a05062ab8ed45c4

folly/Synchronized.h

@@ -523,33 +523,36 @@ struct Synchronized : public SynchronizedBase<
    */
   Synchronized() = default;
 
+ public:
   /**
-   * Copy constructor copies the data (with locking the source and
-   * all) but does NOT copy the mutex. Doing so would result in
-   * deadlocks.
+   * Copy constructor; deprecated
+   *
+   * Enabled only when the data type is copy-constructible.
+   *
+   * Takes a shared-or-exclusive lock on the source mutex while performing the
+   * copy-construction of the destination data from the source data. No lock is
+   * taken on the destination mutex.
    *
-   * Note that the copy constructor may throw because it acquires a lock in
-   * the contextualRLock() method
+   * May throw even when the data type is is nothrow-copy-constructible because
+   * acquiring a lock may throw.
    */
- public:
   /* implicit */ Synchronized(typename std::conditional<
                               std::is_copy_constructible<T>::value,
                               const Synchronized&,
                               NonImplementedType>::type rhs) /* may throw */
-      : Synchronized(rhs, rhs.contextualRLock()) {}
+      : Synchronized(rhs.copy()) {}
 
   /**
-   * Move constructor moves the data (with locking the source and all)
-   * but does not move the mutex.
+   * Move constructor; deprecated
+   *
+   * Move-constructs from the source data without locking either the source or
+   * the destination mutex.
    *
-   * Note that the move constructor may throw because it acquires a lock.
-   * Since the move constructor is not declared noexcept, when objects of this
-   * class are used as elements in a vector or a similar container.  The
-   * elements might not be moved around when resizing.  They might be copied
-   * instead.  You have been warned.
+   * Semantically, assumes that the source object is a true rvalue and therefore
+   * that no synchronization is required for accessing it.
    */
-  Synchronized(Synchronized&& rhs) /* may throw */
-      : Synchronized(std::move(rhs), rhs.contextualLock()) {}
+  Synchronized(Synchronized&& rhs) noexcept(nxMoveCtor)
+      : Synchronized(std::move(rhs.datum_)) {}
 
   /**
    * Constructor taking a datum as argument copies it. There is no
@@ -588,54 +591,49 @@ struct Synchronized : public SynchronizedBase<
                      make_index_sequence<sizeof...(MutexArgs)>{}} {}
 
   /**
-   * The canonical assignment operator only assigns the data, NOT the
-   * mutex. It locks the two objects in ascending order of their
-   * addresses.
+   * Copy assignment operator; deprecated
+   *
+   * Enabled only when the data type is copy-constructible and move-assignable.
+   *
+   * Move-assigns from a copy of the source data.
+   *
+   * Takes a shared-or-exclusive lock on the source mutex while copying the
+   * source data to a temporary. Takes an exclusive lock on the destination
+   * mutex while move-assigning from the temporary.
+   *
+   * This technique consts an extra temporary but avoids the need to take locks
+   * on both mutexes together.
    */
   Synchronized& operator=(typename std::conditional<
-                          std::is_copy_assignable<T>::value,
+                          std::is_copy_constructible<T>::value &&
+                              std::is_move_assignable<T>::value,
                           const Synchronized&,
                           NonImplementedType>::type rhs) {
-    if (this == &rhs) {
-      // Self-assignment, pass.
-    } else if (this < &rhs) {
-      auto guard1 = operator->();
-      auto guard2 = rhs.operator->();
-      datum_ = rhs.datum_;
-    } else {
-      auto guard1 = rhs.operator->();
-      auto guard2 = operator->();
-      datum_ = rhs.datum_;
-    }
-    return *this;
+    return *this = rhs.copy();
   }
 
   /**
-   * Move assignment operator, only assigns the data, NOT the
-   * mutex. It locks the two objects in ascending order of their
-   * addresses.
+   * Move assignment operator; deprecated
+   *
+   * Takes an exclusive lock on the destination mutex while move-assigning the
+   * destination data from the source data. The source mutex is not locked or
+   * otherwise accessed.
+   *
+   * Semantically, assumes that the source object is a true rvalue and therefore
+   * that no synchronization is required for accessing it.
    */
   Synchronized& operator=(Synchronized&& rhs) {
-    if (this == &rhs) {
-      // Self-assignment, pass.
-    } else if (this < &rhs) {
-      auto guard1 = operator->();
-      auto guard2 = rhs.operator->();
-      datum_ = std::move(rhs.datum_);
-    } else {
-      auto guard1 = rhs.operator->();
-      auto guard2 = operator->();
-      datum_ = std::move(rhs.datum_);
-    }
-    return *this;
+    return *this = std::move(rhs.datum_);
   }
 
   /**
    * Lock object, assign datum.
    */
   Synchronized& operator=(const T& rhs) {
-    auto guard = operator->();
-    datum_ = rhs;
+    if (&datum_ != &rhs) {
+      auto guard = operator->();
+      datum_ = rhs;
+    }
     return *this;
   }
 
@@ -643,8 +641,10 @@ struct Synchronized : public SynchronizedBase<
    * Lock object, move-assign datum.
    */
   Synchronized& operator=(T&& rhs) {
-    auto guard = operator->();
-    datum_ = std::move(rhs);
+    if (&datum_ != &rhs) {
+      auto guard = operator->();
+      datum_ = std::move(rhs);
+    }
     return *this;
   }
 

folly/docs/Synchronized.md

@@ -198,27 +198,26 @@ takes an object of type `T` and copies it. For example:
 
 #### Assignment, swap, and copying
 
-The canonical assignment operator locks both objects involved and
-then copies the underlying data objects. The mutexes are not
-copied. The locks are acquired in increasing address order, so
-deadlock is avoided. For example, there is no problem if one
-thread assigns `a = b` and the other assigns `b = a` (other than
-that design probably deserving a Razzie award). Similarly, the
-`swap` method takes a reference to another `Synchronized<T>`
-object and swaps the data. Again, locks are acquired in a well-
-defined order. The mutexes are not swapped.
-
-An additional assignment operator accepts a `const T&` on the
-right-hand side. The operator copies the datum inside a
-critical section.
-
-In addition to assignment operators, `Synchronized<T>` has move
-assignment operators.
-
-An additional `swap` method accepts a `T&` and swaps the data
-inside a critical section. This is by far the preferred method of
-changing the guarded datum wholesale because it keeps the lock
-only for a short time, thus lowering the pressure on the mutex.
+The copy assignment operator copies the underlying source data
+into a temporary with the source mutex locked, and then move the
+temporary into the destination data with the destination mutex
+locked. This technique avoids the need to lock both mutexes at
+the same time. Mutexes are not copied or moved.
+
+The move assignment operator assumes the source object is a true
+rvalue and does lock lock the source mutex. It moves the source
+data into the destination data with the destination mutex locked.
+
+`swap` acquires locks on both mutexes in increasing order of
+object address, and then swaps the underlying data. This avoids
+potential deadlock, which may otherwise happen should one thread
+do `a = b` while another thread does `b = a`.
+
+The data copy assignment operator copies the parameter into the
+destination data while the destination mutex is locked.
+
+The data move assignment operator moves the parameter into the
+destination data while the destination mutex is locked.
 
 To get a copy of the guarded data, there are two methods
 available: `void copy(T*)` and `T copy()`. The first copies data