Bug 1815177 - Use a custom ReadResult to reduce branching during IPDL deserialization, r=ipc-reviewers,mccr8

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

gfx/layers/ipc/LayersMessageUtils.h

@@ -1124,9 +1124,9 @@ struct ParamTraits<mozilla::layers::ZoomTarget> {
       MOZ_ASSERT(rv, "Serialize ##type_## failed");                         \
       WriteParam(aWriter, std::move(v));                                    \
     }                                                                       \
-    static mozilla::Maybe<paramType> Read(MessageReader* aReader) {         \
+    static ReadResult<paramType> Read(MessageReader* aReader) {             \
       mozilla::ipc::ByteBuf in;                                             \
-      mozilla::Maybe<paramType> result;                                     \
+      ReadResult<paramType> result;                                         \
       if (!ReadParam(aReader, &in) || !in.mData) {                          \
         return result;                                                      \
       }                                                                     \
@@ -1135,7 +1135,7 @@ struct ParamTraits<mozilla::layers::ZoomTarget> {
       if (!Servo_##type_##_Deserialize(&in, value.addr())) {                \
         return result;                                                      \
       }                                                                     \
-      result.emplace(std::move(*value.addr()));                             \
+      result = std::move(*value.addr());                                    \
       value.addr()->~paramType();                                           \
       return result;                                                        \
     }                                                                       \

ipc/chromium/src/chrome/common/ipc_message_utils.h

@@ -208,6 +208,156 @@ class MOZ_STACK_CLASS MessageReader final {
   mozilla::ipc::IProtocol* actor_;
 };
 
+namespace detail {
+
+// Helper for checking `T::kHasDeprecatedReadParamPrivateConstructor` using a
+// fallback when the member isn't defined.
+template <typename T>
+inline constexpr auto HasDeprecatedReadParamPrivateConstructor(int)
+    -> decltype(T::kHasDeprecatedReadParamPrivateConstructor) {
+  return T::kHasDeprecatedReadParamPrivateConstructor;
+}
+
+template <typename T>
+inline constexpr bool HasDeprecatedReadParamPrivateConstructor(...) {
+  return false;
+}
+
+}  // namespace detail
+
+/**
+ * Result type returned from some `ParamTraits<T>::Read` implementations, and
+ * from `IPC::ReadParam<T>(MessageReader*)`. Either contains the value or
+ * indicates a failure to deserialize.
+ *
+ * This type can be thought of as a variant on `Maybe<T>`, except that it
+ * unconditionally constructs the underlying value if it is default
+ * constructible. This helps keep code size down, especially when calling
+ * outparameter-based ReadParam implementations (bug 1815177).
+ */
+template <typename T,
+          bool = std::is_default_constructible_v<T> ||
+                 detail::HasDeprecatedReadParamPrivateConstructor<T>(0)>
+class ReadResult {
+ public:
+  ReadResult() = default;
+
+  template <typename U, std::enable_if_t<std::is_convertible_v<U, T>, int> = 0>
+  MOZ_IMPLICIT ReadResult(U&& aData)
+      : mIsOk(true), mData(std::forward<U>(aData)) {}
+
+  template <typename... Args>
+  explicit ReadResult(std::in_place_t, Args&&... aArgs)
+      : mIsOk(true), mData(std::forward<Args>(aArgs)...) {}
+
+  ReadResult(const ReadResult&) = default;
+  ReadResult(ReadResult&&) = default;
+
+  template <typename U, std::enable_if_t<std::is_convertible_v<U, T>, int> = 0>
+  MOZ_IMPLICIT ReadResult& operator=(U&& aData) {
+    mIsOk = true;
+    mData = std::forward<U>(aData);
+    return *this;
+  }
+
+  ReadResult& operator=(const ReadResult&) = default;
+  ReadResult& operator=(ReadResult&&) noexcept = default;
+
+  // Check if the ReadResult contains a valid value.
+  explicit operator bool() const { return isOk(); }
+  bool isOk() const { return mIsOk; }
+
+  // Get the data from this ReadResult.
+  T& get() {
+    MOZ_ASSERT(mIsOk);
+    return mData;
+  }
+  const T& get() const {
+    MOZ_ASSERT(mIsOk);
+    return mData;
+  }
+
+  T& operator*() { return get(); }
+  const T& operator*() const { return get(); }
+
+  T* operator->() { return &get(); }
+  const T* operator->() const { return &get(); }
+
+  // Try to extract a `Maybe<T>` from this ReadResult.
+  mozilla::Maybe<T> TakeMaybe() {
+    if (mIsOk) {
+      mIsOk = false;
+      return mozilla::Some(std::move(mData));
+    }
+    return mozilla::Nothing();
+  }
+
+  // Get the underlying data from this ReadResult, even if not OK.
+  //
+  // This is only available for types which are default constructible, and is
+  // used to optimize old-style `ReadParam` calls.
+  T& GetStorage() { return mData; }
+
+  // Compliment to `GetStorage` used to set the ReadResult into an OK state
+  // without constructing the underlying value.
+  void SetOk(bool aIsOk) { mIsOk = aIsOk; }
+
+ private:
+  bool mIsOk = false;
+  T mData{};
+};
+
+template <typename T>
+class ReadResult<T, false> {
+ public:
+  ReadResult() = default;
+
+  template <typename U, std::enable_if_t<std::is_convertible_v<U, T>, int> = 0>
+  MOZ_IMPLICIT ReadResult(U&& aData)
+      : mData(std::in_place, std::forward<U>(aData)) {}
+
+  template <typename... Args>
+  explicit ReadResult(std::in_place_t, Args&&... aArgs)
+      : mData(std::in_place, std::forward<Args>(aArgs)...) {}
+
+  ReadResult(const ReadResult&) = default;
+  ReadResult(ReadResult&&) = default;
+
+  template <typename U, std::enable_if_t<std::is_convertible_v<U, T>, int> = 0>
+  MOZ_IMPLICIT ReadResult& operator=(U&& aData) {
+    mData.reset();
+    mData.emplace(std::forward<U>(aData));
+    return *this;
+  }
+
+  ReadResult& operator=(const ReadResult&) = default;
+  ReadResult& operator=(ReadResult&&) noexcept = default;
+
+  // Check if the ReadResult contains a valid value.
+  explicit operator bool() const { return isOk(); }
+  bool isOk() const { return mData.isSome(); }
+
+  // Get the data from this ReadResult.
+  T& get() { return mData.ref(); }
+  const T& get() const { return mData.ref(); }
+
+  T& operator*() { return get(); }
+  const T& operator*() const { return get(); }
+
+  T* operator->() { return &get(); }
+  const T* operator->() const { return &get(); }
+
+  // Try to extract a `Maybe<T>` from this ReadResult.
+  mozilla::Maybe<T> TakeMaybe() { return std::move(mData); }
+
+  // These methods are only available if the type is default constructible.
+  T& GetStorage() = delete;
+  void SetOk(bool aIsOk) = delete;
+
+ private:
+  mozilla::Maybe<T> mData;
+};
+
 //-----------------------------------------------------------------------------
 // An iterator class for reading the fields contained within a Message.
 
@@ -313,33 +463,24 @@ template <typename P>
 inline bool WARN_UNUSED_RESULT ReadParam(MessageReader* reader, P* p) {
   if constexpr (!detail::ParamTraitsReadUsesOutParam<P>()) {
     auto maybe = ParamTraits<P>::Read(reader);
-    if (maybe.isNothing()) {
+    if (maybe) {
-      return false;
+      *p = std::move(*maybe);
+      return true;
     }
-    *p = maybe.extract();
+    return false;
-    return true;
   } else {
     return ParamTraits<P>::Read(reader, p);
   }
 }
 
 template <typename P>
-inline mozilla::Maybe<P> WARN_UNUSED_RESULT ReadParam(MessageReader* reader) {
+inline ReadResult<P> WARN_UNUSED_RESULT ReadParam(MessageReader* reader) {
   if constexpr (!detail::ParamTraitsReadUsesOutParam<P>()) {
     return ParamTraits<P>::Read(reader);
-  } else if constexpr (std::is_default_constructible_v<P>) {
-    mozilla::Maybe<P> p{std::in_place};
-    if (!ParamTraits<P>::Read(reader, p.ptr())) {
-      p.reset();
-    }
-    return p;
   } else {
-    static_assert(P::kHasDeprecatedReadParamPrivateConstructor);
+    ReadResult<P> p;
-    P p{};
+    p.SetOk(ParamTraits<P>::Read(reader, &p.GetStorage()));
-    if (!ParamTraits<P>::Read(reader, &p)) {
+    return p;
-      return mozilla::Nothing();
-    }
-    return mozilla::Some(std::move(p));
   }
 }
 
@@ -507,7 +648,7 @@ bool ReadSequenceParamImpl(MessageReader* reader, mozilla::Maybe<I>&& data,
     if (!elt) {
       return false;
     }
-    *data.ref() = elt.extract();
+    *data.ref() = std::move(*elt);
     ++data.ref();
   }
   return true;
@@ -920,16 +1061,16 @@ struct ParamTraitsMozilla<mozilla::NotNull<T>> {
     ParamTraits<T>::Write(writer, p.get());
   }
 
-  static mozilla::Maybe<mozilla::NotNull<T>> Read(MessageReader* reader) {
+  static ReadResult<mozilla::NotNull<T>> Read(MessageReader* reader) {
     auto ptr = ReadParam<T>(reader);
     if (!ptr) {
-      return mozilla::Nothing();
+      return {};
     }
     if (!*ptr) {
       reader->FatalError("unexpected null value");
-      return mozilla::Nothing();
+      return {};
     }
-    return mozilla::Some(mozilla::WrapNotNull(std::move(*ptr)));
+    return mozilla::WrapNotNull(std::move(*ptr));
   }
 };
 

ipc/glue/IPCForwards.h

@@ -22,14 +22,17 @@ namespace IPC {
 class Message;
 class MessageReader;
 class MessageWriter;
-template <typename P>
+template <typename T, bool>
-inline mozilla::Maybe<P> ReadParam(MessageReader*);
+class ReadResult;
 }  // namespace IPC
 
 // TODO(bug 1812271): Remove users of this macro.
 #define ALLOW_DEPRECATED_READPARAM                           \
+ public:                                                     \
   enum { kHasDeprecatedReadParamPrivateConstructor = true }; \
-  template <typename P>                                      \
+  template <typename, bool>                                  \
-  friend mozilla::Maybe<P> IPC::ReadParam(IPC::MessageReader*);
+  friend class IPC::ReadResult;                              \
+                                                             \
+ private:
 
 #endif

ipc/glue/IPCMessageUtilsSpecializations.h

@@ -458,7 +458,7 @@ struct ParamTraits<mozilla::Maybe<T>> {
       return false;
     }
     if (isSome) {
-      mozilla::Maybe<T> tmp = ReadParam<T>(reader);
+      mozilla::Maybe<T> tmp = ReadParam<T>(reader).TakeMaybe();
       if (!tmp) {
         return false;
       }

ipc/glue/SideVariant.h

@@ -161,16 +161,24 @@ struct ParamTraits<mozilla::ipc::SideVariant<ParentSide, ChildSide>> {
     }
   }
 
-  static mozilla::Maybe<paramType> Read(IPC::MessageReader* aReader) {
+  static ReadResult<paramType> Read(IPC::MessageReader* aReader) {
     if (!aReader->GetActor()) {
       aReader->FatalError("actor required to deserialize this type");
-      return mozilla::Nothing();
+      return {};
     }
 
     if (aReader->GetActor()->GetSide() == mozilla::ipc::ParentSide) {
-      return ReadParam<ParentSide>(aReader);
+      auto parentSide = ReadParam<ParentSide>(aReader);
+      if (!parentSide) {
+        return {};
+      }
+      return std::move(*parentSide);
     }
-    return ReadParam<ChildSide>(aReader);
+    auto childSide = ReadParam<ChildSide>(aReader);
+    if (!childSide) {
+      return {};
+    }
+    return std::move(*childSide);
   }
 };
 

ipc/ipdl/ipdl/cxx/ast.py

@@ -855,16 +855,6 @@ class ExprMove(ExprCall):
         ExprCall.__init__(self, ExprVar("std::move"), args=[arg])
 
 
-class ExprNothing(ExprCall):
-    def __init__(self):
-        ExprCall.__init__(self, ExprVar("mozilla::Nothing"))
-
-
-class ExprSome(ExprCall):
-    def __init__(self, arg):
-        ExprCall.__init__(self, ExprVar("mozilla::Some"), args=[arg])
-
-
 class ExprNew(Node):
     # XXX taking some poetic license ...
     def __init__(self, ctype, args=[], newargs=None):

ipc/ipdl/ipdl/lower.py

@@ -317,6 +317,16 @@ def _cxxMaybeType(basetype, const=False, ref=False):
     )
 
 
+def _cxxReadResultType(basetype, const=False, ref=False):
+    return Type(
+        "IPC::ReadResult",
+        T=basetype,
+        const=const,
+        ref=ref,
+        hasimplicitcopyctor=basetype.hasimplicitcopyctor,
+    )
+
+
 def _cxxNotNullType(basetype, const=False, ref=False):
     return Type(
         "mozilla::NotNull",
@@ -462,6 +472,10 @@ def errfnUnreachable(msg):
     return [_logicError(msg)]
 
 
+def readResultError():
+    return ExprCode("{}")
+
+
 class _DestroyReason:
     @staticmethod
     def Type():
@@ -2016,14 +2030,14 @@ class _ParamTraits:
         ifbad = StmtIf(ExprNot(readbytes))
         errmsg = "Error bulk reading fields from %s" % first.ipdltype.name()
         ifbad.addifstmts(
-            [cls.fatalError(cls.readervar, errmsg), StmtReturn(ExprNothing())]
+            [cls.fatalError(cls.readervar, errmsg), StmtReturn(readResultError())]
         )
         block.addstmt(ifbad)
         block.addstmts(
             cls.readSentinel(
                 cls.readervar,
                 cls.bulkSentinelKey(fields),
-                errfnSentinel(ExprNothing())(errmsg),
+                errfnSentinel(readResultError())(errmsg),
             )
         )
 
@@ -2074,7 +2088,7 @@ class _ParamTraits:
     @classmethod
     def _checkedRead(cls, ipdltype, cxxtype, var, sentinelKey, what):
         def errfn(msg):
-            return [cls.fatalError(cls.readervar, msg), StmtReturn(ExprNothing())]
+            return [cls.fatalError(cls.readervar, msg), StmtReturn(readResultError())]
 
         return cls.checkedRead(
             ipdltype,
@@ -2084,7 +2098,7 @@ class _ParamTraits:
             errfn=errfn,
             paramtype=what,
             sentinelKey=sentinelKey,
-            errfnSentinel=errfnSentinel(ExprNothing()),
+            errfnSentinel=errfnSentinel(readResultError()),
         )
 
     @classmethod
@@ -2119,15 +2133,14 @@ class _ParamTraits:
         writemthd.addstmts(write)
         pt.addstmt(writemthd)
 
-        # static Maybe<T> Read(MessageReader*);
+        # static ReadResult<T> Read(MessageReader*);
-        outtype = Type("paramType", ptr=True)
         readmthd = MethodDefn(
             MethodDecl(
                 "Read",
                 params=[
                     Decl(Type("IPC::MessageReader", ptr=True), cls.readervar.name),
                 ],
-                ret=Type("mozilla::Maybe<paramType>"),
+                ret=Type("IPC::ReadResult<paramType>"),
                 methodspec=MethodSpec.STATIC,
             )
         )
@@ -2185,9 +2198,12 @@ class _ParamTraits:
             MOZ_RELEASE_ASSERT(
                 ${readervar}->GetActor(),
                 "Cannot deserialize managed actors without an actor");
-            return ${readervar}->GetActor()
+            mozilla::Maybe<mozilla::ipc::IProtocol*> actor = ${readervar}->GetActor()
-              ->ReadActor(${readervar}, true, ${actortype}, ${protocolid})
+              ->ReadActor(${readervar}, true, ${actortype}, ${protocolid});
-              .map([](mozilla::ipc::IProtocol* actor) { return static_cast<${cxxtype}>(actor); });
+            if (actor.isSome()) {
+                return static_cast<${cxxtype}>(actor.ref());
+            }
+            return {};
             """,
             readervar=cls.readervar,
             actortype=ExprLiteral.String(actortype.name()),
@@ -2249,7 +2265,7 @@ class _ParamTraits:
         resultvar = ExprVar("result__")
         read.append(
             StmtDecl(
-                Decl(_cxxMaybeType(Type("paramType")), resultvar.name),
+                Decl(_cxxReadResultType(Type("paramType")), resultvar.name),
                 initargs=[ExprVar("std::in_place")] + ctorargs,
             )
         )
@@ -2333,7 +2349,7 @@ class _ParamTraits:
                         origenum,
                         "variant " + origenum + " of union " + uniontype.name(),
                     ),
-                    StmtReturn(ExprSome(ExprMove(tmpvar))),
+                    StmtReturn(ExprMove(tmpvar)),
                 ]
             )
             readswitch.addcase(caselabel, readcase)
@@ -2357,7 +2373,7 @@ class _ParamTraits:
                     cls.fatalError(
                         cls.readervar, "unknown variant of union " + uniontype.name()
                     ),
-                    StmtReturn(ExprNothing()),
+                    StmtReturn(readResultError()),
                 ]
             ),
         )