Matt Woodrow (:mattwoodrow) Assignee
	Description • 8 years ago

This is a paint test, so waiting for the paint event is the right thing to do.

We get meaningless results on e10s without this, so it's important to fix.

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 2 • 8 years ago

Local results:

Non-e10s No MozAfterPaint


[#0] big-optimizable-group-opacity-2500.svg  Cycles:25  Average:3378.04  Median:3378.00  stddev:246.17 (7.3%)  stddev-sans-first:187.20
Values: 4167.0  3502.0  3451.0  3505.0  3326.0  3250.0  3538.0  3378.0  3253.0  3264.0  3214.0  3348.0  3491.0  3595.0  3347.0  3302.0  2863.0  3581.0  3496.0  2987.0  3431.0  3498.0  3426.0  3133.0  3105.0

[#1] small-group-opacity-2500.svg  Cycles:25  Average:1687.92  Median:1718.00  stddev:154.88 (9.0%)  stddev-sans-first:134.44
Values: 1296.0  1930.0  1499.0  1725.0  1715.0  1718.0  1717.0  1733.0  1710.0  1993.0  1804.0  1783.0  1330.0  1718.0  1457.0  1668.0  1722.0  1759.0  1648.0  1652.0  1760.0  1754.0  1675.0  1753.0  1679.0


e10s No MozAfterPaint


[#0] big-optimizable-group-opacity-2500.svg  Cycles:25  Average:498.16  Median:501.00  stddev:28.62 (5.7%)  stddev-sans-first:25.30
Values: 567.0  515.0  528.0  539.0  502.0  462.0  519.0  475.0  505.0  472.0  501.0  460.0  482.0  466.0  514.0  466.0  496.0  500.0  481.0  536.0  457.0  526.0  511.0  473.0  501.0

[#1] small-group-opacity-2500.svg  Cycles:25  Average:777.72  Median:785.00  stddev:42.79 (5.5%)  stddev-sans-first:19.95
Values: 595.0  826.0  805.0  801.0  790.0  797.0  800.0  783.0  792.0  782.0  798.0  755.0  785.0  785.0  745.0  753.0  740.0  784.0  785.0  786.0  792.0  808.0  781.0  783.0  792.0


Non-e10s MozAfterPaint


[#0] big-optimizable-group-opacity-2500.svg  Cycles:25  Average:3018.84  Median:2971.00  stddev:163.27 (5.5%)  stddev-sans-first:148.02
Values: 3380.0  2971.0  2845.0  2962.0  2971.0  2919.0  2912.0  2891.0  2919.0  3047.0  3099.0  3030.0  2993.0  2852.0  2835.0  2913.0  2944.0  3004.0  2877.0  3059.0  2945.0  3294.0  3245.0  3133.0  3431.0

[#1] small-group-opacity-2500.svg  Cycles:25  Average:1677.52  Median:1748.00  stddev:132.62 (7.6%)  stddev-sans-first:122.34
Values: 1951.0  1596.0  1396.0  1669.0  1750.0  1542.0  1742.0  1761.0  1800.0  1770.0  1526.0  1748.0  1526.0  1524.0  1537.0  1760.0  1796.0  1762.0  1519.0  1751.0  1753.0  1781.0  1778.0  1523.0  1677.0


e10s MozAfterPaint


[#0] big-optimizable-group-opacity-2500.svg  Cycles:25  Average:2837.16  Median:2819.00  stddev:83.19 (3.0%)  stddev-sans-first:84.98
Values: 2842.0  2780.0  2961.0  2934.0  2865.0  2918.0  2794.0  3049.0  2863.0  2823.0  2828.0  2773.0  2845.0  2757.0  2789.0  2685.0  2734.0  2816.0  2872.0  2802.0  2819.0  3002.0  2798.0  2789.0  2791.0

[#1] small-group-opacity-2500.svg  Cycles:25  Average:1179.52  Median:1173.00  stddev:43.37 (3.7%)  stddev-sans-first:42.93
Values: 1128.0  1168.0  1165.0  1166.0  1173.0  1196.0  1170.0  1180.0  1196.0  1173.0  1160.0  1166.0  1173.0  1195.0  1154.0  1177.0  1156.0  1161.0  1175.0  1178.0  1188.0  1175.0  1183.0  1157.0  1375.0

	Avi Halachmi (:avih)
	Comment 3 • 8 years ago

Comment on attachment 8722758 [details] [diff] [review]
mozafterpaint-svg-opacity

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

The approach seems fine to me, but Joel owns test.py so I leave the r? for him.

As a reminder, we've already considered using MozAfterPaint for tsvgr_opacity in bug 1027481 almost two years ago, but for some reason it didn't seem to affect the results and we stayed with it as is.

Joel, note that this change will "regress" this test on e10s meaningfully, but it's only because it's apparently broken currently, and the new values are more correct.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 4 • 8 years ago

Comment on attachment 8722758 [details] [diff] [review]
mozafterpaint-svg-opacity

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

I believe it is this simple!

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 5 • 8 years ago

Do I land this onto m-c, or the talos repo?

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 6 • 8 years ago

land on m-c (i.e. inbound/fx-team) as you would a normal patch.  We haven't used the talos repo since September.

	Pulsebot
	Comment 7 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/10cb85773212

	Phil Ringnalda (:philor)
	Comment 8 • 8 years ago

Fun thing: we don't actually care about graphserver anymore, or use it, but we still submit results to it, so making the test name tsvgr_opacity_paint, a name which it doesn't know about, made every tsvg run fail, https://treeherder.mozilla.org/logviewer.html#?job_id=22342480&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4969deedebcc

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 9 • 8 years ago

Can you take a look at this please Joel? I'm in over my head now.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 10 • 8 years ago

on it, I will work to get these graph server additions done today in bug 1251209.  I am determined to make this the last time we need to update graph server as we are using perfherder for everything now but leaving graph server running for a few more weeks in case there is a big issue.  Sorry for missing this in the review.

	Avi Halachmi (:avih)
	Comment 11 • 8 years ago

How is it that adding mozafterpaint to test.py ends up with graphserver compatibility issue? does it implicitly change the test name which is submitted to GS somehow?

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 12 • 8 years ago

yes, we add _paint to the test name for graph server purposes.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 13 • 8 years ago

I have fixed this by getting the graph server data updated properly.  you can see a retrigger here:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2c2410d62931&filter-searchStr=svg&selectedJob=22349430

Can we reland this?

	Pulsebot
	Comment 14 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/72d4e5bba9b3

	Carsten Book [:Tomcat]
	Comment 15 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/72d4e5bba9b3

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 16 • 8 years ago

and the results are in (ignore the ts_paint and tsvgx):
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=0d8676e7dff8&newProject=mozilla-inbound&newRevision=72d4e5bba9b3&framework=1

winxp took a big hit here, win7 did as well.  linux64, osx10.10 and win8 are all better in e10s than non-e10s :)

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 17 • 8 years ago

removing the needinfo as this is relanded now.

	Avi Halachmi (:avih)
	Comment 18 • 8 years ago

So, looking at the results now, it seems that at least on windows 7 32 - e10s now appears almost twice worse than non e10s: https://treeherder.allizom.org/perf.html#/graphs?series=[mozilla-inbound,34b003f40b446290f744f15e6041dba786fe43d4,1&series=[mozilla-inbound,39515d8283103ffeb94dfa71444d71ac59cfd348,1]

It appears that the patch affected platforms as follows:

- Win7 32 and XP: non-e10s roughly unchanged, e10s went from much better to much worse than non e10s.

- Win 8 64: both regressed meaningfully, e10s is still considerably "better", but percentage-wise, the diff is smaller.

- OS X 10.10: non-e10s "regressed" slightly, while e10s "regressed" a lot, making the e10s results slightly "better" than non-e10s.

(Link to the latest e10s comparison graphs per platform available from the live e10s dashboard here: https://treeherder.allizom.org/perf.html#/e10s )

Matt, I'm guessing overall this is not expected? If indeed not, what should be the next step?

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 19 • 8 years ago

I had thought this would be expected as we were making the test measure useful things- but maybe there is something off here.

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 20 • 8 years ago

Yeah I think that's all somewhat expected.

It sounds like we have a real regression on xp/win7 due to e10s, and that will need to be investigated. Is it easy to get profiles?

	Avi Halachmi (:avih)
	Comment 21 • 8 years ago

Adding bug 1027481 for reference where we tried some approaches as alternatives to mozafterpaint for tsvg_opacity.

	Jim Mathies [:jimm]
	Comment 22 • 8 years ago

(In reply to Avi Halachmi (:avih) from comment #18)
> So, looking at the results now, it seems that at least on windows 7 32 -
> e10s now appears almost twice worse than non e10s:
> https://treeherder.allizom.org/perf.html#/graphs?series=[mozilla-inbound,
> 34b003f40b446290f744f15e6041dba786fe43d4,1&series=[mozilla-inbound,
> 39515d8283103ffeb94dfa71444d71ac59cfd348,1]

Mind filing a bug on this? This looks kinda bad.

	Avi Halachmi (:avih)
	Comment 23 • 8 years ago

(In reply to Jim Mathies [:jimm] from comment #22)
> Mind filing a bug on this? This looks kinda bad.

Looks bad, but I sill don't feel I can trust those results any better than before.

Once everyone feels these results are to be trusted (hopefully happens either here or at bug 1254628), and if e10s still shows worse than non e10s, then I'll file a bug.

	Avi Halachmi (:avih)
	Comment 24 • 8 years ago

Matt, reopening since I find-it-hard/don't-want to believe these results.

	Chris Peterson [:cpeterson]
	Comment 25 • 8 years ago

Matt and Avi, what are the next steps to determine whether we can trust MozAfterPaint (bug 1254628) or tsvgr_opacity?

tsvgr_opacity is the last metric blocking (bug 1255936) the e10s release criteria for Graphics Performance:

https://wiki.mozilla.org/Electrolysis/Release_Criteria#Graphics_Performance

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 26 • 8 years ago

I haven't been convinced that the results can't be trusted yet, so over to Avi.

	Avi Halachmi (:avih)
	Comment 27 • 8 years ago

The comparison from talos at the e10s dashboard over the last two days ( https://treeherder.allizom.org/perf.html#/e10s ) shows these subtests (only two subtests):

              non-e10s / e10s
- Win 7 big:    740    / 2400 (220% slower)
- WIn 7 small:  430    /  360 (17%  faster)

That's supposedly 2400ms to get the paint on the big file in e10s, which is 3 times slower than non e10s.


Locally, on windows 7 (64, firefox is 32) with latest nightly I get:

In talos:
              non-e10s / e10s
- Win 7 big:    530    /  450 (16% faster)
- WIn 7 small:  450    /  390 (13%  faster)

Compared to the official talos results, both agree that the small file is ~15% faster on e10s. For the big file our official talos data says it's 3x slower on e10s, while my local talos results say e10s is a bit faster.

I then tested with a local patch similar to what we had at bug 1027481 - and outside of talos.

In a nutshell, in ASAP mode, load the SVG element with display:none by default, then change it to display:block and repeat rAF 10 times, and measure how long it took. The rAF deltas look reasonable (the first one is the biggest, followed by 2-3 much smaller ones, followed by a series of 0ms deltas once it settled).

Here's what I get:

              non-e10s / e10s
- Win 7 big:    480    / 380   (20% faster)
- Win 7 small:  370    / 370   (same)


Overall, I don't think it makes sense that on the official win7 talos machines the big file is 3x worse on e10s, while on my local win7 machine it's ~20% faster.

The other platforms also show big differences between e10s to non e10s, be it improvement or regression, and I do not believe them too.

If you you still think the talos numbers make sense, sure, resolve this bug as fixed and let's deal with 3x regression in e10s.

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 28 • 8 years ago

How do I get profiles from tryserver for this?

If I can get a profile for both configurations then we should be able to figure out which one is measuring the right thing.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 29 • 8 years ago

this set of regressions is now on mozilla-beta:
https://treeherder.mozilla.org/perf.html#/alerts?id=1012&filter=opacity

	Avi Halachmi (:avih)
	Comment 30 • 8 years ago

Matt, profiler instructions: https://wiki.mozilla.org/Buildbot/Talos/Profiling

mstange also said he could give you a hand with this.

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 31 • 8 years ago

I had a go, it wasn't very successful.

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

Markus, could you help here please?

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 32 • 8 years ago

(In reply to Avi Halachmi (:avih) from comment #27)
>
> Overall, I don't think it makes sense that on the official win7 talos
> machines the big file is 3x worse on e10s, while on my local win7 machine
> it's ~20% faster.

Avi, were you forcing acceleration off in these tests?

	Avi Halachmi (:avih)
	Comment 33 • 8 years ago

Not knowingly and most probably not. I was running it plainly within talos which controls the profile and the entire firefox setup. AFAIK it should be the same setup as the talos machines.

My suspicion here is that using MozAfterPaint does not provide consistent/reliable behavior depending on some parameters we're not aware of.

	Jonathan Watt [:jwatt]
	Comment 34 • 7 years ago

(In reply to Avi Halachmi (:avih) from comment #27)
> The comparison from talos at the e10s dashboard over the last two days (
> https://treeherder.allizom.org/perf.html#/e10s ) shows these subtests (only
> two subtests):
> 
>               non-e10s / e10s
> - Win 7 big:    740    / 2400 (220% slower)
> - WIn 7 small:  430    /  360 (17%  faster)
> 
> That's supposedly 2400ms to get the paint on the big file in e10s, which is
> 3 times slower than non e10s.

I don't see this any more.

https://treeherder.allizom.org/perf.html#/graphs?series=%5Bmozilla-central,3a48cb9135ccdac0785d142353f5c44421dc661f,1,1%5D&series=%5Bmozilla-central,53c855dff422d32f2175227e3aa24c03afc396d6,1,1%5D&series=%5Bmozilla-central,1e525606622240a0a813356850004a1fba1ded52,1,1%5D&series=%5Bmozilla-central,587d82621808e64c4d074b7fea9a1e4fc9bfd7cc,1,1%5D

Can we close this?

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 35 • 7 years ago

I am fine with that!

	Avi Halachmi (:avih)
	Comment 36 • 7 years ago

Although a bit late, like in many other cases, I agree with jmaher :)

Specifically, the data and information in this bug is probably stale enough to not be worth an effort based on it. RIP.