Enable Stylo Talos tests on more desktop platforms

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: jryans, Unassigned)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [Stylo][PI:August])

User Story

Expand parallel only Stylo Talos runs to additional platforms, so:

* All Talos tests in the `talos` set from test-sets.yml

on platforms:

* Linux64 (already done)
* Win32
* Win64
* macOS

for branches:

* mozilla-central
* try

No special builds are needed.  Stylo parallel mode can be enabled with STYLO_FORCE_ENABLED=1.

Attachments

(8 attachments, 7 obsolete attachments)

47 bytes, text/x-github-pull-request
wlach
: review+
Details | Review | Splinter Review
8.66 KB, patch
kmoir
: review+
Details | Diff | Splinter Review
40.22 KB, text/plain
Details
47 bytes, text/x-github-pull-request
emorley
: review+
Details | Review | Splinter Review
47 bytes, text/x-github-pull-request
Details | Review | Splinter Review
47 bytes, text/x-github-pull-request
emorley
: review+
Details | Review | Splinter Review
24.14 KB, patch
rwood
: review+
Details | Diff | Splinter Review
7.73 KB, patch
rwood
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 months ago
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.
(Reporter)

Updated

3 months ago
See Also: → bug 1380053
Priority: -- → P2
Shing, it seems you could help on enabling it as you did for Talos for linux64.
Flags: needinfo?(shing.lyu)
Summary: Stylo: Enable Stylo Talos tests on more desktop platforms → Enable Stylo Talos tests on more desktop platforms
Whiteboard: [Stylo]
Comment hidden (obsolete)
(Reporter)

Comment 3

3 months ago
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.
Flags: needinfo?(shing.lyu)
(Reporter)

Updated

3 months ago
User Story: (updated)
Whiteboard: [Stylo] → [Stylo][PI:August]
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.
Created attachment 8893901 [details] [diff] [review]
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.
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
Attachment #8893901 - Flags: feedback?(kmoir)
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

3 months ago
Created attachment 8893915 [details]
bug1383789builder.diff

Comment 8

3 months ago
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.
Attachment #8893901 - Flags: feedback?(kmoir) → feedback+
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
Created attachment 8893927 [details] [diff] [review]
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.
Attachment #8893927 - Flags: review?(kmoir)

Comment 11

3 months ago
Created attachment 8893938 [details]
bug1383789builder.diff
Attachment #8893915 - Attachment is obsolete: true

Comment 12

3 months ago
Created attachment 8894047 [details] [review]
[treeherder] jmaher:preseed > mozilla:master
:kmoir, this builderdiff looks much better- If you find the patch acceptable, lets work on landing it.
Flags: needinfo?(kmoir)
Comment on attachment 8894047 [details] [review]
[treeherder] jmaher:preseed > mozilla:master

Not seeing any red flags in this new data, so presumably ok. :)
Attachment #8894047 - Flags: review+

Comment 15

3 months ago
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)
Attachment #8893927 - Flags: review?(kmoir) → review?(bugspam.Callek)
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.
Attachment #8893927 - Flags: review?(bugspam.Callek) → review?(kmoir)

Comment 17

3 months ago
Created attachment 8894912 [details]
bug1388136builder.diff

builder diff, for a win32/win64 master only
Flags: needinfo?(kmoir)
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

3 months ago
Created attachment 8894927 [details]
bug1388136builder.diff

sorry I attached the wrong diff
Attachment #8894912 - Attachment is obsolete: true

Updated

3 months ago
Attachment #8893938 - Attachment is obsolete: true
: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?
Flags: needinfo?(kmoir)

Comment 21

2 months ago
Created attachment 8896988 [details] [diff] [review]
bug1383789.patch

updated patch, unbitrotten
Attachment #8893901 - Attachment is obsolete: true
Attachment #8893927 - Attachment is obsolete: true
Attachment #8893927 - Flags: review?(kmoir)
Flags: needinfo?(kmoir)

Comment 22

2 months ago
Created attachment 8896989 [details]
bug1383789builder.diff

builder diff
Attachment #8894927 - Attachment is obsolete: true
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
Attachment #8896988 - Flags: review?(kmoir)

Updated

2 months ago
Attachment #8896988 - Flags: review?(kmoir) → review+
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.
ok, on try we can run these jobs:
https://treeherder.mozilla.org/#/jobs?repo=try&author=jmaher@mozilla.com&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

2 months ago
Created attachment 8897501 [details] [review]
[treeherder] jmaher:winstylo > mozilla:master
Comment on attachment 8897501 [details] [review]
[treeherder] jmaher:winstylo > mozilla:master

all local tests pass now!
Attachment #8897501 - Flags: review?(emorley)

Comment 28

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

Looks great - thank you!
Attachment #8897501 - Flags: review?(emorley) → review+

Comment 29

2 months ago
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)
thanks!  I will wait until this is on staging and then push to try again so I can validate it on a full push
: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
Flags: needinfo?(jryans)

Comment 32

2 months ago
Created attachment 8897532 [details] [review]
[treeherder] jmaher:winstylo_svg > mozilla:master
(Reporter)

Comment 33

2 months ago
(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.
Flags: needinfo?(jryans) → needinfo?(jmaher)
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
Flags: needinfo?(jmaher)

Comment 35

2 months ago
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

2 months ago
Created attachment 8897543 [details] [review]
[treeherder] jmaher:winstylo_svg > mozilla:master
Attachment #8897543 - Attachment is obsolete: true

Comment 37

2 months ago
Created attachment 8897571 [details] [review]
[treeherder] jmaher:tp6_xperf > mozilla:master
Comment on attachment 8897571 [details] [review]
[treeherder] jmaher:tp6_xperf > mozilla:master

this should handle all the small details to make this perfect.
Attachment #8897571 - Flags: review?(emorley)
Created attachment 8897579 [details] [diff] [review]
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.
Attachment #8897579 - Flags: review?(rwood)

Updated

2 months ago
Attachment #8897571 - Flags: review?(emorley) → review+

Comment 40

2 months ago
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

2 months ago
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...
Attachment #8897579 - Flags: review?(rwood) → review+
good catch, moved those- when treeherder changes are on production, I will deploy this :)

Comment 43

2 months ago
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c85d53f70511
Enable Stylo Talos tests on more desktop platforms. r=rwood

Comment 44

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c85d53f70511
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Reporter)

Comment 45

2 months ago
: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?
Flags: needinfo?(jmaher)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 47

2 months ago
Add these Stylo runs on macOS will likely need the env var tweak in bug 1393229.
Depends on: 1393229
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?
oh, now I see the comments :)  looks like we might have this going before too long :)
Flags: needinfo?(jmaher)
I have a new push on try waiting for osx machine time
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
Attachment #8901164 - Flags: review?(rwood)
Assignee: jmaher → nobody

Comment 52

2 months ago
(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.
yeah, I am not sure there- let me push to try again and see.

Comment 54

2 months ago
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
Attachment #8901164 - Flags: review?(rwood) → review+

Comment 55

2 months ago
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12c2db8bc10b
enable stylo talos tests on osx. r=rwood

Comment 56

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/12c2db8bc10b
Status: REOPENED → RESOLVED
Last Resolved: 2 months ago2 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.