Experimental run-tests.php parallelisation - #2822
Conversation
3807427 to
0d1e4b5
Compare
|
Argh, I notice now this has mixed tabs and spaces. Thanks PHPStorm. |
There was a problem hiding this comment.
I forgot to remove debug code here, perhaps because it never worked in the first place.
I'd like to add an error handler that sends a message with the error.
There was a problem hiding this comment.
You can just use $workerID = (int) getenv("TEST_PHP_WORKER");.
There was a problem hiding this comment.
Oh. Right, yeah. I think I was thinking of JS parseInt() when I wrote this.
There was a problem hiding this comment.
I didn't write this code, so I don't know.
There was a problem hiding this comment.
This should probably set some ini options, such as those, to prevent unexpected messages on STDOUT.
There was a problem hiding this comment.
Yes, it would make sense to log to stderr.
That said, wouldn't writing a user error handler that sends errors as messages work? This is my current plan.
0d1e4b5 to
91d19cb
Compare
|
Did some rebase-fu to fix the accidental use of spaces rather than tabs. At least, all the usages that were my fault ;) |
|
Could you try enabling it on Travis and see whether it can speed up the tests there, too? |
|
@kelunik Very sensible idea. I'll add it maybe as an extra task or whatever so I can see what's broken by the parallelism. |
b8be2ec to
d713b17
Compare
|
@kelunik I'm happy to report that, judging by build #27751, it does indeed make Travis faster, at 530s to run tests with 2 worker children versus 908s in normal mode. Double the number of worker children to four and they can do it in 420s, which seems like a case of diminishing returns (I think Travis maybe only provides 2 logical CPUs, if that).
|
d713b17 to
9a825e3
Compare
There was a problem hiding this comment.
@kelunik I'd guess you know what you're doing here better than I do, so: Would you see any disadvantage to having a higher timeout here, say 1 second? Checking the reads on these is supposedly non-blocking, and there's nothing else the host process needs to do except for wait on reads, so reducing the number of syscalls very slightly would make sense, right?
There was a problem hiding this comment.
Yes, you can totally increase that to 10 seconds or even more. It will be interrupted by any signals or actionable streams.
ef1fbb0 to
1b71858
Compare
|
There's now a flag file (you just I've added that file to a bunch of large directories that seemed safe, and it hasn't broken anything. Unfortunately, it also hasn't improved effective test execution time for me, even on my 16-thread machine; no matter how fast you execute everything else, |
There was a problem hiding this comment.
I don't think the emoji here is a good idea. it will cause problems in Windows/CMD.
There was a problem hiding this comment.
I wouldn't recommend proc_open on Windows anyway, because STDIO are file handles there and block.
There was a problem hiding this comment.
Quick testing on Windows 10 shows that in cmd, the emoji don't cause problems, they just show up as empty rectangles. That's fine, really. Maybe it can serve as an incentive to support Unicode terminal output.
And @kelunik is right about proc_open blocking… it seemingly ignores all all the workers but one because of it. :/
There was a problem hiding this comment.
empty rectangles IS a problem, isn't it? If you insist on these emoji, I suggest you add something like --no-ansi, and turn it on on windows.
There was a problem hiding this comment.
It's just cosmetic, so why bother? And actually, it's just a font support problem. Copy and paste them into another window and it shows up fine. I guess PHP does use Unicode after all.
There was a problem hiding this comment.
I began modifying the code so it can also communicate over sockets, but this is a little more painful than I expected. If I get that working, it should solve the Windows problem.
There was a problem hiding this comment.
…oh, I was using the wrong socket API >.>
|
@hikari-no-yume IIRC @sgolemon has a list of all tests which can ran in parallel (which was created for hhvm in the past) |
38b5452 to
9ba3362
Compare
|
Since, as @kelunik pointed out, Windows's |
|
@hikari-no-yume I suggest we perhaps do a cleanup of tests in Zend/ and put them in sub directories and then merge this but that can even be done after, so lets have a go at this in master. Personally the only negative I have about this, as I stated on Twitter, is the annoying square boxes on my CLI |
|
Again, I suggest to remove, or at least add an option for the emoji. I fully agree they are very cute and I like these cute things. But they are not cute any more when they become squares. The worse thing is for people without related knowledge, they may treat this as a problem of their cmd / php tests, because the abnormal character usually means something wrong. |
|
Anyway, the parallel testing itself is awosome. Let RM make the decision. |
There was a problem hiding this comment.
Would it make sense to use sockets on all platforms to reduce code?
There was a problem hiding this comment.
Maybe. Not using sockets means a simpler codepath less likely to face problems, though.
There was a problem hiding this comment.
No need for a magic constant port. use stream_socket_server("tcp://[::1]:0"), and an unused port will be selected.
$a=stream_socket_server("tcp://[::1]:0");
var_dump(stream_socket_get_name($a, false));
//php shell code:1:
//string(9) "::1:52491"and then we can use ipv4. or at least use it as a fallback.
There was a problem hiding this comment.
Yes, I thought about using :0, I'll try implementing it.
I don't see any value in adding IPv4. Who needs to test PHP 7.3 in parallel on a system with no support for IPv6? It's not like IPv6 connectivity is required, just loopback.
There was a problem hiding this comment.
@hikari-no-yume There's 0 reason to use IPv6. Binding to IPv4 will usually bind both anyway, see http://php.net/manual/en/context.socket.php + ipv6_v6only.
There was a problem hiding this comment.
You know there is almost no IPv6 in China, so sometimes the system administrator will disable all the IPv6 components because they believe it can speed up the system. It's silly but it happens.
There was a problem hiding this comment.
@hikari-no-yume side note, IPv6 support is ALWAYS available on Windows, but can be disabled in custom builds for some odd reason I do not understand
|
@KalleZ Zend's fine, it doesn't need breaking up. I mean, it would be nice to, but this thing already subdivides it automatically with few (no? I don't remember) problems. |
|
@hikari-no-yume lets give it a try in master, then we can all hack on it together, there is nothing else holding it back @jhdxr though we keep run-tests.php in sync across all branches, it should be perfectly fine to play around with in master, I'm sure the RMs for lower branches would merge it once it had some time there |
|
@KalleZ we do keep it in sync? Oh, okay. There's changes that aren't synced right now, and this code will only work on 7.2 as-is… |
871febd to
69b648a
Compare
|
Rebased for current master. |
|
The CI builds fail because it seems to choke on very large test output… |
| error("Could not find worker stdout in array of worker stdouts, THIS SHOULD NOT HAPPEN."); | ||
| } | ||
| // Some tests have *very* long output and fgets() seems to truncate to 8KiB unless we say we want more | ||
| while (FALSE !== ($rawMessage = fgets($workerSock, 64 * 1024))) { |
There was a problem hiding this comment.
This doesn't work… can fgets not handle non-blocking streams or something? :/
There was a problem hiding this comment.
This also apparently doesn't work. I bet there's a non-blocking I/O error in here somewhere.
48d2dc6 to
a15c666
Compare
|
Woo, it doesn't choke on huge test output now. |
|
And perhaps more importantly, very few tests fail with parallelised tests enabled under Travis. However I/O capture tests are skipped, so we couldn't safely use only parallel testing right now… |
|
From what I remember, this PR implements parallelization on the directory level and adds marker files to exclude directories from parallelization. I wanted to propose an alternative approach for handling tests that can't be run together, which I think should be both relatively simple to implement and allows a smooth transition from coarse-grained (to start with) to fine-grained (once we have time to review individual tests) handling: Add a new Additionally a |
|
@nikic Might be something, so we can delivery this :) |
Prerequisite for parallelised testing: #2822 Quoth Nikita @ #3789 (comment): > @hikari-no-yume Please feel free to directly commit the first commit > (to PHP-7.4). That's probably a big and unnecessary source of > conflicts, and makes reviewing harder as well.
|
As implied by 41fbeb6 I'm working on this again (thanks @KalleZ and @nikic for the reminder ^^). Rebasing it is gnarly though and I don't want to trash history for this branch again; I'm going to hand-rebase this on a new branch and then open a new PR once I'm done and close this one. Leaving this one open for the moment. |
|
Superseding with #3838. |
Motivation: As an extension author, I want to speed up running tests in php <=7.3, both locally and in CI (e.g. with valgrind). This can be done by manually copying php 7.4's run-tests.php script to replace the one generated by `phpize` - list() doesn't work in php 7.0 - negative string offset doesn't work in php 7.2 If run-tests.php can be copied from php-src without any manual patches, that would be the easiest. Related to php#2822 - I didn't see any discussion for/against compatibility with older php versions
Motivation: As an extension author, I want to speed up running tests in php <=7.3, both locally and in CI (e.g. with valgrind). This can be done by manually copying php 7.4's run-tests.php script to replace the one generated by `phpize` - list() doesn't work in php 7.0 - negative string offset doesn't work in php 7.2 If run-tests.php can be copied from php-src without any manual patches, that would be the easiest. Related to #2822 - I didn't see any discussion for/against compatibility with older php versions
Prerequisite for parallelised testing: php#2822 Quoth Nikita @ php#3789 (comment): > @hikari-no-yume Please feel free to directly commit the first commit > (to PHP-7.4). That's probably a big and unnecessary source of > conflicts, and makes reviewing harder as well.
Motivation: As an extension author, I want to speed up running tests in php <=7.3, both locally and in CI (e.g. with valgrind). This can be done by manually copying php 7.4's run-tests.php script to replace the one generated by `phpize` - list() doesn't work in php 7.0 - negative string offset doesn't work in php 7.2 If run-tests.php can be copied from php-src without any manual patches, that would be the easiest. Related to php#2822 - I didn't see any discussion for/against compatibility with older php versions
9 participants
Moved to #3838.
You know it.
Only look at changes after the first commit if you want to understand the diff; the first commit is merely a minor refactoring, but because it moves code around it creates a lot of git diff noise. Alternatively, look at the whitespace-ignoring diff.
You should probably read my email to internals: http://news.php.net/php.internals/100838