1479930 - Improve IPDL support for passing types by move reference

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P1)

Description

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

This is inspired by ipc::FileDescriptor, which I was refactoring and tried to add some methods to allow avoiding the unnecessary dup+close we're doing a lot of, except that that wasn't very useful because IPC makes everything pass them by const reference.  But this is more general.

We sort of have support for this: ipc::Shmem, which is logically a resource that's transferrable but non-copiable, is passed to Recv*/Answer* by rvalue reference and to Send* by lvalue reference (!, bug 1441651).  Endpoint<>s are similar, but they tolerate the constness by duplicating the underlying fd instead and hoping nothing goes wrong.

Other things we might want:

* FileDescriptor; probably not a noticeable efficiency win, but it's a papercut and if we can properly support Shmem/Endpoint it can come along for the ride.

* Sending arrays of primitive types by move; in principle they could be adopted into the BufferList (if they're large enough).  We already receive them by move.

* Strings, maybe; that would introduce unnecessary copies on the sending side for dependent/literal strings.  Maybe implicit conversion to a dynamically tagged type (cf. Rust's Cow) could fix that.  This might need to be a separate bug, assuming it's even a good idea in the first place.

** Receiving strings by move could be useful, assuming we don't have refcounted/CoW copying; I notice a REFCOUNTED flag but it doesn't seem to be used much.

* Structs/unions containing any of these; I don't know if this should be automatic or opt-in (to reduce code churn).  Currently, fields of type Shmem are effectively `mutable` (the accessor does a const_cast) and using them for anything depends on this.


(Aside: If/where there's an important difference between send/receive we'll want to make sure we don't get the parameter modes backwards for the promise/resolver case; I tried experimentally changing Recv/Answer to use && by default and it also affected the values being sent back that way, which seems wrong.  Also, there is code that calls Recv/Answer methods directly and would need to be fixed to move the arguments, but that's fixable.)

Comment 1

[Nathan Froyd [:froydnj]]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #0)
> * Strings, maybe; that would introduce unnecessary copies on the sending
> side for dependent/literal strings.  Maybe implicit conversion to a
> dynamically tagged type (cf. Rust's Cow) could fix that.  This might need to
> be a separate bug, assuming it's even a good idea in the first place.
> 
> ** Receiving strings by move could be useful, assuming we don't have
> refcounted/CoW copying; I notice a REFCOUNTED flag but it doesn't seem to be
> used much.

We do have refcounted, CoW string buffers.  Avoiding needless atomic refcounting is still nice, though.  (Relatedly, IPDL Recv methods should take proper nsA*String references, so we could pass in dependent strings: we wouldn't need to allocate copies unless absolutely necessary--though it probably is necessary quite often--and the allocation would happen somewhere inside the implementation code, rather than somewhere inside the IPDL machinery, so allocation profilers like DMD are slightly more useful.)

Comment 2

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

Attachment: WIP

This is a rough draft of solving some of these problems; I wrote it while hacking on bug 1480205, because I wanted move-only types for Mach IPC endpoints.

It adds `using moveonly <type> from <file>` to the syntax, generalizing the earlier `using refcounted` into a notion of “lifetime” (maybe needs a better name), and treats those types roughly like `Shmem`: passed to Recv methods as T&& and to Send methods as T& (I'd prefer T&& there; this part could use more work), and including the hack where IPDL struct accessors const_cast the field so it can be moved/swapped out despite the struct being Recv'd by const reference.  The type still needs to be default-constructible, which generally means adding a null value; that's a known problem with IPDL in general.

This patch doesn't allow `moveonly` annotations on IPDL structs/unions, but that should be possible to add (the new `lifetime` method is defined for all types).

Comment 3

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

Related work: one of the proposed patches in bug 1477756 adds IPDL support for UniquePtr instances, in order to pass endpoints for a new interprocess single-producer/single-consumer queue.  It reuses the current `using` syntax and special-cases `UniquePtr` instances (because the normal code generation for imported types doesn't work with template instances), and adds an overload for Send methods with `paramsems = 'move'` (enabled only when there's a UniquePtr in the arguments, but the condition is factored out so it could be expanded).

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1479930 - std::move() almost all arguments to Recv/Answer methods in IPC; r?froydnj

The one exception is refcounted types, because std::move(RefPtr<T>) does not
coerce to T*, which is what the current IPC methods accept.

This does not rewrite all Recv/Answer methods to take advantage of move
semantics.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/943193dd891e
std::move() almost all arguments to Recv/Answer methods in IPC; r=froydnj

Comment 6

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/943193dd891e