1056381 - Avoid calling UpdateHitTestingTree on paints with no layer attribute changes

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

This is a follow-up to bug 1007728 (see in particular [1]), where we avoid calling APZCTreeManager::UpdatePanZoomControllerTree on repeat transactions during a paint. 

Here the aim is to also avoid calling it for paints where no layer attributes change, and thus the PZC tree wouldn't change.

One issue is that APZC instances still need to be notified of all paints so they can update their TaskThrottlers correctly, so we'd be looking at separating out the TaskThrottler updates from the rest of UPZCT. It's not clear whether this is worth the effort, so we should investigate the magnitude of the potential performance gains first.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1007728#c6

Comment 1

[Botond Ballo [:botond]]

This would involve adding some simple C++ code to track what fraction of paints involve a change to layer attributes, and then using a browser / Firefox OS phone with that code for a while to gather some data.

Comment 2

[Botond Ballo [:botond]]

TaskThrottlers are no more, so we can go back to the original plan of avoiding calling UpdatePanZoomControllerTree if no layer attributes changed.

I'll repurpose this bug for that, keeping it mentored.

Comment 3

[B. Dahse]

I would like to work on this. I most likely won't be able to start until this coming weekend, so feel free to give it away in the mean time.

Comment 4

[Botond Ballo [:botond]]

There's no rush, I'm assigning it to you.

Later this week I'll write some notes that will help you get started.

Comment 5

[B. Dahse]

(In reply to Botond Ballo [:botond] from comment #4)
> There's no rush, I'm assigning it to you.
> 
> Later this week I'll write some notes that will help you get started.

Thank you very much.

Comment 6

[Botond Ballo [:botond]]

First of all, UpdatePanZoomControllerTree has been renamed to UpdateHitTestingTree since this bug was filed :)

Comment 7

[Botond Ballo [:botond]]

So, some notes as promised:

  - The browser has a "main thread" which runs the main rendering loop (parsing HTML,
    laying out the page, etc.), and a "compositor thread" which actually renders to
    the screen. (They can actually be in different processes [1], but we abstract
    over that in the code. And there are also other threads, but they're not
    relevant for this bug.)

  - The main thread sends the compositor thread "layer transactions", which consist
    of rendered textures ("layers") and some metadata about how to arrange them on
    the screen. Layer transactions are also colloquially called "paints".

  - The compositor thread maintains a data structure called a "hit testing tree",
    used during processing of input events. Every time a layer transaction arrives,
    this data structure is updated.

  - A layer transaction consists of different "edits" to the layer tree (which is
    another data structure maintained by the compositor). Each edit has a type.
    The edit types are listed here [2]. They're things like "create a new layer",
    "change the position of a layer in the tree", "change the attributes of a
    layer", etc.

  - Only some types of edits require an update to the hit testing tree. But right
    now, we update the hit testing tree on every layer transaction. The purpose
    of this bug is to implement an optimization where we only update the hit
    testing tree for layer transactions that include an edit that requires it.

I'll say a bit more later.

[1] https://wiki.mozilla.org/Electrolysis
[2] https://dxr.mozilla.org/mozilla-central/rev/043082cb7bd8490c60815f67fbd1f33323ad7663/gfx/layers/ipc/LayersMessages.ipdlh#458

Comment 8

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #7)
> I'll say a bit more later.

  - Layer transactions arrive at the compositor thread in 
    LayerTransactionParent::RecvUpdate() [1].

  - The edit types that require the hit testing tree to be updated are
    all of them EXCEPT for:
      - OpSetDiagnosticTypes
      - OpWindowOverlayChanged
      - CompositableOperation
      - OpAttachCompositable
      - OpAttachAsyncCompositable
    (I think, anyways. We'll run the exact list by :nical before landing.)

  - Updating the hit testing tree happens in 
    APZCTreeManager::UpdateHitTestingTree() [2].

  - RecvUpdate() calls UpdateHitTestingTree() by two paths:

      1) LayerTransactionParent::RecvUpdate()
         --> CompositorBridgeParent::ShadowLayersUpdated()
         --> APZCTreeManager::UpdateHitTestingTree()

      2) LayerTransactionParent::RecvUpdate()
         --> CrossProcessCompositorBridgeParent::ShadowLayersUpdated()
         --> CompositorBridgeParent::NotifyShadowTreeTransaction()
         --> APZCTreeManager::UpdateHitTestingTree()

So, the general idea is to check in RecvUpdate() whether any of the edits require a hit testing tree update, propagate that information to the call sites of UpdateHitTestingTree(), and only actually call UpdateHitTestingTree() if required.

I think that should be enough to get you started. Let me know if you have any questions!

[1] https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/gfx/layers/ipc/LayerTransactionParent.cpp#237
[2] https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/gfx/layers/apz/src/APZCTreeManager.h#139

Comment 9

[B. Dahse]

Attachment: only update hit testing tree when necessary, 1st revision

I was unsure of if the condition should be added to the existing if-statement or added to a new one, in e.g. CompositorBridgeParent::ShadowLayersUpdated

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8752447 [details] [diff] [review]
only update hit testing tree when necessary, 1st revision

Review of attachment 8752447 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good other than the issue described below!

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ +1403,5 @@
>  
>    if (mApzcTreeManager && !aIsRepeatTransaction) {
>      AutoResolveRefLayers resolve(mCompositionManager);
> +
> +    if (aHitTestUpdate) {

Here, we can add the condition into the enclosing "if" statement.

::: gfx/layers/ipc/LayerTransactionParent.cpp
@@ +278,1 @@
>    for (EditArray::index_type i = 0; i < cset.Length(); ++i) {

Note that there is a loop here, meaning that a transaction can consist of multiple edits, possibly of different types.

The way this is currently written, if a transaction contains an edit of a type that needs to trigger a hit testing tree update, and an edit of a type that doesn't, we'll set updateHitTestingTree to false, which is not what we want. We want to set it to true as long as there is at least one edit that requires an update.

::: gfx/layers/ipc/ShadowLayersManager.h
@@ +26,5 @@
>                                       bool aScheduleComposite,
>                                       uint32_t aPaintSequenceNumber,
>                                       bool aIsRepeatTransaction,
> +                                     int32_t aPaintSyncId,
> +                                     bool aHitTestUpdate = true) = 0;

Since this has only one call site, there's no need to have a default argument. Same goes for the declarations of the implementing functions, and for NotifyShadowTreeTransaction().

Comment 11

[B. Dahse]

Attachment: only update hit testing tree when necessary, 2nd revision

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8752986 [details] [diff] [review]
only update hit testing tree when necessary, 2nd revision

Review of attachment 8752986 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

Flagging :nical for feedback on whether we got the set of edits that require hit testing tree updates right. Basically, we're interested in anything that changes the layer structure or the attributes of a layer, but not other things like texture uploads.

Comment 13

[Botond Ballo [:botond]]

Meanwhile, I've also pushed the patch to the Try server to make sure it's passing tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=af935fd90977

Comment 14

[B. Dahse]

What are the Try server results telling us?

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8752986 [details] [diff] [review]
only update hit testing tree when necessary, 2nd revision

Review of attachment 8752986 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay; I've been off sick for a few days.

The Try server results look good, this should be good to land.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae0dd17a0c86

Comment 17

[Botond Ballo [:botond]]

^ Landed

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ae0dd17a0c86

Comment 19

[Botond Ballo [:botond]]

Thanks for your work here, bd339!

Comment 20

[B. Dahse]

(In reply to Botond Ballo [:botond] from comment #19)
> Thanks for your work here, bd339!

You are very welcome.