1271180 - Introduce skeleton framework for the GPU process

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

In this bug, I would like to add the basic necessities for the GPU process: a new GeckoProcessType, an empty top-level IPC protocol, startup/shutdown mechanics, crashreporter integration, etc.

Comment 1

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

Attachment: part 1, new GeckoProcessType

Comment 2

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

Attachment: part 2, IPC WIP

Comment 3

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

Attachment: part 2, IPC

Comment 4

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

Attachment: part 3, XRE/toolkit changes

Comment 5

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

Attachment: part 4, crashreporter integration

Comment 6

[Kelsey Gilbert [:jgilbert]]

I am extremely curious about this.

Comment 7

[George Wright (:gw280) (needinfo me!)]

So am I

Comment 8

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

Attachment: IPC, XRE changes WIP

This approach was based off PGMP/PPluginModule, where the parent actor lives in the parent process and the child actor lives in the child process. With the GPU process, this turns out not to work: the UI process and each content process will have its own child actor, and the parent will live in the GPU process. We can't change this - fundamentally the blocking relationship is different for graphics work. Instead we'll just do the seemingly-weird thing and have XRE_InitChildProcess create a parent actor.

This version also normalizes the relationship between process hosts and IPDL actors. For other protocols, the parent actor owns a ChildProcessHost, and the ChildProcess owns the child actor. In the GPU model, the ChildProcessHost (now, GPUProcessHost) owns the child IPDL actor (GPUChild), and the ChildProcess (GPUProcessImpl) owns the parent IPDL actor (GPUParent). For all intents and purposes all of these are singletons.

The GPU process is launched asynchronously, and we instantiate the GPUParent when IPDL successfully connects. Any use of GPUProcessManager will require that we either (1) have decided not to use the GPU process, or (2) have finished waiting for the GPU process to connect, which has a timeout. Over time that will change as we move critical state machinery out of CompositorBridgeParent into GPUProcessManager.

I'll try to clean and split this up for review shortly. Properly handling shutdown will be a totally separate patch since it's rather complicated.

Comment 9

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

Attachment: part 1, move TaskFactory

Move TaskFactory from plugins to ipc/glue, since it comes in handy elsewhere.

Comment 10

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

Attachment: part 2, GPUProcessManager fixes

We don't want to create GPUProcessManager on anything but the parent process.

Comment 11

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

Attachment: part 3, GeckoProcessType_GPU

Add GeckoProcessType_GPU.

Comment 12

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

Attachment: part 4, IPDL and XRE framework

See comment #8 - this is intended to be the minimal code to do basic launching and teardown. Full support for IPDL teardown will be separate patches.

Comment 13

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

Attachment: part 4, IPDL and XRE framework v2

Same as previous patch but a bool is replaced by an enum.

Comment 14

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

Attachment: part 4, IPDL and XRE framework v3

Another small change - use a Listener class for callbacks instead of lambdas.

Comment 15

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

Comment on attachment 8755977 [details] [diff] [review]
part 3, GeckoProcessType_GPU

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

::: toolkit/xre/nsAppRunner.cpp
@@ +943,5 @@
>  
>  SYNC_ENUMS(DEFAULT, Default)
>  SYNC_ENUMS(PLUGIN, Plugin)
>  SYNC_ENUMS(CONTENT, Content)
>  SYNC_ENUMS(IPDLUNITTEST, IPDLUnitTest)

Can you add a line for GMP as well?

Comment 16

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

Comment on attachment 8756520 [details] [diff] [review]
part 4, IPDL and XRE framework v3

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

Maybe we should start talking about "subprocesses" rather than "child processes". Could you fix up the comments to use this language?

It seems like shutdown is still a work in progress here? I don't see any calls to Close() and BeginShutdown seems to be unused. That seems fine though.

::: gfx/ipc/GPUParent.cpp
@@ +6,5 @@
> +#include "mozilla/Assertions.h"
> +#include "mozilla/ipc/ProcessChild.h"
> +#include "GPUParent.h"
> +#include "GPUProcessHost.h"
> +#include "gfxConfig.h"

I think the include order is wrong here. GPUParent.h should be first and then everything else after that, alphabetically. Same for other files. Also, please leave an empty line after the license.

::: gfx/ipc/GPUProcessHost.cpp
@@ +15,5 @@
> + : GeckoChildProcessHost(GeckoProcessType_GPU),
> +   mListener(aListener),
> +   mTaskFactory(this),
> +   mLaunchPhase(LaunchPhase::Unlaunched)
> +{

I think it would be good to have MOZ_COUNT_CTOR/MOZ_COUNT_DTOR in this class (as well as some of your other ones).

::: gfx/ipc/GPUProcessManager.cpp
@@ +40,4 @@
>  {
> +  if (nsCOMPtr<nsIObserverService> svc = services::GetObserverService()) {
> +    mObserver = new Observer(this);
> +    svc->AddObserver(mObserver, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);

Slightly easier to use RegisterShutdownObserver here. Same for UnregisterShutdownObserver.

@@ +138,5 @@
> +  if (!mProcess) {
> +    return;
> +  }
> +
> +  mProcess->Shutdown();

Shouldn't you |delete mProcess| here as well?

::: gfx/thebes/gfxPlatform.cpp
@@ +2129,5 @@
> +  if (XRE_IsParentProcess()) {
> +    if (gfxPrefs::GPUProcessDevEnabled()) {
> +      // We want to hide this from about:support, so only set a default if the
> +      // pref is known to be true.
> +      gfxConfig::SetDefaultFromPref(

Maybe someone else should look at this. No idea what's going on here.

Comment 17

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

(In reply to Bill McCloskey (:billm) from comment #16)
> Maybe we should start talking about "subprocesses" rather than "child
> processes". Could you fix up the comments to use this language?

That sounds good.

> It seems like shutdown is still a work in progress here? I don't see any
> calls to Close() and BeginShutdown seems to be unused. That seems fine
> though.

Yeah, I'm handling all shutdown stuff in separate patches.

> > +  mProcess->Shutdown();
> 
> Shouldn't you |delete mProcess| here as well?

This calls DissociateActor, which ultimately deletes the GPUProcessHost on the IO thread. I don't think I can just delete it. But I'll comment so that's clearer.

> ::: gfx/thebes/gfxPlatform.cpp
> @@ +2129,5 @@
> > +  if (XRE_IsParentProcess()) {
> > +    if (gfxPrefs::GPUProcessDevEnabled()) {
> > +      // We want to hide this from about:support, so only set a default if the
> > +      // pref is known to be true.
> > +      gfxConfig::SetDefaultFromPref(
> 
> Maybe someone else should look at this. No idea what's going on here.

Fair enough :)

Comment 18

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

Comment on attachment 8756520 [details] [diff] [review]
part 4, IPDL and XRE framework v3

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

Jeff, could you look at the gfxFeature/gfxPlatform changes?

Comment 19

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

Attachment: part 5, IPDL actor shutdown

This implements proper cleanup of GPUChild. It now holds a reference to the GPUProcessHost that created it, and notifies it of any ActorDestroy calls.

GPUProcessHost only self-destructs if Shutdown() is called. If the IPC channel is connected, we send a message via GPUChild asking the subprocess to clean up. When ActorDestroy/GPUProcessHost::OnChannelClosed occurs, clean-up proceeds.

If Shutdown is called in any state other than "working", we just clean up immediately.

Finally, if OnChannelClosed is called when we're not expecting a shutdown, we consider this an unexpected shutdown and notify the GPUProcessManager. This is how we'll implement restarting the GPU process later.

Comment 20

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

Attachment: part 6, ensure processes are killed

It's not clear to me how much of this is necessary, if any. I think we'll want to submit logging messages and telemetry from the GPU process, so shutdown will almost certainly become two-stage later on (like ContentParent). But if it never does, there's really no reason we couldn't immediately KillHard from the UI process?

Anyway, a bunch of this was lifted from ContentParent, so if it is something we want, it'd be good to figure out a common place to put it.

Comment 21

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

I'm getting a little lost in the different patches. Can you please post a single roll-up patch? One thing I can't find is where Close() gets called on PGPU.

Also, I checked in on the telemetry issue. It's bug 1218576 and Chris H-C is actively working on it. The plan is to send up new telemetry at least once per second. I think it would be better to assume that that will land soon (or before you want to start recording telemetry in GPU processes). In that case, I think you can simplify things. In opt builds (i.e., when !NS_FREE_PERMANENT_DATA) we can kill the GPU process on shutdown without any communication. In debug builds, we'll Close() the channel and wait for the process to die, but we don't need any timers or special shutdown messages.

By the way, you can see an example of a fairly simple shutdown sequence in PProcessHangMonitor.

Comment 22

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

Attachment: part 5, implement shutdown

Okay, I think this is closer to your comments, Bill - IPDL doesn't allow empty protocols so the BeginShutdown message got renamed to Nothing. I'll remove that when the first message gets added.

I tested shutdown with both NS_FREE_PERMAMENT_DATA and !NS_FREE_PERMAMENT_DATA, and with performing `kill -s 9` on the GPU process, and it seems to work.

Rollup:
[1] Commits: https://github.com/mozilla/gecko-dev/compare/mozilla:master...dvander:gpu-proc
[2] Patch: https://github.com/mozilla/gecko-dev/compare/mozilla:master...dvander:gpu-proc.patch

Comment 23

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

Comment on attachment 8758407 [details] [diff] [review]
part 5, implement shutdown

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

Looks good!

::: gfx/ipc/GPUChild.h
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  #ifndef _include_mozilla_gfx_ipc_GPUChild_h_
>  #define _include_mozilla_gfx_ipc_GPUChild_h_
>  
> +#include "mozilla/UniquePtr.h"

I think this should go second?

Comment 24

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

Attachment: part 6, crashreporter integration

This depends on bug 1277997, which rename CrashReporterParent/Child to CrashReporterHost/Client.

Comment 25

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

Comment on attachment 8759898 [details] [diff] [review]
part 6, crashreporter integration

Going to try bug 1278717 instead, which unblocks this snice we don't need new boilerplate.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81079e787b8a
Move TaskFactory from dom/plugins to ipc/glue. (bug 1271180 part 1, r=aklotz)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4472dfbc1dc6
Don't create GPUProcessManager in child processes. (bug 1271180 part 2, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/353da876be33
Add GeckoProcessType_GPU. (bug 1271180 part 3, r=billm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/204b084385f8
Add skeletal code for launching a GPU process. (bug 1271180 part 4, r=billm,jrmuizel)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfb53b780b18
Implement GPU process shutdown. (bug 1271180 part 5, r=billm)

Comment 27

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a173da18f0dd25e0f53a514bd3586bc02012f055 for static analysis bustage, https://treeherder.mozilla.org/logviewer.html#?job_id=30014711&repo=mozilla-inbound

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27a9eed3f2de
Move TaskFactory from dom/plugins to ipc/glue. (bug 1271180 part 1, r=aklotz)
https://hg.mozilla.org/integration/mozilla-inbound/rev/10446866aa69
Don't create GPUProcessManager in child processes. (bug 1271180 part 2, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3184a7ef70b3
Add GeckoProcessType_GPU. (bug 1271180 part 3, r=billm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/13f8e929e05e
Add skeletal code for launching a GPU process. (bug 1271180 part 4, r=billm,jrmuizel)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9599664f3e8b
Implement GPU process shutdown. (bug 1271180 part 5, r=billm)

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27a9eed3f2de
https://hg.mozilla.org/mozilla-central/rev/10446866aa69
https://hg.mozilla.org/mozilla-central/rev/3184a7ef70b3
https://hg.mozilla.org/mozilla-central/rev/13f8e929e05e
https://hg.mozilla.org/mozilla-central/rev/9599664f3e8b