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 344386 e0f2e64261cc3e1a535b1500f13cefe7e32be2dd
parent 344385 c34603e73ded3e732e3a9423ca3ba0e157775cd5
child 344387 bc8c006751c6b4e526de1b15f4e8048f7a6cf8e6
push id87350
push userjwalden@mit.edu
push dateThu, 23 Feb 2017 04:15:19 +0000
treeherdermozilla-inbound@e0f2e64261cc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1287006
milestone54.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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);