There is relatively-frequently occurring problem where the main process main thread gets blocked on Windows, because it sends a sync input event to the compositor thread and waits for it to get untransformed before proceeding. However the compositor thread is busy doing compositing work and so this blocks the main thread unnecessarily. If we move the APZ controller thread to a different thread (e.g. the GPU process's main thread, which is mostly idle) then we might be able to solve this.

This is slightly less trivial than I thought. Right now the PAPZCTreeManager protocol, which is where these sync IPDL messages live, is created in three possible scenarios: main process main thread <--> GPU process compositor thread content process main thread <--> GPU process compositor thread content process main thread <--> main process compositor thread (if GPU process is not enabled) Instead, we'd want it to be like so: main process main thread <--> GPU process main thread content process main thread <--> GPU process compositor thread content process main thread <--> main process compositor thread (if GPU process is not enabled) (i.e. in the first case we want to attach to the GPU process main thread instead of the GPU process compositor thread). The problem here is that it can no longer be a sub-protocol of PCompositorBridge, because PCompositorBridge is going to attach on the compositor thread and you can't have a subprotocol attach on a different thread. So this sort of implies we want to break out PAPZCTreeManager into a standalone protocol. But we can't do that because for the content process cases we need to maintain ordering of messages with respect to things that are going over PCompositorBridge (e.g. the layer transaction messages). So maybe we need to extract another protocol from PAPZCTreeManager that just contains the sync event delivery messages, and make that a top-level protocol that connects the main process main thread to the GPU process main thread. I'll think about this more tomorrow and double-check what threads things are running on. [1] https://searchfox.org/mozilla-central/rev/056a4057575029f58eb69ae89bc7b6c3c11e2e28/gfx/layers/ipc/PAPZCTreeManager.ipdl#49-53

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1) > So maybe we need to extract another protocol from PAPZCTreeManager that just > contains the sync event delivery messages, and make that a top-level > protocol that connects the main process main thread to the GPU process main > thread. It does seem like the sync messages in PAPZCTreeManager, which are conceptually about handling input at the widget level, are orthogonal to the other messages, which are conceptually about content passing information / requests to APZ. So separating the sync messages out seems like a reasonable idea to me.

I have a WIP patchset. It should have all the necessary pieces but likely has bugs. Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=5102cd0338e027bd21a895cba1fa22278965e5f4

Updated WIP: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b114d4070ccb5a4572d09662377304a63d1e7d99

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0df6bcb5fb9a2fe7edf4eed1d78aea2d2fa89ac7 Looking good. (The patches visible in this try push build on top of the patches in bug 1445662).

Final try push with some more tweaks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae3ccb9954006a361b9491bb4a5d17410e2c0a8c

@froydnj: I need an IPC peer to r+ the sync-messages.ini changes. It's just moving a bunch of sync messages from one protocol to another. Although if you want to take a look at the IPC setup in part 3 to make sure there's obvious problems I'd appreciate that too.

Comment on attachment 8959534 [details] Bug 1441324 - Move the input event messages from PAPZCTreeManager to PAPZInputBridge. https://reviewboard.mozilla.org/r/228336/#review234234 r=me on the sync-messages.ini change.

The try push in comment 6 exposed some unified build failures on windows on m-c. Here's a windows-only try push with an extra patch to fix those: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6198177bcf868d639b24c2239f22edd99eb0885

Comment on attachment 8959531 [details] Bug 1441324 - Extract an APZInputBridge interface from IAPZCTreeManager. https://reviewboard.mozilla.org/r/228330/#review234286 ::: gfx/layers/apz/public/APZInputBridge.h:22 (Diff revision 1) > +namespace layers { > + > +struct ScrollableLayerGuid; > + > +/** > + * This class lives on the main thread of the main process, and provides a Is it the controller thread of the main process? Or only the main thread of the main process. I think this interface will always be on the controller thread, but I suppose further constrained to the main thread so it can use PGPU. So I suppose either is fine. ::: gfx/layers/apz/public/IAPZCTreeManager.h:134 (Diff revision 1) > > + /** > + * Returns an APZInputBridge interface that can be used to send input > + * events to APZ in a synchronous manner. This will always be non-null, and > + * the returned object's lifetime will match the lifetime of this > + * IAPZCTreeManager implementation. It may be worth adding here that this function is only valid to call within the UI process.

Comment on attachment 8959532 [details] Bug 1441324 - Move the APZCTreeManager initialization for the GPU process to CompositorBridgeParent initialization. https://reviewboard.mozilla.org/r/228332/#review234338

Comment on attachment 8959533 [details] Bug 1441324 - Introduce an empty APZInputBridge protocol managed by PGPU. https://reviewboard.mozilla.org/r/228334/#review234324 ::: gfx/ipc/GPUProcessManager.cpp:832 (Diff revision 1) > return nullptr; > } > apz = static_cast<APZCTreeManagerChild*>(papz); > + > + PAPZInputBridgeChild* pinput = mGPUChild->SendPAPZInputBridgeConstructor(aRootLayerTreeId); > + apz->SetInputBridge(static_cast<APZInputBridgeChild*>(pinput)); We should null check PAPZInputBridgeChild here like we do with PAPZCTreeManagerChild earlier in the function. Just in case IPC goes down and we need to create a new GPU process and restart.

Comment on attachment 8959534 [details] Bug 1441324 - Move the input event messages from PAPZCTreeManager to PAPZInputBridge. https://reviewboard.mozilla.org/r/228336/#review234340

Thanks for the well designed patch set! I hope this makes a difference, it's a good change.

(In reply to Ryan Hunt [:rhunt] from comment #14) > > + * This class lives on the main thread of the main process, and provides a > > Is it the controller thread of the main process? Or only the main thread of > the main process. > > I think this interface will always be on the controller thread, but I > suppose further constrained to the main thread so it can use PGPU. So I > suppose either is fine. Hm, good point. When I wrote the comment I was mostly thinking of the PGPU case, where the controller thread is the same as the main thread, and "main thread" is more specific, so I went with that. But it's true that the APZInputBridge interface is also used without the PGPU stuff, and in that case, it would be more correct to say it's used on the controller thread (which is not the main thread on Android, for example). I'll reword the comment a little bit to make sure it's more clear. > ::: gfx/layers/apz/public/IAPZCTreeManager.h:134 > > + /** > > + * Returns an APZInputBridge interface that can be used to send input > > + * events to APZ in a synchronous manner. This will always be non-null, and > > + * the returned object's lifetime will match the lifetime of this > > + * IAPZCTreeManager implementation. > > It may be worth adding here that this function is only valid to call within > the UI process. Good point, will do. (In reply to Ryan Hunt [:rhunt] from comment #16) > > + > > + PAPZInputBridgeChild* pinput = mGPUChild->SendPAPZInputBridgeConstructor(aRootLayerTreeId); > > + apz->SetInputBridge(static_cast<APZInputBridgeChild*>(pinput)); > > We should null check PAPZInputBridgeChild here like we do with > PAPZCTreeManagerChild earlier in the function. > > Just in case IPC goes down and we need to create a new GPU process and > restart. Ah good catch, I'll add that also.

I also squashed the unified build fixes into part 3, since that's the part that fiddles with moz.build files and exposes the faults. And slightly updated the commit message of part 4 to explicitly mention that the controller thread in the GPU process is now changed to be the main thread.

Here's a build-only try push with the latest versions on m-c to make sure there's no more unified build failures that snuck in: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11fcee7e65a20b20c86157636e53a493b62504d4

Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c85a2550e02c Extract an APZInputBridge interface from IAPZCTreeManager. r=rhunt https://hg.mozilla.org/integration/autoland/rev/908c0e96e38c Move the APZCTreeManager initialization for the GPU process to CompositorBridgeParent initialization. r=rhunt https://hg.mozilla.org/integration/autoland/rev/91e7da46a9b1 Introduce an empty APZInputBridge protocol managed by PGPU. r=rhunt https://hg.mozilla.org/integration/autoland/rev/ccc6b9010664 Move the input event messages from PAPZCTreeManager to PAPZInputBridge. r=froydnj,rhunt

https://hg.mozilla.org/mozilla-central/rev/c85a2550e02c https://hg.mozilla.org/mozilla-central/rev/908c0e96e38c https://hg.mozilla.org/mozilla-central/rev/91e7da46a9b1 https://hg.mozilla.org/mozilla-central/rev/ccc6b9010664