1263053 - BasicCompositor on OS X is using sync transactions

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Markus Stange [:mstange]]

Sync transactions are bad.

I noticed this on https://www.just-eat.ca/restaurants-casa-di-giorgio-queen-east/menu?opt=0a765732 because the sidebars were jittering much more than they need to.

Comment 1

[Sotaro Ikeda [:sotaro]]

:mstange, is it BasicCompositor specific problem?

Comment 2

[Markus Stange [:mstange]]

Yes. It looks like we're not using tiles when BasicCompositor is used. Using tiles might fix both this bug and bug 1262863.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Enable tiling on BasicCompositor

Comment 4

[Nicolas Silva [:nical]]

Comment on attachment 8741244 [details] [diff] [review]
patch - Enable tiling on BasicCompositor

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

::: gfx/layers/client/ClientPaintedLayer.cpp
@@ +164,5 @@
>  #ifndef MOZ_X11
>        && (AsShadowForwarder()->GetCompositorBackendType() == LayersBackend::LAYERS_OPENGL ||
>            AsShadowForwarder()->GetCompositorBackendType() == LayersBackend::LAYERS_D3D9 ||
> +          AsShadowForwarder()->GetCompositorBackendType() == LayersBackend::LAYERS_D3D11 ||
> +          AsShadowForwarder()->GetCompositorBackendType() == LayersBackend::LAYERS_BASIC)

That's about all of the backends so you can just remove the whole #ifndef block!

Comment 5

[Sotaro Ikeda [:sotaro]]

Yea, I am going to update the patch.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Enable tiling on BasicCompositor

Apply the comment. Carry "r=nical".

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70ec09a17094

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/70ec09a17094

Comment 9

[Benoit Girard (:BenWa)]

This caused regression bug 1265873.

Comment 10

[Sotaro Ikeda [:sotaro]]

:BenWa, is it better to back out until bug 1265873 is addressed?

Comment 11

[Benoit Girard (:BenWa)]

We're uplifting this coming Monday so I think it is better to back this out and reland with the fix at the start of the next cycle. Unless we measured a good performance win for this patch but I don't see any Talos improvement notices.

Comment 12

[Markus Stange [:mstange]]

We're not running Talos with BasicCompositor on OS X.

Comment 13

[Sotaro Ikeda [:sotaro]]

It seems better to backout to prevent regression.

Comment 14

[Pulsebot]

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

Comment 15

[Sotaro Ikeda [:sotaro]]

backout is in aurora
  https://hg.mozilla.org/releases/mozilla-aurora/rev/fa784f0160f0

Comment 16

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=8850ff6c53b3&newProject=try&newRevision=b9585336a3ef&framework=1&showOnlyImportant=0

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bc29971aa0a

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3bc29971aa0a

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]: Improve overall performance on OS X systems without hardware acceleration.
[Links (documentation, blog post, etc)]:

Comment 20

[Marcia Knous [:marcia]]

49+ for relnote - added "Improve overall performance on OS X systems without hardware acceleration"

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Milan, do we have numbers about that?
Are we talking about 2% or 20?
thanks

Comment 22

[Sotaro Ikeda [:sotaro]]

By comment 15, it was backed out from firefox 48.

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

But it's still in 49, right?

Comment 24

[Sotaro Ikeda [:sotaro]]

(In reply to Milan Sreckovic [:milan] from comment #23)
> But it's still in 49, right?

Yes. It was checked in again by Comment 18.