Chris Peterson [:cpeterson] Reporter
	Description • 7 years ago

After we start building Stylo support in regular Linux64 builds (pref'd off) in bug 1356991, we can:

1. Run Stylo tests using regular builds (with a pref flip or environment variable like WebRender's linux64-qr tests) instead of the linux64-stylo builds.

2. Stop building linux64-stylo.

Running Stylo tests on platforms other than Linux64 can happen later.

	Chris Peterson [:cpeterson] Reporter
	Comment 1 • 7 years ago

Running Stylo tests using regular builds will also affect Talos.

	Chris Peterson [:cpeterson] Reporter
	Comment 2 • 7 years ago

The pref to enable Stylo is "layout.css.servo.enabled" = true.

	Kim Moir [:kmoir] ET
	Comment 3 • 7 years ago

patch to disable stylo specific builds and run the stylo-refests with a pref enabled 

I also disabled the sequential tests please confirm that this is expected

	Kim Moir [:kmoir] ET
	Comment 4 • 7 years ago

Builder diff if these changes were implemented on m-c

cpeterson please confirm that this is what you are looking for

	Kim Moir [:kmoir] ET
	Comment 5 • 7 years ago

Also I noticed that there are stylo jobs running on beta now, can these be disabled?

	Chris Peterson [:cpeterson] Reporter
	Comment 6 • 7 years ago

(In reply to Kim Moir [:kmoir] from comment #5)
> Also I noticed that there are stylo jobs running on beta now, can these be
> disabled?

Yes. We don't need to run Stylo jobs on Beta 55 (or Beta 56 next cycle). We will want them for Beta 57 and thereafter, if all goes well.

	Chris Peterson [:cpeterson] Reporter
	Comment 7 • 7 years ago

(In reply to Kim Moir [:kmoir] from comment #3)
> Created attachment 8880856 [details] [diff] [review]
> bug1374748.patch
> 
> patch to disable stylo specific builds and run the stylo-refests with a pref
> enabled 
> 
> I also disabled the sequential tests please confirm that this is expected

We will still want to run Stylo some tests in parallel (default) mode and sequential mode (bug 1356122) and some in e10s and non-e10s mode. I updated our test matrix in the Stylo spreadsheet:

https://docs.google.com/spreadsheets/d/1HmBsxrEpFfCgpLBG5Mvj5wEA9oEq5N8m4cxQYsSo4S8/edit#gid=1737640065


(In reply to Kim Moir [:kmoir] from comment #4)
> Created attachment 8880859 [details]
> m-cbuilder.diff
> 
> Builder diff if these changes were implemented on m-c
> 
> cpeterson please confirm that this is what you are looking for

It looks like this diff removes Stylo crashtests, mochitests, Talos, and web-platform-tests and non-e10s mode. We will still want to run all the test suites we're running now.

	Kim Moir [:kmoir] ET
	Comment 8 • 7 years ago

So this is a bit tricky because you want the stylo tests to run in parallel with the non-stylo tests but they have all have the same name. Is this an an accurate assessment?

For instance
Run crashtest against linux64/opt
Run crashtest against linux64/opt with '--parallel-stylo-traversal and (enable stylo pref) and layout.css.servo.enabled=true for parallel tests
Run crashtest against linux64/opt with '--setperf=layout.css.servo.enabled=true' for sequential tests

	Chris Peterson [:cpeterson] Reporter
	Comment 9 • 7 years ago

(In reply to Kim Moir [:kmoir] from comment #8)
> So this is a bit tricky because you want the stylo tests to run in parallel
> with the non-stylo tests but they have all have the same name. Is this an an
> accurate assessment?
> 
> For instance
> Run crashtest against linux64/opt
> Run crashtest against linux64/opt with '--parallel-stylo-traversal and
> (enable stylo pref) and layout.css.servo.enabled=true for parallel tests
> Run crashtest against linux64/opt with
> '--setperf=layout.css.servo.enabled=true' for sequential tests

I think that is correct. Would adding new names for these Stylo tests make configuration easier? It looks like Treeherder has separate test names for linux64-stylo's reftests: opt-reftest-e10s (R1-R8) and opt-reftest-stylo-e10s (Rs1-Rs16).

	Kim Moir [:kmoir] ET
	Comment 10 • 7 years ago

patch to disable stylo jobs from running on beta.  will attach diff

Some of the tests weren't limited by branch

	Pulsebot
	Comment 13 • 7 years ago

Pushed by kmoir@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c923a5e8169f
Run Stylo tests using regular builds and stop building linux64-stylo builds r=aki DONTBUILD

	Carsten Book [:Tomcat]
	Comment 14 • 7 years ago
bugherder

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

	Kim Moir [:kmoir] ET
	Comment 15 • 7 years ago

Given that we need to set a couple of properties and run the regular set of tests against linux64 builds as well as the stylo ones, I think we need to set a property in the same way that they e10s tests run. i.e. setting this flag essentially allows the tests to be duplicated and copied with new properties like in 

@transforms.add
def split_e10s(config, tests):

in taskcluster/taskgraph/transforms/tests.py

and give them new names as well to distinguish them from non-stylo tests.

	Kim Moir [:kmoir] ET
	Comment 16 • 7 years ago

I tried a few permutations of this but wasn't able to get it working before I left for PTO.

	Chris AtLee [:catlee]
	Comment 17 • 7 years ago

I took a quick look at this, and found one problem that's going to make this hard to do: there's no common way to enable stylo across test suites.

It would be nice if --enable-stylo worked for mozharness and the various underlying suites, but that's not the case right now. Another approach would be to enable stylo if an environment variable is set, e.g. MOZ_STYLO=1, but that also is not supported right now.

	Chris Peterson [:cpeterson] Reporter
	Comment 18 • 7 years ago

(In reply to Chris AtLee [:catlee] from comment #17)
> It would be nice if --enable-stylo worked for mozharness and the various
> underlying suites, but that's not the case right now. Another approach would
> be to enable stylo if an environment variable is set, e.g. MOZ_STYLO=1, but
> that also is not supported right now.

So an environment variable would be adequate? How is Quantum Render enabled for the linux64-qr test runs?

Setting an environment variable sounds a lot easier than adding --enable-stylo flags to multiple test suites for a feature that will soon by enabled by default.

	Chris AtLee [:catlee]
	Comment 19 • 7 years ago

The QR tests use an environment variable MOZ_WEBRENDER, which is set in configs in places like this: https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/scripts/desktop_unittest.py#709

And then used in the code here: https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp#2388

I agree that using an environment various sounds a lot easier right now. In general though, I do think it would be nice if test harnesses all supported setting arbitrary prefs. Some already do, judging by some green test runs on this try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9159ff2650470993fc39126ab279589521b03b23

	Bobby Holley (:bholley)
	Comment 20 • 7 years ago

Given that MOZ_STYLO is already the name of our #define, I think FORCE_ENABLE_STYLO is a better name.

Easiest way is to check the env var here:

http://searchfox.org/mozilla-central/rev/238406d4c1b3f147522ce0a45a4c6f84a8115781/layout/base/nsLayoutUtils.cpp#7798

If the var is set, we just set sStyloEnabled to true and avoid hooking it up to the pref system.

	Bobby Holley (:bholley)
	Comment 21 • 7 years ago

Or maybe FORCE_STYLO_ENABLED, I guess.

	Mike Hommey [:glandium]
	Comment 22 • 7 years ago

How about using STYLO_THREADS as a proxy? If you set it to a positive value, it uses that number of threads, if you set it to 0 (or -1, maybe), it finds a good number of threads, and you don't set it, stylo is not enabled.

	Bobby Holley (:bholley)
	Comment 23 • 7 years ago

STYLO_THREADS is already overloaded and used in a entirely different part of the code. I don't see any advantage to overloading it more for this very specific and short-lived use-case of overriding the normal pref mechanism.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 24 • 7 years ago

I filed bug 1379300 to add an env var for Stylo.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 25 • 7 years ago

With bug 1379300 landed, you should be able to use STYLO_FORCE_ENABLED=1 for this purpose.

	Chris Peterson [:cpeterson] Reporter
	Comment 26 • 7 years ago

Now that we have Stylo support in Nightly builds for Linux64, Mac, and Windows, we should run our Stylo tests on those platforms. We should run all the same test configurations as we do for linux64-stylo on inbound, autoland, and central:

https://docs.google.com/spreadsheets/d/1HmBsxrEpFfCgpLBG5Mvj5wEA9oEq5N8m4cxQYsSo4S8/edit#gid=1737640065

We don't need to run the Mac and Windows tests in Stylo sequential mode, just parallel mode.

	Chris AtLee [:catlee]
	Comment 27 • 7 years ago

Simply setting the env var isn't enough. The mozinfo file claims that stylo is off, which means that all of the skip-if=stylo tests run.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 28 • 7 years ago

:catlee attempted a try run to use the STYLO_FORCE_ENABLED=1 env var, but there's an additional issue of the test annotations.  Some tests use mozinfo to decide what tests should run, and this checks MOZ_STYLO_ENABLE from build time when setting the `stylo` field, which will be false if we use only the STYLO_FORCE_ENABLED=1 env var.  This means in the current state, Stylo is getting enabled, but the test harness thinks it is disabled, so it tries to run tests that are known to fail with Stylo.

:ahal, is there a good place to override mozinfo during test runs to adjust mozinfo based on an env var?

	Andrew Halberstadt [:ahal]
	Comment 29 • 7 years ago

Is this for mochitest? Looks like there's an --extra-mozinfo-json argument that will do what you want:
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/mochitest_options.py#356

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 30 • 7 years ago

Okay, I filed bug 1380082 to update the test harnesses to run the right tests with the env var.

	Chris Peterson [:cpeterson] Reporter
	Comment 31 • 7 years ago

(In reply to Andrew Halberstadt [:ahal] from comment #29)
> Is this for mochitest? Looks like there's an --extra-mozinfo-json argument
> that will do what you want:
> https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/
> mochitest_options.py#356

We currently run mochitests, reftests, crashtests, and web-platform-tests with Stylo enabled. Also, we run reftests in two different Stylo modes: comparing Stylo-vs-Stylo (similar to the default Gecko-vs-Gecko mode) and Stylo-vs-Gecko.

This spreadsheet describes the various combinations of tests and Stylo/e10s modes we run on the inbound, autoland, and central repos:

https://docs.google.com/spreadsheets/d/1HmBsxrEpFfCgpLBG5Mvj5wEA9oEq5N8m4cxQYsSo4S8/edit#gid=1737640065

	Andrew Halberstadt [:ahal]
	Comment 32 • 7 years ago

Ah ok, it's probably best to handle each of those within the respective harnesses since they use different manifest formats. I think this is most analogous to e10s, if you foresee developers wanting to run this locally a lot, it may be worth adding a --stylo flag to the harnesses or something.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 33 • 7 years ago

Here's a tweaked version of :catlee's changes to switch back to regular builds using the env var.

This depends on other patches in bug 1380082.  Here's a try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66695eac04fe2aa9e5514c05edd85714a896cf97

I am not sure what other steps might be needed, so I'll leave this part here for someone else to pick up.

	Chris Peterson [:cpeterson] Reporter
	Comment 34 • 7 years ago

Kim, Ryan is configuring the Stylo tests to use regular builds (instead of linux64-stylo) and to run on Mac and Windows. Once he has the tests running, you can turn off the linux64-stylo custom builds. In the meantime, I don't think you need to do anything for this bug.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 35 • 7 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #33)
> Created attachment 8885934 [details] [diff] [review]
> Use regular builds for Stylo via env var
> 
> Here's a tweaked version of :catlee's changes to switch back to regular
> builds using the env var.
> 
> This depends on other patches in bug 1380082.  Here's a try run:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=66695eac04fe2aa9e5514c05edd85714a896cf97
> 
> I am not sure what other steps might be needed, so I'll leave this part here
> for someone else to pick up.

Linux64 appears to work in this mode (with just the env var).  I am still triaging the state of other desktop platforms in bug 1380053.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 36 • 7 years ago

After talking on IRC, updating the scope of this bug to focus on the Linux64 part only.

For Stylo tests on other desktop platforms, we'll use the separate bug 1380053 (first to check tests, perhaps another is needed for actual task changes).

With Linux64, the tests appear green in my last try run from comment 33:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66695eac04fe2aa9e5514c05edd85714a896cf97

However, the special Stylo builds still ran, so it seems more steps are needed to actually remove them.  I don't think I am best placed to make this change, or at least I would need some pointers on what remains.

:catlee, since you started the WIP patch I used above, do you know what's left here?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 37 • 7 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #36)
> However, the special Stylo builds still ran, so it seems more steps are
> needed to actually remove them.

Ah, my mistake, the linux64-stylo builds do appear to be gone in the try run.  So maybe it's ready to go, but there could be other bits to clean up that I am unaware of.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 38 • 7 years ago

The try run in comment 33 was tests only, let's verify Talos as well:

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 39 • 7 years ago

Talos is driven by Buildbot, so we need a different way to enable Stylo.  Trying that based on :catlee's suggestion:

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 40 • 7 years ago

Another attempt to activate Stylo for Talos doesn't seem to be working:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a547800299e604ee7f7810edb044b4b76106dc9

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 41 • 7 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #40)
> Another attempt to activate Stylo for Talos doesn't seem to be working:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9a547800299e604ee7f7810edb044b4b76106dc9

Ah, my mistake, I must have opened the wrong log at first. :(

Okay, I'll fold this version into my patch.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 43 • 7 years ago

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

	Chris AtLee [:catlee]
	Comment 44 • 7 years ago
mozreview-review

Comment on attachment 8887711 [details]
Bug 1374748 - Use regular builds for Stylo via env var.

https://reviewboard.mozilla.org/r/158616/#review164244

	Pulsebot
	Comment 46 • 7 years ago

Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c21ec4c07f82
Use regular builds for Stylo via env var. r=catlee

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 47 • 7 years ago

Backing this out for breaking talos on at least Mac platforms.

Regular mac builds: https://treeherder.mozilla.org/logviewer.html#?job_id=115672376&repo=autoland
Devedition mac builds: https://treeherder.mozilla.org/logviewer.html#?job_id=115672403&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/22e24b1ada9431c21c7f1f8a950d68ec94842c83

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 49 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64a679a29253ad346446dce32cda8a59f1477665

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 51 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=442aec6e50e0f8e95ace7ad0945a9fbf0cb6eb96

	Pulsebot
	Comment 52 • 7 years ago

Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6c77cacaf82d
Use regular builds for Stylo via env var. r=catlee

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 53 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6c77cacaf82d