Bootstrap out-of-process PCompositorBridge

RESOLVED FIXED in Firefox 50

Status

()

Core
Graphics
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

(Blocks: 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(8 attachments, 5 obsolete attachments)

18.13 KB, patch
mattwoodrow
: review+
billm
: review+
Details | Diff | Splinter Review
13.81 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
16.89 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
8.32 KB, patch
billm
: review+
Details | Diff | Splinter Review
2.54 KB, patch
billm
: review+
Details | Diff | Splinter Review
30.17 KB, patch
billm
: review+
Details | Diff | Splinter Review
19.83 KB, patch
billm
: review+
Details | Diff | Splinter Review
3.73 KB, patch
billm
: review+
Details | Diff | Splinter Review
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.)
Created attachment 8765327 [details] [diff] [review]
part 1, 2-step PCompositorBridge init WIP
Created attachment 8766641 [details] [diff] [review]
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.
Attachment #8765327 - Attachment is obsolete: true
Attachment #8766641 - Flags: review?(matt.woodrow)
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.
Attachment #8766641 - Flags: review?(wmccloskey)
Attachment #8766641 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8766641 - Flags: review?(wmccloskey) → review+
Created attachment 8768963 [details] [diff] [review]
part 2, move InProcessCompositorSession

This just moves InProcessCompositorSession into its own file.
Attachment #8765341 - Attachment is obsolete: true
Attachment #8768963 - Flags: review?(matt.woodrow)
Created attachment 8768964 [details] [diff] [review]
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.
Attachment #8768964 - Flags: review?(matt.woodrow)
Created attachment 8769009 [details] [diff] [review]
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.
Attachment #8769009 - Flags: review?(wmccloskey)
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.
s/the layers id/an id/
Attachment #8768963 - Flags: review?(matt.woodrow) → review+
Attachment #8768964 - Flags: review?(matt.woodrow) → review+
Created attachment 8769052 [details] [diff] [review]
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.
Attachment #8769052 - Flags: review?(wmccloskey)
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 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.
Attachment #8769009 - Flags: review?(wmccloskey) → review+
Created attachment 8769949 [details] [diff] [review]
part 4, synchronize ActorDestroys

Using monotonic counter instead of nonce pointer.
Attachment #8769009 - Attachment is obsolete: true
Attachment #8769949 - Flags: review?(wmccloskey)
Created attachment 8769950 [details] [diff] [review]
part 5, RemoteCompositorSession

Rebased for part 4 changes.
Attachment #8769052 - Attachment is obsolete: true
Attachment #8769052 - Flags: review?(wmccloskey)
Attachment #8769950 - Flags: review?(wmccloskey)
Created attachment 8769952 [details] [diff] [review]
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.
Attachment #8769952 - Flags: review?(wmccloskey)
Created attachment 8770027 [details] [diff] [review]
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.
Attachment #8769950 - Attachment is obsolete: true
Attachment #8769950 - Flags: review?(wmccloskey)
Attachment #8770027 - Flags: review?(wmccloskey)
Created attachment 8770838 [details] [diff] [review]
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.
Attachment #8770838 - Flags: review?(wmccloskey)
Created attachment 8770839 [details] [diff] [review]
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.
Attachment #8770839 - Flags: review?(wmccloskey)
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.
Attachment #8769949 - Flags: review?(wmccloskey) → review+
Attachment #8769952 - Flags: review?(wmccloskey) → review+
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.
Attachment #8770027 - Flags: review?(wmccloskey) → review+
Attachment #8770838 - Flags: review?(wmccloskey) → review+
Attachment #8770839 - Flags: review?(wmccloskey) → review+
(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 24

2 years ago
Pushed by danderson@mozilla.com:
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)
No longer depends on: 1282364
You need to log in before you can comment on or make changes to this bug.