-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Speed up CTest, particularly for RDataFrame #19702
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
base: master
Are you sure you want to change the base?
Conversation
Test Results 20 files 20 suites 3d 19h 41m 18s ⏱️ For more details on these failures, see this check. Results for commit c166439. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
cmake/modules/RootMacros.cmake
Outdated
@@ -2714,7 +2714,7 @@ macro(ROOTTEST_GENERATE_EXECUTABLE executable) | |||
endif() | |||
|
|||
if(CMAKE_GENERATOR MATCHES Ninja AND NOT MSVC) | |||
set_property(TEST ${GENERATE_EXECUTABLE_TEST} PROPERTY RUN_SERIAL true) | |||
set_property(TEST ${GENERATE_EXECUTABLE_TEST} PROPERTY RESOURCE_LOCK NINJA_COMPILATION) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important note: This changes works IF AND ONLY IF the ninja compilation does not induce spurious rebuild of the core libraries .. which unfortunately does happens from time to time (for example on Windows or any other machine with 'low' timestamp granularity where ninja gets confused and rebuild things ... things may or may not be better after we recently removed a large swat of spurious updates/rebuild when doing 2 consecutive builds)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As can be seen from the line above, Windows was never part of this adventure, so I guess that we don't need to worry about this in this context.
Regarding rebuilds, I'm testing at the moment a fix for the outdated build tree problem mentioned in the PR description. Will ping once done.
For the |
…ests. In roottest, some targets are added to the Ninja build graph. These are compiled as part of the test suite, but this would create race conditions when many builds are started in parallel. Therefore, all these builds were flagged with RUN_SERIAL, significantly reducing the parallelism of roottest. Now, the tests that compile have "RESOURCE_LOCK NINJA_COMPILATION", so only a single test that compiles exectables can run, but it can run in parallel with tests that don't compile.
In addition to correctly specifying the number of CPUs in CMake, the tutorials were also setting a resource lock, which prevented them from running in parallel with other RDF tutorials. Since ROOT honours ROOT_MAX_THREADS, this resource lock can be removed. Co-authored-by: Philippe Canal <[email protected]>
0dc047e
to
c166439
Compare
Allow Ninja compilation in CTest to run in parallel with other tests
roottest Ninja targets are rebuilt as part of a ctest run. If two tests invoke Ninja, race conditions will occur.
To prevent that, ROOT used to run all tests with Ninja compilation serially, reducing the CTest throughput.
Here, by using a
RESOURCE_LOCK
in ctest, Ninja compilations are not run from multiple tests at the same time. They may, however, run in parallel with pre-compiled tests.The following is now outdated:
The above problem is fixed by adding another test that updates the build tree, running in complete isolation. This is equivalent to running
ninja && ctest
, only that the ninja invocation is part of the ctest run if tests that require compilation are part of the test suite.Allow multi-threaded RDF tutorials to run in parallel with others
ROOT used to forbid running an MT RDF tutorial in parallel with another MT RDF tutorial. When running
ctest -R dataframe -jN
, this can lead to a long tail of tutorials running completely sequentially. Nevertheless, since ROOT honoursROOT_MAX_THREADS
, each was only running with 4 cores.Here, the strict
RESOURCE_LOCK
is removed, and CMake is just informed about the required number of CPU cores. This allows for running a few of the MT tutorials in parallel.