Run tests in parallel by default - #22939
Conversation
|
Not an expert in conflicts file. Should cc @bukka in this. |
FWIW, the doc change is only a clarification because after this tests run in parallel by default. This PR has no changes to |
arnaud-lb
left a comment
arnaud-lb
left a comment
There was a problem hiding this comment.
Looks good to me otherwise, but I will wait for other reviews
| $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)) { |
There was a problem hiding this comment.
This is unrelated to this change :)
There was a problem hiding this comment.
Yes, they are unrelated. Two reasons:
- Each commit on it's own cannot be a separate PR; they depend or interact with each other (though, not the case here).
- You didn't want me the spam all as separate PRs.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Distributing tests in a more round robin fashion
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.
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
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!
As long as you don't open too many PRs at the same time it's not spamming. Splitting independent changes is...
Roger that. I am on it.
06569f4 to
9fb7077
Compare
0374c74 to
472acd1
Compare
|
|
||
| // Get info from php | ||
| $info_file = __DIR__ . '/run-test-info.php'; | ||
| $info_file = __DIR__ . '/run-test-info-' . getmypid() . '.php'; |
There was a problem hiding this comment.
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.
3 participants
Extracted from #22917.
Runs the test suite in parallel by default; automatically selects up to 10 workers. Explicit
-j1remains the opt-out to run tests serial. Smaller batches improve balancing.Intended to merge after #22957 so eligible Windows test subprocesses no longer use the shell wrapper when parallel execution becomes the default.
Result
Values in seconds; measured on macOS.
(none)Reproduction:
Ref: 9b36b91 (review comments)
cc @arnaud-lb