re-enable bug 931668 optimizations to avoid restyling all descendants for certain restyles

RESOLVED FIXED in Firefox 37

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: dbaron, Assigned: heycam)

Tracking

(Blocks 1 bug)

Trunk
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35 unaffected, firefox36+ wontfix, firefox37+ fixed, firefox38+ fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment)

bug 1092363 disabled the performance work from bug 931668.  We should fix the underlying issues and re-enable it.
Depends on: 1127198
Posted patch patchSplinter Review
For when bug 1127198 is ready.
Attachment #8558155 - Flags: review?(dbaron)
Tracking (and also in bug 1127198) so that we hopefully don't ship this perf regression.
Blocks: 1125224
Cameron, what are you plan about this bug for 36? Thanks
Flags: needinfo?(cam)
I spoke with David about this yesterday, and we think that the fix in bug 1127198 is too risky to land on beta.
Flags: needinfo?(cam)
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
Attachment #8558155 - Flags: approval-mozilla-aurora?
Attachment #8558155 - Flags: review?(dbaron) → review+
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+
Attachment #8558155 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
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 :)
https://hg.mozilla.org/mozilla-central/rev/e7d74758846b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Flags: needinfo?(ryanvm)
Flags: needinfo?(cam)
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.
Flags: needinfo?(ryanvm)
I will investigate this early next week.
Depends on: 1136010
Looking into this in bug 1136010.
Flags: needinfo?(cam)
Depends on: 1137031
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?
Flags: needinfo?(cam)
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+
Attachment #8558155 - Flags: approval-mozilla-beta+
Yes let's uplift this patch too, in conjunction with the bug 1136010 patch.
Flags: needinfo?(cam)
Note that this bug missed 37 Beta 3. It will be included in 37 Beta 4.
Depends on: 1145957
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.
:heycam, can you confirm that this isn't on firefox 38?
Flags: needinfo?(cam)
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?
Flags: needinfo?(cam) → needinfo?(jmaher)
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)?
Flags: needinfo?(jmaher) → needinfo?(cam)
No longer depends on: 1145957
clearing the needinfo here
Flags: needinfo?(cam)
You need to log in before you can comment on or make changes to this bug.