Skip to content
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

[Process] Allow running multiple commands at once #43162

Closed
m-vo opened this issue Sep 24, 2021 · 12 comments · Fixed by #52409
Closed

[Process] Allow running multiple commands at once #43162

m-vo opened this issue Sep 24, 2021 · 12 comments · Fixed by #52409

Comments

@m-vo
Copy link
Contributor

m-vo commented Sep 24, 2021

Description
When running a lot of small shell commands after each other, the overhead of creating the underlying processes becomes noticeable. It would be handy if the process component would support chaining commands.

Working directly on a Linux/Windows shell you would typically do this:

# Linux
$ foo; bar; baz
$ foo && bar && baz

# Windows
$ foo & bar & baz
$ foo && bar && baz

(Yes there are other chaining operators as well like ||.)

This is currently not possible with the process component.

API Example
I like that Symfony\Component\Process\Process became somewhat immutable now (the command line is baked in after the constructor ran), so the API could maybe look like this:

$process = new Process(['foo']);

// foo; bar
$process = $process->withChainedCommand(['bar']); // returns a new process instance

// foo && bar
$process = $process->withAndChainedCommand(['bar']); // returns a new process instance

What do you think? Is this a worthwhile addition?

@m-vo
Copy link
Contributor Author

m-vo commented Sep 30, 2021

In non-windows environments, we're currently replacing the current shell (that PHP starts automatically) with the command to run by prepending it with exec . This makes sure signaling to the process is possible.

For chained commands this cannot work because there is no shell to start the subsequent commands (exec foo && bar). We could however prepend a pipe in that case so that subprocesses are started (exec | foo && bar), though I'm not 100% sure this will work in all cases.

Ideas and insights are welcome. ☺️ /cc @nicolas-grekas

@derrabus
Copy link
Member

Sounds like a good idea to me.

@nicolas-grekas
Copy link
Member

I'm not sure I would recommend adding withChainedCommand on the Process class.
But a shell command line builder would allow doing that I suppose.

@m-vo
Copy link
Contributor Author

m-vo commented Sep 30, 2021

But a shell command line builder would allow doing that I suppose.

You mean a builder that creates shell system specific shell commands that are then treated as "raw input" like with Process::fromShellCommandline()? I think we would loose things like signaling then as well.

We could also introduce a new Operator class that you could use like this:

$process = new Process([
    'foo', Operator::chain(), 'bar', Operator::and(), 'baz'
]);

So the array of commands would be array<string|Process> and the operators are substituted with unescaped literals when starting the process (here: foo; bar && baz or foo & bar && baz).

@nicolas-grekas
Copy link
Member

Why would we loose anything? In the end, the only way to achieve what you describe is to build a shell command line, there is no other way. So yes, I'm thinking about a builder that would generate a string that is then provided to fromShellCommandline().

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 30, 2021

Another idea: since PHP 7.4, proc_open() accepts an array directly. Maybe this removes the overhead you're mentioning, since that's the main motivation for this request. Would you like to have a look?

@m-vo
Copy link
Contributor Author

m-vo commented Sep 30, 2021

Why would we loose anything? In the end, the only way to achieve what you describe is to build a shell command line, there is no other way

I get your point. My thinking was: We're only adding exec to the array version, now, but we want it to be an implementation detail. And then it would be coupled to the builder class as well (if it's needed for chained commands).

Another idea: since PHP 7.4, proc_open() accepts an array directly. Maybe this removes the overhead you're mentioning, since that's the main motivation for this request. Would you like to have a look?

👍 I'll have a look.

@nicolas-grekas
Copy link
Member

We're only adding exec to the array version, now, but we want it to be an implementation detail

We could achieve that by wrapping the call to fromShellCommandline inside a getProcess() method on the builder maybe.

since PHP 7.4, proc_open() accepts an array directly

BTW, I'm sure we can cleanup the code on branch 6.0 by leveraging this! PR welcome :)

@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@carsonbot
Copy link

Could I get a reply or should I close this?

@carsonbot
Copy link

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

@ausi
Copy link
Contributor

ausi commented Oct 31, 2023

Another idea: since PHP 7.4, proc_open() accepts an array directly. Maybe this removes the overhead you're mentioning, since that's the main motivation for this request. Would you like to have a look?

I just tested this:
using proc_open(['/usr/bin/true'], …) is about 2x as fast as proc_open('/usr/bin/true', …)
(1.34ms vs 3.03ms)

test code used:

$start = microtime(true);
for ($i = 0; $i < 1000; ++$i) {
	proc_close(proc_open(['/usr/bin/true'], [], $pipes));
}
var_dump(microtime(true) - $start); // 1.34 ms

$start = microtime(true);
for ($i = 0; $i < 1000; ++$i) {
	proc_close(proc_open('/usr/bin/true', [], $pipes));
}
var_dump(microtime(true) - $start); // 3.03 ms

$start = microtime(true);
proc_close(proc_open('/usr/bin/true'.str_repeat(' && /usr/bin/true', 999), [], $pipes));
var_dump(microtime(true) - $start); // 0.96 ms

As the performance gain of using && chaining compared to using proc_open() with an array is only 28%, moving to the array usage should be enough (and would also improve the speed of single executions). (UPDATE: with PHP 8.3 proc_open() with array is faster than && chaining)

@nicolas-grekas should I open a new issue (or PR) for that? #52409

2023-11-02 UPDATE: PHP 8.3 brings another 2x speedup to the array version, down to 0.7ms on my system.

nicolas-grekas added a commit that referenced this issue Nov 1, 2023
This PR was submitted for the 6.3 branch but it was merged into the 6.4 branch instead.

Discussion
----------

Remove unused code from Process

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | -
| License       | MIT

While trying to work on #43162 (comment) I noticed that the private member `useFileHandles` is not used for anything anymore.
It is always set in the constructor to `'\\' === \DIRECTORY_SEPARATOR` so it basically has a constant value and its only usage is in an `elseif` after a check to the very same constant value: `if ('\\' === \DIRECTORY_SEPARATOR)`.
Therefore this code does nothing and can be removed.

Commits
-------

7a5eba2 [Process] Remove dead code from Process
nicolas-grekas added a commit that referenced this issue Nov 21, 2023
… (ausi)

This PR was squashed before being merged into the 7.1 branch.

Discussion
----------

[Process] Pass the commandline as array to `proc_open()`

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | Fix #43162 (comment)
| License       | MIT

Since PHP 7.4, `proc_open()` accepts an array which makes it twice as fast (4x as fast with PHP 8.3), see #43162 (comment)

This pull request enables calling `proc_open()` with an array if:

1. `$this->commandline` is an array
2. and we are not on Windows
3. and PHP was compiled without `--enable-sigchild`

Commits
-------

a300de8 [Process] Pass the commandline as array to `proc_open()`
symfony-splitter pushed a commit to symfony/process that referenced this issue Nov 21, 2023
… (ausi)

This PR was squashed before being merged into the 7.1 branch.

Discussion
----------

[Process] Pass the commandline as array to `proc_open()`

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | Fix symfony/symfony#43162 (comment)
| License       | MIT

Since PHP 7.4, `proc_open()` accepts an array which makes it twice as fast (4x as fast with PHP 8.3), see symfony/symfony#43162 (comment)

This pull request enables calling `proc_open()` with an array if:

1. `$this->commandline` is an array
2. and we are not on Windows
3. and PHP was compiled without `--enable-sigchild`

Commits
-------

a300de8615e [Process] Pass the commandline as array to `proc_open()`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants