1664362 - IPDL's binding code doesn't use move semantics in many cases.

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Jean-Yves Avenard [:jya]]

Trying to fix bug 1663227; we have to write an image container that would be usable over IPC and would manage the lifecycle of the image (which lives in the in GPU or RDD process). To be able to handle that task, we need to move the object; it can't be copied.

When sending those images over IPC we send them in an array of RemoteVideoDataIPDL objects.

Those are returned wrapped in a MozPromise resolved.

Issue 1:

• The IPC MozPromise resolver doesn't use move semantics, instead it takes the object to be returned as a const reference which forces a copy.
  the code generated is:

            DecodeResolver resolver = [this, self__, id__, seqno__](const DecodeResultIPDL& aParam) {
                if ((!(self__))) {
                    NS_WARNING("Not resolving response because actor is dead.");
                    return;
                }
                bool resolve__ = true;
                DecodeResultIPDL result;
                result = std::move(aParam);
                IPC::Message* reply__ = PRemoteDecoder::Reply_Decode(id__);
                WriteIPDLParam(reply__, self__, resolve__);
                // Sentinel = 'resolve__'
                (reply__)->WriteSentinel(322044863);
                WriteIPDLParam(reply__, self__, result);
                // Sentinel = 'result'
                (reply__)->WriteSentinel(153223840);
                (reply__)->set_seqno(seqno__);

                if (mozilla::ipc::LoggingEnabledFor("PRemoteDecoderParent")) {
                    mozilla::ipc::LogMessageForProtocol(
                        "PRemoteDecoderParent",
                        OtherPid(),
                        "Sending reply ",
                        reply__->type(),
                        mozilla::ipc::MessageDirection::eSending);
                }
                bool sendok__ = ChannelSend(reply__);
                if ((!(sendok__))) {
                    NS_WARNING("Error sending reply");
                }
            };

while it pass a const reference, it still copies the content into a temporary variable (using a useless move)

• Issue 2.
  The IPC bindings generated for arrays also use const references rather than move semantics. As such object in the arrays are always copied rather than moved.

Comment 1

[Simon Giesecke [:sg] [he/him]]

Bug 1572054 is another aspect of missing use of move semantics. Not sure if that's the same thing you meant by Issue 2.

Comment 2

[Jean-Yves Avenard [:jya]]

Need this to continue bug 1663227; so will give it a shot.

Comment 3

[Jean-Yves Avenard [:jya]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #1)

Bug 1572054 is another aspect of missing use of move semantics. Not sure if that's the same thing you meant by Issue 2.

yes, this is one of the issue I've seen.

Right now, I only need the 1st one sorted out. I'll see how I go about it.

Comment 4

[Jean-Yves Avenard [:jya]]

For the above code, the IPDL binding could generate the following instead:

DecodeResolver resolver = [this, self__, id__, seqno__](auto&& aParam) {
                if ((!(self__))) {
                    NS_WARNING("Not resolving response because actor is dead.");
                    return;
                }
                bool resolve__ = true;
                DecodeResultIPDL result = std::forward<decltype(aParam)>(aParam);
                IPC::Message* reply__ = PRemoteDecoder::Reply_Decode(id__);
                WriteIPDLParam(reply__, self__, resolve__);
                // Sentinel = 'resolve__'
                (reply__)->WriteSentinel(322044863);
                WriteIPDLParam(reply__, self__, result);
                // Sentinel = 'result'
                (reply__)->WriteSentinel(153223840);
                (reply__)->set_seqno(seqno__);

                if (mozilla::ipc::LoggingEnabledFor("PRemoteDecoderParent")) {
                    mozilla::ipc::LogMessageForProtocol(
                        "PRemoteDecoderParent",
                        OtherPid(),
                        "Sending reply ",
                        reply__->type(),
                        mozilla::ipc::MessageDirection::eSending);
                }
                bool sendok__ = ChannelSend(reply__);
                if ((!(sendok__))) {
                    NS_WARNING("Error sending reply");
                }
            };

using C++14 auto&& in lambdas in combination with std::forward to a declared temporary variable to force the input type.

Comment 5

[Simon Giesecke [:sg] [he/him]]

That's not that hard to change then, since no additional knowledge on the type is required.

The lambda parameter type is generated at https://searchfox.org/mozilla-central/rev/eb9d5c97927aea75f0c8e38bbc5b5d288099e687/ipc/ipdl/ipdl/lower.py#4520-4528 and could be pulled out of the if it will be auto&& in all cases now.

The result assignment is generated at https://searchfox.org/mozilla-central/rev/eb9d5c97927aea75f0c8e38bbc5b5d288099e687/ipc/ipdl/ipdl/lower.py#4539-4541. There's just no ExprForward yet, which should probably be added.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1664362. Use move semantics with IPC's MozPromise resolver. r?nika

We use C++14's generic lambdas and its auto&& type in the generated code, in combination with a typed local variable to ensure the argument type is enforced.

The object is moved as necessary, no copies will occur.

Comment 7

[Jean-Yves Avenard [:jya]]

Any chance we can move on this?

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Reassigned review to Nika, who knows this code better.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b84a8128b70b
Use move semantics with IPC's MozPromise resolver. r=nika

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b84a8128b70b