1277439 - Add a non-Parent/Child protocol model

Status: RESOLVED WONTFIX

(Core :: IPC, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

The GPU process has an inverted child/parent relationship - inverted in that the child lives in the UI process, and the parent lives in the subprocess. The GPU process is not allowed to block on the UI process.

This makes it difficult to use existing protocols like PCrashReporter, since CrashReporterParent must exist in the UI process, but IPDL forbids mixing Child/Parent relationships like this. In cases like PCrashReporter though, the entire protocol is async and there is no risk of deadlocking due to mutual sync messages.

The proposal to address this is to introduce a new "generic" protocol keyword. Instead of generating "Parent" and "Child" actors, it will generate "Host" and "Client" actors. When another protocol manages a generic protocol, it must specify which side creates the Host (and implicitly which side creates the Client).

Generic protocols may only use async messages.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, change _actorName

This changes _actorName so that a protocol type must be passed as well. No functionality change.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, generic protocols

This adds and implements the "generic" keyword, which goes before the "protocol" keyword or semantic specifier. This was mostly straightforward except for one hitch: mozbuild looks for .ipdl files and hardcodes Parent/Child.cpp source names for them. It seems hard to fix this without a new extension or some kind of extra communication between IPDL and mozbuild.

So I just kept that as-is for now. The .h files *are* named correctly which seems more important.

I also didn't attempt to change the "parent" and "child" keywords that occur within generic protocols... there are a lot more places that'd affect, but I could attempt it if it's too weird as-is.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

To see what it looks like when PCrashReporter becomes generic:

https://github.com/dvander/gecko-dev/commit/8c9208fdb8c2af9c74444ff50d198ff9564161b3

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, add some context to ProtocolType.manages

ProtocolType.manages tracks the ProtocolType, but we'll need to carry along whether the relationship is inverted as well. This patch changes the array to carry a new container object.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, "parent creates client" impl

This patch introduces a new syntax, like:

   manages PCrashReporter parent creates client;

Any PCrashReporters managed by the actor will have an inverted side. That is - a PThingParent will create a PCrashReporterClient, and a PThingChild will create a PCrashReporterHost.

I did a trial run by inverting PGMP's crash reporter. This patch was good enough to fix all instances, except there's a lingering extra friend declaration for the original side.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8759010 [details] [diff] [review]
part 2, generic protocols

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

::: ipc/ipdl/ipdl/type.py
@@ +1370,5 @@
>                         "protocol `%s' is not top-level and so cannot declare |opens|",
>                         pname)
>  
> +        if ptype.isGeneric:
> +            if ptype.isSync():

I think we need to forbid intr too.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8759072 [details] [diff] [review]
part 4, "parent creates client" impl

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

A few requests:
- If the protocol is generic, can you force the user to specify either "parent creates client" or "parent creates host"? I think that would be clearer.
- Can we put the generic spec in parens? So: |manages PFoo (parent creates host)|. That reads a little better.
- This needs one or two tests.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Bill McCloskey (:billm) from comment #7)
> Comment on attachment 8759072 [details] [diff] [review]
> part 4, "parent creates client" impl
> 
> Review of attachment 8759072 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few requests:
> - If the protocol is generic, can you force the user to specify either
> "parent creates client" or "parent creates host"? I think that would be
> clearer.
> - Can we put the generic spec in parens? So: |manages PFoo (parent creates
> host)|. That reads a little better.
> - This needs one or two tests.

I tried "parent creates host" originally, but "host" is used as an identifier in some ipdl files. If you don't mind stealing that as a keyword I can do this. Unfortunately ply doesn't seem to allow multi-character literals, i.e. PARENT CREATES "host" didn't work.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

How about PARENT CREATES ID, then throw an exception if the ID isn't "host" or "client"?

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Sounds good.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, "parent creates client" impl v2

w/ comments addressed and an IPDL cxx test. The test found one bug (single-manager actors wouldn't compile), so that's fixed as well.

I tried to add grammar tests but it looks like most of them just fail currently.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8759882 [details] [diff] [review]
part 4, "parent creates client" impl v2

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

Honestly, I would rather not do this. It's more complicated than I expected and it's not clear that it will be useful beyond the PCrashReporter thing. I just hate the idea of making lower.py even more complex than it already is.

If you really want this, we can do it. But I'd rather find another way. For example, I think bug 1278717 would be one solution that I thought of today. I'd be willing to do that bug myself if it would help you.

::: ipc/ipdl/ipdl/parser.py
@@ +469,5 @@
> +    p[0] = ManagesStmt(locFromTok(p, 1), p[2], p[3])
> +
> +def p_ManagesRelationshipOpt(p):
> +    """ManagesRelationshipOpt : "(" PARENT CREATES ID ")"
> +                              | "(" PARENT CREATES CLIENT ")"

I think we can just remove this line and stop marking "client" as a keyword.

::: ipc/ipdl/ipdl/type.py
@@ +351,5 @@
>          return False
>      def isManagedBy(self, pt):
>          return pt in self.managers
> +    def manageInfoFor(self, pt):
> +        return [m for m in self.manages if m.type == pt][0]

The convention in this code is to put spaces around list comprehensions:
  [ x for x in e ]

@@ +1607,5 @@
> +                self.error(
> +                    loc,
> +                    "ctor for protocol `%s', which is not managed by protocol `%s'",
> +                    mname[:-len('constructor')], pname)
> +            md.managedInfo = [m for m in ptype.manages if m.type == mtype.constructedType()][0]

Can you use manageInfoFor here?

::: ipc/ipdl/test/cxx/PTestGenericActor.ipdl
@@ +5,5 @@
> +
> +generic async protocol PTestGenericActor
> +{
> +    manager PTestGeneric;
> +parent:

Gee, this is pretty awkward and confusing. Could you at least allow client|host as well as child|parent?

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Tabling this for now. I like the idea of having generic protocols, but we can come back to it later if a really compelling case comes up.