Closed Bug 1121673 Opened 5 years ago Closed 5 years ago

Use move references in IPDL

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: khuey, Assigned: khuey)

References

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
This doesn't change consumers, just method signatures.  Some of the consumers could later be changed to take advantage of this.
Attachment #8549148 - Flags: review?(bent.mozilla)
Comment on attachment 8549148 [details] [diff] [review]
Patch

Yeah, bug 968520 has us trying to remove InfallibleTArray...
Comment on attachment 8549148 [details] [diff] [review]
Patch

Review of attachment 8549148 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but the last question is important:

::: dom/network/UDPSocketParent.h
@@ +32,5 @@
>                          const bool& aAddressReuse, const bool& aLoopback) MOZ_OVERRIDE;
>  
>    virtual bool RecvOutgoingData(const UDPData& aData, const UDPSocketAddr& aAddr) MOZ_OVERRIDE;
>  
>    virtual bool RecvClose() MOZ_OVERRIDE;

Unrelated!

::: dom/notification/Notification.cpp
@@ +333,5 @@
>    nsTArray<nsString> emptyOptions;
>    return nsContentPermissionUtils::CreatePermissionArray(NS_LITERAL_CSTRING("desktop-notification"),
> +							 NS_LITERAL_CSTRING("unused"),
> +							 emptyOptions,
> +							 aTypes);

Line endings?

::: ipc/ipdl/ipdl/lower.py
@@ +552,5 @@
> +    t = _cxxBareType(ipdltype, side)
> +    if ipdltype.isIPDL() and ipdltype.isArray():
> +        t.ref = 2
> +        return t
> +    if ipdltype.isIPDL() and ipdltype.isShmem():

Nit: We could combine these two blocks since they're identical:

  if ipdltype.isIPDL() and (ipdltype.isArray() or ipdltype.isShmem()):
  ...

@@ +958,5 @@
>  
>          def makeDecl(d, sems):
>              if sems is 'in':
>                  return Decl(d.inType(side), d.name)
> +            elif sems is 'move':

Hm, since moveType falls back to the same as inType is there any reason to introduce a new 'sems' type? What happens if you just make 'in' use the moveType directly?
Attachment #8549148 - Flags: review?(bent.mozilla) → review+
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #2)
> Comment on attachment 8549148 [details] [diff] [review]
> Patch
> 
> Review of attachment 8549148 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, but the last question is important:
> 
> ::: dom/network/UDPSocketParent.h
> @@ +32,5 @@
> >                          const bool& aAddressReuse, const bool& aLoopback) MOZ_OVERRIDE;
> >  
> >    virtual bool RecvOutgoingData(const UDPData& aData, const UDPSocketAddr& aAddr) MOZ_OVERRIDE;
> >  
> >    virtual bool RecvClose() MOZ_OVERRIDE;
> 
> Unrelated!

Fixed.

> ::: dom/notification/Notification.cpp
> @@ +333,5 @@
> >    nsTArray<nsString> emptyOptions;
> >    return nsContentPermissionUtils::CreatePermissionArray(NS_LITERAL_CSTRING("desktop-notification"),
> > +							 NS_LITERAL_CSTRING("unused"),
> > +							 emptyOptions,
> > +							 aTypes);
> 
> Line endings?

Tabs, actually.  idk how that happened.

> ::: ipc/ipdl/ipdl/lower.py
> @@ +552,5 @@
> > +    t = _cxxBareType(ipdltype, side)
> > +    if ipdltype.isIPDL() and ipdltype.isArray():
> > +        t.ref = 2
> > +        return t
> > +    if ipdltype.isIPDL() and ipdltype.isShmem():
> 
> Nit: We could combine these two blocks since they're identical:
> 
>   if ipdltype.isIPDL() and (ipdltype.isArray() or ipdltype.isShmem()):
>   ...

Done.

> @@ +958,5 @@
> >  
> >          def makeDecl(d, sems):
> >              if sems is 'in':
> >                  return Decl(d.inType(side), d.name)
> > +            elif sems is 'move':
> 
> Hm, since moveType falls back to the same as inType is there any reason to
> introduce a new 'sems' type? What happens if you just make 'in' use the
> moveType directly?

mmm, I think you're correct.
> > @@ +958,5 @@
> > >  
> > >          def makeDecl(d, sems):
> > >              if sems is 'in':
> > >                  return Decl(d.inType(side), d.name)
> > > +            elif sems is 'move':
> > 
> > Hm, since moveType falls back to the same as inType is there any reason to
> > introduce a new 'sems' type? What happens if you just make 'in' use the
> > moveType directly?
> 
> mmm, I think you're correct.

Actually, as we discussed on IRC, this is because we want the semantics to be different between SendFoo. and RecvFoo.  After IPDL deserializes a message and calls out it will always have no more need for the data, so it makes sense to provide move references to allow the callee to adopt the data.  But when we're serializing messages, a) the caller may want to hold on to the data and b) IPDL can't make much use of the move reference anyways since everything has to be serialized into the Pickle.
https://hg.mozilla.org/mozilla-central/rev/f498989ed56f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1123873
You need to log in before you can comment on or make changes to this bug.