-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Run tests in parallel by default #22939
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?
Changes from all commits
f1c2b79
9fb7077
e1a6312
472acd1
73fa923
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,9 +34,9 @@ function show_usage(): void | |
| php run-tests.php [options] [files] [directories] | ||
|
|
||
| Options: | ||
| -j<workers> Run up to <workers> simultaneous testing processes in parallel for | ||
| quicker testing on systems with multiple logical processors. | ||
| Note that this is experimental feature. | ||
| -j<workers> Run up to <workers> simultaneous testing processes. By default, | ||
| the worker count is detected automatically. Use -j1 to run | ||
| tests sequentially. | ||
|
|
||
| -l <file> Read the testfiles to be executed from <file>. After the test | ||
| has finished all failed tests are written to the same <file>. | ||
|
|
@@ -351,6 +351,7 @@ function main(): void | |
| $shuffle = false; | ||
| $bless = false; | ||
| $workers = null; | ||
| $workersExplicit = false; | ||
| $context_line_count = 3; | ||
| $num_repeats = 1; | ||
| $show_progress = true; | ||
|
|
@@ -412,6 +413,7 @@ function main(): void | |
|
|
||
| switch ($switch) { | ||
| case 'j': | ||
| $workersExplicit = true; | ||
| $workers = substr($argv[$i], 2); | ||
| if ($workers == 0 || !preg_match('/^\d+$/', $workers)) { | ||
| error("'$workers' is not a valid number of workers, try e.g. -j16 for 16 workers"); | ||
|
|
@@ -641,6 +643,19 @@ function main(): void | |
| } | ||
| } | ||
|
|
||
| if (!$workersExplicit && (!$selected_tests || count($test_files) > 1)) { | ||
| $workers = get_default_worker_count(); | ||
| if ( | ||
| $workers !== null | ||
| && ($valgrind !== null || isset($environment['SKIP_ASAN'])) | ||
| ) { | ||
| $workers = min($workers, 2); | ||
| } | ||
| if ($workers !== null && !can_create_parallel_worker_socket()) { | ||
| $workers = null; | ||
| } | ||
| } | ||
|
|
||
| if ($online === null && !isset($environment['SKIP_ONLINE_TESTS'])) { | ||
| $online = false; | ||
| } | ||
|
|
@@ -800,6 +815,55 @@ function main(): void | |
| } | ||
| } | ||
|
|
||
| function get_default_worker_count(): ?int | ||
| { | ||
| if (IS_WINDOWS) { | ||
| $workerCount = getenv('NUMBER_OF_PROCESSORS'); | ||
| return is_string($workerCount) ? parse_default_worker_count($workerCount) : null; | ||
| } | ||
|
|
||
| $commands = [ | ||
| 'nproc 2>/dev/null', | ||
| 'getconf _NPROCESSORS_ONLN 2>/dev/null', | ||
| 'getconf NPROCESSORS_ONLN 2>/dev/null', | ||
| 'sysctl -n hw.logicalcpu 2>/dev/null', | ||
| 'sysctl -n hw.ncpu 2>/dev/null', | ||
| ]; | ||
| foreach ($commands as $command) { | ||
| $workerCount = shell_exec($command); | ||
| if ( | ||
| is_string($workerCount) | ||
| && ($workerCount = parse_default_worker_count($workerCount)) !== null | ||
| ) { | ||
| return $workerCount; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| function parse_default_worker_count(string $workerCount): ?int | ||
| { | ||
| $workerCount = trim($workerCount); | ||
| if (preg_match('/^[0-9]+$/D', $workerCount) !== 1) { | ||
| return null; | ||
| } | ||
|
|
||
| $workerCount = (int) $workerCount; | ||
| return $workerCount >= 2 ? min($workerCount, 10) : null; | ||
| } | ||
|
|
||
| function can_create_parallel_worker_socket(): bool | ||
| { | ||
| $socket = @stream_socket_server('tcp://127.0.0.1:0'); | ||
| if ($socket === false) { | ||
| return false; | ||
| } | ||
|
|
||
| fclose($socket); | ||
| return true; | ||
| } | ||
|
|
||
| function verify_config(string $php): void | ||
| { | ||
| if (empty($php) || !file_exists($php)) { | ||
|
|
@@ -820,7 +884,7 @@ function write_information(array $user_tests, $phpdbg): void | |
| $php_escaped = escapeshellarg($php); | ||
|
|
||
| // Get info from php | ||
| $info_file = __DIR__ . '/run-test-info.php'; | ||
| $info_file = __DIR__ . '/run-test-info-' . getmypid() . '.php'; | ||
| @unlink($info_file); | ||
| $php_info = '<?php echo " | ||
| PHP_SAPI : " , PHP_SAPI , " | ||
|
|
@@ -1571,10 +1635,10 @@ function run_all_tests_parallel(array $test_files, array $env, ?string $redir_te | |
| // - If this is running a small enough number of tests, | ||
| // reduce the batch size to give batches to more workers. | ||
| $files = []; | ||
| $maxBatchSize = $valgrind ? 1 : ($shuffle ? 4 : 32); | ||
| $maxBatchSize = $valgrind ? 1 : 4; | ||
| $averageFilesPerWorker = max(1, (int) ceil($totalFileCount / count($workerProcs))); | ||
| $batchSize = min($maxBatchSize, $averageFilesPerWorker); | ||
| while (count($files) <= $batchSize && $file = array_pop($test_files)) { | ||
| while (count($files) < $batchSize && $file = array_pop($test_files)) { | ||
|
Comment on lines
-1574
to
+1641
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is unrelated to this change :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, they are unrelated. Two reasons:
My idea was to group everything in ~10 sensible, logically grouped PRs (down from 26 commits). Hence, I wanted to combine these two here. Tried to reach you on Discord. Not sure what's the right balance, and split -- open for input!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I'm not often on Discord. And no worries. As long as you don't open too many PRs at the same time it's not spamming. Splitting independent changes is convenient for reviews and ensures that each change is reviewed with the same attention. It also avoids blocking an entire PR if one of the changes is blocking. For this particular change I think that it would be worth it to explore other solutions. I believe that the long tail is caused by slow tests being grouped together in the same batches (because related tests tend to have similar durations), so some batches are longer than others. Distributing tests in a more round robin fashion (or shuffling) would be an alternative way to fix the long tail problem without increasing communication overhead by reducing batch size.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't object. But allow me to add that I was thinking about duration based sharding as a follow up. Personally think that's the cleanest and (likely) will have the best results.
Agreed. Commented this elsewhere already, this can optimised further. Though, the question is, must this happen now or can/should it be a follow up? No strong opinion from my side. I, however, think the 1.8x gain this brings now is merge-worthy on it's own (without blocking later changes). If we then would decide for either, round robin or duration based sharding, it likely takes some time to get them discussed and right. This one is ready, and could make the suite faster starting tomorrow until a follow up lands. But again, I am good with either and I leave that decision to you!
Roger that. I am on it. |
||
| foreach ($fileConflictsWith[$file] as $conflictKey) { | ||
| if (isset($activeConflicts[$conflictKey])) { | ||
| $waitingTests[$conflictKey][] = $file; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| --TEST-- | ||
| Automatic worker detection is capped for regular and instrumented runs | ||
| --SKIPIF-- | ||
| <?php | ||
| if (PHP_OS_FAMILY === 'Windows') { | ||
| die('skip requires a POSIX shell'); | ||
| } | ||
| ?> | ||
| --ENV-- | ||
| TEST_PHP_FORK_SERVER=0 | ||
| --FILE-- | ||
| <?php | ||
| $root = __DIR__ . '/automatic_worker_limit_' . getmypid(); | ||
| $bin = $root . '/bin'; | ||
| $tests = $root . '/tests'; | ||
| mkdir($bin, recursive: true); | ||
| mkdir($tests); | ||
|
|
||
| $nproc = $bin . '/nproc'; | ||
| file_put_contents($nproc, "#!/bin/sh\nprintf '64\\n'\n"); | ||
| chmod($nproc, 0755); | ||
|
|
||
| $testFiles = []; | ||
| for ($i = 0; $i < 11; $i++) { | ||
| $testFiles[] = $file = $tests . "/$i.phpt"; | ||
| file_put_contents($file, <<<PHPT | ||
| --TEST-- | ||
| worker cap $i | ||
| --FILE-- | ||
| <?php echo "ok\\n"; ?> | ||
| --EXPECT-- | ||
| ok | ||
| PHPT); | ||
| } | ||
|
|
||
| $environment = [ | ||
| 'PATH' => $bin . PATH_SEPARATOR . getenv('PATH'), | ||
| 'TEST_PHP_EXECUTABLE' => getenv('TEST_PHP_EXECUTABLE'), | ||
| 'TEST_PHP_FORK_SERVER' => '0', | ||
| ]; | ||
| foreach (['TEMP', 'TMPDIR'] as $name) { | ||
| if (($value = getenv($name)) !== false) { | ||
| $environment[$name] = $value; | ||
| } | ||
| } | ||
|
|
||
| $runTests = static function (array $arguments) use ($environment, $testFiles): array { | ||
| $process = proc_open( | ||
| [ | ||
| getenv('TEST_PHP_EXECUTABLE'), | ||
| dirname(__DIR__, 2) . '/run-tests.php', | ||
| '-q', | ||
| '--no-progress', | ||
| ...$arguments, | ||
| ...$testFiles, | ||
| ], | ||
| [ | ||
| 0 => ['pipe', 'r'], | ||
| 1 => ['pipe', 'w'], | ||
| 2 => ['redirect', 1], | ||
| ], | ||
| $pipes, | ||
| null, | ||
| $environment, | ||
| ); | ||
| fclose($pipes[0]); | ||
| $output = stream_get_contents($pipes[1]); | ||
| fclose($pipes[1]); | ||
|
|
||
| return [proc_close($process), $output]; | ||
| }; | ||
|
|
||
| [$exitCode, $output] = $runTests([]); | ||
| var_dump($exitCode); | ||
| var_dump(str_contains($output, 'Spawning 10 workers...')); | ||
| var_dump(str_contains($output, 'Spawning 11 workers...')); | ||
|
|
||
| [$exitCode, $output] = $runTests(['--asan']); | ||
| var_dump($exitCode); | ||
| var_dump(str_contains($output, 'Spawning 2 workers...')); | ||
| var_dump(str_contains($output, 'Spawning 10 workers...')); | ||
|
|
||
| [$exitCode, $output] = $runTests(['--asan', '-j3']); | ||
| var_dump($exitCode); | ||
| var_dump(str_contains($output, 'Spawning 3 workers...')); | ||
| ?> | ||
| --CLEAN-- | ||
| <?php | ||
| foreach (glob(__DIR__ . '/automatic_worker_limit_*') ?: [] as $root) { | ||
| foreach (glob($root . '/tests/*.phpt') ?: [] as $file) { | ||
| unlink($file); | ||
| } | ||
| @unlink($root . '/bin/nproc'); | ||
| @rmdir($root . '/tests'); | ||
| @rmdir($root . '/bin'); | ||
| @rmdir($root); | ||
| } | ||
| ?> | ||
| --EXPECT-- | ||
| int(0) | ||
| bool(true) | ||
| bool(false) | ||
| int(0) | ||
| bool(true) | ||
| bool(false) | ||
| int(0) | ||
| bool(true) |
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.
Very rare; but hit it once so it cannot hurt. Can, for instance, happen when testing tests.
Just highlighting it because I moved this here after the first approval was made.