1383789 - Enable Stylo Talos tests on more desktop platforms

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Now that Stylo is building by default on more platforms, we'd like to capture Talos perf data for Stylo runs (similar to what we already do for linux64).

The exact set of additional platforms TBD at the next Stylo meeting, but it seems safe to assume at least win64, since that's a focus for Quantum.

Comment 1

[Astley Chen (inactive)]

Shing, it seems you could help on enabling it as you did for Talos for linux64.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

We'd like to have the full Talos suite for Stylo (including the new tp6 run, which already runs for Stylo on Windows, see bug 1382524), so something like:

* `talos` test set 
* `talos-quantum-pageload-stylo` test

on platforms:

* Linux64
* Linux32 (once Stylo builds there...)
* Win32
* Win64
* macOS

for branches:

* mozilla-central
* try

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

After talking on IRC with :jmaher, there's no Talos on Linux32.

Also, there's already bug 1366355 to expand tp6 to more platforms, so let's leave it out of this one.

We only need parallel mode for these new platforms.

I'll use the user story field to update the request.  :jmaher said they could handle this, I'm going to send a PI request.

Comment 4

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

as a note, we are planning on adding talos jobs for osx stylo (which already has builds available) and for win32/64.  This will be a duplicate set of talos jobs and will run on mozilla-central only.

Comment 5

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

Attachment: buildbot_configs patch for getting win7/10 stylo talos tests on m-c + try

:kmoir, could you take this patch and give me a builder diff?  I am not having any luck with my local setup of a buildbot master.

Comment 6

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

the general flow for resolving this bug will be:
1) get buildbot-configs patch r+, landed, and a reconfig
2) add treeherder symbols to ensure a 's' version of each job exists (or more likely a T-s(...) group
3) add in-tree support in talos.json and taskcluster tests.yml and test-sets.yml
4) profit

Comment 7

[Kim Moir [:kmoir] ET]

Attachment: bug1383789builder.diff

Comment 8

[Kim Moir [:kmoir] ET]

Comment on attachment 8893901 [details] [diff] [review]
buildbot_configs patch for getting win7/10 stylo talos tests on m-c + try

The master I generated the diff on just had win32/win64 jobs enabled but the patch looks good to me.

Comment 9

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

odd it appears my patch is scheduling on other platforms, I was expecting:
* all *stylo-e10s jobs to be on mozilla-central  and try, not inbound, autoland, date, etc.
* tp6-stylo, tp6-stylo-threads to be on osx and linux

let me look a bit more

Comment 10

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

Attachment: buildbot_configs patch for getting win7/10 stylo talos tests on m-c + try

simple logic error, I think things are good now- if the patch looks good, please upload a builder diff and I can confirm prior to landing.

Comment 11

[Kim Moir [:kmoir] ET]

Attachment: bug1383789builder.diff

Comment 12

[GitHub Autolander Bot]

Attachment: [treeherder] jmaher:preseed > mozilla:master

Comment 13

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

:kmoir, this builderdiff looks much better- If you find the patch acceptable, lets work on landing it.

Comment 14

[William Lachance (:wlach)]

Comment on attachment 8894047 [details] [review]
[treeherder] jmaher:preseed > mozilla:master

Not seeing any red flags in this new data, so presumably ok. :)

Comment 15

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/9ec2f1293f1d747bc93ae85213de7a39de630d76
Bug 1383789 - Enable Stylo Talos tests on more desktop platforms - add preseed data (#2684)

Comment 16

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

Comment on attachment 8893927 [details] [diff] [review]
buildbot_configs patch for getting win7/10 stylo talos tests on m-c + try

ok, back to :kmoir as :Callek isn't comfortable doing the review.

Comment 17

[Kim Moir [:kmoir] ET]

Attachment: bug1388136builder.diff

builder diff, for a win32/win64 master only

Comment 18

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

ok, this isn't what I want, that builder diff only shows tp6/quantum-pageload tests changing.  I am sort of stumped on why this isn't working.

Comment 19

[Kim Moir [:kmoir] ET]

Attachment: bug1388136builder.diff

sorry I attached the wrong diff

Comment 20

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

:kmoir, I see the updated builder diff, but the data is just quantum-pageload and tp6, no other tests, my patch specifically changes all the tests- I had a different patch early last week which just changed quantum-pageload and tp6, possibly the patch on this bug got mixed up?

Is it possible for you to run a builderdiff again?

Comment 21

[Kim Moir [:kmoir] ET]

Attachment: bug1383789.patch

updated patch, unbitrotten

Comment 22

[Kim Moir [:kmoir] ET]

Attachment: bug1383789builder.diff

builder diff

Comment 23

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

Comment on attachment 8896988 [details] [diff] [review]
bug1383789.patch

thanks for the updated builder diff, this looks exactly what I would have expected it to look like now

Comment 24

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

thanks!

landed:
 https://hg.mozilla.org/build/buildbot-configs/rev/39c4aeb5bae37b0f2d9cfb51af0084defa53b32d

we will need to do a reconfig after the magic test bot posts success in  #releng.

Comment 25

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

ok, on try we can run these jobs:
https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&fromchange=4f1f13c4524d429eb6879ca5dc466e8694939c52&tochange=1f520b379ece50a7cea00f601da359dc3356682a

you can see the patch on there needed for in-tree changes.

Next up is treeherder symbols, then verify it looks right and land the in-tree changes.

Comment 26

[GitHub Autolander Bot]

Attachment: [treeherder] jmaher:winstylo > mozilla:master

Comment 27

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

Comment on attachment 8897501 [details] [review]
[treeherder] jmaher:winstylo > mozilla:master

all local tests pass now!

Comment 28

[Ed Morley [:emorley]]

Comment on attachment 8897501 [details] [review]
[treeherder] jmaher:winstylo > mozilla:master

Looks great - thank you!

Comment 29

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/aec4ef67d74776ec5c29433a4682a85d2b01f3a3
Bug 1383789 - Add support for more desktop Stylo Talos tests (#2708)

Comment 30

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

thanks!  I will wait until this is on staging and then push to try again so I can validate it on a full push

Comment 31

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

:jryans, other than the one ?, is this a reasonable view for treeherder:
https://treeherder.allizom.org/#/jobs?repo=try&revision=aea85b03032634cef4b893463937151abd76a24f&exclusion_profile=false&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=116653047&group_state=expanded

the Ts-e10s() is stylo- specifically the new tests we are duplicating, not the existing tp6,tp6s,tp6st

Comment 32

[GitHub Autolander Bot]

Attachment: [treeherder] jmaher:winstylo_svg > mozilla:master

Comment 33

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #31)
> :jryans, other than the one ?, is this a reasonable view for treeherder:
> https://treeherder.allizom.org/#/
> jobs?repo=try&revision=aea85b03032634cef4b893463937151abd76a24f&exclusion_pro
> file=false&filter-tier=1&filter-tier=2&filter-
> tier=3&selectedJob=116653047&group_state=expanded
> 
> the Ts-e10s() is stylo- specifically the new tests we are duplicating, not
> the existing tp6,tp6s,tp6st

Is it true that Talos runs don't print the full env vars used to run the browser?  I was trying to verify the Stylo mode is correctly enabled for the right runs, which I believe is handled here for Talos:

http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/testing/talos/talos/ttest.py#96-97

However, it doesn't seem like Talos prints the final environment anywhere, so I can't easily search for STYLO_FORCE_ENABLED in the log like with other test types.

Assuming the correct runs are enabling Stylo, the label of the new runs under Ts-e10s() seems good.

It might need a separate bug, but would it make sense to move tp6s and tp6st over to Ts-e10s() as well?  That way all the runs with Stylo on are in the same group.

Comment 34

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

I hunted around for a bit to determine it was correct, if you click on the reported number inside of perfherder, it will connect you to perfherder and there you can see what ran, in the case of the stylo tests, it runs something like "canvasmark opt e10s stylo" vs "canvasmark opt e10s".

I agree about moving over the tp6s and tp6st to the ts-e10s() group

Comment 35

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/cbfabd04059f88db6868f40e9bac1c0a74c72ccd
Bug 1383789 - Add support for Talos-Stylo svg (#2711)

Comment 36

[GitHub Autolander Bot]

Attachment: [treeherder] jmaher:winstylo_svg > mozilla:master

Comment 37

[GitHub Autolander Bot]

Attachment: [treeherder] jmaher:tp6_xperf > mozilla:master

Comment 38

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

Comment on attachment 8897571 [details] [review]
[treeherder] jmaher:tp6_xperf > mozilla:master

this should handle all the small details to make this perfect.

Comment 39

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

Attachment: in-tree changes for talos on win stylo

I figured I would get this in the queue- when all the treeherder changes are in production, we can land this- if there are review tweaks we can get those done now while finishing up the treeherder bits.

Comment 40

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/38015b74d9508fb90ebcd0e8ebfd7c63211f769d
Bug 1383789 - Support xperf jobs and move tp6 in stylo mode (#2713)

Comment 41

[Robert Wood [:rwood]]

Comment on attachment 8897579 [details] [diff] [review]
in-tree changes for talos on win stylo

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

R+ with that one comment (issue?) addressed

::: taskcluster/ci/test/test-sets.yml
@@ +161,5 @@
>      - talos-tp6
>      - talos-tp6-stylo
>      - talos-tp6-stylo-threads
>  
> +windows-talos-stylo:

Don't 'talos-tp6-stylo' and 'talos-tp6-stylo-threads' need to be moved out of the section above and into this one? If I'm understanding correctly...

Comment 42

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

good catch, moved those- when treeherder changes are on production, I will deploy this :)

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c85d53f70511
Enable Stylo Talos tests on more desktop platforms. r=rwood

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 45

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:jmaher, the initial request here mentions both Windows and macOS, but it seems like it was only added for Windows.  Is that correct?  Was macOS omitted because not enough hardware?

Comment 46

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

oh, this was an oversight on my part, thanks for catching this.  We should be able to get these running without trouble on osx for m-c/try.  Luckily this is just a taskcluster config change, not a buildbot set of changes.

Comment 47

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Add these Stylo runs on macOS will likely need the env var tweak in bug 1393229.

Comment 48

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

I know that :rwood reported that --stylo for osx talos didn't work, I tried running all the tests and got the same results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bdca449f4d19abd768e5626cebc34e877479565


I see this in the 'c' log file:
14:00:09     INFO -  Application command: /Users/cltbld/tasks/task_1503521916/build/application/Nightly.app/Contents/MacOS/firefox  http://localhost:49216/getInfo.html -foreground -profile /var/folders/hm/hwgk9grd06xfg3kkmwzct32000000w/T/tmpaqTI9d/profile
14:00:09     INFO -  TEST-INFO | started process 1570 (/Users/cltbld/tasks/task_1503521916/build/application/Nightly.app/Contents/MacOS/firefox  http://localhost:49216/getInfo.html -foreground)
14:00:14     INFO -  TEST-INFO | 1570: exit 0
14:00:14     INFO -  Browser initialized.
14:00:14     INFO -  Running cycle 1/20 for tresize test...
14:00:14     INFO -  TEST-UNEXPECTED-ERROR | tresize | execve() arg 3 contains a non-string value


:jryans, do you have any thoughts on what might be going wrong here?  I don't think it is wise to enable this if it is perma failing.  then again, I could enable it on try/mozilla-central and let it be- thoughts?

Comment 49

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

oh, now I see the comments :)  looks like we might have this going before too long :)

Comment 50

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

I have a new push on try waiting for osx machine time

Comment 51

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

Attachment: enable all talos tests for osx-stylo

and a try push for fun:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2ba25222e2e3f209cdd5691d8d03979ab72c530&filter-searchStr=osx

Comment 52

[Robert Wood [:rwood]]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #51)
> Created attachment 8901164 [details] [diff] [review]
> enable all talos tests for osx-stylo
> 
> and a try push for fun:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2ba25222e2e3f209cdd5691d8d03979ab72c530&filter-searchStr=osx

Hmmm it didn't run tp6-stylo-threads (tp6st) for some reason, also it's not putting the stylo flavours (tp6s) in tc-Ts-e10s like it does on windows, it put it in tc-e10s.

Comment 53

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

yeah, I am not sure there- let me push to try again and see.

Comment 54

[Robert Wood [:rwood]]

Comment on attachment 8901164 [details] [diff] [review]
enable all talos tests for osx-stylo

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

I see 'tp6st' showing up on your latest try push; r+ in that we can figure out the task grouping issue later if need be

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12c2db8bc10b
enable stylo talos tests on osx. r=rwood

Comment 56

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/12c2db8bc10b