1382524 - run tp6 on talos windows (7,10) with stylo

Status: RESOLVED FIXED

(Testing :: Talos, enhancement)

Description

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

Work description: The Stylo team is preparing to enable Stylo by default in Nightly 57. Before then, we'd like to track Stylo's performance on the new Talos tp6 page load tests compared to regular Firefox.

1. When tp6 is run on windows7-32 and windows10-64 (automatically for each commit on Treeherder and on demand on Try), also run the tests in Stylo mode by setting the STYLO_FORCE_ENABLED=1 environment variable.
2. Report the Stylo results into Perfherder as new platforms "windows7-32-stylo" and "windows10-64-stylo" so we can track them alongside regular Firefox runs (windows7-32 and windows10-64) on Perfherder like https://is.gd/TF6dV6.
3. Report the Stylo results on Treeherder. tp6's Treeherder job symbol is "q1", so tp6 in Stylo mode could be called something like "q1s".
4. After Stylo is enabled by default in Nightly 57, we can stop the separate runs of tp6 in Stylo mode.


I believe this translates into:
* in-tree patch for adding --stylo cli option to talos as well as mozharness bits to support --stylo.  This would set the env variable and also set the proper options in the PERFHERDER_DATA
* in-tree patch to fix testing/talos/talos.json to add q1s which uses the --stylo flag to mozharness + talos
* buildbot-configs patch for adding the new test type as a runnable option
* taskcluster patch for adding q1s as a possible job type (we will be running all windows jobs via BBB)
* updating the wiki with a mention of what is happening
* updating try chooser to document the unique test

Comment 1

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

Attachment: and quantum stylo as an option in buildbot configs

Comment 2

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

Attachment: talos.json cleanup and changes for q1s

Comment 3

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

Attachment: taskcluster changes for q1s

Comment 4

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

Attachment: taskcluster changes for q1s

updated with test-sets.yml changes as well.

Comment 5

[Robert Wood [:rwood]]

Comment on attachment 8888362 [details] [diff] [review]
and quantum stylo as an option in buildbot configs

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

LGTM

Comment 6

[Robert Wood [:rwood]]

Comment on attachment 8888372 [details] [diff] [review]
talos.json cleanup and changes for q1s

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

Much cleaner

Comment 7

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

buildbot configs landed:
https://hg.mozilla.org/build/buildbot-configs/rev/2c0bf76c1689cea9f112d6456901205d8daeb742

Comment 8

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

Attachment: in-tree talos changes required

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 8888544 [details] [diff] [review]
in-tree talos changes required

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

::: testing/talos/talos/cmdline.py
@@ +169,5 @@
>              dest='no_upload_results',
>              help="If given, it disables uploading of talos results.")
> +    add_arg('--stylo', action="store_true",
> +            dest='stylo',
> +            help='If given, enable style via Environment variables and '

"Stylo", not "style" :)

Comment 10

[Robert Wood [:rwood]]

Comment on attachment 8888487 [details] [diff] [review]
taskcluster changes for q1s

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

I see the 'q1s' job showing up on your try run in tc-T-e10s.

Note: The original 'q1' job is buildbot only and just in the 'T-e10s' group on treeherder. Should you add an entry for 'q1' / talos-quantum-pageload (non-stylo) here now too?

Comment 11

[Robert Wood [:rwood]]

Comment on attachment 8888544 [details] [diff] [review]
in-tree talos changes required

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

LGTM, r+ (with :cpeterson's nit)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7fbe9dd8b2f
run tp6 on talos windows (7,10) with stylo- talos.json changes. r=rwood
https://hg.mozilla.org/integration/mozilla-inbound/rev/e79374a18b47
run tp6 on talos windows (7,10) with stylo. talos changes. r=rwood
https://hg.mozilla.org/integration/mozilla-inbound/rev/e68cd63cc5ef
run tp6 on talos windows (7,10) with stylo - taskcluster changes. r=rwood

Comment 13

[Bobby Holley (:bholley)]

(In reply to Pulsebot from comment #12)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a7fbe9dd8b2f
> run tp6 on talos windows (7,10) with stylo- talos.json changes. r=rwood
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e79374a18b47
> run tp6 on talos windows (7,10) with stylo. talos changes. r=rwood
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e68cd63cc5ef
> run tp6 on talos windows (7,10) with stylo - taskcluster changes. r=rwood

\o/

We've found it very useful on Talos to compare the performance on sequential stylo as well (STYLO_THREADS=1), so that we can separate the effects of the parallelism from the overall stylo architecture. Would it be straightforward to add this configuration as well? Sorry if I should have asked about this before.

Comment 14

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

would this be a 3rd job to run, either:
1) both environment variables defined STYLO_FORCE_ENABLED=1 and STYLO_THREADS=1
2) keep existing environment variable STYLO_FORCE_ENABLED=1 defined, then add an additional job/test which has both defined?

If it is #1 where we modify the test that landed, that is easy in-tree to change, effectively add another env variable here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e79374a18b47bd7f3dade36bd39deddad3a10791#l4.14

if it is #2, we need to do additional work and that will probably be later next week due to PTO schedules and other scheduled changes.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Joel Maher ( :jmaher) (UTC-8) from comment #14)
> would this be a 3rd job to run, either:
> 1) both environment variables defined STYLO_FORCE_ENABLED=1 and
> STYLO_THREADS=1
> 2) keep existing environment variable STYLO_FORCE_ENABLED=1 defined, then
> add an additional job/test which has both defined?
> 
> If it is #1 where we modify the test that landed, that is easy in-tree to
> change, effectively add another env variable here:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> e79374a18b47bd7f3dade36bd39deddad3a10791#l4.14

We would have 3 jobs total:
(1) The regular non-stylo job that we had before this bug
(2) The stylo job we added in this bug, i.e. configuration (1) + STYLO_FORCE_ENABLED=1
(3) An additional sequential stylo job, i.e. configuration (2) + STYLO_THREADS=1 (or put differently, configuration (1) + STYLO_FORCE_ENABLED=1 + STYLO_THREADS=1).

Does that answer the question? I was a bit confused by the framing, given that your comment described it as a third job (which it is), but then the first option seems to suggest replacing the job we added in this bug (at which point we'd have 2 total jobs, not the 3 that we want). Maybe I'm totally misunderstanding something here.

> 
> if it is #2, we need to do additional work and that will probably be later
> next week due to PTO schedules and other scheduled changes.

That's likely fine. I really appreciate how attentive and responsive your team has been to stylo's needs!

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a7fbe9dd8b2f
https://hg.mozilla.org/mozilla-central/rev/e79374a18b47
https://hg.mozilla.org/mozilla-central/rev/e68cd63cc5ef