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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 1

[Cameron McCormack (:heycam)]

Attachment: patch

For when bug 1127198 is ready.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 3

[Sylvestre Ledru [:Sylvestre]]

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

Comment 4

[Cameron McCormack (:heycam)]

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

Comment 5

[Sylvestre Ledru [:Sylvestre]]

OK, wontfix for 36.

Comment 6

[Cameron McCormack (:heycam)]

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

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 8

[Cameron McCormack (:heycam)]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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 :)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 15

[Cameron McCormack (:heycam)]

I will investigate this early next week.

Comment 16

[Cameron McCormack (:heycam)]

Looking into this in bug 1136010.

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 19

[Cameron McCormack (:heycam)]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 23

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

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.

Comment 24

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

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

Comment 25

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

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

Comment 26

[Cameron McCormack (:heycam)]

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?

Comment 27

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

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)?

Comment 28

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

clearing the needinfo here