Closed Bug 1330412 (stylo-nightly) Opened 4 years ago Closed 3 years ago

[meta] Enable Stylo in the Nightly channel

Categories

(Core :: CSS Parsing and Computation, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: cpeterson, Assigned: jryans)

References

(Depends on 4 open bugs)

Details

(Keywords: meta)

Attachments

(9 files)

59 bytes, text/x-review-board-request
bholley
: review+
froydnj
: review+
Details
59 bytes, text/x-review-board-request
jmaher
: review+
Details
59 bytes, text/x-review-board-request
jmaher
: review+
Details
59 bytes, text/x-review-board-request
jmaher
: review+
Details
59 bytes, text/x-review-board-request
jmaher
: review+
Details
14.04 KB, patch
kmoir
: review+
Details | Diff | Splinter Review
22.16 KB, text/plain
jryans
: feedback+
Details
59 bytes, text/x-review-board-request
jmaher
: review+
Details
47 bytes, text/x-github-pull-request
Details | Review
We may need a separate bug for Stylo riding the trains to release. We can use the top-level Stylo meta bug 1243581 for post-Nightly work in the meantime.
Depends on: stylo-central
Priority: -- → P2
Depends on: 1323140
Depends on: 1330051
Depends on: 1328765
Depends on: stylo-perf-test
Depends on: stylo-reftest
Depends on: 1323643
Depends on: 1323145
Depends on: stylo-devtools
Depends on: 1322656
Depends on: stylo-mochitest
Depends on: 1304797
Depends on: 1301001
Depends on: stylo-chrome
Depends on: stylo-jemalloc
Depends on: 1289935
Depends on: 1288352
Depends on: 1330041
Depends on: 1330902
Depends on: stylo-alexa
Depends on: 1331578
Depends on: stylo-fuzzing
Depends on: 1201802
Depends on: 1310117
Depends on: 1297064
Depends on: 1268290
Depends on: 1315894
Depends on: 1276085
Depends on: 1309165
Depends on: 1330060
Depends on: 1316247
Depends on: 1270305
Depends on: 1308234
Depends on: 1288873
Depends on: 1287435
Depends on: 1275913
Depends on: 1295662
Depends on: 1278647
Depends on: 1321157
Depends on: 1329871
Depends on: 1337229
Depends on: 1334287
Depends on: 1337693
Depends on: 1333311
Depends on: 1335998
Depends on: 1333310
Depends on: 1339091
Depends on: 1339356
Depends on: 1339385
Depends on: 1340224
Depends on: 1341086
Depends on: 1341095
Depends on: 1341637
Depends on: 1341651
Depends on: 1341703
Depends on: 1341714
Depends on: 1342104
Depends on: 1342106
Depends on: 1342316
Depends on: 1342319
Depends on: 1342323
Depends on: 1342331
Depends on: 1342656
Depends on: 1342869
Depends on: 1342871
Depends on: 1343159
Depends on: 1343162
Depends on: 1343165
No longer depends on: 1342656
No longer depends on: 1201802
No longer depends on: 1322656
No longer depends on: stylo-perf-test
Priority: P2 → P5
No longer depends on: 1343162
No longer depends on: 1342106
No longer depends on: 1323140
Depends on: 1349180
Depends on: 1354565
Depends on: 1354772
Depends on: 1355672
Depends on: 1356458
Depends on: 1356988
No longer depends on: 1356988
No longer depends on: 1339356, 1342316, 1342331, 1343159
Depends on: 1357060
No longer depends on: 1357060
No longer depends on: stylo-jemalloc
Depends on: 1365915
Depends on: 1365937
Depends on: 1365993
Depends on: 1366142
No longer depends on: 1365937
No longer depends on: 1365993
Depends on: 1366146
Depends on: 1363976
Depends on: 1366048
Depends on: 1366050
Depends on: 1365254
No longer depends on: 1366048
No longer depends on: 1366050
Depends on: 1366972
Depends on: 1367622
Depends on: 1367984
Depends on: 1362982
Depends on: 1369903
Depends on: stylo-wpt
Depends on: 1372812
Depends on: 1372062
Depends on: stylo-cssom
No longer depends on: 1366146
Depends on: stylo-shadow
No longer depends on: stylo-fuzzing
Depends on: 1367374
Depends on: 1374748
No longer depends on: 1365254
No longer depends on: stylo-chrome
Depends on: 1375292
Depends on: 1375911
Depends on: 1323140
Depends on: 1376071
No longer depends on: 1376071
Depends on: 1373878
Depends on: 1378526
Depends on: 1378871
Depends on: 1379585
Depends on: 1379888
Depends on: 1379889
Depends on: 1379893
Depends on: 1379895
Depends on: 1380053
Depends on: 1379566
See Also: → 1371643
Depends on: stylo-pref-study
Depends on: 1384187
Depends on: 1384824
Depends on: 1385150
No longer depends on: stylo-bughunter
No longer depends on: stylo-shadow
I guess I'll assign this since I am preparing the code change and reviewing test status here.
Assignee: cpeterson → jryans
Status: NEW → ASSIGNED
Depends on: 1394487
Depends on: 1394489
Blocks: 1394487
No longer depends on: 1394487
No longer depends on: stylo-mochitest-crashes
No longer depends on: stylo-reftest-crashes
Here's a try run with the current state:

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

The renamed Talos jobs aren't being scheduled on Linux / Windows yet, but I believe that's expected because they'll need BB changes too.
Comment on attachment 8903339 [details]
Bug 1330412 - Enable Stylo by default.

https://reviewboard.mozilla.org/r/175124/#review180182

::: toolkit/moz.configure:591
(Diff revision 1)
>          if target.os == 'Android':
>              # Stylo on Android is happening Later(tm).
>              pass
>          else:
>              build_stylo = True
> +            enable_stylo = milestone.is_nightly

Hm, it seems safer just to set this to true, and then change it to do this if some disaster strikes and we decide not to ship stylo in 57. Otherwise it seems likely that we might forget and then have a surprise on beta when we think stylo is on but it's not.

r=me with that.
Attachment #8903339 - Flags: review?(bobbyholley) → review+
:kmoir, I think this should add the Stylo disabled Talos suites to Buildbot, but I've never attempted a Buildbot change.

Please take a look.  I'm not sure how to produce a builder diff, so it would be great if you could do so.
Attachment #8903363 - Flags: review?(kmoir)
Comment on attachment 8903340 [details]
Bug 1330412 - Stylo seq. runs for memory / perf only.

https://reviewboard.mozilla.org/r/175126/#review180400
Attachment #8903340 - Flags: review?(jmaher) → review+
Comment on attachment 8903341 [details]
Bug 1330412 - Limit reftest-stylo to linux64.

https://reviewboard.mozilla.org/r/175128/#review180402
Attachment #8903341 - Flags: review?(jmaher) → review+
Comment on attachment 8903342 [details]
Bug 1330412 - Schedule Linux Stylo Talos like others.

https://reviewboard.mozilla.org/r/175130/#review180404

::: taskcluster/ci/test/test-sets.yml:139
(Diff revision 1)
>  
> +linux-talos-stylo:
> +    - talos-chrome-stylo
> +    - talos-dromaeojs-stylo
> +    - talos-g1-stylo
> +    - talos-g2-stylo

as a note, we run talos-g3 on linux only; in this case we need to add it here as talos-g3-stylo and then create an entry in tests.yml for talos-g3-stylo.
Attachment #8903342 - Flags: review?(jmaher) → review-
Comment on attachment 8903343 [details]
Bug 1330412 - Convert Stylo jobs to Stylo disabled.

https://reviewboard.mozilla.org/r/175132/#review180406

a lot of changes, overall this is great.

::: taskcluster/ci/test/test-sets.yml:140
(Diff revision 1)
> -linux-talos-stylo:
> -    - talos-chrome-stylo
> -    - talos-dromaeojs-stylo
> -    - talos-g1-stylo
> -    - talos-g2-stylo
> -    - talos-g4-stylo
> +linux-talos-stylo-disabled:
> +    - talos-chrome-stylo-disabled
> +    - talos-dromaeojs-stylo-disabled
> +    - talos-g1-stylo-disabled
> +    - talos-g2-stylo-disabled
> +    - talos-g4-stylo-disabled

note: in the previous patch, I mentioned a need for talos-g3-stylo, that will need to be updated here as well.

::: taskcluster/ci/test/tests.yml:1313
(Diff revision 1)
>  
> -talos-chrome-stylo:
> -    description: "Talos Stylo chrome"
> +talos-chrome-stylo-disabled:
> +    description: "Talos Stylo disabled chrome"
>      suite: talos
> -    try-name: chromez-stylo
> +    try-name: chromez-stylo-disabled
>      treeherder-symbol: tc-Ts(c)

I think we should consider changing the treeherder-symbol, currently it is tc-Ts(x), but Ts stands for Talos Stylo- possibly we clean this up for the talos jobs in a followup (you need to add tc-Tsd(x) in many places)
Attachment #8903343 - Flags: review?(jmaher) → review+
Comment on attachment 8903339 [details]
Bug 1330412 - Enable Stylo by default.

https://reviewboard.mozilla.org/r/175124/#review180482

I think I agree with bholley here.  The alternate path would be to enable for `milestone.is_nightly` now and then prior to the merge, if nothing has shown up, flip it to `True`.  But that would require remembering, and with a slightly accelerated merge date, we'd have to remember a little sooner than we would otherwise.  So let's flip it now, and then we can flip back if there's a problem.
Attachment #8903339 - Flags: review?(nfroyd) → review+
Attached file bug1330412builder.diff
builder diff from the buildbot talos changes, keep in mind that I ran the diff on a test master just configured to run windows tests.
Comment on attachment 8903632 [details]
bug1330412builder.diff

Great, this diff looks correct to me.  We have added stylo-disabled variants for all the Talos runs as expected.

Please review the BB change, then we can land and reconfigure BB.
Attachment #8903632 - Flags: feedback+
Attachment #8903632 - Flags: review+
Attachment #8903632 - Flags: review+
Attachment #8903363 - Flags: review?(kmoir) → review+
Comment on attachment 8903342 [details]
Bug 1330412 - Schedule Linux Stylo Talos like others.

https://reviewboard.mozilla.org/r/175130/#review180594
Attachment #8903342 - Flags: review?(jmaher) → review+
Comment on attachment 8903729 [details]
Bug 1330412 - Clean up Stylo Treeherder symbols.

https://reviewboard.mozilla.org/r/175482/#review180598

lots of great changes!
Attachment #8903729 - Flags: review?(jmaher) → review+
Comment on attachment 8903339 [details]
Bug 1330412 - Enable Stylo by default.

https://reviewboard.mozilla.org/r/175124/#review180182

> Hm, it seems safer just to set this to true, and then change it to do this if some disaster strikes and we decide not to ship stylo in 57. Otherwise it seems likely that we might forget and then have a surprise on beta when we think stylo is on but it's not.
> 
> r=me with that.

Okay, sounds reasonable to me!  (I wasn't sure which approach was preferred here...  I only did the "nightly only" approach originally because a separate "stylo-release" / "let it ride the trains" bug 1374034 exists.)
Comment on attachment 8903342 [details]
Bug 1330412 - Schedule Linux Stylo Talos like others.

https://reviewboard.mozilla.org/r/175130/#review180404

> as a note, we run talos-g3 on linux only; in this case we need to add it here as talos-g3-stylo and then create an entry in tests.yml for talos-g3-stylo.

Thanks for catching that!  I've added g3 stylo here.  I also noticed the stylo variants weren't set to run on Linux, so I've changed that too.
I believe this should be ready to land, but here's a fresh try so we can be more confident:

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

Even if it is ready, we don't plan to land this change until next Tues. Sept 5th because the SETA team will need to make extra changes immediately after it lands, and we're worried about staffing on Friday and then Monday is a US holiday.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #27)
> > Hm, it seems safer just to set this to true, and then change it to do this if some disaster strikes and we decide not to ship stylo in 57. Otherwise it seems likely that we might forget and then have a surprise on beta when we think stylo is on but it's not.
> > 
> > r=me with that.
> 
> Okay, sounds reasonable to me!  (I wasn't sure which approach was preferred
> here...  I only did the "nightly only" approach originally because a
> separate "stylo-release" / "let it ride the trains" bug 1374034 exists.)

We could set it to `milestone.is_nightly or milestone.is_beta` (would have to write in support for is_beta) if we wanted to let bug 1374034 have something to do--or if we're super-scared about letting Stylo slip out to release accidentally.
(In reply to Nathan Froyd [:froydnj] from comment #30)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #27)
> > > Hm, it seems safer just to set this to true, and then change it to do this if some disaster strikes and we decide not to ship stylo in 57. Otherwise it seems likely that we might forget and then have a surprise on beta when we think stylo is on but it's not.
> > > 
> > > r=me with that.
> > 
> > Okay, sounds reasonable to me!  (I wasn't sure which approach was preferred
> > here...  I only did the "nightly only" approach originally because a
> > separate "stylo-release" / "let it ride the trains" bug 1374034 exists.)
> 
> We could set it to `milestone.is_nightly or milestone.is_beta` (would have
> to write in support for is_beta) if we wanted to let bug 1374034 have
> something to do--or if we're super-scared about letting Stylo slip out to
> release accidentally.

I am much more worried about accidentally _not_ enabling stylo on release. We're planning to ship it, so let's orient the gears towards that. If we decide to disable it for 57, it will be a very careful and deliberate process, and we'll be sure to get it right. If we don't decide to disable it for 57, there is no obvious process by which we'll remember to enable it for the next train.

So let's just do the simple thing. Stylo should be the default for all trains starting with 57, and we can switch it off reactively.
Blocks: 1394489
No longer depends on: 1394489
Added Treeherder changes so that the new Talos runs from BB will show up correctly.
(In reply to Treeherder Bugbot from comment #35)
> Commit pushed to master at https://github.com/mozilla/treeherder
> 
> https://github.com/mozilla/treeherder/commit/dfa920cbd8819ae72af4884e319081a914ae7477
> Bug 1330412 - Add stylo-disabled Talos jobs, update tp6 groups (#2750)

I've cherrypicked this to the Treeherder production branch since I'm still waiting on bug 1215587 comment 49 before I can deploy master. It should be deployed within ~10 minutes, at which point there'll be an IRC notification in #treeherder, and an email to:
https://groups.google.com/forum/#!forum/mozilla.tools.treeherder
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7b3215e18443
Enable Stylo by default. r=bholley,froydnj
https://hg.mozilla.org/integration/autoland/rev/d37df1ba1ca9
Stylo seq. runs for memory / perf only. r=jmaher
https://hg.mozilla.org/integration/autoland/rev/ca968311ed00
Limit reftest-stylo to linux64. r=jmaher
https://hg.mozilla.org/integration/autoland/rev/d6c23b724e86
Schedule Linux Stylo Talos like others. r=jmaher
https://hg.mozilla.org/integration/autoland/rev/76e1adb5416d
Convert Stylo jobs to Stylo disabled. r=jmaher
https://hg.mozilla.org/integration/autoland/rev/62cebea1d157
Clean up Stylo Treeherder symbols. r=jmaher
here is the difference between stylo and non-stylo firefox according to talos:
== Change summary for alert #9238 (as of September 05 2017 20:06 UTC) ==

Regressions:

  4%  damp summary windows7-32 opt e10s     275.34 -> 286.46
  3%  tp5o Private Bytes linux64 pgo e10s   991,235,072.54 -> 1,021,448,764.77
  3%  tp5o Private Bytes linux64 opt e10s   984,508,752.03 -> 1,012,869,617.46
  3%  damp summary windows7-32 pgo e10s     220.16 -> 226.24
  3%  tp5o_webext Main_RSS linux64 opt e10s 188,299,423.56 -> 193,335,479.51
  3%  tp5o_webext Main_RSS linux64 pgo e10s 181,101,414.59 -> 185,885,824.40
  2%  tp5o Main_RSS linux64 opt e10s        186,032,430.68 -> 189,797,789.83

Improvements:

 90%  bloom_basic_singleton summary windows7-32 opt e10s     951.17 -> 98.04
 87%  bloom_basic_singleton summary windows10-64 opt e10s    712.40 -> 92.85
 86%  bloom_basic_singleton summary osx-10-10 opt e10s       782.15 -> 106.48
 83%  bloom_basic summary windows10-64 opt e10s              6.55 -> 1.08
 79%  bloom_basic summary windows7-32 opt e10s               2.86 -> 0.59
 78%  bloom_basic_singleton summary windows7-32 pgo e10s     398.69 -> 87.22
 73%  bloom_basic_singleton summary linux64 pgo e10s         318.11 -> 85.19
 73%  bloom_basic_singleton summary windows10-64 pgo e10s    319.41 -> 86.76
 71%  bloom_basic_singleton summary linux64 opt e10s         309.87 -> 89.94
 27%  tp6_amazon summary windows7-32 opt e10s                957.73 -> 701.92
 27%  tp6_amazon summary windows10-64 opt e10s               779.96 -> 572.29
 23%  tp6_amazon summary windows10-64 pgo e10s               629.33 -> 483.08
 22%  tp6_amazon summary windows7-32 pgo e10s                677.25 -> 530.21
  8%  a11yr summary windows10-64 opt e10s                    593.95 -> 544.29
  8%  tp5o_webext summary windows7-32 opt e10s               323.13 -> 298.30
  7%  tp5o summary windows7-32 opt e10s                      322.72 -> 299.28
  7%  a11yr summary windows7-32 opt e10s                     748.45 -> 694.64
  7%  tp5o_webext summary windows10-64 opt e10s              286.51 -> 266.33
  7%  tp5o summary windows10-64 opt e10s                     289.67 -> 269.89
  5%  a11yr summary linux64 opt e10s                         581.70 -> 549.92
  4%  tp5o summary osx-10-10 opt e10s                        272.70 -> 261.91
  3%  tp5o_webext summary linux64 opt e10s                   293.33 -> 283.50
  3%  tp6_google summary windows7-32 opt e10s                515.08 -> 498.29
  3%  tp5o summary windows10-64 pgo e10s                     226.14 -> 218.98
  3%  tp5o summary windows7-32 pgo e10s                      226.01 -> 219.32

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9238

do we believe these are accurate?  I am excited to see the large wins, and only some small memory and devtools regressions.
Flags: needinfo?(jryans)
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #47)
> do we believe these are accurate?  I am excited to see the large wins, and
> only some small memory and devtools regressions.

Yep! Glad to see it reflected across various other suites I hadn't been tracking.
Flags: needinfo?(jryans)
Per the metrics in comment 47, can anyone say how many cores talos is using?

I'm under the impression that Stylo's performance has a somewhat linear correlation to CPU cores.
(In reply to Caspy7 from comment #49)
> Per the metrics in comment 47, can anyone say how many cores talos is using?

It differs by platform and test type.  Here's a summary of cores available and threads used by Stylo in different situations:

https://docs.google.com/spreadsheets/d/1z9aPLTFtFvuLvlDEaD2DFzAKtF2u-miJol-XauO7SOM/edit
now here is a list of memory regressions:
== Change summary for alert #9239 (as of September 05 2017 20:06 UTC) ==

Regressions:

 26%  Images summary windows7-32 pgo      4,345,721.15 -> 5,457,115.99
 20%  Images summary windows7-32 opt      4,438,344.04 -> 5,329,100.60
 18%  Heap Unclassified summary windows10-64 pgo 47,031,798.71 -> 55,380,832.57
 18%  Images summary windows10-64 opt     5,654,544.77 -> 6,648,518.57
 17%  Heap Unclassified summary windows10-64 opt 46,969,653.15 -> 55,153,061.35
 17%  Images summary osx-10-10 opt        5,379,158.77 -> 6,300,984.78
 14%  Images summary linux64 opt          5,156,052.18 -> 5,868,148.55
 14%  Heap Unclassified summary windows7-32 pgo 39,807,626.24 -> 45,297,380.30
 14%  Heap Unclassified summary windows7-32 opt 39,750,311.57 -> 45,207,843.72
 13%  Heap Unclassified summary linux64 opt 55,363,623.45 -> 62,777,263.63
 10%  Heap Unclassified summary osx-10-10 opt 69,689,260.72 -> 76,409,872.41
  8%  Explicit Memory summary windows7-32 opt 226,844,163.55 -> 244,667,087.05
  8%  Explicit Memory summary windows10-64 pgo 296,065,177.27 -> 318,684,944.36
  7%  Explicit Memory summary windows7-32 pgo 226,196,686.15 -> 243,142,482.22
  7%  Explicit Memory summary linux64 opt 295,081,056.88 -> 315,783,152.15
  7%  Explicit Memory summary windows10-64 opt 296,619,695.65 -> 316,909,818.14
  6%  Resident Memory summary windows7-32 opt 290,401,474.10 -> 309,109,527.62
  6%  Resident Memory summary linux64 opt 424,821,351.92 -> 451,728,421.81
  6%  Explicit Memory summary osx-10-10 opt 308,346,756.13 -> 325,895,170.89
  5%  Resident Memory summary windows10-64 opt 440,113,115.57 -> 463,174,702.35
  5%  Resident Memory summary windows10-64 pgo 432,057,921.38 -> 452,979,401.56

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9239

can we confirm these are expected
The explicit and resident size changes agree with what we were tracking.  But I don't recall discussion of the images size change.  :bholley, do we need to track this?
Flags: needinfo?(bobbyholley)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #52)
> The explicit and resident size changes agree with what we were tracking. 
> But I don't recall discussion of the images size change.  :bholley, do we
> need to track this?

njn's recent memory reporting work (bug 1394729) caused some memory used by Stylo to be shifted from heap-unclassified to images (as seen in bug 1395576). I'd guess that this is the same thing. So it isn't a problem per se, as long as explicit is still changing as you expected (as images is a subset of explicit), but it might highlight an area that could be targeted for memory improvements.
Acknowledge on the memory regressions. There will be some regression from stylo, but we should be able to shrink it a good bit.

Nick has a nice explanation for the image stuff in bug 1395576 comment 11.
Flags: needinfo?(bobbyholley)
Depends on: 1410028
Depends on: 1412251
Depends on: 1418152
Depends on: 1418433
Depends on: 1419158
Depends on: 1419210
Depends on: 1420741
Depends on: 1420680
Depends on: 1458715
Blocks: 1356815
Depends on: 1472567
Depends on: 1486252
Regressions: 1618190
Summary: Enable Stylo in the Nightly channel → [meta] Enable Stylo in the Nightly channel
No longer regressions: 1618190
You need to log in before you can comment on or make changes to this bug.