Closed Bug 1410911 Opened 2 years ago Closed 2 years ago

test-verify/jsreftest/jittest runs too often

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gbrown, Assigned: dustin)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Since approximately bug 1403322, test-verify (TV) runs on every push (unless the build has been coalesced or otherwise excluded), rather than only when a .html/.js/.xul/.xhtml file is modified.
A quick but speculative fix. This does no harm:

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

and I prefer having just the one "test-verify" name in play, but I don't know if it eliminates the extra runs.
Attachment #8921116 - Flags: review?(dustin)
Comment on attachment 8921116 [details] [diff] [review]
use 'test-verify' consistently

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

Sorry, I think that was my fault.
Attachment #8921116 - Flags: review?(dustin) → review+
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37db725b2308
Only run test-verify when certain file types are changed; r=dustin
Keywords: leave-open
It looks like my change is not fixing this problem. (But I don't mind keeping it / no need to backout.)


Looking more closely, other test suites, like jit and jsreftest, are also running on most pushes, seemingly regardless of which files have changed. I think it is a regression from bug 1403322, but I don't see what's going wrong.

Consider:

https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=jsref&tochange=1da1df814ad3bcb7aefb5f2c00c18b3c55f72284&fromchange=4dc78384cb58c67de770ef207bbd700e7a982960

:dustin -- Can you take this?
Flags: needinfo?(dustin)
I'm at a meeting all week, so not quickly.

It might be a start to look at the SCHEDULES components that are determined in the decision task -- do those include test-verify more often than they should?
Flags: needinfo?(dustin)
(In reply to Dustin J. Mitchell [:dustin] from comment #5)
> It might be a start to look at the SCHEDULES components that are determined
> in the decision task -- do those include test-verify more often than they
> should?

I'm not sure I know how to answer that.


I had a look at SkipUnlessSchedules' should_remove_task(). For a push with only a couple of python files modified, and considering a task like "test-linux64/opt-test-verify-e10s", should_remove_task() sees:

scheduled == set([u'robocop', u'geckoview', u'reftest', u'web-platform-tests', u'macosx', u'talos', u'cppunittest', u'awsy', u'mochitest', u'web-platform-tests-reftests', u'web-platform-tests-wdspec', u'linux', u'telemetry-tests-client', u'windows', u'marionette', u'firefox-ui', u'gtest', u'xpcshell', u'android', u'xpcshell-coverage'])

conditions == set(['test-verify', 'linux'])

so scheduled & conditions == set(['linux']) and should_remove_task() returns false.
Summary: test-verify runs too often → test-verify/jsreftest/jittest runs too often
I have a feeling that we can't combine SCHEDULES.inclusive with SCHEDULES.exclusive. Consider test-verify. It has a it's own SCHEDULES.inclusive tag which should govern when it gets optimized or not. However, it also has an OS family (one of linux, macosx, windows or android). This means that this statement will always return True and the task will never be optimized:
https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/optimize.py#386

Maybe there's something clever we can do here to better distinguish between inclusive and exclusive components, but maybe the cleanest option is to have two separate optimization: "skip-unless-schedules-exclusive" and "skip-unless-schedules-inclusive", and each task would need to decide which one to use.
I had thought that we might be able to just change:

         scheduled = self.scheduled_by_push(params['head_repository'], params['head_rev'])
         conditions = set(conditions)
-        # if *any* of the condition components are scheduled, do not optimize
-        if conditions & scheduled:
+        # if *all* of the condition components are scheduled, do not optimize
+        if conditions & scheduled == conditions:
             return False

Indeed, that seems to fix the jittest and jsreftest cases, and possibly some test-verify issues.

But, especially with test-verify, there can be a conflict between inclusive and exclusive. For instance, testing/mochitest has 

    SCHEDULES.exclusive = ['mochitest', 'robocop']

But that directory also contains .js/.html test files that should trigger test-verify:

    with Files("**/*.js"):
        SCHEDULES.inclusive += ['test-verify']

    with Files("**/*.html"):
        SCHEDULES.inclusive += ['test-verify']

and that gets confusing:

  mach file-info schedules testing/mochitest/tests/Harness_sanity/test_spawn_task.html
  robocop, mochitest
Yeah, `conditions & schedules == conditions` won't work. I stared at this for quite some time and I'm pretty sure the only way to properly solve it is to split inclusive and exclusive into two separate optimizations.
Your reasoning makes sense, and I feel silly for not seeing it.
See Also: → 1412349
:dustin - It sounds like you are the best person for this...hope you have the time.
Assignee: gbrown → dustin
We continue to run lots of unnecessary tests -- Android jsreftest alone is an extra 140 tasks per push on central (mitigated by SETA elsewhere).

Can we back out bug 1403322, or fall back to when: files-changed?
Flags: needinfo?(dustin)
If you think that's not too risky right now, please feel free.
Flags: needinfo?(dustin)
Comment on attachment 8927400 [details]
Bug 1410911 - back out bug 1403322;

https://reviewboard.mozilla.org/r/198704/#review203820

Thanks. This looks right.

I think the reward justifies the risk here, but you have a more complete perspective: I understand if you want to wait before checking in.
Attachment #8927400 - Flags: review?(gbrown) → review+
OK, the complaint of this bug was fixed by the backout.  I'll re-land in bug 1403322, with the fix in bug 1426254.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.