Bug 1661428 - Use CompactPair instead of Variant for trivially default-constructible V and UnusedZero E. r=froydnj

It seems that the clang toolchain has trouble optimizing away the tag access in
a `mozilla::Variant`. Use of a `CompactPair` seems to be better when `E` is
small (all cases where `UnusedZero<E>` is true are small right now anyway).
While this doesn't change `sizeof(Result<V, E>)` in most cases, it makes
the generated code simpler/smaller: As of now, this reduce the overall VM size
of libxul.so by ca. 59K.

Without additional effort, this requires and leads to default-construction of
a `V` even in cases where it is never accessed, so this is restricted to
trivially default-constructible `V` for now.

This could be avoided by replacing  `CompactPair<V, E>` by
`CompactPair<AlignedStorage2<V>, E>`.

Differential Revision: https://phabricator.services.mozilla.com/D88393

dom/indexedDB/IDBResult.h

@@ -28,8 +28,9 @@ struct OkType final {
   T mValue;
 };
 
+// Not final to allow use with mozilla::CompactPair.
 template <>
-struct OkType<void> final {};
+struct OkType<void> {};
 
 template <IDBSpecialValue Value>
 using SpecialConstant = std::integral_constant<IDBSpecialValue, Value>;

mfbt/Result.h

@@ -13,6 +13,7 @@
 #include "mozilla/Alignment.h"
 #include "mozilla/Assertions.h"
 #include "mozilla/Attributes.h"
+#include "mozilla/CompactPair.h"
 #include "mozilla/Types.h"
 #include "mozilla/Variant.h"
 
@@ -94,51 +95,64 @@ class ResultImplementation<V, E&, PackingStrategy::Variant> {
   const E& inspectErr() const { return *mStorage.template as<E*>(); }
 };
 
+// XXX For NullIsOk, we don't actually need default-constructibility. If V is
+// not, we just couldn't use a CompactPair<V, ...> but would need to use
+// uninitialized storage, which is much more complex (but would also be more
+// efficient if default construction of V is complex).
+
 /**
- * Specialization for when the success type is Ok (or another empty class) and
- * the error type is a reference.
+ * Specialization for when the success type is default-constructible and the
+ * error type is a reference.
  */
 template <typename V, typename E>
 class ResultImplementation<V, E&, PackingStrategy::NullIsOk> {
-  E* mErrorValue;
+  CompactPair<V, E*> mValue;
 
  public:
-  explicit ResultImplementation(V) : mErrorValue(nullptr) {}
-  explicit ResultImplementation(E& aErrorValue) : mErrorValue(&aErrorValue) {}
+  explicit ResultImplementation(const V& aSuccessValue)
+      : mValue(aSuccessValue, nullptr) {}
+  explicit ResultImplementation(V&& aSuccessValue)
+      : mValue(std::move(aSuccessValue), nullptr) {}
+  explicit ResultImplementation(E& aErrorValue) : mValue(V{}, &aErrorValue) {}
 
-  bool isOk() const { return mErrorValue == nullptr; }
+  bool isOk() const { return mValue.second() == nullptr; }
 
-  const V& inspect() const = delete;
-  V unwrap() { return V(); }
+  const V& inspect() const { return mValue.first(); }
+  V unwrap() { return std::move(mValue.first()); }
 
-  const E& inspectErr() const { return *mErrorValue; }
-  E& unwrapErr() { return *mErrorValue; }
+  const E& inspectErr() const { return *mValue.second(); }
+  E& unwrapErr() { return *mValue.second(); }
 };
 
 /**
- * Specialization for when the success type is Ok (or another empty class) and
- * the error type is a value type which can never have the value 0 (as
- * determined by UnusedZero<>).
+ * Specialization for when the success type is default-constructible and the
+ * error type is a value type which can never have the value 0 (as determined by
+ * UnusedZero<>).
  */
 template <typename V, typename E>
 class ResultImplementation<V, E, PackingStrategy::NullIsOk> {
   static constexpr E NullValue = E(0);
 
-  E mErrorValue;
+  CompactPair<V, E> mValue;
+
+  static_assert(std::is_trivially_copyable_v<E>);
 
  public:
-  explicit ResultImplementation(V) : mErrorValue(NullValue) {}
-  explicit ResultImplementation(E aErrorValue) : mErrorValue(aErrorValue) {
+  explicit ResultImplementation(const V& aSuccessValue)
+      : mValue(aSuccessValue, NullValue) {}
+  explicit ResultImplementation(V&& aSuccessValue)
+      : mValue(std::move(aSuccessValue), NullValue) {}
+  explicit ResultImplementation(E aErrorValue) : mValue(V{}, aErrorValue) {
     MOZ_ASSERT(aErrorValue != NullValue);
   }
 
-  bool isOk() const { return mErrorValue == NullValue; }
+  bool isOk() const { return mValue.second() == NullValue; }
 
-  const V& inspect() const = delete;
-  V unwrap() { return V(); }
+  const V& inspect() const { return mValue.first(); }
+  V unwrap() { return std::move(mValue.first()); }
 
-  const E& inspectErr() const { return mErrorValue; }
-  E unwrapErr() { return std::move(mErrorValue); }
+  const E& inspectErr() const { return mValue.second(); }
+  E unwrapErr() { return std::move(mValue.second()); }
 };
 
 /**
@@ -267,17 +281,18 @@ struct HasFreeLSB<T&> {
 template <typename V, typename E>
 struct SelectResultImpl {
   static const PackingStrategy value =
-      (std::is_empty_v<V> && UnusedZero<E>::value)
-          ? PackingStrategy::NullIsOk
-          : (detail::HasFreeLSB<V>::value && detail::HasFreeLSB<E>::value)
-                ? PackingStrategy::LowBitTagIsError
+      (HasFreeLSB<V>::value && HasFreeLSB<E>::value)
+          ? PackingStrategy::LowBitTagIsError
+          : (std::is_trivially_default_constructible_v<V> &&
+             UnusedZero<E>::value && sizeof(E) <= sizeof(uintptr_t))
+                ? PackingStrategy::NullIsOk
                 : (std::is_default_constructible_v<V> &&
                    std::is_default_constructible_v<E> &&
                    IsPackableVariant<V, E>::value)
                       ? PackingStrategy::PackedVariant
                       : PackingStrategy::Variant;
 
-  using Type = detail::ResultImplementation<V, E, value>;
+  using Type = ResultImplementation<V, E, value>;
 };
 
 template <typename T>

mfbt/tests/TestResult.cpp

@@ -14,10 +14,26 @@ using mozilla::Ok;
 using mozilla::Result;
 using mozilla::UniquePtr;
 
+enum struct UnusedZeroEnum : int32_t { Ok = 0, NotOk = 1 };
+
+namespace mozilla::detail {
+template <>
+struct UnusedZero<UnusedZeroEnum> {
+  static const bool value = true;
+};
+}  // namespace mozilla::detail
+
 struct Failed {
   int x;
 };
 
+// V is trivially default-constructible, and E has UnusedZero<E>::value == true,
+// for a reference type and for a non-reference type
+static_assert(mozilla::detail::SelectResultImpl<uintptr_t, Failed&>::value ==
+              mozilla::detail::PackingStrategy::NullIsOk);
+static_assert(mozilla::detail::SelectResultImpl<Ok, UnusedZeroEnum>::value ==
+              mozilla::detail::PackingStrategy::NullIsOk);
+
 static_assert(sizeof(Result<Ok, Failed&>) == sizeof(uintptr_t),
               "Result with empty value type should be pointer-sized");
 static_assert(sizeof(Result<int*, Failed&>) == sizeof(uintptr_t),
@@ -56,6 +72,10 @@ static GenericErrorResult<Failed&> Fail() {
   return Err<Failed&>(failed);
 }
 
+static GenericErrorResult<UnusedZeroEnum> FailUnusedZeroEnum() {
+  return Err(UnusedZeroEnum::NotOk);
+}
+
 static Result<Ok, Failed&> Task1(bool pass) {
   if (!pass) {
     return Fail();  // implicit conversion from GenericErrorResult to Result
@@ -63,12 +83,27 @@ static Result<Ok, Failed&> Task1(bool pass) {
   return Ok();
 }
 
+static Result<Ok, UnusedZeroEnum> Task1UnusedZeroEnumErr(bool pass) {
+  if (!pass) {
+    return FailUnusedZeroEnum();  // implicit conversion from GenericErrorResult
+                                  // to Result
+  }
+  return Ok();
+}
+
 static Result<int, Failed&> Task2(bool pass, int value) {
   MOZ_TRY(
       Task1(pass));  // converts one type of result to another in the error case
   return value;      // implicit conversion from T to Result<T, E>
 }
 
+static Result<int, UnusedZeroEnum> Task2UnusedZeroEnumErr(bool pass,
+                                                          int value) {
+  MOZ_TRY(Task1UnusedZeroEnumErr(
+      pass));    // converts one type of result to another in the error case
+  return value;  // implicit conversion from T to Result<T, E>
+}
+
 static Result<int, Failed&> Task3(bool pass1, bool pass2, int value) {
   int x, y;
   MOZ_TRY_VAR(x, Task2(pass1, value));
@@ -82,6 +117,15 @@ static void BasicTests() {
   MOZ_RELEASE_ASSERT(!Task1(false).isOk());
   MOZ_RELEASE_ASSERT(Task1(false).isErr());
 
+  MOZ_RELEASE_ASSERT(Task1UnusedZeroEnumErr(true).isOk());
+  MOZ_RELEASE_ASSERT(!Task1UnusedZeroEnumErr(true).isErr());
+  MOZ_RELEASE_ASSERT(!Task1UnusedZeroEnumErr(false).isOk());
+  MOZ_RELEASE_ASSERT(Task1UnusedZeroEnumErr(false).isErr());
+  MOZ_RELEASE_ASSERT(UnusedZeroEnum::NotOk ==
+                     Task1UnusedZeroEnumErr(false).inspectErr());
+  MOZ_RELEASE_ASSERT(UnusedZeroEnum::NotOk ==
+                     Task1UnusedZeroEnumErr(false).unwrapErr());
+
   // MOZ_TRY works.
   MOZ_RELEASE_ASSERT(Task2(true, 3).isOk());
   MOZ_RELEASE_ASSERT(Task2(true, 3).unwrap() == 3);
@@ -89,6 +133,12 @@ static void BasicTests() {
   MOZ_RELEASE_ASSERT(Task2(false, 3).isErr());
   MOZ_RELEASE_ASSERT(Task2(false, 3).unwrapOr(6) == 6);
 
+  MOZ_RELEASE_ASSERT(Task2UnusedZeroEnumErr(true, 3).isOk());
+  MOZ_RELEASE_ASSERT(Task2UnusedZeroEnumErr(true, 3).unwrap() == 3);
+  MOZ_RELEASE_ASSERT(Task2UnusedZeroEnumErr(true, 3).unwrapOr(6) == 3);
+  MOZ_RELEASE_ASSERT(Task2UnusedZeroEnumErr(false, 3).isErr());
+  MOZ_RELEASE_ASSERT(Task2UnusedZeroEnumErr(false, 3).unwrapOr(6) == 6);
+
   // MOZ_TRY_VAR works.
   MOZ_RELEASE_ASSERT(Task3(true, true, 3).isOk());
   MOZ_RELEASE_ASSERT(Task3(true, true, 3).unwrap() == 6);