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

cmake: allow running the tests in parallel like in the Makefile (#102) #103

Conversation

isaac-io
Copy link
Contributor

@isaac-io isaac-io commented Aug 2, 2022

Allow running tests in parallel by default by setting the CTEST_PARALLEL_LEVEL
environment variable. The functionality is added using a CMake script in
order to determine options when running the tests (rather than when configuring
CMake) in compatibility with the Makefile (in particular -- the J and
TEST_TMPDIR environment variables).

Test Plan: run the check target using different generators (ninja, make)
to see the improvement in test runtime.

@isaac-io isaac-io added the Upstreamable can be upstreamed to RocksDB label Aug 2, 2022
@isaac-io isaac-io requested a review from mrambacher August 2, 2022 16:27
@isaac-io isaac-io self-assigned this Aug 2, 2022
@isaac-io isaac-io linked an issue Aug 2, 2022 that may be closed by this pull request
@isaac-io isaac-io force-pushed the 102-cmake-allow-running-the-tests-in-parallel-like-in-the-makefile branch 4 times, most recently from 89ab443 to 0c20a9f Compare August 2, 2022 18:52
Comment on lines 29 to 96
if(NOT DEFINED ENV{TEST_TMPDIR} AND "${CMAKE_HOST_SYSTEM_NAME}" STREQUAL "Linux")
# Use /dev/shm if the sticky bit is set
if(IS_DIRECTORY /dev/shm)
execute_process(COMMAND test -k /dev/shm RESULT_VARIABLE status OUTPUT_QUIET ERROR_QUIET)
if(status EQUAL 0)
set(test_dir /dev/shm)
endif()
endif()
# Fall back to /tmp if exists
if(NOT DEFINED test_dir AND IS_DIRECTORY /tmp)
set(test_dir /tmp)
endif()
# Create a temporary directory under the test location that we determined
if(DEFINED test_dir)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the same as the Makefile.

  • I think tests always create a directory under TEST_TMPDIR of the form rocksdb.XXX.
  • The Makefile does this on all non-Windows machines (not just Linux).
  • If TEST_TMPDIR is not set, it should default to TMPDIR (or /tmp)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Tests create their directories in the following format: ${TEST_TMPDIR:-/tmp/rocksdbtest-$(id -u)}/<test_name>_<pid>_<tid>. This is not a safe thing to do when running tests in parallel due to PID and TID reuse (not to mention two tests runs on the same machine).
  • Right, but the Makefile is wrong. /dev/shm is a Linux thing, not a POSIX thing, and many POSIX compliant OSs simply don't have it, or don't allow it to be used as a general purpose fileystem. I'll allow the rest of the temporary directory creation logic to work for other POSIX systems as well though.
  • Will do.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Tests create their directories in the following format: ${TEST_TMPDIR:-/tmp/rocksdbtest-$(id -u)}/<test_name>_<pid>_<tid>. This is not a safe thing to do when running tests in parallel due to PID and TID reuse (not to mention two tests runs on the same machine).

I understand this is not a safe thing to do. I actually put a PR into RocksDB to attempt to fix this, but it has not been accepted yet.

  • Right, but the Makefile is wrong. /dev/shm is a Linux thing, not a POSIX thing, and many POSIX compliant OSs simply don't have it, or don't allow it to be used as a general purpose fileystem. I'll allow the rest of the temporary directory creation logic to work for other POSIX systems as well though.

Agreed that /dev/shm is a Linux thing. But ALL of the code for creating rocksdbtest-* is under Linux and that is a Posix thing.

cmake/CTestRunner.cmake Outdated Show resolved Hide resolved
@isaac-io isaac-io force-pushed the 102-cmake-allow-running-the-tests-in-parallel-like-in-the-makefile branch 5 times, most recently from 88f8efa to 4b6e6a1 Compare August 24, 2022 10:45
@mrambacher
Copy link
Contributor

I am guessing this parallel test is invoked via J=# make check, but when I do that, I get an error:

J=20 make check

CMake Warning (dev) at /Users/markrambacher/Documents/GitHub/speedb/cmake/CTestRunner.cmake:19 (set):
Syntax error in cmake code at

/Users/markrambacher/Documents/GitHub/speedb/cmake/CTestRunner.cmake:19

when parsing string

${ENV{J}}

syntax error, unexpected {, expecting } (6)

Policy CMP0010 is not set: Bad variable reference syntax is an error. Run

@isaac-io isaac-io force-pushed the 102-cmake-allow-running-the-tests-in-parallel-like-in-the-makefile branch from 4b6e6a1 to a45bc46 Compare August 25, 2022 16:31
@isaac-io
Copy link
Contributor Author

Oops, my bad (a leftover from earlier experimentation). I pushed a fix for this.

cmake/CTestRunner.cmake Show resolved Hide resolved
Comment on lines 47 to 52
if(NOT DEFINED test_dir AND IS_DIRECTORY "$ENV{TMPDIR}")
set(test_dir $ENV{TMPDIR})
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

On Windows, should this also check ENV{TMP}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to a Raymond Chen post on the subject, we should probably check both TMP and TEMP on Windows. I'll add that (though the code in WinFileSystem::GetTestDirectory() already checks TMP on its own in case TEST_TMPDIR isn't set).

@isaac-io isaac-io force-pushed the 102-cmake-allow-running-the-tests-in-parallel-like-in-the-makefile branch 2 times, most recently from 0fcc3c6 to 59bb462 Compare August 28, 2022 09:33
Copy link
Contributor

@mrambacher mrambacher left a comment

Choose a reason for hiding this comment

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

LGTM

@isaac-io isaac-io force-pushed the 102-cmake-allow-running-the-tests-in-parallel-like-in-the-makefile branch from 59bb462 to bc75b1b Compare September 13, 2022 08:04
@isaac-io isaac-io force-pushed the 102-cmake-allow-running-the-tests-in-parallel-like-in-the-makefile branch from bc75b1b to 8e288ca Compare September 23, 2022 09:54
Allow running tests in parallel by default by setting the CTEST_PARALLEL_LEVEL
environment variable. The functionality is added using a CMake script in
order to determine options when running the tests (rather than when configuring
CMake) in compatibility with the Makefile (in particular -- the `J` and
`TEST_TMPDIR` environment variables).

While at it, add a test timeout parameter in order to kill stuck tests,
and set the default timeout to 10 minutes (we don't currently have tests
that are running longer than that).

Test Plan: run the check target using different generators (ninja, make)
to see the improvement in test runtime.
@isaac-io isaac-io force-pushed the 102-cmake-allow-running-the-tests-in-parallel-like-in-the-makefile branch from 8e288ca to 4010bf5 Compare October 3, 2022 10:43
@isaac-io isaac-io merged commit fb0127d into main Oct 3, 2022
@isaac-io isaac-io deleted the 102-cmake-allow-running-the-tests-in-parallel-like-in-the-makefile branch October 3, 2022 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Upstreamable can be upstreamed to RocksDB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cmake: allow running the tests in parallel like in the Makefile
2 participants