Implement fine-grained conflict handling

nikic · nikic · commit c0e15a3b7fef · 2019-02-20T11:20:40.000+01:00
Tests can specify conflict keys, either in --CONFLICTS-- or
a per-directory CONFLICTS file. Non-conflicting tests may be run
in parallel.
diff --git a/Zend/tests/@CAN_BE_PARALLELISED b/Zend/tests/@CAN_BE_PARALLELISED
diff --git a/Zend/tests/traits/@CAN_BE_PARALLELISED b/Zend/tests/traits/@CAN_BE_PARALLELISED
diff --git a/ext/curl/tests/CONFLICTS b/ext/curl/tests/CONFLICTS
@@ -0,0 +1 @@
+server
diff --git a/ext/date/tests/@CAN_BE_PARALLELISED b/ext/date/tests/@CAN_BE_PARALLELISED
diff --git a/ext/dom/tests/@CAN_BE_PARALLELISED b/ext/dom/tests/@CAN_BE_PARALLELISED
diff --git a/ext/ftp/tests/CONFLICTS b/ext/ftp/tests/CONFLICTS
@@ -0,0 +1 @@
+server
diff --git a/ext/mysqli/tests/CONFLICTS b/ext/mysqli/tests/CONFLICTS
@@ -0,0 +1 @@
+mysql
diff --git a/ext/opcache/tests/bug66338.phpt b/ext/opcache/tests/bug66338.phpt
@@ -4,6 +4,8 @@ Bug #66338 (Optimization binding of class constants is not safely opcacheable)
 opcache.enable=0
 --SKIPIF--
 <?php if (!extension_loaded('Zend OPcache') || php_sapi_name() != "cli") die("skip CLI only"); ?>
+--CONFLICTS--
+server
 --FILE--
 <?php
 $root  = str_replace('.php', "", __FILE__);
diff --git a/ext/opcache/tests/issue0115.phpt b/ext/opcache/tests/issue0115.phpt
@@ -8,6 +8,8 @@ phar.readonly=0
 <?php require_once('skipif.inc'); ?>
 <?php if (!extension_loaded("phar")) die("skip"); ?>
 <?php if (php_sapi_name() != "cli") die("skip CLI only"); ?>
+--CONFLICTS--
+server
 --FILE--
 <?php
 $stub = '<?php
diff --git a/ext/opcache/tests/issue0149.phpt b/ext/opcache/tests/issue0149.phpt
@@ -8,6 +8,8 @@ phar.readonly=0
 <?php require_once('skipif.inc'); ?>
 <?php if (!extension_loaded("phar")) die("skip"); ?>
 <?php if (php_sapi_name() != "cli") die("skip CLI only"); ?>
+--CONFLICTS--
+server
 --FILE--
 <?php
 $stub = "<?php header('Content-Type: text/plain;');
diff --git a/ext/opcache/tests/revalidate_path_01.phpt b/ext/opcache/tests/revalidate_path_01.phpt
@@ -7,6 +7,8 @@ opcache.revalidate_path=1
 --SKIPIF--
 <?php require_once('skipif.inc'); ?>
 <?php if (php_sapi_name() != "cli") die("skip CLI only"); ?>
+--CONFLICTS--
+server
 --FILE--
 <?php
 $dir = dirname(__FILE__);
diff --git a/ext/openssl/tests/CONFLICTS b/ext/openssl/tests/CONFLICTS
@@ -0,0 +1 @@
+server
diff --git a/ext/pcre/tests/@CAN_BE_PARALLELISED b/ext/pcre/tests/@CAN_BE_PARALLELISED
diff --git a/ext/pdo_mysql/tests/CONFLICTS b/ext/pdo_mysql/tests/CONFLICTS
@@ -0,0 +1 @@
+mysql
diff --git a/ext/pdo_pgsql/tests/CONFLICTS b/ext/pdo_pgsql/tests/CONFLICTS
@@ -0,0 +1 @@
+pgsql
diff --git a/ext/pgsql/tests/CONFLICTS b/ext/pgsql/tests/CONFLICTS
@@ -0,0 +1 @@
+pgsql
diff --git a/ext/reflection/tests/@CAN_BE_PARALLELISED b/ext/reflection/tests/@CAN_BE_PARALLELISED
diff --git a/ext/session/tests/CONFLICTS b/ext/session/tests/CONFLICTS
@@ -0,0 +1 @@
+session
diff --git a/ext/soap/tests/bug73037.phpt b/ext/soap/tests/bug73037.phpt
@@ -1,5 +1,7 @@
 --TEST--
 Bug #73037 SoapServer reports Bad Request when gzipped, var 0
+--CONFLICTS--
+server
 --SKIPIF--
 <?php
 	require_once('skipif.inc');
diff --git a/ext/standard/tests/file/windows_mb_path/CONFLICTS b/ext/standard/tests/file/windows_mb_path/CONFLICTS
@@ -0,0 +1 @@
+windows_mb_path
diff --git a/ext/standard/tests/http/CONFLICTS b/ext/standard/tests/http/CONFLICTS
@@ -0,0 +1 @@
+server
diff --git a/ext/standard/tests/math/@CAN_BE_PARALLELISED b/ext/standard/tests/math/@CAN_BE_PARALLELISED
diff --git a/ext/standard/tests/serialize/@CAN_BE_PARALLELISED b/ext/standard/tests/serialize/@CAN_BE_PARALLELISED
diff --git a/ext/standard/tests/streams/bug64433.phpt b/ext/standard/tests/streams/bug64433.phpt
@@ -9,6 +9,8 @@ if(!$res) {
 	die("skip could not open cli server script");
 }
 ?>
+--CONFLICTS--
+server
 --FILE--
 <?php
 include dirname(__FILE__)."/../../../../sapi/cli/tests/php_cli_server.inc";
diff --git a/ext/standard/tests/streams/bug70198.phpt b/ext/standard/tests/streams/bug70198.phpt
@@ -4,6 +4,8 @@ Bug #70198 Checking liveness does not work as expected
 <?php
 if (getenv("SKIP_SLOW_TESTS")) die("skip slow test");
 ?>
+--CONFLICTS--
+server
 --FILE--
 <?php
 
diff --git a/ext/standard/tests/strings/@CAN_BE_PARALLELISED b/ext/standard/tests/strings/@CAN_BE_PARALLELISED
diff --git a/ext/standard/tests/url/get_headers_error_003.phpt b/ext/standard/tests/url/get_headers_error_003.phpt
@@ -1,5 +1,7 @@
 --TEST--
 Test get_headers() function : test with context
+--CONFLICTS--
+server
 --FILE--
 <?php
 
diff --git a/run-tests.php b/run-tests.php
@@ -1348,73 +1348,37 @@ function run_all_tests_parallel($test_files, $env, $redir_tested) {
 	echo "====⚡️==== WELCOME TO THE FUTURE: run-tests PARALLEL EDITION ====⚡️====\n";
 	echo "====⚡️===========================================================⚡️====\n";
 
-	// Because some of the PHP test suite has not been written with
-	// parallel execution in mind, it is not safe to just run any two tests
-	// concurrently.
-	// Therefore, we divide the test set into directories and test multiple
-	// directories at once, but not multiple tests within them.
-
-	$testDirsToGo = [];
-
+	// Each test may specify a list of conflict keys. While a test that conflicts with
+	// key K is running, no other test that conflicts with K may run. Conflict keys are
+	// specified either in the --CONFLICTS-- section, or CONFLICTS file inside a directory.
+	$dirConflictsWith = [];
+	$fileConflictsWith = [];
 	foreach ($test_files as $file) {
-		$dirSeparator = strrpos($file, DIRECTORY_SEPARATOR);
-		if ($dirSeparator !== FALSE) {
-			$testDirsToGo[substr($file, 0, $dirSeparator)][] = $file;
+		$contents = file_get_contents($file);
+		if (preg_match('/^--CONFLICTS--(.+?)^--/ms', $contents, $matches)) {
+			$conflicts = array_map('trim', explode("\n", trim($matches[1])));
 		} else {
-			$testDirsToGo[""][] = $file;
-		}
-	}
-
-	// We assume most test directories should be executed in serial, but for
-	// big directories, this would waste time if they can actually be parallel.
-	// Therefore, if a directory has a special '@CAN_BE_PARALLELISED' file, we
-	// will divide it up into smaller “directories” automatically.
-	foreach ($testDirsToGo as $dir => $tests) {
-		if (count($tests) < 64 || !is_string($dir)) {
-			continue;
-		}
-		if (file_exists($dir . DIRECTORY_SEPARATOR . '@CAN_BE_PARALLELISED')) {
-			foreach (array_chunk($tests, 64) as $testsChunk) {
-				$testDirsToGo[] = $testsChunk;
+			// Cache per-directory conflicts in a separate map, so we compute these only once.
+			$dir = dirname($file);
+			if (!isset($dirConflictsWith[$dir])) {
+				$dirConflicts = [];
+				if (file_exists($dir . '/CONFLICTS')) {
+					$contents = file_get_contents($dir . '/CONFLICTS');
+					$dirConflicts = array_map('trim', explode("\n", trim($contents)));
+				}
+				$dirConflictsWith[$dir] = $dirConflicts;
 			}
-			unset($testDirsToGo[$dir]);
+			$conflicts = $dirConflictsWith[$dir];
 		}
-	}
-
-	// Sort test dirs so the biggest ones are handled first, so we spend less
-	// time waiting on workers tasked with very large dirs.
-	// This is an ascending sort because items are popped off the end.
-	// Thank you Rasmus for this idea :)
-	uasort($testDirsToGo, function ($a, $b) {
-		return count($a) <=> count($b);
-	});
-
-	$testDirsInProgress = 0;
-
-	echo "Isolated ", count($testDirsToGo), " directories to be tested in parallel.\n";
-
-	$shamedDirs = array_reverse(array_filter($testDirsToGo, function ($files) {
-		return count($files) > 100;
-	}), true);
 
-	if ($shamedDirs) {
-		$shameList = "";
-		foreach ($shamedDirs as $dir => $shame) {
-			$shameList .= "\n$dir: " .  count($shame) . " files";
-		}
-
-		echo <<<NAME_AND_SHAME
-----⚠️-----------------------------------------------------------⚠️----
-To effectively utilise parallelism, test directories should not contain
-large numbers of tests that can't be run simultaneously. The following
-directories contain more than 100 test files and do not contain a
-'@CAN_BE_PARALLELISED' file:
-$shameList
-----⚠️-----------------------------------------------------------⚠️----
-
-NAME_AND_SHAME;
+		$fileConflictsWith[$file] = $conflicts;
 	}
 
+	// Some tests assume that they are executed in a certain order. We will be popping from
+	// $test_files, so reverse its order here. This makes sure that order is preserved at least
+	// for tests with a common conflict key.
+	$test_files = array_reverse($test_files);
+
 	echo "Spawning workers… ";
 
 	// We use sockets rather than STDIN/STDOUT for comms because on Windows,
@@ -1500,9 +1464,15 @@ function run_all_tests_parallel($test_files, $env, $redir_tested) {
 	echo "\n";
 
 	$rawMessageBuffers = [];
+	$testsInProgress = 0;
+
+	// Map from conflict key to worker ID.
+	$activeConflicts = [];
+	// Tests waiting due to conflicts. Map from conflict key to array.
+	$waitingTests = [];
 
 escape:
-	while ($testDirsToGo || ($testDirsInProgress > 0)) {
+	while ($test_files || $testsInProgress > 0) {
 		$toRead = array_values($workerSocks);
 		$toWrite = NULL;
 		$toExcept = NULL;
@@ -1532,15 +1502,43 @@ function run_all_tests_parallel($test_files, $env, $redir_tested) {
 					}
 
 					switch ($message["type"]) {
-						case "dir_finished":
-							$testDirsInProgress--;
+						case "tests_finished":
+							$testsInProgress--;
+							foreach ($activeConflicts as $key => $workerId) {
+								if ($workerId === $i) {
+									unset($activeConflicts[$key]);
+									if (isset($waitingTests[$key])) {
+										while ($test = array_pop($waitingTests[$key])) {
+											$test_files[] = $test;
+										}
+										unset($waitingTests[$key]);
+									}
+								}
+							}
 							// intentional fall-through
 						case "ready":
-							if ($testDir = array_pop($testDirsToGo)) {
-								$testDirsInProgress++;
+							// Batch multiple tests to reduce communication overhead.
+							$files = [];
+							$batchSize = 32;
+							while (count($files) <= $batchSize && $file = array_pop($test_files)) {
+								foreach ($fileConflictsWith[$file] as $conflictKey) {
+									if (isset($activeConflicts[$conflictKey])) {
+										$waitingTests[$conflictKey][] = $file;
+										continue 2;
+									}
+								}
+								$files[] = $file;
+							}
+							if ($files) {
+								foreach ($files as $file) {
+									foreach ($fileConflictsWith[$file] as $conflictKey) {
+										$activeConflicts[$conflictKey] = $i;
+									}
+								}
+								$testsInProgress++;
 								send_message($workerSocks[$i], [
 									"type" => "run_tests",
-									"test_files" => $testDir,
+									"test_files" => $files,
 									"env" => $env,
 									"redir_tested" => $redir_tested
 								]);
@@ -1609,8 +1607,8 @@ function run_all_tests_parallel($test_files, $env, $redir_tested) {
 
 	kill_children($workerProcs);
 
-	if ($testDirsInProgress < 0) {
-		error("$testDirsInProgress test directories “in progress”, which is less than zero. THIS SHOULD NOT HAPPEN.");
+	if ($testsInProgress < 0) {
+		error("$testsInProgress test batches “in progress”, which is less than zero. THIS SHOULD NOT HAPPEN.");
 	}
 }
 
@@ -1677,7 +1675,7 @@ function run_worker() {
 			case "run_tests":
 				run_all_tests($command["test_files"], $command["env"], $command["redir_tested"]);
 				send_message($workerSock, [
-					"type" => "dir_finished"
+					"type" => "tests_finished"
 				]);
 				break;
 			default:
@@ -1793,7 +1791,7 @@ function run_test($php, $file, $env)
 				'CAPTURE_STDIO', 'STDIN', 'CGI', 'PHPDBG',
 				'INI', 'ENV', 'EXTENSIONS',
 				'SKIPIF', 'XFAIL', 'CLEAN',
-				'CREDITS', 'DESCRIPTION',
+				'CREDITS', 'DESCRIPTION', 'CONFLICTS',
 			))) {
 				$bork_info = 'Unknown section "' . $section . '"';
 			}
diff --git a/sapi/cli/tests/CONFLICTS b/sapi/cli/tests/CONFLICTS
@@ -0,0 +1 @@
+server
diff --git a/tests/basic/bug67198.phpt b/tests/basic/bug67198.phpt
@@ -2,6 +2,8 @@
 php://input is empty when enable_post_data_reading=Off
 --INI--
 allow_url_fopen=1
+--CONFLICTS--
+server
 --SKIPIF--
 <?php
 include __DIR__."/../../sapi/cli/tests/skipif.inc";
diff --git a/tests/classes/@CAN_BE_PARALLELISED b/tests/classes/@CAN_BE_PARALLELISED
diff --git a/tests/lang/@CAN_BE_PARALLELISED b/tests/lang/@CAN_BE_PARALLELISED
diff --git a/tests/security/CONFLICTS b/tests/security/CONFLICTS
@@ -0,0 +1 @@
+open_basedir
