Avoid calling UpdateHitTestingTree on paints with no layer attribute changes

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: botond, Assigned: ronoueb, Mentored)

Tracking

({perf})

Trunk
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [lang=c++][gfx-noted])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
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.
Mentor: botond
Whiteboard: [lang=c++]
Keywords: perf
Whiteboard: [lang=c++] → [lang=c++][gfx-noted]
(Reporter)

Comment 2

3 years ago
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.
Summary: Investigate potential performance gains from avoiding UpdatePanZoomControllerTree calls on paints with no layer attribute changes → Avoid calling UpdatePanZoomControllerTree on paints with no layer attribute changes
(Assignee)

Comment 3

3 years ago
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.
(Reporter)

Comment 4

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

Later this week I'll write some notes that will help you get started.
Assignee: nobody → bd339
(Assignee)

Comment 5

3 years ago
(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.
(Reporter)

Comment 6

3 years ago
First of all, UpdatePanZoomControllerTree has been renamed to UpdateHitTestingTree since this bug was filed :)
Summary: Avoid calling UpdatePanZoomControllerTree on paints with no layer attribute changes → Avoid calling UpdateHitTestingTree on paints with no layer attribute changes
(Reporter)

Comment 7

3 years ago
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
(Reporter)

Comment 8

3 years ago
(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
(Assignee)

Comment 9

3 years ago
Created attachment 8752447 [details] [diff] [review]
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
Attachment #8752447 - Flags: feedback?(botond)
(Reporter)

Comment 10

3 years ago
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().
Attachment #8752447 - Flags: feedback?(botond)
(Assignee)

Comment 11

3 years ago
Created attachment 8752986 [details] [diff] [review]
only update hit testing tree when necessary, 2nd revision
Attachment #8752447 - Attachment is obsolete: true
Attachment #8752986 - Flags: feedback?(botond)
(Reporter)

Comment 12

3 years ago
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.
Attachment #8752986 - Flags: feedback?(nical.bugzilla)
Attachment #8752986 - Flags: feedback?(botond)
Attachment #8752986 - Flags: feedback+
(Reporter)

Comment 13

3 years ago
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
Attachment #8752986 - Flags: feedback?(nical.bugzilla) → feedback+
(Assignee)

Comment 14

3 years ago
What are the Try server results telling us?
Flags: needinfo?(botond)
(Reporter)

Comment 15

3 years ago
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.
Attachment #8752986 - Flags: feedback+ → review+
(Reporter)

Comment 17

3 years ago
^ Landed
Flags: needinfo?(botond)

Comment 18

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ae0dd17a0c86
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Reporter)

Comment 19

3 years ago
Thanks for your work here, bd339!
(Assignee)

Comment 20

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

You are very welcome.
You need to log in before you can comment on or make changes to this bug.