Bug 1287006 - Use |alignas(T) unsigned char mStorage[sizeof(T)]| instead of AlignedStorage2 inside Maybe. r=froydnj
authorJeff Walden <jwalden@mit.edu>
Mon, 30 Jan 2017 15:56:04 -0800
changeset 373492 e0f2e64261cc3e1a535b1500f13cefe7e32be2dd
parent 373491 c34603e73ded3e732e3a9423ca3ba0e157775cd5
child 373493 bc8c006751c6b4e526de1b15f4e8048f7a6cf8e6
push id10863
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 23:02:23 +0000
treeherdermozilla-aurora@0931190cd725 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1287006
milestone54.0a1
Bug 1287006 - Use |alignas(T) unsigned char mStorage[sizeof(T)]| instead of AlignedStorage2 inside Maybe. r=froydnj
mfbt/Maybe.h
mfbt/tests/TestMaybe.cpp
--- a/mfbt/Maybe.h
+++ b/mfbt/Maybe.h
@@ -78,31 +78,28 @@ struct Nothing { };
  *     unfortunately, it lacks equivalents of the type-inferred constructor
  *     functions |Some()| and |Nothing()|.
  *
  * N.B. GCC has missed optimizations with Maybe in the past and may generate
  * extra branches/loads/stores. Use with caution on hot paths; it's not known
  * whether or not this is still a problem.
  */
 template<class T>
-class Maybe
+class MOZ_NON_PARAM Maybe
 {
-  bool mIsSome;
+  alignas(T) unsigned char mStorage[sizeof(T)];
+  char mIsSome; // not bool -- guarantees minimal space consumption
 
-  // To support |Maybe<const Type>| we give |mStorage| the type |T| with any
-  // const-ness removed.  That allows us to |emplace()| an object into
-  // |mStorage|.  Since we treat the contained object as having type |T|
-  // everywhere else (both internally, and when exposed via public methods) the
-  // contained object is still treated as const once stored since |const| is
-  // part of |T|'s type signature.
-  typedef typename RemoveCV<T>::Type StorageType;
-  AlignedStorage2<StorageType> mStorage;
+  // GCC fails due to -Werror=strict-aliasing if |mStorage| is directly cast to
+  // T*.  Indirecting through these functions addresses the problem.
+  void* data() { return mStorage; }
+  const void* data() const { return mStorage; }
 
 public:
-  typedef T ValueType;
+  using ValueType = T;
 
   Maybe() : mIsSome(false) { }
   ~Maybe() { reset(); }
 
   MOZ_IMPLICIT Maybe(Nothing) : mIsSome(false) { }
 
   Maybe(const Maybe& aOther)
     : mIsSome(false)
@@ -326,23 +323,23 @@ public:
     MOZ_ASSERT(mIsSome);
     return ptr();
   }
 
   /* Returns the contents of this Maybe<T> by ref. Unsafe unless |isSome()|. */
   T& ref()
   {
     MOZ_ASSERT(mIsSome);
-    return *mStorage.addr();
+    return *static_cast<T*>(data());
   }
 
   const T& ref() const
   {
     MOZ_ASSERT(mIsSome);
-    return *mStorage.addr();
+    return *static_cast<const T*>(data());
   }
 
   /*
    * Returns the contents of this Maybe<T> by ref. If |isNothing()|, returns
    * the default value provided.
    */
   T& refOr(T& aDefault)
   {
@@ -454,17 +451,17 @@ public:
   /*
    * Constructs a T value in-place in this empty Maybe<T>'s storage. The
    * arguments to |emplace()| are the parameters to T's constructor.
    */
   template<typename... Args>
   void emplace(Args&&... aArgs)
   {
     MOZ_ASSERT(!mIsSome);
-    ::new (KnownNotNull, mStorage.addr()) T(Forward<Args>(aArgs)...);
+    ::new (KnownNotNull, data()) T(Forward<Args>(aArgs)...);
     mIsSome = true;
   }
 
   friend std::ostream&
   operator<<(std::ostream& aStream, const Maybe<T>& aMaybe)
   {
     if (aMaybe) {
       aStream << aMaybe.ref();
--- a/mfbt/tests/TestMaybe.cpp
+++ b/mfbt/tests/TestMaybe.cpp
@@ -5,16 +5,17 @@
 
 #include <utility>
 
 #include "mozilla/Assertions.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/Compiler.h"
 #include "mozilla/Maybe.h"
 #include "mozilla/Move.h"
+#include "mozilla/TemplateLib.h"
 #include "mozilla/Types.h"
 #include "mozilla/TypeTraits.h"
 #include "mozilla/UniquePtr.h"
 
 using mozilla::IsSame;
 using mozilla::Maybe;
 using mozilla::Move;
 using mozilla::Nothing;
@@ -773,17 +774,17 @@ TestVirtualFunction() {
 static Maybe<int*>
 ReturnSomeNullptr()
 {
   return Some(nullptr);
 }
 
 struct D
 {
-  explicit D(Maybe<int*>) {}
+  explicit D(const Maybe<int*>&) {}
 };
 
 static bool
 TestSomeNullptrConversion()
 {
   Maybe<int*> m1 = Some(nullptr);
   MOZ_RELEASE_ASSERT(m1.isSome());
   MOZ_RELEASE_ASSERT(m1);
@@ -811,17 +812,17 @@ static Maybe<Base*>
 ReturnDerivedPointer()
 {
   Derived* d = nullptr;
   return Some(d);
 }
 
 struct ExplicitConstructorBasePointer
 {
-  explicit ExplicitConstructorBasePointer(Maybe<Base*>) {}
+  explicit ExplicitConstructorBasePointer(const Maybe<Base*>&) {}
 };
 
 static bool
 TestSomePointerConversion()
 {
   Base base;
   Derived derived;
 
@@ -1060,16 +1061,31 @@ TestTypeConversion()
     dest = Move(src);
     MOZ_RELEASE_ASSERT(src.isNothing());
     MOZ_RELEASE_ASSERT(dest.isSome() && *dest == 2);
   }
 
   return true;
 }
 
+// These are quasi-implementation details, but we assert them here to prevent
+// backsliding to earlier times when Maybe<T> for smaller T took up more space
+// than T's alignment required.
+
+static_assert(sizeof(Maybe<char>) == 2 * sizeof(char),
+              "Maybe<char> shouldn't bloat at all ");
+static_assert(sizeof(Maybe<bool>) <= 2 * sizeof(bool),
+              "Maybe<bool> shouldn't bloat");
+static_assert(sizeof(Maybe<int>) <= 2 * sizeof(int),
+              "Maybe<int> shouldn't bloat");
+static_assert(sizeof(Maybe<long>) <= 2 * sizeof(long),
+              "Maybe<long> shouldn't bloat");
+static_assert(sizeof(Maybe<double>) <= 2 * sizeof(double),
+              "Maybe<double> shouldn't bloat");
+
 int
 main()
 {
   RUN_TEST(TestBasicFeatures);
   RUN_TEST(TestCopyAndMove);
   RUN_TEST(TestFunctionalAccessors);
   RUN_TEST(TestApply);
   RUN_TEST(TestMap);