This patch introduces a new top-level actor called PVsyncBridge. It bridges the compositor thread of the GPU process to a new "VsyncIOThread" in the parent process. The plan of attack is that widgets will post vsync notifications to the VsyncIOThread, where they will then dispatch to the compositor directly. All notifications will be sent over the top-level actor - VsyncBridgeParent will use the layers id to dispatch it to the correct compositor. Most of the complexity in this patch is shutdown related - we use XPCOM threads to take advantage of AsyncShutdown, and use a VsyncIOThreadHolder abstraction so the thread only goes away when no actors are left. I wasn't sure, but I think we might need something similar on the parent side. Bill, does VsyncBridgeParent need a ref to CompositorThreadHolder?

This patch removes CompositorVsyncDispatcher from the CompositorWidget API, and instead exposes VsyncObserver directly. We'll continue to use CompositorVsyncDispatcher in-process. When CompositorBridgeParent is out of process, IPDL will be responsible for dispatching vsync instead.

Note that we have to destroy mCompositorScheduler sooner with this patch, since the widget is no longer valid after StopAndClearResources returns.

This patch implements vsync for the GPU process. In out-of-process mode, CompositorVsyncDispatcher forwards to the VsyncIOThread instead of the compositor. On the GPU process side, notifications arrive on the compositor thread. We then use the layer id => compositor map to deliver the notification. This patch is a little hacky since it uses the VsyncObserver through CompositorWidget, to keep the out-of-process and in-process paths as similar as possible. That is, the observer then notifies the scheduler, and the scheduler posts a composite task. We could imagine changing this a bit though. We could bypass all of that and just directly composite off the vsync notification, since it comes in on the compositor thread. Even if we don't want to do that, we could bypass using the observer and scheduler since they basically don't do anything here. If that route seems more appealing, we could ditch part 2 and instead have an "CompositorVsyncObserver" exposed from CompositorWidget. This class would just have opaque "Start/Stop" methods. When in-process, it'd be a CompositorVsyncDispatcher + VsyncObserver pair. CompositorVsyncScheduler implement VsyncObserver instead of creating its own. Out of process, CompositorVsyncObserver would just go over IPDL.

correct patch

Same patch with some mistakes cleaned up.

Comment on attachment 8770294 [details] [diff] [review] part 2, CompositorWidget changes Review of attachment 8770294 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/CompositorBridgeParent.cpp @@ +738,5 @@ > > + if (mCompositorScheduler) { > + CancelCurrentCompositeTask(); > + mCompositorScheduler->Destroy(); > + mCompositorScheduler = nullptr; The compositor scheduler already cancels the current composite task during Destroy(), so you don't have to do it here again. Also please add a comment saying this needs to be destroyed right after the widget. ::: widget/windows/WinCompositorWidget.cpp @@ +178,3 @@ > { > RefPtr<CompositorVsyncDispatcher> cvd = mWindow->GetCompositorVsyncDispatcher(); > + cvd->SetCompositorVsyncObserver(aObserver); From comment 2, it looks like we should only be using CompositorVsyncDispatcher in process. That doesn't seem to be the case here? Also, can you add this architecture design in https://dxr.mozilla.org/mozilla-central/source/widget/VsyncDispatcher.h#35. We probably also want to update the docs https://dxr.mozilla.org/mozilla-central/source/gfx/doc/Silk.md?q=Silk.md&redirect_type=direct to show what happens with the GPU process.

Comment on attachment 8770319 [details] [diff] [review] part 3, implementation Review of attachment 8770319 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/ipc/CompositorWidgetVsyncObserver.cpp @@ +19,5 @@ > +} > + > +bool > +CompositorWidgetVsyncObserver::NotifyVsync(TimeStamp aTimeStamp) > +{ nit: Assert which thread this is happening on or comment it. Also assert this is not the parent process. ::: gfx/ipc/VsyncBridgeChild.cpp @@ +81,5 @@ > + > +void > +VsyncBridgeChild::NotifyVsync(TimeStamp aTimeStamp, const uint64_t& aLayersId) > +{ > + MOZ_ASSERT(MessageLoop::current() != mLoop); If VSyncBridge connects ParentProcess::VsyncIOThread -> GPUProcess::CompositorThread, why is this the case? Shouldn't this message be arriving on the compositor thread already and we shouldn't have to post a task to another loop? Or I'm messing this up in my head and the VsyncBridgeChild lives in the parent process and VsyncBridgeParent lives in the GPU process? ::: gfx/ipc/VsyncBridgeParent.cpp @@ +47,2 @@ > { > + CompositorBridgeParent::NotifyVsync(aTimeStamp, aLayersId); nit: Please add asserts as to which thread everything in this class executes on. ::: widget/CompositorWidget.h @@ +222,5 @@ > */ > virtual already_AddRefed<gfx::SourceSurface> EndBackBufferDrawing(); > > /** > + * Observer or unobserve vsync. nit: trailing whitespace.

> We could imagine changing this a bit though. We could bypass all of that and > just directly composite off the vsync notification, since it comes in on the > compositor thread. Even if we don't want to do that, we could bypass using > the observer and scheduler since they basically don't do anything here. > > If that route seems more appealing, we could ditch part 2 and instead have > an "CompositorVsyncObserver" exposed from CompositorWidget. This class would > just have opaque "Start/Stop" methods. When in-process, it'd be a > CompositorVsyncDispatcher + VsyncObserver pair. CompositorVsyncScheduler > implement VsyncObserver instead of creating its own. > > Out of process, CompositorVsyncObserver would just go over IPDL. I think it's ok to keep it as is. Even if the scheduler is doing nothing and just posting a task to itself, that's fine. It shouldn't be a performance bottleneck and conceptually keeps the architecture uniform across the platforms.

(In reply to Mason Chang [:mchang] from comment #9) > Comment on attachment 8770319 [details] [diff] [review] > part 3, implementation > > Review of attachment 8770319 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/ipc/VsyncBridgeChild.cpp > @@ +81,5 @@ > > + > > +void > > +VsyncBridgeChild::NotifyVsync(TimeStamp aTimeStamp, const uint64_t& aLayersId) > > +{ > > + MOZ_ASSERT(MessageLoop::current() != mLoop); > > If VSyncBridge connects ParentProcess::VsyncIOThread -> > GPUProcess::CompositorThread, why is this the case? Shouldn't this message > be arriving on the compositor thread already and we shouldn't have to post a > task to another loop? Or I'm messing this up in my head and the > VsyncBridgeChild lives in the parent process and VsyncBridgeParent lives in > the GPU process? Right - VsyncBridgeChild is in the parent process. Here "mLoop" is the Vsync I/O Thread and MessageLoop::current() should be the platform-specific vsync thread.

Comment on attachment 8770294 [details] [diff] [review] part 2, CompositorWidget changes Review of attachment 8770294 [details] [diff] [review]: ----------------------------------------------------------------- Please add a part 4 with an update to the docs. Thanks!

Comment on attachment 8770319 [details] [diff] [review] part 3, implementation Review of attachment 8770319 [details] [diff] [review]: ----------------------------------------------------------------- Mostly looks good to me. r+ on condition of lots of asserts on which process and thread everything is executing on. Too many files to keep track of. It's nice to see an assert() to know which thread / process stuff is being executed on. I'm hitting a mental block thinking child == parent process here. Thanks! ::: gfx/ipc/VsyncBridgeChild.cpp @@ +89,5 @@ > + > +void > +VsyncBridgeChild::NotifyVsyncImpl(TimeStamp aTimeStamp, const uint64_t& aLayersId) > +{ > + if (!mProcessToken) { assert on vsync i/o thread. ::: widget/CompositorWidget.cpp @@ +71,5 @@ > } > > +RefPtr<VsyncObserver> > +CompositorWidget::GetVsyncObserver() const > +{ nit: assert is OOP compositor. ::: widget/windows/CompositorWidgetChild.cpp @@ +63,2 @@ > { > // Not supported in out-of-process mode. nit: Probably want to add an NS_WARNING then?

Now that the bootstrapping patches are done, I was able to more fully test this, and realized I left something important out of the last patch. CompositorWidgetParent never hooked ObserveVsync up to IPDL, so we were never actually observing vsync in the GPU process. This patch fixes that. Unfortunately that broke some asserts in VsyncDispatcher.cpp because we can now observe from the main thread in addition to the compositor thread. It wasn't clear how to fix this assert, and it already had hacks for gtests, so I just removed it. I can add it back if you've got suggestions on how to mend it. Requesting re-review, anyway. I added the other asserts you requested and made an attempt at updating Silk.md and the VsyncDispatcher header.

Comment on attachment 8770848 [details] [diff] [review] part 3, implementation v2 w/ docs Review of attachment 8770848 [details] [diff] [review]: ----------------------------------------------------------------- I'd still like to keep the AssertOnCompositor thread. Perhaps just rename it to something like AssertCorrectThreadAndProcess()? ::: gfx/doc/Silk.md @@ +62,5 @@ > Each **CompositorBridgeParent** is associated with one widget and is created when a new platform window or **nsBaseWidget** is created. > The **CompositorBridgeParent**, **CompositorVsyncDispatcher**, **CompositorVsyncObserver**, and **nsBaseWidget** all have the same lifetimes, which are created and destroyed together. > > +##Out-of-process Compositors > +When compositing out-of-process, this model changes slightly. In this case there are effectively two observers: a UI process observer (**CompositorWidgetVsyncObserver**), and the **CompositorVsyncObserver** in the GPU process. There are also two dispatchers: the widget dispatcher in the UI process (**CompositorVsyncDispatcher**), and the IPDL-based dispatcher in the GPU process (**CompositorBridgeParent::NotifyVsync**). The UI process observer and the GPU process dispatcher are linked via an IPDL protocol called PVsyncBridge. **PVsyncBridge** is a top-level protocol for sending vsync notifications to the compositor thread in the GPU process. The compositor controls vsync observation through a separate actor, **PCompositorWidget**, which (as a subactor for **CompositorBridgeChild**) links the compositor thread in the GPU process to the main thread in the UI process. nit: Separate each sentence into a new line. This makes it easier to see diffs whenever people change the document. Also, can we rename "CompositorVsyncObserver" to "CompositorGPUVsyncObserver"? The current naming isn't very clear as to where each one lives. Please add these objects up top here - https://dxr.mozilla.org/mozilla-central/source/gfx/doc/Silk.md?q=Silk.md&redirect_type=direct#17 ::: gfx/ipc/CompositorWidgetVsyncObserver.cpp @@ +14,5 @@ > + RefPtr<VsyncBridgeChild> aVsyncBridge, > + const uint64_t& aRootLayerTreeId) > + : mVsyncBridge(aVsyncBridge), > + mRootLayerTreeId(aRootLayerTreeId) > +{ nit: same asserts here. ::: gfx/ipc/VsyncBridgeChild.cpp @@ +81,5 @@ > + > +void > +VsyncBridgeChild::NotifyVsync(TimeStamp aTimeStamp, const uint64_t& aLayersId) > +{ > + // This should be on the Vsync thread (not the Vsync I/O thread). nit: Probably just wrap this in a AssertIsOnVsyncIOThread()? ::: gfx/ipc/VsyncBridgeParent.cpp @@ +47,2 @@ > { > + CompositorBridgeParent::NotifyVsync(aTimeStamp, aLayersId); nit: Assert on Compositor thread. ::: widget/CompositorWidget.cpp @@ +74,5 @@ > +CompositorWidget::GetVsyncObserver() const > +{ > + // This should only used when the widget is in the GPU process, since the > + // GPU process does not have a CompositorVsyncDispatcher. > + MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_GPU); Can this be called on any thread? Since there's no locks here, I guess it can only be on the Compositor thread and because this is on the GPU process? If so, assert that please. ::: widget/VsyncDispatcher.cpp @@ -54,5 @@ > } > > void > -CompositorVsyncDispatcher::AssertOnCompositorThread() > -{ Can we just change this so that if it's in process, assert on main thread, otherwise if in GPU process, assert on compositor thread? ::: widget/windows/CompositorWidgetChild.cpp @@ +67,5 @@ > > +bool > +CompositorWidgetChild::RecvObserveVsync() > +{ > + mVsyncDispatcher->SetCompositorVsyncObserver(mVsyncObserver); nit: assert UI process and thread.

Comment on attachment 8770668 [details] [diff] [review] part 1, PVsyncBridge Review of attachment 8770668 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/ipc/GPUProcessManager.cpp @@ +101,5 @@ > if (!mProcess->Launch()) { > DisableGPUProcess("Failed to launch GPU process"); > } > + > + EnsureVsyncIOThread(); Is there a chance that OnProcessLaunchComplete could run before this? It looks like it's possible. And in that case mVsyncIOThread will be null, which seems bad. @@ +202,5 @@ > // can happen if the ActorDestroy for a bridged protocol fires > // before the ActorDestroy for PGPUChild. > DestroyProcess(); > } > +void Need a separating line. ::: gfx/ipc/PVsyncBridge.ipdl @@ +9,5 @@ > +namespace gfx { > + > +// This protocol only serves one purpose: deliver vsync notifications from a > +// dedicated thread in the UI process to the compositor thread in the > +// compositor process. Can you say which side is the child and which the parent? ::: gfx/ipc/VsyncBridgeChild.cpp @@ +26,5 @@ > + Endpoint<PVsyncBridgeChild>&& aEndpoint) > +{ > + RefPtr<VsyncBridgeChild> child = new VsyncBridgeChild(aThread, aProcessToken); > + > + RefPtr<nsIRunnable> task = NewRunnableFunction(&VsyncBridgeChild::Open, child, Move(aEndpoint)); Any reason you can't use NewRunnableMethod? ::: gfx/ipc/VsyncBridgeParent.cpp @@ +13,5 @@ > +VsyncBridgeParent::Start(Endpoint<PVsyncBridgeParent>&& aEndpoint) > +{ > + RefPtr<VsyncBridgeParent> parent = new VsyncBridgeParent(); > + MessageLoop* loop = CompositorThreadHolder::Loop(); > + loop->PostTask(NewRunnableFunction( Again, it seems like NewRunnableMethod would be a better fit here. @@ +30,5 @@ > + MOZ_COUNT_DTOR(VsyncBridgeParent); > +} > + > +void > +VsyncBridgeParent::Bind(RefPtr<VsyncBridgeParent> aParent, If this remains static, please mark it /* static */. ::: gfx/ipc/VsyncBridgeParent.h @@ +31,5 @@ > + > + static void Bind(RefPtr<VsyncBridgeParent> aParent, > + Endpoint<PVsyncBridgeParent>&& aEndpoint); > + > + void DeferredDestroy(); This looks unused. @@ +35,5 @@ > + void DeferredDestroy(); > + > +private: > + // Cleared in ActorDestroy. > + RefPtr<VsyncBridgeParent> mSelfRef; This appears to be unused.

(In reply to Bill McCloskey (:billm) from comment #16) > Comment on attachment 8770668 [details] [diff] [review] > part 1, PVsyncBridge > > Review of attachment 8770668 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/ipc/GPUProcessManager.cpp > @@ +101,5 @@ > > if (!mProcess->Launch()) { > > DisableGPUProcess("Failed to launch GPU process"); > > } > > + > > + EnsureVsyncIOThread(); > > Is there a chance that OnProcessLaunchComplete could run before this? It > looks like it's possible. And in that case mVsyncIOThread will be null, > which seems bad. Nice catch, thanks. > > + RefPtr<nsIRunnable> task = NewRunnableFunction(&VsyncBridgeChild::Open, child, Move(aEndpoint)); > > Any reason you can't use NewRunnableMethod? I was having trouble with template errors, but got it working now. It looks like NewRunnableMethod requires you to use "NewRunnableMethod<Endpoint<PVsyncBridgeChild>&&>" whereas NewRunnableFunction doesn't.

This version adds some more asserts and cleans up the docs more. It also splits WinCompositorWidget into two classes, so that WinCompositorWidget is no longer confusingly implementing functions that can't be called. InProcessCompositorWidget stores an nsWindow and uses CompositorVsyncDispatcher. CompositorWidgetParent exposes a VsyncObserver for CompositorBridgeParent in the GPU process.

rm outdated comment

A few static functions in gfxPlatform query Preferences instead of gfxPrefs, which breaks in the GPU process.

Comment on attachment 8771853 [details] [diff] [review] part 4, use gfxPrefs for vsync prefs Review of attachment 8771853 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for cleaning this up, just wondering why the pref needs to be a live one? ::: gfx/thebes/gfxPrefs.h @@ +505,5 @@ > DECL_GFX_PREF(Live, "layout.css.touch_action.enabled", TouchActionEnabled, bool, false); > DECL_GFX_PREF(Live, "layout.display-list.dump", LayoutDumpDisplayList, bool, false); > DECL_GFX_PREF(Live, "layout.display-list.dump-content", LayoutDumpDisplayListContent, bool, false); > DECL_GFX_PREF(Live, "layout.event-regions.enabled", LayoutEventRegionsEnabledDoNotUseDirectly, bool, false); > + DECL_GFX_PREF(Live, "layout.frame_rate", LayoutFrameRate, int32_t, -1); Why does this preference need to become a live preference?

Comment on attachment 8771556 [details] [diff] [review] part 3, impl v3 Review of attachment 8771556 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Sorry this took so many review cycles. ::: gfx/doc/Silk.md @@ +13,5 @@ > 1. Hardware Vsync event occurs on an OS specific *Hardware Vsync Thread* on a per monitor basis. > 2. The *Hardware Vsync Thread* attached to the monitor notifies the **CompositorVsyncDispatchers** and **RefreshTimerVsyncDispatcher**. > 3. For every Firefox window on the specific monitor, notify a **CompositorVsyncDispatcher**. The **CompositorVsyncDispatcher** is specific to one window. > +4. The **CompositorVsyncDispatcher** notifies a **CompositorWidgetVsyncObserver** when remote compositing, or a **CompositorVsyncScheduler::Observer** when compositing in-process. > +5. If remote compositing, an IPDL message is dispatched from **VsyncBridgeChild** on the UI process to **VsyncBridgeParent** on the compositor thread of the GPU process, which then dispatches to **CompositorVsyncScheduler::Observer**. nit: "If remote compositing, a vsync notification is sent from the CompositorWidgetVsyncObserver to the VsyncBridgeChild on the UI process which sends an IPDL message to the VsyncBridgeParent on the compositor thread of the GPU process, which then dispatches to CompositorVsyncSCheduler::Observer." Too many names here, sorry about that :(. ::: gfx/ipc/VsyncBridgeChild.cpp @@ +76,5 @@ > +void > +VsyncBridgeChild::NotifyVsync(TimeStamp aTimeStamp, const uint64_t& aLayersId) > +{ > + // This should be on the Vsync thread (not the Vsync I/O thread). > + MOZ_ASSERT(MessageLoop::current() != mLoop); nit: wrap MessageLoop::Current() != mLoop into something like IsOnVsyncIOThread(). ::: widget/windows/CompositorWidgetParent.cpp @@ +62,5 @@ > +void > +CompositorWidgetParent::ObserveVsync(VsyncObserver* aObserver) > +{ > + if (aObserver) > + SendObserveVsync(); nit: wrap in {} please. Thanks!

For comment 21.

(In reply to Mason Chang [:mchang] from comment #21) > Comment on attachment 8771853 [details] [diff] [review] > part 4, use gfxPrefs for vsync prefs > > Review of attachment 8771853 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for cleaning this up, just wondering why the pref needs to be a live > one? Using Preferences is live, so I was trying to keep the old behavior. I don't know if it *actually* needs to be live though.

(In reply to David Anderson [:dvander] from comment #24) > (In reply to Mason Chang [:mchang] from comment #21) > > Comment on attachment 8771853 [details] [diff] [review] > > part 4, use gfxPrefs for vsync prefs > > > > Review of attachment 8771853 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Thanks for cleaning this up, just wondering why the pref needs to be a live > > one? > > Using Preferences is live, so I was trying to keep the old behavior. I don't > know if it *actually* needs to be live though. Oh TIL. Well we didn't respect the liveness of it anyway then.

Comment on attachment 8771853 [details] [diff] [review] part 4, use gfxPrefs for vsync prefs Review of attachment 8771853 [details] [diff] [review]: ----------------------------------------------------------------- Just change the pref back to a Once pref. Thanks! ::: gfx/thebes/gfxPrefs.h @@ +505,5 @@ > DECL_GFX_PREF(Live, "layout.css.touch_action.enabled", TouchActionEnabled, bool, false); > DECL_GFX_PREF(Live, "layout.display-list.dump", LayoutDumpDisplayList, bool, false); > DECL_GFX_PREF(Live, "layout.display-list.dump-content", LayoutDumpDisplayListContent, bool, false); > DECL_GFX_PREF(Live, "layout.event-regions.enabled", LayoutEventRegionsEnabledDoNotUseDirectly, bool, false); > + DECL_GFX_PREF(Live, "layout.frame_rate", LayoutFrameRate, int32_t, -1); Why does this preference need to become a live preference?

Pushed by danderson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/de2f89a5b4c3 Add a top-level protocol for sending vsync messages to the GPU process. (bug 1285625 part 1, r=billm) https://hg.mozilla.org/integration/mozilla-inbound/rev/522249786c4c Don't expose CompositorVsyncDispatcher from CompositorWidget. (bug 1285625 part 2, r=mchang) https://hg.mozilla.org/integration/mozilla-inbound/rev/3fdb8ca3e6ff Implement vsync notification for remote compositors. (bug 1285625 part 3, r=mchang) https://hg.mozilla.org/integration/mozilla-inbound/rev/22fd4e206012 Don't use Preferences to get the vsync rate pref. (bug 1285625 part 4, r=mchang)

https://hg.mozilla.org/mozilla-central/rev/de2f89a5b4c3 https://hg.mozilla.org/mozilla-central/rev/522249786c4c https://hg.mozilla.org/mozilla-central/rev/3fdb8ca3e6ff https://hg.mozilla.org/mozilla-central/rev/22fd4e206012

I've only skimmed this bug so I might be missing something, but wouldn't it make more sense for the vsync notifications to come from the GPU process?

It's not clear which is better. If the GPU process goes down then we have to restart vsync on the UI process, and then move it again when the GPU process starts up. I think it's easier to keep it in the UI for now. I suspect longer-term we'll look into removing only d3d11/d3d9 calls and keeping the compositor in the UI (chromium style). This would remove a lot of the negotiating, boilerplate, and wrapper logic we need with the current approach - at the expense of making layers more complicated. But I think it's worth looking into that before moving more stuff out of process.

s/removing/remoting

(In reply to David Anderson [:dvander] from comment #30) > It's not clear which is better. If the GPU process goes down then we have to > restart vsync on the UI process, and then move it again when the GPU process > starts up. I think it's easier to keep it in the UI for now. Do we need vsync for the period of time when GPU process is coming back up? I'd think we'd only have to move it if we give up on GPU process altogether.

I'm not sure - probably not, unless we want to paint with software until the GPU process comes back online. (Chrome seems to do something like this - the GPU process comes back silently after a bit, but it's not clear in what capacity.)