1137905 - Enable vsync compositor on OS X

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[Mason Chang [Inactive] [:mchang]]

Turn on the vsync compositor on OS X.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: Enable vsync compositor on os x

Landing the vsync compositor on os x. Waiting for bug 1128690 to land first, then will land this.

Comment 2

[Mason Chang [Inactive] [:mchang]]

Try result - https://treeherder.mozilla.org/#/jobs?repo=try&revision=55d3493940ba

Comment 3

[Mason Chang [Inactive] [:mchang]]

From http://compare-talos.mattn.ca/dev/?oldBranch=Try&oldRevs=742011f28b13&newBranch=Try&newRev=55d3493940ba&submit=true

Two talos regressions. 1 on tresize, and another on tscrollx. The tscrollx regression goes away once we enable the refresh driver from https://bugzilla.mozilla.org/show_bug.cgi?id=1128690#c7.

Comment 4

[Mason Chang [Inactive] [:mchang]]

Also looks like CanvasMark is "higher is better". So we improve CanvasMark a bit in this case, and I can't reproduce the 1ms regression on tresize. Waiting on try to hopefully get a profile.

Comment 5

[Mason Chang [Inactive] [:mchang]]

Two profiles scrolling engadget:

silk:
http://people.mozilla.org/~bgirard/cleopatra/#report=d6f32f5cdfd38a068ff2de45637f30b77aebb55b

off
http://people.mozilla.org/~bgirard/cleopatra/#report=e43e4b5c6ae6995202bd24be8d7f9883fc1e5432

Silk w/ compositor only sometimes misses a composite and janks because the refresh driver finishes late and we wait until the next vsync. This is the same problem we have on b2g [1], but is fixed by the vsync refresh driver. Otherwise, the composite uniformity is a lot better.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp?from=CompositorParent.cpp#270

Comment 6

[Mason Chang [Inactive] [:mchang]]

A final check before landing this, based on parent 43fb1f92e8d4

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdc0cdab1b58

Comment 7

[Mason Chang [Inactive] [:mchang]]

From https://bugzilla.mozilla.org/show_bug.cgi?id=1123762#c31

Asking for QA check before landing.

Comment 8

[Mason Chang [Inactive] [:mchang]]

From irc, ask Marcia Knous for a QA check. 

@Marcia - You can test this on OS X by going into about:config and setting these two prefs to true:

gfx.vsync.compositor
gfx.vsync.hw-vsync.enabled

Can you please test this to make sure everything looks ok to you? It's a high risk patch, thanks!

Comment 9

[Marcia Knous [:marcia]]

Sorry Mason - I don't handle testing the graphics area of QA at the moment. If you post a link to the tryserver build, I can try to get some community eyes on it if that will help.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The change has very little to do with graphics specifically -- it will affect literally all aspects of the browser.  Flipping the pref and going through any QA work would be valuable for this change.

Comment 11

[Mason Chang [Inactive] [:mchang]]

I've had a few people test this daily for the past couple of days. I've been running it for about three weeks on nightly and dev-edition. So far it has been pretty stable, but yes if QA can test that would be great.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5c077c2831

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f5c077c2831

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

after uplifting to aurora, we see a 5.6% osx 10.6 regression for the tresize test.  I am not sure if that is related to this.  How could we determine this is[not] the cause?

here is a graph highlighting the regression:
http://graphs.mozilla.org/graph.html#tests=[[254,52,21],[254,63,21]]&sel=1425949157752.5168,1427158615295.798,15.245901639344261,26.557377049180328&displayrange=30&datatype=geo

odd we didn't see any alerts generated for this.

Comment 14

[Mason Chang [Inactive] [:mchang]]

(In reply to Joel Maher (:jmaher) from comment #13)
> after uplifting to aurora, we see a 5.6% osx 10.6 regression for the tresize
> test.  I am not sure if that is related to this.  How could we determine
> this is[not] the cause?
> 
> here is a graph highlighting the regression:
> http://graphs.mozilla.org/graph.html#tests=[[254,52,21],[254,63,
> 21]]&sel=1425949157752.5168,1427158615295.798,15.245901639344261,26.
> 557377049180328&displayrange=30&datatype=geo
> 
> odd we didn't see any alerts generated for this.

Hmm, I'm confused. Why was this uplifted to aurora? It shouldn't have been. This should only be enabled on 39. Are those charts on aurora on Gecko 38? It looks like it has also merged back to equal as of two days ago?

In terms of actually causing a regression, this could be the cause. You can check by setting these preferences to false:

gfx.vsync.hw-vsync.enabled
gfx.vsync.compositor
gfx.vsync.refreshdriver

A lot of the discussion for tresize is in bug 1138502, which should've fixed most of it. Just to make sure though, why was this uplifted to aurora? Aurora should be 39 now, so there shouldn't have been any uplifts..

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

I don't think this patch was specifically landed on aurora, we uplifted v.39 from trunk -> aurora on Monday March 30th, so as this is on 39 it is on aurora.  On Aurora our values have changed since the uplift:
http://graphs.mozilla.org/graph.html#tests=[[254,52,21]]&sel=1425493731300,1428085731300&displayrange=30&datatype=geo

should I push to try with those preferences set to false to see if that fixes the issue on aurora?

Comment 16

[Mason Chang [Inactive] [:mchang]]

(In reply to Joel Maher (:jmaher) from comment #15)
> I don't think this patch was specifically landed on aurora, we uplifted v.39
> from trunk -> aurora on Monday March 30th, so as this is on 39 it is on
> aurora.  On Aurora our values have changed since the uplift:
> http://graphs.mozilla.org/graph.html#tests=[[254,52,21]]&sel=1425493731300,
> 1428085731300&displayrange=30&datatype=geo
> 
> should I push to try with those preferences set to false to see if that
> fixes the issue on aurora?

Sure, we can see if this is the issue. So what I think this means is that 39 regressed versus 38 then? We're comparing aurora 38 vs aurora 39 right?

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

yes, this would be aurora 38 (last week) to aurora 39 (this week).

Comment 18

[Mason Chang [Inactive] [:mchang]]

Hmm, I wonder why the emails didn't get sent. We should have a known 3% due to this, that we really can't do much about. When I investigated it, the 3% regression wasn't "real" in the sense that the compositor just had lucky timing before versus now we wait for vsync. See http://compare-talos.mattn.ca/dev/?oldBranch=Try&oldRevs=b77ed0031ed6&newBranch=Try&newRev=c2ee333a0503&submit=true

The other 3%, not really where it came from.

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

ok, sounds like this is known and accepted!  thanks for the info.

Comment 20

[Mason Chang [Inactive] [:mchang]]

(In reply to Joel Maher (:jmaher) from comment #19)
> ok, sounds like this is known and accepted!  thanks for the info.

Yeah 3% should be known, the other 3% I don't really know. You can try disabling it and see if it actually covers all 6%, versus the other 3% went somewhere else.