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

test: limit test-run jobs number to CPU count #7494

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ylobankov
Copy link
Member

@ylobankov ylobankov commented Jul 29, 2022

This patch is intended to limit the number of test-run jobs to the CPU
count. Before this change test-run always ran with its default value
for jobs number - 2 x CPU count. However, such a value may potentially
lead to flaking test results and it really annoys everyone. So it was
decided to give a chance to this patch and see what would happen.

NO_DOC=testing stuff
NO_TEST=testing stuff
NO_CHANGELOG=testing stuff

@ylobankov ylobankov requested a review from ligurio July 29, 2022 12:10
@ylobankov ylobankov added the full-ci Enables all tests for a pull request label Jul 29, 2022
@coveralls
Copy link

coveralls commented Jul 29, 2022

Coverage Status

Coverage increased (+0.02%) to 84.237% when pulling c371512 on ylobankov/use-nproc-in-test-run into 822396d on master.

@ylobankov ylobankov force-pushed the ylobankov/use-nproc-in-test-run branch 2 times, most recently from 5c621fe to c371512 Compare July 29, 2022 14:08
@Totktonada
Copy link
Member

  1. Did you check how overall time for the whole test suite is changed?
  2. Why don't change the default in test-run (less code is better, IMHO)?

Copy link
Member

@ligurio ligurio left a comment

Choose a reason for hiding this comment

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

Yaroslav, thanks for the patch!

Before this change test-run always ran with its default value
for jobs number - 2 x CPU count. However, such a value may potentially
lead to flaking test results and it really annoys everyone. So it was
decided to give a chance to this patch and see what would happen.

Without proofs, it is just a hypothesis.
I see another value in your patch - ability to set number of jobs for compiler, test-run.py, prove (using in LuaJIT) in a single place.

add_custom_target(test-unit
DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/small
COMMAND ${PROJECT_SOURCE_DIR}/test/test-run.py
--jobs ${TEST_RUN_JOBS}
--builddir=${PROJECT_BINARY_DIR}
Copy link
Member

Choose a reason for hiding this comment

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

Should we use symbol = in --jobs as in --builddir option? The same below.

Comment on lines 50 to +59

# Get the test-run jobs count from the env variable. If the variable is not
# defined, use the CPU count instead. If the ProcessorCount fails to determine
# the number of cores, it yields 0, which means for test-run 2 x CPU count.
set(TEST_RUN_JOBS $ENV{TEST_RUN_JOBS})
if(NOT TEST_RUN_JOBS)
include(ProcessorCount)
ProcessorCount(TEST_RUN_JOBS)
endif()

Copy link
Member

Choose a reason for hiding this comment

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

Can we reuse CMake module made for LuaJIT 1 (see discussion 2)?
I would reuse the same approach: CMake uses CMAKE_BUILD_PARALLEL_LEVEL 3 when it is set, or automatically set it to a maximum number of CPU's when it is empty.

Footnotes

  1. https://github.com/tarantool/luajit/blob/tarantool/cmake/SetBuildParallelLevel.cmake

  2. https://lists.tarantool.org/tarantool-patches/Ys6aKWKAEeY%2F4NYl@tarantool.org/T/#t

  3. https://cmake.org/cmake/help/latest/envvar/CMAKE_BUILD_PARALLEL_LEVEL.html

@@ -17,6 +17,7 @@ MAX_PROCS ?= 2048
MAX_FILES ?= 4096

VARDIR ?= /tmp/t
TEST_RUN_JOBS ?= ${NPROC}
Copy link
Member

Choose a reason for hiding this comment

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

How it is supposed to work? It is not clear what is a ${NPROC} and where it is set.

# Get the test-run jobs count from the env variable. If the variable is not
# defined, use the CPU count instead. If the ProcessorCount fails to determine
# the number of cores, it yields 0, which means for test-run 2 x CPU count.
set(TEST_RUN_JOBS $ENV{TEST_RUN_JOBS})
Copy link
Member

Choose a reason for hiding this comment

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

I believe it is better to have a single variable with number of jobs for everything: compilers, test-run.py, prove etc.
I can't imagine reasons to set a separate number of jobs for all three things.

@ylobankov ylobankov removed the full-ci Enables all tests for a pull request label Aug 1, 2022
@ylobankov ylobankov force-pushed the ylobankov/use-nproc-in-test-run branch 3 times, most recently from 9dd02bd to 61daa32 Compare August 16, 2022 10:20
@ylobankov ylobankov added the do not merge Not ready to be merged label Aug 16, 2022
@ylobankov ylobankov marked this pull request as draft August 16, 2022 11:28
@ylobankov ylobankov force-pushed the ylobankov/use-nproc-in-test-run branch 4 times, most recently from 587e260 to 90dbaaa Compare August 16, 2022 12:06
This patch is intended to limit the number of test-run jobs to the CPU
count. Before this change test-run always ran with its default value
for jobs number - 2 x CPU count. However, such a value may potentially
lead to flaking test results and it really annoys everyone. So it was
decided to give a chance to this patch and see what would happen.

NO_DOC=testing stuff
NO_TEST=testing stuff
NO_CHANGELOG=testing stuff
@ylobankov ylobankov force-pushed the ylobankov/use-nproc-in-test-run branch from 90dbaaa to b744fc5 Compare August 16, 2022 13:50
@NickVolynkin
Copy link
Contributor

Issue tarantool/tarantool-qa#80 seems to be related.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Not ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants