Closed Bug 1294350 Opened 4 years ago Closed 4 years ago

Fix GPUProcess startup issues

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

A few problems - some Init/Shutdown pairings are missing. We don't initialize COM. We don't initialize XPCOM.
Comment on attachment 8779978 [details] [diff] [review]
part 2, init COM

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

LGTM
Attachment #8779978 - Flags: review?(aklotz) → review+
Attachment #8779977 - Flags: review?(rhunt) → review+
This initializes XPCOM in the GPU process via a new NS_InitMinimalXPCOM function. It initializes just enough for logging + threads to work, and for NS_ShutdownXPCOM to be called without asserting.
Attachment #8780368 - Flags: review?(nfroyd)
Comment on attachment 8780368 [details] [diff] [review]
part 3, init XPCOM

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

At first I was a little surprised that NS_ShutdownXPCOM works properly, but poking around, I see that virtually all of the things it calls are checking to see if things have been initialized anyway.

How are we even getting to the point where we can send and receive IPC messages, but we don't have XPCOM initialized?  Where does the GPU process's main() start from?

Why do we want to minimally init() things anyway?  We want to have as much memory as possible for the GPU process to use?  We want to reduce the amount of code the GPU process is running (efficiency and attack surface reasons?)?
ni? dvander just to ensure comment 5 gets some discussion.
Flags: needinfo?(dvander)
(In reply to Nathan Froyd [:froydnj] from comment #6)
> ni? dvander just to ensure comment 5 gets some discussion.

The main function is the usual XRE_InitChildProcess [1]. Right now we use a chromium message loop and things just appear to work. It looks like XRE_InitChildProcess redundantly initializes some things that NS_InitXPCOM2 initializes as well.

I'd like to minimally init for a few reasons. It reduces memory usage. It means less interfaces are available, so it will be obvious when shared code is broken (since most interfaces won't work anyway). It reduces startup time. None of that is critically important, but it'd be nice to only start out with what we need. 

[1] http://searchfox.org/mozilla-central/source/toolkit/xre/nsEmbedFunctions.cpp#635
Flags: needinfo?(dvander)
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b72c47cbd089
Ensure gfx-related services are started and shutdown properly in the GPU process. (bug 1294350 part 1, r=rhunt)
https://hg.mozilla.org/integration/mozilla-inbound/rev/daa0763d313f
Initialize COM in the GPU process. (bug 1294350 part 2, r=aklotz)
Comment on attachment 8780368 [details] [diff] [review]
part 3, init XPCOM

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

OK, let's try this.  I think there are some things we can do to clean this up, but those can be done later.
Attachment #8780368 - Flags: review?(nfroyd) → review+
This initializes nsComponentManager, but turns off all components for the GPU process. This is achieved in two ways. First, ProcessSelector is now a bit mask, and no component or module is loaded in the GPU process unless the ALLOW_IN_GPU_PROCESS bit is set.

Second, omnijars are not loaded. I don't expect we want any JS loading in the GPU process since it has to be able to start as quickly as possible.
Attachment #8783057 - Flags: review?(nfroyd)
This enables just enough components to get nsAppShell working in the GPU process.
Attachment #8783059 - Flags: review?(nfroyd)
This switches the GPU process to use an XPCOM event loop, which is necessary to get the media decoder working.
Attachment #8783061 - Flags: review?(nfroyd)
nsThread assumes BackgroundChildImpl is used, but it's not in the GPU process.
Attachment #8783062 - Flags: review?(wmccloskey)
Comment on attachment 8783061 [details] [diff] [review]
part 6, use XPCOM event loop

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

The more XPCOM event loop things, the better.
Attachment #8783061 - Flags: review?(nfroyd) → review+
Comment on attachment 8783057 [details] [diff] [review]
part 4, init nsComponentManager

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

See comments; everything else looks straightforward.

::: xpcom/components/Module.h
@@ +124,5 @@
> +  /**
> +   * Optional flags which control whether the module loads on a process-type
> +   * basis.
> +   */
> +  ProcessSelector selector;

Why do we have to care whether the Module loads on a per-process basis, rather than just the individual CID or contract ID entries?  Is this just trying to follow the principle of "load as little code as possible?  Does it actually make a difference to load modules that don't have any CID or contract IDs loaded?

::: xpcom/components/nsComponentManager.cpp
@@ +359,5 @@
> +  bool loadChromeManifests = true;
> +  switch (XRE_GetProcessType()) {
> +  case GeckoProcessType_GPU:
> +    loadChromeManifests = false;
> +    break;

I'm pretty sure this is going to cause warnings somewhere; can we either add a default: case that sets loadChromeManifests to true, or write this as:

bool loadChromeManifests = XRE_GetProcessType() != GeckoProcessType_GPU;

?
(In reply to Nathan Froyd [:froydnj] from comment #16)
> See comments; everything else looks straightforward.

Forgot to say: the bitflags setup is a bit unusual, though.
(In reply to Nathan Froyd [:froydnj] from comment #16)
> Comment on attachment 8783057 [details] [diff] [review]
> part 4, init nsComponentManager
> 
> Review of attachment 8783057 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why do we have to care whether the Module loads on a per-process basis,
> rather than just the individual CID or contract ID entries?  Is this just
> trying to follow the principle of "load as little code as possible?  Does it
> actually make a difference to load modules that don't have any CID or
> contract IDs loaded?

Yeah, load as little code as possible. It makes almost no difference when I measure. What I wanted to avoid was calling the ctor/dtor functions on static modules, which might initialize stuff that is broken or unneeded. It's easier to start from a clean slate.

> ::: xpcom/components/nsComponentManager.cpp
> @@ +359,5 @@
> > +  bool loadChromeManifests = true;
> > +  switch (XRE_GetProcessType()) {
> > +  case GeckoProcessType_GPU:
> > +    loadChromeManifests = false;
> > +    break;
> 
> I'm pretty sure this is going to cause warnings somewhere; can we either add
> a default: case that sets loadChromeManifests to true, or write this as:
> 
> bool loadChromeManifests = XRE_GetProcessType() != GeckoProcessType_GPU;
> 
> ?

Sure, that looks better.

Re: bitmask. I guess in retrospect it was not needed, we didn't have any cases (yet) that were MAIN|CONTENT_PROCESS_ONLY *and* ALLOW_IN_GPU_PROCESS. So I could change it back to an enum if you'd prefer. Though I'd prefer to keep it opt-in versus rewrite every module struct.
Comment on attachment 8783057 [details] [diff] [review]
part 4, init nsComponentManager

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

r =me with the change to nsComponentManager.cpp previously noted.
Attachment #8783057 - Flags: review?(nfroyd) → review+
Attachment #8783059 - Flags: review?(nfroyd) → review+
Comment on attachment 8783062 [details] [diff] [review]
part 7, make BackgroundImpl optional

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

I guess ideally we'd have some sort of observer thing so that BackgroundImpl can shut itself down rather than calling directly from nsThread. Oh well.

::: ipc/glue/BackgroundImpl.cpp
@@ +1761,5 @@
>  void
>  ChildImpl::CloseForCurrentThread()
>  {
> +  if (sThreadLocalIndex == kBadThreadLocalIndex) {
> +    MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_GPU,

I'm kind of in favor of dumping this assertion.
Attachment #8783062 - Flags: review?(wmccloskey) → review+
One more... when I switched to XPCOM threads, nsAppShell started spamming warnings about not having a power management service. It's a little odd that that service is included as part of layout, but assuming that's the right place for it, we'll have to initialize some of the layout module in the GPU process.

Alternately I could move it somewhere else, but I'm not sure where.
Attachment #8783774 - Flags: review?(nfroyd)
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/249d8c1dd892
Initialize a subset of XPCOM in the GPU process. (bug 1294350 part 3, r=froydnj)
https://hg.mozilla.org/integration/mozilla-inbound/rev/52e0e8f44ca3
Enable nsComponentManager in the GPU process. (bug 1294350 part 4, r=nfroyd)
https://hg.mozilla.org/integration/mozilla-inbound/rev/793e8dee47ea
Initialize nsAppShell in the GPU process. (bug 1294350 part 5, r=nfroyd)
https://hg.mozilla.org/integration/mozilla-inbound/rev/49447a2f2b95
Switch the GPU process main thread to have an XPCOM event loop. (bug 1294350 part 6, r=froydnj)
https://hg.mozilla.org/integration/mozilla-inbound/rev/37014a3e11f3
Make BackgroundImpl optional in the GPU process. (bug 1294350 part 7, r=billm)
Comment on attachment 8783774 [details] [diff] [review]
part 8, more appshell dependencies

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

I was a little worried when you said we have to initialize the layout module--that's a lot of code!  I can't think of a better place to put the power manager service, though; it seems like it's the sort of thing that belongs in XPCOM, but moving it there would be a bit tricky...

r=me with something like the comment modification below applied.

::: layout/build/nsLayoutModule.cpp
@@ +419,5 @@
>      return NS_ERROR_FAILURE;
>    }
> +  if (XRE_GetProcessType() == GeckoProcessType_GPU) {
> +    // In the GPU process, we only initialize the Layout module for AppShell
> +    // dependencies.

This comment reads a little odd: we only initialize the layout module for appshell dependencies, but we're returning early from the initialization function, so we're not actually doing any initialization?  Seems a bit contradictory.

Maybe something like:

"We mark the layout module as being available in the GPU process so that the XPCOM component manager sees that the power manager service is available in the GPU process.  However, we don't actually need to initialize anything in the layout module itself."

?
Attachment #8783774 - Flags: review?(nfroyd) → review+
Depends on: 1297465
No longer depends on: 1297465
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09a27892f811
Ensure the power management service gets initialized in the GPU process. (bug 1294350 part 8, r=froydnj)
https://hg.mozilla.org/mozilla-central/rev/09a27892f811
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.