Closed Bug 1271180 Opened 4 years ago Closed 4 years ago

Introduce skeleton framework for the GPU process

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 10 obsolete files)

5.47 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
1.94 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.76 KB, patch
billm
: review+
Details | Diff | Splinter Review
30.29 KB, patch
billm
: review+
jrmuizel
: review+
Details | Diff | Splinter Review
11.88 KB, patch
billm
: review+
Details | Diff | Splinter Review
16.43 KB, patch
Details | Diff | Splinter Review
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.
Attached patch part 2, IPC (obsolete) — Splinter Review
Attachment #8750189 - Attachment is obsolete: true
I am extremely curious about this.
Attached patch IPC, XRE changes WIP (obsolete) — Splinter Review
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.
Attachment #8751427 - Attachment is obsolete: true
Attachment #8751428 - Attachment is obsolete: true
Attachment #8751429 - Attachment is obsolete: true
Move TaskFactory from plugins to ipc/glue, since it comes in handy elsewhere.
Attachment #8750188 - Attachment is obsolete: true
Attachment #8755733 - Attachment is obsolete: true
Attachment #8755975 - Flags: review?(aklotz)
We don't want to create GPUProcessManager on anything but the parent process.
Attachment #8755976 - Flags: review?(matt.woodrow)
Add GeckoProcessType_GPU.
Attachment #8755977 - Flags: review?(wmccloskey)
Attached patch part 4, IPDL and XRE framework (obsolete) — Splinter Review
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.
Attachment #8755980 - Flags: review?(wmccloskey)
Attachment #8755975 - Flags: review?(aklotz) → review+
Same as previous patch but a bool is replaced by an enum.
Attachment #8755980 - Attachment is obsolete: true
Attachment #8755980 - Flags: review?(wmccloskey)
Attachment #8756039 - Flags: review?(wmccloskey)
Attachment #8755976 - Flags: review?(matt.woodrow) → review+
Another small change - use a Listener class for callbacks instead of lambdas.
Attachment #8756039 - Attachment is obsolete: true
Attachment #8756039 - Flags: review?(wmccloskey)
Attachment #8756520 - Flags: review?(wmccloskey)
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?
Attachment #8755977 - Flags: review?(wmccloskey) → review+
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.
Attachment #8756520 - Flags: review?(wmccloskey) → review+
(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 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?
Attachment #8756520 - Flags: review?(jmuizelaar)
Attached patch part 5, IPDL actor shutdown (obsolete) — Splinter Review
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.
Attachment #8757124 - Flags: review?(wmccloskey)
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.
Attachment #8757127 - Flags: review?(wmccloskey)
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.
Flags: needinfo?(dvander)
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
Attachment #8757124 - Attachment is obsolete: true
Attachment #8757127 - Attachment is obsolete: true
Attachment #8757124 - Flags: review?(wmccloskey)
Attachment #8757127 - Flags: review?(wmccloskey)
Flags: needinfo?(dvander)
Attachment #8758407 - Flags: review?(wmccloskey)
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?
Attachment #8758407 - Flags: review?(wmccloskey) → review+
Attachment #8756520 - Flags: review?(jmuizelaar) → review+
This depends on bug 1277997, which rename CrashReporterParent/Child to CrashReporterHost/Client.
Attachment #8759898 - Flags: review?(ted)
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.
Attachment #8759898 - Flags: review?(ted)
Pushed by danderson@mozilla.com:
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)
Pushed by danderson@mozilla.com:
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)
You need to log in before you can comment on or make changes to this bug.