1282348 - Bootstrap out-of-process PCompositorBridge

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

To do this I think we need the trifecta of CompositorWidget, APZCTreeManager, and CompositorVsyncDispatcher remoted over IPDL. (APZCTM technically optional to bootstrap, but would be good to have.)

Comment 1

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

Attachment: part 1, 2-step PCompositorBridge init WIP

Comment 2

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

Attachment: part 2, bootstrap WIP

Comment 3

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

Attachment: part 1, 2-step init

There's a bit of a problem - CompositorBridgeParent needs widget, APZCTM, and vsync objects to initialize. However at least two of these will be actors on the PCompositorBridge protocol, so they can't be constructed until after the protocol exists.

This patch splits CompositorBridgeParent initialization out of the constructor so we can delay initialization in the out-of-process case.

This introduces another problem, which is that CompositorBridgeParent holds a reference to itself. The only way this is released is through ActorDestroy, meaning the actor has been bound to IPDL. So this patch changes that as well: the self reference is now assigned as part of being bound to IPDL, so if we never initialize the compositor, it won't leak.

Lastly, CompositorBridgeChild is now responsible for creating in-process CompositorBridgeParents. This just puts the in-process initialization logic in one place.

Comment 4

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

Comment on attachment 8766641 [details] [diff] [review]
part 1, 2-step init

Bill, is this MOZ_RELEASE_ASSERT okay? It looks like in-process Open() calls are infallible, but better to make sure.

Comment 5

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

Comment on attachment 8766641 [details] [diff] [review]
part 1, 2-step init

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

Well, in theory this could fail. If the channel opens and then quickly dies due to an error, I think. However, we already assert that doesn't happen:
http://searchfox.org/mozilla-central/rev/261fe13dcd88cfd2e99e65755e7ca4b7a2e583df/ipc/glue/MessageChannel.cpp#705
So your assertion isn't going to hurt anyone.

Comment 6

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

Attachment: part 2, move InProcessCompositorSession

This just moves InProcessCompositorSession into its own file.

Comment 7

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

Attachment: part 3, move layer tree ids

This moves layer tree ID allocation into GPUProcessManager. The GPM is going to maintain a map from layer tree id => compositor session, among other things, so it makes more sense to have it control the ids.

Comment 8

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

Attachment: part 4, synchronize ActorDestroys

The GPU process is going to host a number of top-level actors: VsyncBridge, ImageBridge, and a CompositorBridge per-window. If the process dies, the order "ActorDestroy" is called for these actors in the UI process is undefined. While we will have to safely clean up each protocol individually, we don't want to block painting on each individual actor being cleaned up.

This patch lets GPUProcessManager receive a notification when an actor it creates has unexpectedly died. It will then be responsible for revoking all existing actors from Gecko, pushing them out of the way so we can create a new GPU process as quickly as possible. The old actors will still hang around waiting to be cleaned up, but GPUProcessManager won't be waiting on them, and widgets will no longer have a CompositorSession.

Comment 9

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

Comment on attachment 8769009 [details] [diff] [review]
part 4, synchronize ActorDestroys

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

Note: the token's job is just to be a nonce. We can use the layers id instead if you think that's better.

Comment 10

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

s/the layers id/an id/

Comment 11

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

Attachment: part 5, RemoteCompositorSession

Bill, could you review the IPDL-related changes in this patch? The initialization process for a RemoteCompositorSession is:

 1. Create endpoints
 2. Bind child endpoint
 3. Send parent endpoint to GPU process
 4. Bind parent endpoint in compositor thread of GPU process
 5. Send PCompositorWidget constructor
 6. Send PCompositorBridge initialization

We attempt steps 5-6 without waiting on step 4.

In addition CompositorBridgeChild uses the ActorDestroy notification from part 4, which is otherwise unused so far except to kill the process. Finally ProcessingError accounts for MsgDropped spam.

Comment 12

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

With these patches we get as far as instantiating a CompositorBridgeParent in the remote process, but of course immediately die trying to access gfxPlatform. That's bug 1282364.

Comment 13

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

Comment on attachment 8769009 [details] [diff] [review]
part 4, synchronize ActorDestroys

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

Yeah, I think a monotonically increasing uint64_t instead of GPUProcessToken would be simpler.

::: gfx/ipc/GPUProcessToken.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-

c-basic-offset should be 2.

@@ +3,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef _include_mozilla_gfx_ipc_GPUProcessToken_h_

Please follow the coding style here: mozilla_gfx_ipc_GPUProcessToken_h.

Comment 14

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

Attachment: part 4, synchronize ActorDestroys

Using monotonic counter instead of nonce pointer.

Comment 15

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

Attachment: part 5, RemoteCompositorSession

Rebased for part 4 changes.

Comment 16

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

Attachment: part 6, fix shutdown bug

When the GPU process terminates early, it asks GPUProcessManager to handle the error. The GPUProcessManager then asks the process to shutdown. This was causing MessageChannel::Close() to be called twice, once in ActorDestroy and another time in GPUProcessHost.

This patch fixes that and adds a comment to help clarify the order of events.

Comment 17

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

Attachment: part 5, RemoteCompositorSession

This is the same as the previous patch, but I moved RemoteCompositorSession construction to GPUProcessManager. With the vsync patches the number of arguments we have to pass around were getting ridiculous. GPUProcessManager is a better place since it knows where everything is.

Comment 18

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

Attachment: part 7, use endpoints for cross-process compositors

This replaces the existing PCompositorBridge::Opens code with endpoints instead. This will allow us to bridge the GPU process to the content process.

Comment 19

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

Attachment: part 8, e10s support

This implements the bridge from the GPU process to the content process. With this and the vsync stuff, both the UI and content process successfully render to the GPU process. * ** ***

* With the vsync patches, and some hacks to disable gfxPlatform.
** Software compositing only, so far.
*** Does not work with content sandboxing for some reason. Will file separate bug for that.

Comment 20

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

Comment on attachment 8769949 [details] [diff] [review]
part 4, synchronize ActorDestroys

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

::: gfx/ipc/GPUProcessHost.h
@@ +75,5 @@
>    }
>  
> +  // Return a unique id for this process, guaranteed not to be shared with any
> +  // past or future instance of GPUProcessHost.
> +  const uint64_t& GetProcessToken() const;

Why not uint64_t instead of const uint64_t& ?

::: gfx/ipc/GPUProcessManager.cpp
@@ +1,1 @@
>  /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

Please change c-basic-offset to 2.

@@ +8,2 @@
>  #include "mozilla/layers/InProcessCompositorSession.h"
> +#include "mozilla/StaticPtr.h"

No need for StaticPtr anymore I assume.

@@ +162,5 @@
> +void
> +GPUProcessManager::NotifyRemoteActorDestroyed(const uint64_t& aProcessToken)
> +{
> +  if (!NS_IsMainThread()) {
> +    NS_DispatchToMainThread(mTaskFactory.NewTask<NotifyRemoteActorDestroyedTask>(

If you use mTaskFactory.NewRunnableMethod here, I think you can avoid the new to write a custom runnable.

Comment 21

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

Comment on attachment 8770027 [details] [diff] [review]
part 5, RemoteCompositorSession

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

::: gfx/ipc/GPUParent.h
@@ +22,5 @@
>              IPC::Channel* aChannel);
>  
>    bool RecvInit(nsTArray<GfxPrefSetting>&& prefs) override;
>    bool RecvUpdatePref(const GfxPrefSetting& pref) override;
> +  bool RecvNewWindowCompositor(

Given how overloaded the term "window" is, I feel like NewChromeCompositor or something would be a better name.

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ +613,5 @@
>  #endif
>  {
> +  // Always run destructor on the main thread
> +  MOZ_ASSERT(NS_IsMainThread());
> +  SetMessageLoopToPostDestructionTo(MessageLoop::current());

Do we still need this? It used to be that actors had to be destroyed on the main thread, but this is no longer the case (bug 1121676). Might be possible to simplify some code here. Reading AtomicRefCountedWithFinalize.h makes me want to barf.

Comment 22

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

(In reply to Bill McCloskey (:billm) from comment #21)
> Comment on attachment 8770027 [details] [diff] [review]
> part 5, RemoteCompositorSession
> 
> Review of attachment 8770027 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Given how overloaded the term "window" is, I feel like NewChromeCompositor
> or something would be a better name.

I'll go with "Widget" unless you think that's overloaded too.

> ::: gfx/layers/ipc/CompositorBridgeParent.cpp
> @@ +613,5 @@
> >  #endif
> >  {
> > +  // Always run destructor on the main thread
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  SetMessageLoopToPostDestructionTo(MessageLoop::current());
> 
> Do we still need this? It used to be that actors had to be destroyed on the
> main thread, but this is no longer the case (bug 1121676). Might be possible
> to simplify some code here. Reading AtomicRefCountedWithFinalize.h makes me
> want to barf.

Interesting - probably not. I'll try this in a separate bug.

Comment 23

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

Filed bug 1287232.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/842631f306bc
Split up CompositorBridgeParent initialization. (bug 1282348 part 1, r=mattwoodrow,billm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/31ee282cfbbd
Split InProcessCompositorBridge to its own file. (bug 1282348 part 2, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f26200be9e80
Move layers ID allocation to GPUProcessManager. (bug 1282348 part 3, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/127687211494
Allow top-level protocols the ability to notify GPUProcessManager when their actors are unexpectedly destroyed. (bug 1282348 part 4, r=billm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6bd9062617f
Add a remote implementation of CompositorSession. (bug 1282348 part 5, r=billm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0520526e9ff3
Don't call Close twice when the GPU process unexpectedly terminates. (bug 1282348 part 6, r=billm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/39636df7a0fe
Send content compositor bridges using endpoints rather than Opens. (bug 1282348 part 7, r=billm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b09494ecac5f
Support compositor bridges from the content process to the GPU process. (bug 1282348 part 8, r=billm)

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/842631f306bc
https://hg.mozilla.org/mozilla-central/rev/31ee282cfbbd
https://hg.mozilla.org/mozilla-central/rev/f26200be9e80
https://hg.mozilla.org/mozilla-central/rev/127687211494
https://hg.mozilla.org/mozilla-central/rev/e6bd9062617f
https://hg.mozilla.org/mozilla-central/rev/0520526e9ff3
https://hg.mozilla.org/mozilla-central/rev/39636df7a0fe
https://hg.mozilla.org/mozilla-central/rev/b09494ecac5f