1517260 - build support for boilerplate protocols directly into the IPDL compiler

Status: NEW

(Core :: IPC, enhancement, P2)

Description

[Nathan Froyd [:froydnj]]

We have several "standard" services that separate processes may want to setup:

* memory reporting
* profiling
* crash reporting
* telemetry
* etc.

And we currently have boilerplate-y code for supporting them, e.g.

* memory reporting

https://searchfox.org/mozilla-central/search?q=RecvRequestMemoryReport&path=
https://searchfox.org/mozilla-central/search?q=AddMemoryReport&path=
https://searchfox.org/mozilla-central/search?q=FinishMemoryReport&path=

* profiling

https://searchfox.org/mozilla-central/search?q=RecvInitProfiler&case=false&regexp=false&path=

* crash reporting

https://searchfox.org/mozilla-central/search?q=InitCrashReporter&path=

and so forth.

This is not ideal.

My proposed solution (also, possibly not ideal) is to declare the necessary service support directly in IPDL, viz.:

// Typical setup.
protocol PContent {
child:
  supports memoryreporting; // crashreporting, profiling, etc.
...
};

// Also supports non-standard setups where the parent is in the child process.
// `supports FOO` is invalid in a `both:` stanza.
protocol PGPU {
parent:
  supports memoryreporting;
  ...
};

and then have the IPDL compiler take care of generating whatever code is necessary to send/recv the appropriate messages.  At first I thought that lower.py would have to manually generate the method bodies (which would be terrible), but I think we could get by with generating appropriate enums for the messages, a little bit of code sprinkled elsewhere, and inheriting from some common superclass.  Which means that the major modifications could be made in C++-land, not in IPDL land.

Thoughts?  Other ideas?

Comment 1

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

My vague idea for this was to bundle those messages into a managed actor with a common implementation, so the top-level actors would still need some glue to construct it, but not like what there is now.  Probably the biggest problem there are the upside-down protocols (PGPU and PRDD), but those might be fixable: PRDD can just be renamed, and PGPU could have the sync messages factored out into a separate toplevel.  (In fact, memory reporting *was* factored into a subactor, but that was un-factored into its current state to allow PGPU to be backwards.)

That wouldn't need any change to the IPDL compiler, but there might be more work on the C++ side, and there'd end up being yet another list of every process type in the PChildProcessUtils (or whatever) manager declaration.

About inheriting implementation from superclasses: unless there's something big about C++ I don't know (which is entirely possible), we'd need actor devirtualization to do that without glue (contrast https://searchfox.org/mozilla-central/search?q=return+nsIContentParent%3A%3A&case=false&regexp=false&path=dom/ipc )

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ from comment #1)
> My vague idea for this was to bundle those messages into a managed actor
> with a common implementation, so the top-level actors would still need some
> glue to construct it, but not like what there is now.  Probably the biggest
> problem there are the upside-down protocols (PGPU and PRDD), but those might
> be fixable: PRDD can just be renamed, and PGPU could have the sync messages
> factored out into a separate toplevel.  (In fact, memory reporting *was*
> factored into a subactor, but that was un-factored into its current state to
> allow PGPU to be backwards.)
> 
> That wouldn't need any change to the IPDL compiler, but there might be more
> work on the C++ side, and there'd end up being yet another list of every
> process type in the PChildProcessUtils (or whatever) manager declaration.

"wouldn't need any changes to the IPDL compiler" is definitely a plus.  I'm unclear what "more work on the C++ side" entails: is that work that needs to be done in common code, or someplace else?  How much C++ does this proposal require new process type implementors to write?

> About inheriting implementation from superclasses: unless there's something
> big about C++ I don't know (which is entirely possible), we'd need actor
> devirtualization to do that without glue (contrast
> https://searchfox.org/mozilla-central/
> search?q=return+nsIContentParent%3A%3A&case=false&regexp=false&path=dom/ipc )

I should have been more precise: I meant something more like a mixin class with non-virtual methods whose method names were known to the IPDL compiler.  So message dispatch on the actor would know to call method M, and M would have been implemented by this mixin class.  The IPDL compiler would not generate method declarations for Recv/Send as appropriate.  You have to know to inherit from the class, I suppose, but perhaps that's OK.

For what would be ideal to get rid of, see:

https://phabricator.services.mozilla.com/D14155

which adds memory reporting for the socket process.  Bug 1517276 makes some of that go away, and this bug would presumably make more.  We'd need a third one for the SocketProcessHost (and maybe the nsMemoryReporterManager stuff), but that patch is ~150 lines, which is some 130-140 lines too many.

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

When you were thinking about doing this in the IPDL compiler, was the implementation you were imagining basically just syntax sugar for a suite of child actors, or something more integrated?

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #3)

When you were thinking about doing this in the IPDL compiler, was the implementation you were imagining basically just syntax sugar for a suite of child actors, or something more integrated?

Do you mean "a suite of child messages"? I'm not clear on what the question is getting at.

The idea was that you declare something in the protocol description, and the IPDL compiler--let's use memory reporting, because that's very standardized--emits something so you don't have to write:

https://searchfox.org/mozilla-central/source/dom/media/ipc/RDDChild.cpp#61-86
https://searchfox.org/mozilla-central/source/dom/media/ipc/RDDParent.cpp#152-166

and supporting code. Well, maybe you have to add some inheritance declaration in your concrete actor class(es). Because those 40ish lines are virtually identical for all actors implementing memory reporting.

Crash reporting, skimming over it now, is a little harder, I think: setting up crash reporting is pretty standardized, but actually doing something with the crash report when things go wrong is not as standardized, so that would require a little more thought.

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

Ok, I spent a bit of time thinking about it, and I think I ended up roughly with where you were, but which I mis-read originally :-)

My idea was to add something roughly like:

protocol PFoo {
    delegates PMemoryReporter;
};

This would be sort of like a child actor, except there'd always be one-PMemoryReporter for every PFoo. The IPDL compiler would add a little additional logic to OnMessageReceived to dispatch those messages. It'd also add a GetMemoryReporter()-or-similar method to PFoo which returned an instance. It would automatically use the concrete implementation of PMemoryReporter as discussed in bug 1512990, so we could just implement that once then delegate it in each child class.

Basically, the goal would be to get child-actor-like ability to group functionality together and reuse it, but without the child-actor lifecycle.

The end result is that a new top process simple adds (TM):

   delegates PMemoryReporter, PProfiler, PTelemetry, ...;

And they get what we want.

Is that what you were describing originally? If so, whoops! I agree! I think I was confused by the idea of somehow making the IPDL compiler know about the idea of memory reporting specifically, instead of just knowing about how to handle this pattern of code reuse.