David Baron :dbaron: Reporter
	Description • 10 years ago

bug 1092363 disabled the performance work from bug 931668.  We should fix the underlying issues and re-enable it.

	Cameron McCormack (:heycam) Assignee
	Comment 1 • 10 years ago

For when bug 1127198 is ready.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 2 • 10 years ago

Tracking (and also in bug 1127198) so that we hopefully don't ship this perf regression.

	Sylvestre Ledru [:Sylvestre]
	Comment 3 • 10 years ago

Cameron, what are you plan about this bug for 36? Thanks

	Cameron McCormack (:heycam) Assignee
	Comment 4 • 10 years ago

I spoke with David about this yesterday, and we think that the fix in bug 1127198 is too risky to land on beta.

	Sylvestre Ledru [:Sylvestre]
	Comment 5 • 10 years ago

OK, wontfix for 36.

	Cameron McCormack (:heycam) Assignee
	Comment 6 • 10 years ago

Comment on attachment 8558155 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 931668
[User impact if declined]: restyle performance
[Describe test coverage new/current, TreeHerder]: tested locally (will land in conjunction with bug 1127198)
[Risks and why]: not high risk (opts back in to restyle optimizations that we previously had enabled), but easily backoutable
[String/UUID change made/needed]: N/A

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 7 • 10 years ago

Comment on attachment 8558155 [details] [diff] [review]
patch

Glad that we're OK to re-enable this set of optimizations. As dbaron noted, this should land along with bug 1127198. Aurora+

	Cameron McCormack (:heycam) Assignee
	Comment 8 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7d74758846b
https://hg.mozilla.org/releases/mozilla-aurora/rev/12be2975c7a4

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 10 years ago

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/12be2975c7a4

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 10 years ago

Backed out for crashes in test_sort_contacts.py that appeared when this was merged to b2g37 from Aurora. We don't run B2G tests on Aurora, so we wouldn't have seen it there. Interestingly, we haven't seen these crashes on trunk since it landed on inbound.
https://hg.mozilla.org/releases/mozilla-aurora/rev/7917cdc37925
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7917cdc37925

https://treeherder.mozilla.org/logviewer.html#?job_id=41423&repo=mozilla-b2g37_v2_2

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 10 years ago

I had to push a follow-up to change the assertion expectations on 1108104.html as well (it dropped from 3 to 2). Please be sure to revert it when this re-lands.
https://hg.mozilla.org/releases/mozilla-aurora/rev/9fcec67bd0f6

On the bright side, the backout did fix the crashes from comment 10 :)

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 10 years ago

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

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 13 • 10 years ago

Cameron - Can you please investigate the test crashes and see about whether this can be related on 37?

Ryan - Were there any adverse affects for Desktop/Android? If not, perhaps we should reland for 37 and deal with B2G as a follow-up separately.

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 10 years ago

Desktop and Android were fine on Aurora. The issues only appeared when Aurora was merged to b2g37. But as Cameron mentioned on IRC at the time, there's no guarantees that the issue was legitimately B2G-only vs. an issue that just happened to be exercised by that specific test.

	Cameron McCormack (:heycam) Assignee
	Comment 15 • 10 years ago

I will investigate this early next week.

	Cameron McCormack (:heycam) Assignee
	Comment 16 • 9 years ago

Looking into this in bug 1136010.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 17 • 9 years ago

With the fix to bug 1136010 I think that this bug is ready to uplift to 37 again. Is there any reason not to uplift?

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 18 • 9 years ago

Comment on attachment 8558155 [details] [diff] [review]
patch

Adding Beta approval to reland this bug now that the tests have been fixed. I've asked Ryan to land this after the other changesets incase heycam has a reason not to include this in Beta 3. Beta+

	Cameron McCormack (:heycam) Assignee
	Comment 19 • 9 years ago

Yes let's uplift this patch too, in conjunction with the bug 1136010 patch.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 9 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/a38e1a8b5b7d

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 9 years ago

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a38e1a8b5b7d

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 22 • 9 years ago

Note that this bug missed 37 Beta 3. It will be included in 37 Beta 4.

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

this optimization landed on mozilla-beta, march 5th:
http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=3146d4299a62

according to the comments (and graph server), I don't see this on mozilla-aurora (then firefox 38) around that time:
http://graphs.mozilla.org/graph.html#tests=[[293,53,33],[293,52,33]]&sel=none&displayrange=90&datatype=geo

when we uplifted Aurora (v.38) to Beta, we have a regression in linux32, tart.

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

:heycam, can you confirm that this isn't on firefox 38?

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

as a note, this is suspected to be showing *regressions* on firefox 38 in beta for linux32 and linux64:
http://graphs.mozilla.org/graph.html#tests=[[293,53,35],[293,52,35]]&sel=1424660218488.5222,1427992789917.0935,2.4918032786885247,3.4754098360655736&displayrange=90&datatype=geo

	Cameron McCormack (:heycam) Assignee
	Comment 26 • 9 years ago

The optimization is turned on for Firefox 38, landed on then-aurora in comment 8.  Was there a regression at the time this landed on then-aurora?  Can you explain what the graph in comment 25 is showing?

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

comment 8 landed on Feb 17 (firefox 37, and inbound firefox 38) which moved to beta Feb 23.  Then in comment 20 (March 5th) we landed on Beta (firefox 37) and see an improvement which corresponds with that.

When we landed on trunk Feb 17th, we see a corresponding win (trunk only, not aurora):
http://graphs.mozilla.org/graph.html#tests=[[293,52,35],[293,53,35],[293,1,35]]&sel=1420313743360.8127,1428064206415&displayrange=90&datatype=geo

Now on March 30, we uplift again and this time we see a regression.  it isn't 100% guaranteed that this is the cause of the regression, but I wanted to see what branches this should be on.

I wonder what landed on March 5th from comment 20 and why it made an improvement and if we had accidentally landed something only on beta (firefox 37)?

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

clearing the needinfo here