Skip to content

Run test subprocesses without shell - #22957

Open
NickSdot wants to merge 12 commits into
php:masterfrom
NickSdot:perf/test-subprocesses-without-shell
Open

Run test subprocesses without shell#22957
NickSdot wants to merge 12 commits into
php:masterfrom
NickSdot:perf/test-subprocesses-without-shell

Conversation

@NickSdot

@NickSdot NickSdot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Extracted from #22917.

Runs eligible test, SKIPIF, and CLEAN subprocesses through structured proc_open() commands instead of a shell. This avoids shell parsing and Windows cmd.exe launch overhead; tests requiring shell semantics are unchanged.

This is actually more like a prerequisite for the rest, but has performance gain as well.

Result

Values in seconds; measured on macOS.

mode master this PR saved speedup
-j1 368.084 341.594 26.490 1.08×
-j10 64.496 55.843 8.653 1.15×

Reproduction:

Run on master and this PR:

nice -n 10 sapi/cli/php -n run-tests.php -n -q -j1 Zend/tests
nice -n 10 sapi/cli/php -n run-tests.php -n -q -j10 Zend/tests
@NickSdot
NickSdot marked this pull request as ready for review July 31, 2026 07:22

@arnaud-lb arnaud-lb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Representing commands as arrays is a good idea in general, and the speed up from the reduced overhead is nice.

However, can we always do that, and always skip the shell? Otherwise we are increasing complexity.

Comment thread run-tests.php Outdated
Comment thread run-tests.php Outdated
Comment thread run-tests.php Outdated
@NickSdot

Copy link
Copy Markdown
Contributor Author

Representing commands as arrays is a good idea in general, and the speed up from the reduced overhead is nice.

However, can we always do that, and always skip the shell? Otherwise we are increasing complexity.

Did as good as possible. Naturally this widened scope and hence diff churn. Either it's just fine as is (diff churn wise) or we cherry the last commit in yet another PR.

always skip the shell?

We still need create_shell_invocation() for --ARGS-- or we would have to parse it. I don't see the win. Please let's see that out of scope here. The whole thing ideally needs as split first (#22961), and then a cleanup. Let's decide that later.

@arnaud-lb arnaud-lb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks great!

Just two minor comments. I will merge once these are resolved.

Feel free to add a NEWS entry

Comment thread run-tests.php Outdated
Comment thread run-tests.php Outdated
@NickSdot

NickSdot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

c36e664 reverts some of the previously commit changes because of the CI failure. No other idea for that unless, as said earlier, we would parse --ARGS-- ourselves. If CI passes now I'd say we call it done; really don't want to open that file again before it is split up. 😵‍💫

Edit:
--ARGS-- should IMO use something like the below. Is there any reason to not do that? It's under 20 tests using that section/feature. Seems like complexity for little added value.

--ARGS--
['--yoo', '--ayy']
@NickSdot
NickSdot force-pushed the perf/test-subprocesses-without-shell branch from 2a8b3ef to 3cae725 Compare July 31, 2026 20:55
@NickSdot
NickSdot force-pushed the perf/test-subprocesses-without-shell branch from 3cae725 to 074aaba Compare July 31, 2026 21:10
@NickSdot

Copy link
Copy Markdown
Contributor Author

Got it back passing on Windows; CI is green.

Aside: I had edited the test matrix to only run Windows tests while debugging, that added the CI label and is probably why Tim was requested as reviewer. The edit that triggered it is already reverted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

2 participants