1441324 - Make the APZ controller thread on GPU-process-enabled windows the GPU process main thread instead of the compositor thread

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

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

Comment 2

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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

Comment 4

[Kartikaya Gupta (email:[e-mail])]

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

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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).

Comment 6

[Kartikaya Gupta (email:[e-mail])]

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

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1441324 - Extract an APZInputBridge interface from IAPZCTreeManager.

This separates the methods that are used to deliver input events
synchronously over IPDL to the compositor; this interface will be
remoted over a new APZInputBridge IPDL protocol in future patches.

Review commit: https://reviewboard.mozilla.org/r/228330/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228330/

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1441324 - Move the APZCTreeManager initialization for the GPU process to CompositorBridgeParent initialization.

This is important because the RecvInitialize method in CompositorBridgeParent
is run via a sync IPC message, and so we are guaranteed that when return to the
caller in the UI process, the APZCTreeManager will have been created. This
ensures that when we create the APZInputBridge actors (which will happen on a
different top-level protocol, but be triggered after the sync RecvInitialize
is complete) we know that the concrete APZCTreeManager is ready for use.

Review commit: https://reviewboard.mozilla.org/r/228332/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228332/

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1441324 - Introduce an empty APZInputBridge protocol managed by PGPU.

This just adds the boilerplate that goes with the new protocol, without
adding any of the actual messages. The protocol is managed by PGPU, and
there will be one instance per compositor. The parent side lives on the
main thread of the GPU process, and the child side lives on the main
thread of the UI process. The protocol is only instantiated if the GPU
process is active.

Review commit: https://reviewboard.mozilla.org/r/228334/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228334/

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1441324 - Move the input event messages from PAPZCTreeManager to PAPZInputBridge.

This remotes the APZInputBridge interface over the PAPZInputBridge
protocol in the case of the GPU process. If there is no GPU process we
continue as before and the APZInputBridge interface implementation is
the concrete APZCTreeManager instance in the UI process. The main
changes in this patch are moving all the code associated with these
messages out of APZCTreeManager{Parent,Child} and into
APZInputBridge{Parent,Child}. APZCTreeManagerChild now returns an
APZInputBridgeChild instance via InputBridge(), instead of returning
itself.

Review commit: https://reviewboard.mozilla.org/r/228336/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228336/

Comment 11

[Kartikaya Gupta (email:[e-mail])]

@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 12

[Nathan Froyd [:froydnj]]

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.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

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 14

[Ryan Hunt [:rhunt]]

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 15

[Ryan Hunt [:rhunt]]

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 16

[Ryan Hunt [:rhunt]]

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 17

[Ryan Hunt [:rhunt]]

Comment on attachment 8959534 [details]
Bug 1441324 - Move the input event messages from PAPZCTreeManager to PAPZInputBridge.

https://reviewboard.mozilla.org/r/228336/#review234340

Comment 18

[Ryan Hunt [:rhunt]]

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

Comment 19

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8959531 [details]
Bug 1441324 - Extract an APZInputBridge interface from IAPZCTreeManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228330/diff/1-2/

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8959532 [details]
Bug 1441324 - Move the APZCTreeManager initialization for the GPU process to CompositorBridgeParent initialization.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228332/diff/1-2/

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8959533 [details]
Bug 1441324 - Introduce an empty APZInputBridge protocol managed by PGPU.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228334/diff/1-2/

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8959534 [details]
Bug 1441324 - Move the input event messages from PAPZCTreeManager to PAPZInputBridge.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228336/diff/1-2/

Comment 25

[Kartikaya Gupta (email:[e-mail])]

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

Comment 27

[Andrei Ciure[:aciure]]

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