TV task scheduling is inefficient

NEW
Unassigned

Status

enhancement
P3
normal
5 months ago
7 days ago

People

(Reporter: gbrown, Unassigned)

Tracking

(Blocks 2 bugs, {leave-open})

Version 3
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Reporter

Description

5 months ago

I see some inefficiencies in how TV/TVg/TVw tasks are scheduled currently. I don't think there are major gains to be had here, but it could be improved.

Specifically:

  1. On desktop platforms, it seems like TVw is scheduled whenever TV is scheduled. I think TVw could be scheduled only when a push contains a change in testing/web-platform/tests.
  2. Similar to 1, if a push only contains changes in testing/web-platform/tests, I think TV and TVg could be skipped.
  3. We may be over-estimating the number of chunks required. Sometimes I see 3 TV chunks, but only 1 test run in a chunk.
  4. If gpu tests could be more conclusively identified in the decision task, we could better determine when TVg should be scheduled vs TV. Could we move gpu tests to /gpu sub-directories perhaps?
  5. Do we need to run TV/TVg/TVw on merges? Could that be avoided? Could we bail out faster?

I don't want to impact TV effectiveness; we'll need to coordinate with those working on TV (:bc and I).

Reporter

Updated

5 months ago
Reporter

Updated

5 months ago
Priority: -- → P3

Updated

2 months ago

(In reply to Geoff Brown [:gbrown] from comment #0)

  1. On desktop platforms, it seems like TVw is scheduled whenever TV is scheduled. I think TVw could be scheduled only when a push contains a change in testing/web-platform/tests.

TVw isn't always scheduled with TV/TVg which can be seen by inspecting mozilla-central and from the schedule in web-platform/moz.build.

  1. Similar to 1, if a push only contains changes in testing/web-platform/tests, I think TV and TVg could be skipped.

If TVw is scheduled then TV/TVg are scheduled because of the schedules in moz.build which schedule TV and TVg whenever any js, html, xhtml or xul files are changed anywhere in the tree.

I think it is incorrect to schedule TV/TVg whenever these files are changed anywhere in the tree. We should schedule it when files in any of the test frameworks change as is already done for web-platform but not for arbitrary files.

We could do a quick and dirty change to schedule TV/TVg if a manifest file like reftest.list, crashtest*.list or mochitest.ini changed. This would not catch all changes to the actual tests however. To be complete, we would need to add the appropriate schedules to the various moz.builds scattered through the tree but I think this would be the right way forward.

Has my inexperience with TV and schedules led me astray? Thoughts?

Flags: needinfo?(gbrown)
Reporter

Comment 2

Last month

(In reply to Bob Clary [:bc:] from comment #1)

(In reply to Geoff Brown [:gbrown] from comment #0)

  1. On desktop platforms, it seems like TVw is scheduled whenever TV is scheduled. I think TVw could be scheduled only when a push contains a change in testing/web-platform/tests.

TVw isn't always scheduled with TV/TVg which can be seen by inspecting mozilla-central and from the schedule in web-platform/moz.build.

Agreed. I think something has been fixed since I reported this bug. Let's consider part 1 worksforme.

  1. Similar to 1, if a push only contains changes in testing/web-platform/tests, I think TV and TVg could be skipped.

If TVw is scheduled then TV/TVg are scheduled because of the schedules in moz.build which schedule TV and TVg whenever any js, html, xhtml or xul files are changed anywhere in the tree.

I think it is incorrect to schedule TV/TVg whenever these files are changed anywhere in the tree. We should schedule it when files in any of the test frameworks change as is already done for web-platform but not for arbitrary files.

We could do a quick and dirty change to schedule TV/TVg if a manifest file like reftest.list, crashtest*.list or mochitest.ini changed. This would not catch all changes to the actual tests however. To be complete, we would need to add the appropriate schedules to the various moz.builds scattered through the tree but I think this would be the right way forward.

We definitely want to run TV when an existing test is updated, and that is the tricky part: There may be no changes to a test manifest, but only changes to some js/html/etc file -- and those files might look just like non-test files. Once TV is scheduled, test-verify support in mozharness usually does a good job of determining if a changed file is a test file:

https://searchfox.org/mozilla-central/rev/cc280c4be94ff8cf64a27cc9b3d6831ffa49fa45/testing/mozharness/mozharness/mozilla/testing/per_test_base.py#45

...by searching for file names (of files changed in the push) in all the test manifests associated with test suites supported by TV. But that code runs in a TV task. For task scheduling, code runs in the decision task, which doesn't normally have easy access to test manifests (no test packages downloaded) and needs to remain reliable and as fast as possible. That's why we resort to this kind of hackiness:

https://searchfox.org/mozilla-central/rev/cc280c4be94ff8cf64a27cc9b3d6831ffa49fa45/taskcluster/taskgraph/util/perfile.py#22

Schedules in moz.build might be a better way. But would there be a danger of the moz.build changes getting out of sync with manifests? If someone adds new tests to a new test manifest, but doesn't update moz.build schedules, what happens?

Flags: needinfo?(gbrown)

(In reply to Geoff Brown [:gbrown] from comment #2)

Schedules in moz.build might be a better way. But would there be a danger of the moz.build changes getting out of sync with manifests? If someone adds new tests to a new test manifest, but doesn't update moz.build schedules, what happens?

I think there are 3 parts to this:

  1. schedules for when a test framework's source changes.
    What should we do here? I would think this should verify all of the tests but from recent personal experience, that would take far too long.
  2. schedules for when a test manifest is added or modified.
    This would take care of your concern above about manifest changes.
  3. schedules for when a test is added or modified.

#1 and #2 could be done in the global moz.build. #3 would require us to find the directories containing tests, then update the ancestor moz.build to add a schedule for the contained files. It would be a little tedious but not too much work.

I can see a possible problem when someone adds a new directory of tests but does not add schedule verify for them in the local moz.build file. We would still see the newly added manifest file as long as it's filename matches our known patterns so we would get scheduled the first time it was added and any time later that it was modified but not automatically when an individual file in a test was changed. Is there a lint-like solution that would enforce a verify schedule for a test directory's moz.build?

Let me work up a patch and see how it does and we can proceed from there.

At first I tried to do this on the level of the test moz.builds, but after some work I decided I could actually accomplish the same thing using the global moz.build. I used the manifest file names along with directory names to select when to schedule TV/TVg.

In the simple test I performed, it was very similar but there was one noticeable difference.

./mach try fuzzy --query 'verify'

moz.build patch: https://hg.mozilla.org/try/rev/54548a533d2f01e5c6838e6ef064f7269c248eb8
trigger patch : https://hg.mozilla.org/try/rev/e498cc84eb14a827057c15127b648711cc31cd4c

Without the trigger patch, neither run performs any test verifications which is expected.

With the trigger patch, the run with the moz.build patch contains more tests for
test-android-em-4.3-arm7-api-16/pgo-test-verify-1proc TV. The others were identical.

+Per-test run found test testing/mochitest/tests/Harness_sanity/test_sanitySimpletest.html (mochitest-plain/None)
+Per-test run found test widget/tests/test_sizemode_events.xul (mochitest-chrome/None)

Not sure why. Thoughts?

Flags: needinfo?(gbrown)

I think I messed up and accidentally removed docs for .js. Will fix before I phab it.

Reporter

Comment 6

Last month

(In reply to Bob Clary [:bc:] from comment #4)

With the trigger patch, the run with the moz.build patch contains more tests for
test-android-em-4.3-arm7-api-16/pgo-test-verify-1proc TV. The others were identical.

+Per-test run found test testing/mochitest/tests/Harness_sanity/test_sanitySimpletest.html (mochitest-plain/None)
+Per-test run found test widget/tests/test_sizemode_events.xul (mochitest-chrome/None)

Not sure why. Thoughts?

Both tests are skipped on Android. Look at the logs for "Per-test run using mozinfo" -- one has os == 'android' while the other has os == 'linux'. Why would that be?

Flags: needinfo?(gbrown)

Weird, the run with the moz.build patch but without the trigger show the correct mozinfo but the one with the trigger is missing any indication that it is an android build. I'll try this again and see if that is reproducible.

Comment 9

Last month
Pushed by bclary@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d14aa373383
schedule test-verify by test framework and directories rather than test source files, r=gbrown.

Updated

Last month
Keywords: leave-open

Backed out changeset 8d14aa373383 (Bug 1522113) for Bugzilla lint failures

Push with failures - uh... not necessarily failure but a big increase in the time needed to complete the job: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linting%2Copt%2Csource-test-file-metadata-bugzilla-components%2C%28bugzilla%29&fromchange=0013ca341d7c9e7dbb6195967002e7b1963da679&tochange=c062846ae62bc7817f35e4994cb7f2e524c017ff&selectedJob=247084785

Backout link: https://hg.mozilla.org/integration/autoland/rev/c062846ae62bc7817f35e4994cb7f2e524c017ff

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=247086317&repo=autoland&lineNumber=194

[vcs 2019-05-17T17:44:10.935Z] updating [==============================================> ] 264900/276879 02s
[vcs 2019-05-17T17:44:11.726Z] updating [================================================> ] 272000/276879 01s
[vcs 2019-05-17T17:44:21.694Z]
[vcs 2019-05-17T17:44:21.694Z] 276879 files updated, 0 files merged, 0 files removed, 0 files unresolved
[vcs 2019-05-17T17:44:21.978Z] updated to 31953bf83dfd9710390419a006d41c51ff61b101
[vcs 2019-05-17T17:44:21.982Z] PERFHERDER_DATA: {"framework": {"name": "vcs"}, "suites": [{"extraOptions": ["c3.2xlarge"], "lowerIsBetter": true, "name": "clone", "serverUrl": "us-east-1.hgmointernal.net", "shouldAlert": false, "subtests": [], "value": 126.12949419021606}, {"extraOptions": ["c3.2xlarge"], "lowerIsBetter": true, "name": "update", "serverUrl": "us-east-1.hgmointernal.net", "shouldAlert": false, "subtests": [], "value": 49.984090089797974}, {"extraOptions": ["c3.2xlarge"], "lowerIsBetter": true, "name": "overall", "serverUrl": "us-east-1.hgmointernal.net", "shouldAlert": false, "subtests": [], "value": 176.2576129436493}, {"extraOptions": ["c3.2xlarge"], "lowerIsBetter": true, "name": "overall_clone", "serverUrl": "us-east-1.hgmointernal.net", "shouldAlert": false, "subtests": [], "value": 176.2576129436493}, {"extraOptions": ["c3.2xlarge"], "lowerIsBetter": true, "name": "overall_clone_fullcheckout", "serverUrl": "us-east-1.hgmointernal.net", "shouldAlert": false, "subtests": [], "value": 176.2576129436493}]}
[vcs 2019-05-17T17:44:22.272Z] TinderboxPrint:<a href=https://us-east-1.hgmointernal.net/integration/autoland/rev/31953bf83dfd9710390419a006d41c51ff61b101 title='Built from autoland revision 31953bf83dfd9710390419a006d41c51ff61b101'>31953bf83dfd9710390419a006d41c51ff61b101</a>
[task 2019-05-17T17:44:22.272Z] executing ['bash', '-cx', 'cd $GECKO_PATH && ./mach file-info bugzilla-automation /builds/worker/artifacts']
[task 2019-05-17T17:44:22.274Z] + cd /builds/worker/checkouts/gecko
[task 2019-05-17T17:44:22.274Z] + ./mach file-info bugzilla-automation /builds/worker/artifacts
[task 2019-05-17T17:45:15.067Z] WARNING: Not a supported OS_TARGET for NSPR in moz.build: "". Use --with-system-nspr
[task 2019-05-17T17:45:17.453Z] WARNING: Cpu arch is not expected
[task 2019-05-17T17:45:17.453Z] WARNING: Stack alignment cannot be zero.
[taskcluster:error] Task timeout after 2700 seconds. Force killing container.
[taskcluster 2019-05-17 18:26:25.279Z] === Task Finished ===
[taskcluster 2019-05-17 18:26:25.280Z] Unsuccessful task run with exit code: -1 completed in 2725.523 seconds

Flags: needinfo?(bob)

I guess the schedules were too much at the top level. Thanks and sorry.

Flags: needinfo?(bob)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:bc, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(bob)

Comment 13

17 days ago

It did land and was backed out due to increasing the bugzilla-automation step's run time. I've spent some time looking into it but the inefficiencies of the moz.build Files contexts are not something that can be fixed easily. Since then I have been pulled into something else and I will get back to this when I have a chance.

Flags: needinfo?(bob)

Updated

8 days ago
Depends on: 1558931

Comment 14

7 days ago
Pushed by bclary@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/397a8c465915
schedule test-verify by test framework and directories rather than test source files, r=gbrown.
You need to log in before you can comment on or make changes to this bug.