Gregory Szorc [:gps] Assignee
	Description • 7 years ago

We currently optimize away tasks for non-stylo platforms when only files in stylo/ change. Now that we're building Stylo in some build configurations, we shouldn't be doing this.

	Nathan Froyd [:froydnj]
	Comment 2 • 7 years ago
mozreview-review

Comment on attachment 8880929 [details]
Bug 1375976 - Schudule tasks for linux64 for Servo changes;

https://reviewboard.mozilla.org/r/152296/#review157344

r-'ing for now, but that is mostly so we can converse about the below.  If you can convince me, I can r+.

::: taskcluster/taskgraph/filter_tasks.py:40
(Diff revision 1)
>      """bug 1339542 When Servo vcs sync service lands Servo commits in
>      autoland repo, run linux64-stylo tests but skip other
>      platforms (to  reduce test load)."""
>  
>      SERVO_PLATFORMS = {
> +        'linux64',

This function is about making sure that we run *tests* for the given platforms, right?  I'm not really sure that we need to be running tests for stylo-built-but-disabled just because stylo (and not even necessarily stylo, this is for every servo commit!) got updated.  I mean, I kind of understand the careful approach here, but I'm not sure it's warranted.  Want to lay out a more convincing rationale?

	Gregory Szorc [:gps] Assignee
	Comment 3 • 7 years ago
mozreview-review-reply

Comment on attachment 8880929 [details]
Bug 1375976 - Schudule tasks for linux64 for Servo changes;

https://reviewboard.mozilla.org/r/152296/#review157344

> This function is about making sure that we run *tests* for the given platforms, right?  I'm not really sure that we need to be running tests for stylo-built-but-disabled just because stylo (and not even necessarily stylo, this is for every servo commit!) got updated.  I mean, I kind of understand the careful approach here, but I'm not sure it's warranted.  Want to lay out a more convincing rationale?

The function is called for *all* tasks.

I agree we probably don't need to run tests for *all* Servo changes. However, since Stylo is being built as part of Linux64 now, we need CI coverage that things work, otherwise we may not identify breakage in Linux64 until a subsequent push.

I see the following potential solutions:

1. This patch. We run a lot of tests for most changes to Servo that won't impact anything in Firefox. Not optimal.
2. Bump linux64-stylo build to tier 1, leave tests as tier 2. We'll have CI coverage warranting a backout and tree closure if a Servo change breaks the build. We'll effectively be testing --enable-stylo instead of enable-stylo=build. Not sure if that matters.
3. Schedule linux64 builds but not tests for Servo changes.
4. Schedule linux64 builds and some tests, possibly depending on which paths under servo/ change.

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

(In reply to Gregory Szorc [:gps] from comment #0)
> We currently optimize away tasks for non-stylo platforms when only files in
> stylo/ change. Now that we're building Stylo in some build configurations,
> we shouldn't be doing this.

Nit: servo, rather than stylo/.

Also, it would be really great if we could avoid building for changes to crates that stylo doesn't build in (most of servo, really). That said, given how many members of the servo community (and thus percentage of servo PRs) are currently working on stylo, we can probably deal with this later.

	Nathan Froyd [:froydnj]
	Comment 5 • 7 years ago

(In reply to Gregory Szorc [:gps] from comment #3)
> Comment on attachment 8880929 [details]
> Bug 1375976 - Schudule tasks for linux64 for Servo changes;
> 
> https://reviewboard.mozilla.org/r/152296/#review157344
> 
> > This function is about making sure that we run *tests* for the given platforms, right?  I'm not really sure that we need to be running tests for stylo-built-but-disabled just because stylo (and not even necessarily stylo, this is for every servo commit!) got updated.  I mean, I kind of understand the careful approach here, but I'm not sure it's warranted.  Want to lay out a more convincing rationale?
> 
> The function is called for *all* tasks.
>
> I agree we probably don't need to run tests for *all* Servo changes.
> However, since Stylo is being built as part of Linux64 now, we need CI
> coverage that things work, otherwise we may not identify breakage in Linux64
> until a subsequent push.

I guess I'm skeptical that "linux64-stylo builds, but linux64 does not" for such changes, given that they're basically building the exact same bits, and all that's different between them is a pref flip.  Whether the tests for linux64 are subject to massive changes depends on your taste for risk levels.

I guess perhaps it would still be good to have build/talos/etc. metrics for linux64 sooner rather than later for servo changes--I'm not sure that we're necessarily tracking some of those for the linux64-stylo builds?

> I see the following potential solutions:
> 
> 1. This patch. We run a lot of tests for most changes to Servo that won't
> impact anything in Firefox. Not optimal.

Agreed...but also the soonest to discover problems, most likely.

> 2. Bump linux64-stylo build to tier 1, leave tests as tier 2. We'll have CI
> coverage warranting a backout and tree closure if a Servo change breaks the
> build. We'll effectively be testing --enable-stylo instead of
> enable-stylo=build. Not sure if that matters.

I don't think this changes much, honestly.

> 3. Schedule linux64 builds but not tests for Servo changes.

As above, I'm skeptical that the linux64 and the linux64-stylo builds would ever come back with different results.

> 4. Schedule linux64 builds and some tests, possibly depending on which paths
> under servo/ change.

This is promising, particularly if we could run the same set of tests somehow.

But maybe it's not worth it...servo/servo gets somewhere around 100 commits/week.  So 100 commits/week * 1 linux64 test run/commit * $1/run (guess) * 8 weeks before we merge these things anyway...almost to the point where we should stop arguing about it.

Let's just do this; it might catch some stuff, and anything more complicated is, well, more complicated.

	Nathan Froyd [:froydnj]
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8880929 [details]
Bug 1375976 - Schudule tasks for linux64 for Servo changes;

https://reviewboard.mozilla.org/r/152296/#review157380

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

BTW, why not remove the linux64-stylo builds entirely, and do like e10s, run tests with the pref turned on?

	Gregory Szorc [:gps] Assignee
	Comment 8 • 7 years ago

(In reply to Mike Hommey [:glandium] from comment #7)
> BTW, why not remove the linux64-stylo builds entirely, and do like e10s, run
> tests with the pref turned on?

This is the goal. I didn't want to scope bloat bug 1374824.

	Gregory Szorc [:gps] Assignee
	Comment 9 • 7 years ago

As a concrete example of why we should run automation, a few hours ago glandium's changes to thread pools landed in upstream Servo. That *should* reduce RSS in Firefox Talos. However, no Talos jobs ran for that push (https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=fea287f1739e78b2d13b17085a2c51a920f0c20f). So the presumed Perfherder alert will likely be attributed to the wrong push :/

	Pulsebot
	Comment 10 • 7 years ago

Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9e6e06dfae20
Schudule tasks for linux64 for Servo changes; r=froydnj

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

https://hg.mozilla.org/mozilla-central/rev/9e6e06dfae20