1007728 - Remove UpdatePanZoomControllerTree calls on repeat transactions

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

With tiling enabled, we can get multiple layers updates for a single paint. Only the first of them actually brings changes to the layers tree; the others ("repeat transactions") just involve transferring additional textures.

We currently call UpdatePanZoomControllerTree() for all layers updates, but it's unnecessary to call them for repeat transactions UPZCT() is only interested in changes to the layer tree / metrics.

I propose that we remove the call to UPZCT() for repeat transactions. In addition to the performance benefit, this would make our lives easier in bug 961289, because a single paint will only be associated with a single PZC tree update, so it's easier to correlate them for testing purposes.

BenWa suggested that we could go even further and omit calling UPZCT() for transactions that do not change the layer tree, even if they are not repeat transactions.

Comment 1

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

(In reply to Botond Ballo [:botond] from comment #0)
> BenWa suggested that we could go even further and omit calling UPZCT() for
> transactions that do not change the layer tree, even if they are not repeat
> transactions.

That would be good too. Is it possible to detect that easily? Note that we still want it called in cases where the structure of the layer tree doesn't change, but the FrameMetrics on the layers does change.

Comment 2

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)
> (In reply to Botond Ballo [:botond] from comment #0)
> > BenWa suggested that we could go even further and omit calling UPZCT() for
> > transactions that do not change the layer tree, even if they are not repeat
> > transactions.
> 
> That would be good too. Is it possible to detect that easily? Note that we
> still want it called in cases where the structure of the layer tree doesn't
> change, but the FrameMetrics on the layers does change.

BenWa would know this better, but I believe we can tell by looking at the type of Edits in the transaction [1].
Some edit types, like TOpCreateXXXLayer and TOpSetLayerAttributes are of interest to UPZCT, while others, like presumably TOpAttachCompositable, are not. We can add logic to only call UPZCT if we have at least one edit of interest to it.

[1] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/LayerTransactionParent.cpp?from=LayerTransactionparent.cpp#221

Comment 3

[Botond Ballo [:botond]]

Attachment: bug1007728.patch

Here's a patch that implements the basic concept (avoiding UpdatePanZoomControllerTree for repeat transactions).

I'd like to improve this to also avoid calling UPZCT on an initial transaction if that transaction did not make a change to the layer tree, as described in earlier comments.

Comment 4

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

Something I thought of yesterday - we should make sure that this doesn't break paint throttling (the call at http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=1d2b67fc93f7#2049 is kinda important)

Comment 5

[Botond Ballo [:botond]]

Attachment: bug1007728.patch

Unbitrotted.

Comment 6

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Something I thought of yesterday - we should make sure that this doesn't
> break paint throttling (the call at
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/
> AsyncPanZoomController.cpp?rev=1d2b67fc93f7#2049 is kinda important)

That's a good point!

Not calling UPZCT during repeat transactions is fine in this regard - if anything, calling it for repeat transactions gives misleading results, because if there is a paint outstanding when the first transaction for a paint arrives, the second transaction for that paint will be treated as the completion of the new paint.

However, not calling UPZCT for a first transaction of a paint when the paint does not involve a change to the layer tree structure of layer attributes means we miss a TaskComplete() call that we care about.

I discussed this with BenWa, and I propose the following:

  1) As a first step, just omit UPZCT calls for repeat transaction.
     This fixes something currently broken with the paint throttler,
     doesn't break anything else, and is an efficiency gain.

  2) BenWa believes that the additional efficiency gain from not
     doing UPZCT on first transactions that don't change the
     layer tree in relevant ways - for example, drawing on a
     canvas at 60 fps - might be valuable. So as a follow-up,
     investigate the potential performance gain here, and if so,
     split out the TaskComplete() call (which needs to happen
     on every paint) from the rest of the work in UPZCT (which
     doesn't).

Accordingly, I will post my current patch, which does just (1), for review, and file a follow-up for (2).

Comment 7

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

Comment on attachment 8459841 [details] [diff] [review]
bug1007728.patch

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

That's a lot of changes for one bool. But looks ok to me.

Comment 8

[Botond Ballo [:botond]]

https://tbpl.mozilla.org/?tree=Try&rev=5695f3449f06

Comment 9

[Botond Ballo [:botond]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #6)
> Accordingly, I will post my current patch, which does just (1), for review,
> and file a follow-up for (2).

Filed bug 1056381 for (2).