Don't launder the Optional's storage

Summary:
`Optional` is often used for arguments and return values, and
when the function is inlined it is important to be able to perform
optimizations such as constant folding.

`launder` forces a load on every access to the `Optional` in GCC,
making it unsuitable for small hot functions. This is not specific to
the `asm` trick we use to backport `launder` to pre-GCC7: the same
code is generated in GCC7 with the builtin `std::launder`.

`launder` is needed for correctness, as replacing an object that
contains const or reference members in the same storage is
UB. However, it seems to be a benign UB that real compilers don't take
advantage of. In fact, the implementation of `std::optional` in both
libstdc++ and libc++ does not launder the storage:

https://github.com/gcc-mirror/gcc/blob/20d1a0756a0cf5072d0cdf3d2adab00063c224a7/libstdc%2B%2B-v3/include/std/optional#L881
https://github.com/llvm-mirror/libcxx/blob/8dd2afa20a01ee70e1a49c15de3de343aa8aa7d6/include/optional#L295

So it should be safe to follow these implementations and recover the
perf hit.

Reviewed By: luciang

Differential Revision: D7689228

fbshipit-source-id: 8283de56b0934583773a0d19f315ae7a8d556e8c

folly/Optional.h

@@ -63,7 +63,6 @@
 #include <folly/Portability.h>
 #include <folly/Traits.h>
 #include <folly/Utility.h>
-#include <folly/lang/Launder.h>
 
 namespace folly {
 
@@ -109,14 +108,14 @@ class Optional {
   Optional(const Optional& src) noexcept(
       std::is_nothrow_copy_constructible<Value>::value) {
     if (src.hasValue()) {
-      storage_.construct(src.value());
+      construct(src.value());
     }
   }
 
   Optional(Optional&& src) noexcept(
       std::is_nothrow_move_constructible<Value>::value) {
     if (src.hasValue()) {
-      storage_.construct(std::move(src.value()));
+      construct(std::move(src.value()));
       src.clear();
     }
   }
@@ -125,18 +124,18 @@ class Optional {
 
   FOLLY_CPP14_CONSTEXPR /* implicit */ Optional(Value&& newValue) noexcept(
       std::is_nothrow_move_constructible<Value>::value) {
-    storage_.construct(std::move(newValue));
+    construct(std::move(newValue));
   }
 
   FOLLY_CPP14_CONSTEXPR /* implicit */ Optional(const Value& newValue) noexcept(
       std::is_nothrow_copy_constructible<Value>::value) {
-    storage_.construct(newValue);
+    construct(newValue);
   }
 
   template <typename... Args>
   FOLLY_CPP14_CONSTEXPR explicit Optional(in_place_t, Args&&... args) noexcept(
       std::is_nothrow_constructible<Value, Args...>::value) {
-    storage_.construct(std::forward<Args>(args)...);
+    construct(std::forward<Args>(args)...);
   }
 
   // Used only when an Optional is used with coroutines on MSVC
@@ -170,17 +169,17 @@ class Optional {
 
   void assign(Value&& newValue) {
     if (hasValue()) {
-      *storage_.value_pointer() = std::move(newValue);
+      storage_.value = std::move(newValue);
     } else {
-      storage_.construct(std::move(newValue));
+      construct(std::move(newValue));
     }
   }
 
   void assign(const Value& newValue) {
     if (hasValue()) {
-      *storage_.value_pointer() = newValue;
+      storage_.value = newValue;
     } else {
-      storage_.construct(newValue);
+      construct(newValue);
     }
   }
 
@@ -205,7 +204,8 @@ class Optional {
   template <class... Args>
   Value& emplace(Args&&... args) {
     clear();
-    return storage_.construct(std::forward<Args>(args)...);
+    construct(std::forward<Args>(args)...);
+    return value();
   }
 
   template <class U, class... Args>
@@ -214,7 +214,8 @@ class Optional {
       Value&>::type
   emplace(std::initializer_list<U> ilist, Args&&... args) {
     clear();
-    return storage_.construct(ilist, std::forward<Args>(args)...);
+    construct(ilist, std::forward<Args>(args)...);
+    return value();
   }
 
   void reset() noexcept {
@@ -240,34 +241,34 @@ class Optional {
 
   FOLLY_CPP14_CONSTEXPR const Value& value() const & {
     require_value();
-    return *storage_.value_pointer();
+    return storage_.value;
   }
 
   FOLLY_CPP14_CONSTEXPR Value& value() & {
     require_value();
-    return *storage_.value_pointer();
+    return storage_.value;
   }
 
   FOLLY_CPP14_CONSTEXPR Value&& value() && {
     require_value();
-    return std::move(*storage_.value_pointer());
+    return std::move(storage_.value);
   }
 
   FOLLY_CPP14_CONSTEXPR const Value&& value() const && {
     require_value();
-    return std::move(*storage_.value_pointer());
+    return std::move(storage_.value);
   }
 
   const Value* get_pointer() const & {
-    return storage_.value_pointer();
+    return storage_.hasValue ? &storage_.value : nullptr;
   }
   Value* get_pointer() & {
-    return storage_.value_pointer();
+    return storage_.hasValue ? &storage_.value : nullptr;
   }
   Value* get_pointer() && = delete;
 
   FOLLY_CPP14_CONSTEXPR bool has_value() const noexcept {
-    return storage_.hasValue();
+    return storage_.hasValue;
   }
 
   FOLLY_CPP14_CONSTEXPR bool hasValue() const noexcept {
@@ -301,8 +302,8 @@ class Optional {
   // Return a copy of the value if set, or a given default if not.
   template <class U>
   FOLLY_CPP14_CONSTEXPR Value value_or(U&& dflt) const & {
-    if (storage_.hasValue()) {
-      return *storage_.value_pointer();
+    if (storage_.hasValue) {
+      return storage_.value;
     }
 
     return std::forward<U>(dflt);
@@ -310,8 +311,8 @@ class Optional {
 
   template <class U>
   FOLLY_CPP14_CONSTEXPR Value value_or(U&& dflt) && {
-    if (storage_.hasValue()) {
-      return std::move(*storage_.value_pointer());
+    if (storage_.hasValue) {
+      return std::move(storage_.value);
     }
 
     return std::forward<U>(dflt);
@@ -319,78 +320,63 @@ class Optional {
 
  private:
   void require_value() const {
-    if (!storage_.hasValue()) {
+    if (!storage_.hasValue) {
       detail::throw_optional_empty_exception();
     }
   }
 
+  template <class... Args>
+  void construct(Args&&... args) {
+    const void* ptr = &storage_.value;
+    // For supporting const types.
+    new (const_cast<void*>(ptr)) Value(std::forward<Args>(args)...);
+    storage_.hasValue = true;
+  }
+
   struct StorageTriviallyDestructible {
-   protected:
-    bool hasValue_;
-    typename std::aligned_storage<sizeof(Value), alignof(Value)>::type
-        value_[1];
+    union {
+      char emptyState;
+      Value value;
+    };
+    bool hasValue;
 
-   public:
-    StorageTriviallyDestructible() : hasValue_{false} {}
+    StorageTriviallyDestructible() : hasValue{false} {}
     void clear() {
-      hasValue_ = false;
+      hasValue = false;
     }
   };
 
   struct StorageNonTriviallyDestructible {
-   protected:
-    bool hasValue_;
-    typename std::aligned_storage<sizeof(Value), alignof(Value)>::type
-        value_[1];
-
-   public:
-    StorageNonTriviallyDestructible() : hasValue_{false} {}
+    union {
+      char emptyState;
+      Value value;
+    };
+    bool hasValue;
+
+    FOLLY_PUSH_WARNING
+    // These are both informational warnings, but they trigger rare
+    // enough that we've left them enabled. Needed as long as MSVC
+    // 2015 is supported.
+    FOLLY_MSVC_DISABLE_WARNING(4587) // constructor of .value is not called
+    FOLLY_MSVC_DISABLE_WARNING(4588) // destructor of .value is not called
+    StorageNonTriviallyDestructible() : hasValue{false} {}
     ~StorageNonTriviallyDestructible() {
       clear();
     }
+    FOLLY_POP_WARNING
 
     void clear() {
-      if (hasValue_) {
-        hasValue_ = false;
-        launder(reinterpret_cast<Value*>(value_))->~Value();
+      if (hasValue) {
+        hasValue = false;
+        value.~Value();
       }
     }
   };
 
-  struct Storage : std::conditional<
-                       std::is_trivially_destructible<Value>::value,
-                       StorageTriviallyDestructible,
-                       StorageNonTriviallyDestructible>::type {
-    bool hasValue() const noexcept {
-      return this->hasValue_;
-    }
-
-    Value* value_pointer() {
-      if (this->hasValue_) {
-        return launder(reinterpret_cast<Value*>(this->value_));
-      }
-      return nullptr;
-    }
-
-    Value const* value_pointer() const {
-      if (this->hasValue_) {
-        return launder(reinterpret_cast<Value const*>(this->value_));
-      }
-      return nullptr;
-    }
-
-    template <class... Args>
-    Value& construct(Args&&... args) {
-      new (raw_pointer()) Value(std::forward<Args>(args)...);
-      this->hasValue_ = true;
-      return *launder(reinterpret_cast<Value*>(this->value_));
-    }
-
-   private:
-    void* raw_pointer() {
-      return static_cast<void*>(this->value_);
-    }
-  };
+  using Storage = typename std::conditional<
+      std::is_trivially_destructible<Value>::value,
+      StorageTriviallyDestructible,
+      StorageNonTriviallyDestructible>::type;
 
   Storage storage_;
 };

folly/test/OptionalTest.cpp

@@ -34,6 +34,8 @@ using std::shared_ptr;
 
 namespace folly {
 
+namespace {
+
 template <class V>
 std::ostream& operator<<(std::ostream& os, const Optional<V>& v) {
   if (v) {
@@ -49,6 +51,8 @@ struct NoDefault {
   char a, b, c;
 };
 
+} // namespace
+
 static_assert(sizeof(Optional<char>) == 2, "");
 static_assert(sizeof(Optional<int>) == 8, "");
 static_assert(sizeof(Optional<NoDefault>) == 4, "");
@@ -133,6 +137,8 @@ TEST(Optional, Simple) {
   EXPECT_FALSE(bool(opt));
 }
 
+namespace {
+
 class MoveTester {
  public:
   /* implicit */ MoveTester(const char* s) : s_(s) {}
@@ -157,6 +163,8 @@ bool operator==(const MoveTester& o1, const MoveTester& o2) {
   return o1.s_ == o2.s_;
 }
 
+} // namespace
+
 TEST(Optional, value_or_rvalue_arg) {
   Optional<MoveTester> opt;
   MoveTester dflt = "hello";
@@ -616,6 +624,8 @@ TEST(Optional, SelfAssignment) {
 # pragma clang diagnostic pop
 #endif
 
+namespace {
+
 class ContainsOptional {
  public:
   ContainsOptional() { }
@@ -632,6 +642,8 @@ class ContainsOptional {
   Optional<int> opt_;
 };
 
+} // namespace
+
 /**
  * Test that a class containing an Optional can be copy and move assigned.
  * This was broken under gcc 4.7 until assignment operators were explicitly
@@ -669,12 +681,16 @@ TEST(Optional, NoThrowDefaultConstructible) {
   EXPECT_TRUE(std::is_nothrow_default_constructible<Optional<bool>>::value);
 }
 
+namespace {
+
 struct NoDestructor {};
 
 struct WithDestructor {
   ~WithDestructor();
 };
 
+} // namespace
+
 TEST(Optional, TriviallyDestructible) {
   // These could all be static_asserts but EXPECT_* give much nicer output on
   // failure.
@@ -690,4 +706,40 @@ TEST(Optional, Hash) {
   std::hash<Optional<int>>()(none);
   std::hash<Optional<int>>()(3);
 }
+
+namespace {
+
+struct WithConstMember {
+  /* implicit */ WithConstMember(int val) : x(val) {}
+  const int x;
+};
+
+// Make this opaque to the optimizer by preventing inlining.
+FOLLY_NOINLINE void replaceWith2(Optional<WithConstMember>& o) {
+  o.emplace(2);
+}
+
+} // namespace
+
+TEST(Optional, ConstMember) {
+  // Verify that the compiler doesn't optimize out the second load of
+  // o->x based on the assumption that the field is const.
+  //
+  // Current Optional implementation doesn't defend against that
+  // assumption, thus replacing an optional where the object has const
+  // members is technically UB and would require wrapping each access
+  // to the storage with std::launder, but this prevents useful
+  // optimizations.
+  //
+  // Implementations of std::optional in both libstdc++ and libc++ are
+  // subject to the same UB. It is then reasonable to believe that
+  // major compilers don't rely on the constness assumption.
+  Optional<WithConstMember> o(1);
+  int sum = 0;
+  sum += o->x;
+  replaceWith2(o);
+  sum += o->x;
+  EXPECT_EQ(sum, 3);
+}
+
 } // namespace folly