Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve testing infrastructure #366

Merged

Conversation

matt-gretton-dann
Copy link
Collaborator

This PR improves the testing infrastructure as follows:

  • Uses CTest more effectively so that we can tests in parallel.
  • Updates the GitHub Actions CI loop to run tests in parallel.
  • Adds an test-accept build target which which enables test changes to be accepted using the build system.
  • Improves documentation.

Importantly these tests can now be run from within Visual Studio (2019 v16.6 tested), and test changes can be accepted using the Visual Studio GUI. (Although I recommend using the command line as it runs the tests faster). Documentation here: https://github.com/mrc-ide/covid-sim/blob/1221aac5772aed8d7ba4b2a8410d809841fb1361/docs/build.md

@dlaydon, @NeilFerguson: This should hopefully improve your experience - but I would appreciate any comments questions you have.

Matthew Gretton-Dann added 8 commits June 5, 2020 11:28
This improves the parallelism of the testing infrastructure.

So if the tests are invoked with -j2 up to two tests will run at once.
We also note which tests are multi-processor and ensure they request the
correct resources.
This enables us to do:

`make test-accept`

to accept test changes, and means that all testing can be done by
invocations of cmake or the underlying build system.
These are now part of make test and make test-accept
Copy link
Contributor

@ozmorph ozmorph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Are there any theories as to why the Windows regression tests take 2.1-2.4 times longer than any of the Linux or MacOS tests?

For instance, this pull request's regression tests on Windows took about 1 hour 24 minutes. Ubuntu on the other hand took 36 minutes.

If we're sure it isn't Azure/VM related, then I may try diagnosing the problem further.

@matt-gretton-dann
Copy link
Collaborator Author

Looks great! Are there any theories as to why the Windows regression tests take 2.1-2.4 times longer than any of the Linux or MacOS tests?

For instance, this pull request's regression tests on Windows took about 1 hour 24 minutes. Ubuntu on the other hand took 36 minutes.

If we're sure it isn't Azure/VM related, then I may try diagnosing the problem further.

I'm not sure whether it is VM related or not. I experience similar slow-downs when running on my Windows VM on my dev machine - and I don't have access to a similarly specced bare-metal Windows machine to compare to my Windows one.

The first thing I would check is whether removing all *printf() calls speed us up (as these turn into file writes and that's always my first suspect), and I wonder if there are any OpenMP performance issues on Windows (I haven't researched this).

@ozmorph
Copy link
Contributor

ozmorph commented Jun 17, 2020

@matt-gretton-dann Thanks for responding!

I thought the same thing initially. I haven't ruled out *printf() calls being the culprit, but I'm doing most of my development on Windows as well and frequently test both on a ext4 partition in WSL2 and on an NTFS filesystem using either Clang 10 or MSVC. To this point, I haven't seen a noticable difference in run-time between the two, which leads me to believe that it's not a file buffering or file-system issue. But I have no evidence to point to the contrary yet.

As you may or may not have seen in #388, I've discovered several locations in the code that benefit greatly (43% run-time reduction for the US test) from some minor alterations to memory storage and access. These have greatly improved the CPU caching behavior for AssignPeopleToPlaces(), at least on my workstation. I've also discovered another great opportunity within InfectSweep() in the past 24 hours that I hope will bring down the run-time even more.

@weshinsley weshinsley self-requested a review June 18, 2020 10:15
@matt-gretton-dann matt-gretton-dann merged commit effd349 into master Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants