Closed Bug 1441324 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files)

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.
Blocks: 1432858
Priority: -- → P3
Whiteboard: [gfx-noted]
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
Assignee: nobody → bugmail
@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.
Attachment #8959534 - Flags: review?(nfroyd) → review+
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.
Attachment #8959531 - Flags: review?(rhunt) → review+
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
Attachment #8959532 - Flags: review?(rhunt) → review+
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.
Attachment #8959533 - Flags: review?(rhunt) → review+
Comment on attachment 8959534 [details]
Bug 1441324 - Move the input event messages from PAPZCTreeManager to PAPZInputBridge.

https://reviewboard.mozilla.org/r/228336/#review234340
Attachment #8959534 - Flags: review?(rhunt) → review+
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
You need to log in before you can comment on or make changes to this bug.