Closed Bug 1522113 Opened 6 years ago Closed 5 years ago

TV task scheduling is inefficient

Categories

(Testing :: General, enhancement, P3)

Product:
Testing
Component:
General
Version:
Version 3
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: gbrown, Assigned: bc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1522113 - schedule test-verify by test framework and directories rather than test source files, r=gbrown.
47 bytes, text/x-phabricator-request
Details | Review

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).

Priority: -- → P3

(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)

(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.

(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.

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.
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)

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)
Depends on: 1558931
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.

We still have the problem where changes to testing/web-platform/tests will cause not only TVw to be scheduled but TV and TVg as well.

The top level rule in moz.build

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

combined with the rule in testing/web-platform/moz.build

with Files("tests/**"):
    BUG_COMPONENT = ("Testing", "web-platform-tests")
    SCHEDULES.inclusive += ['test-verify-wpt']

means both schedules are invoked for changes to testing/web-platform/tests.

I can't think of a way to write a top level rule that would work for **/tests/** while excluding testing/web-platform/tests/**.

We have three choices:

  1. Remove the rule from the top-level moz.build for **/tests/** and place appropriate rules in the moz.build files of the appropriate child subdirectories of the top level. This choice involves a lot of changes to various moz.build files and would incur the risk of missing new ones.
  2. Rename testing/web-platform/tests to a unique name such as testing/web-platform/wpt-tests so that web-platform's moz.build could schedule test-verify-wpt when files in that directory changed but which would not cause TV or TVg to be scheduled. This choice seems to be the easiest path but is contingent on jgraham's approval.
  3. Do nothing. The extra jobs are mostly empty and do not take a long time to run. We could just live with it. This choice isn't ideal but isn't a horrible choice either.

jgraham, gbrown: What do you think?

Flags: needinfo?(james)
Flags: needinfo?(gbrown)

Renaming testing/web-platform/tests would unfortunately break a number of things that depend on that path e.g. the sync. There are also path length concerns with making the path longer. Therefore I'm pretty against option 2. I'm not sure what the restrictions are on the moz.build rules, but it would make sense to figure out a way to write more complex conditions, or to remove previously set values from lists rather than trying to force the tree to match the limitations of the system.

Flags: needinfo?(james)

Ok.

Flags: needinfo?(gbrown)

"Do nothing" may be the best we can do; I'm okay with that.

The leave-open keyword is there and there is no activity for 6 months.
:gbrown, maybe it's time to close this bug?

Flags: needinfo?(gbrown)

I have been pleased with the scheduling since comment 14.

Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(gbrown)
Keywords: leave-open
Resolution: --- → FIXED
Assignee: nobody → bob
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: