Run PhpCsFixer tasks asynchronously #210
Conversation
|
Great idea! Can you move the logic of the chunked process runner into a separate service named Currently the processes are running in chunks of 10 which means that it waits untill every task is finished before it starts it's next chunk. It would be really nice if it would start a new task once an old task finished with a maximum of 10 tasks. Would that be possible? |
|
I've pushed a refactor that uses a new |
| $this->running = 0; | ||
|
|
||
| while ($this->watchProcesses()) { | ||
| usleep(1000); |
There was a problem hiding this comment.
Maybe this setting could also be configurable?
| unset($this->processes[$key]); | ||
| } | ||
|
|
||
| $shouldRunAgain = max($shouldRunAgain, !$isTerminated); |
There was a problem hiding this comment.
why are you using the max() method on booleans? This is just a regular or isn't it?
| { | ||
| $isTerminated = false; | ||
|
|
||
| if (!$process->isStarted() && $this->running < $this->config->getProcessAsyncLimit()) { |
There was a problem hiding this comment.
Try to avoid else / elseif's.
Just invert the if and return early.
We're trying to folow the object calisthenics rules.
| * | ||
| * @package GrumPHP\Process | ||
| */ | ||
| class AsyncProcessRunner |
There was a problem hiding this comment.
Can you add some spec tests to make sure this class works as expected?
| $shouldRunAgain = max($shouldRunAgain, !$isTerminated); | ||
| } | ||
|
|
||
| return $shouldRunAgain; |
There was a problem hiding this comment.
Can't the logic for $shouldRunAgain be simplified in count($this->processes) !== 0
|
Refactored it a bit and added some tests 👍 |
| @@ -73,6 +73,22 @@ public function ignoreUnstagedChanges() | |||
| } | |||
|
|
|||
| /** | |||
| * @return int | |||
| */ | |||
| public function getProcessAsyncLimit() | |||
There was a problem hiding this comment.
Can you ass a spec for this method?
| /** | ||
| * @return int | ||
| */ | ||
| public function getProcessAsyncWaitTime() |
There was a problem hiding this comment.
Can you ass a spec for this method?
| public function let(GrumPHP $grumPHP) { | ||
| $this->beConstructedWith($grumPHP); | ||
|
|
||
| $grumPHP->getProcessAsyncWaitTime()->willReturn(1000); |
There was a problem hiding this comment.
Maybe you can set this value to 0 to make sure the tests aren't slowed down by the usleep() call?
| /** | ||
| * @mixin AsyncProcessRunner | ||
| */ | ||
| class AsyncProcessRunnerSpec extends ObjectBehavior |
There was a problem hiding this comment.
Maybe it's a good idea to test the async process limit part as well?
|
Done and done! |
|
Thanks for your work! |
PhpCsFixer takes quite some time when running it on changed files, and this is of course because GrumPHP needs to run one process per file. Symfony's Process component supports running processes asynchronously, so I thought why not give it a spin!
The following benchmark is performed on 23 files with PhpCsFixerV2 as the only task. The former is GrumPHP 0.9.6 and the latter is with this PR. From 10s to 2s is pretty sweet!
The
array_chunk()and extraforeachloop is there because I don't like the idea of potentially kicking off hundreds ofphp-cs-fixerprocesses at the same time. Right now the size of each chuck is hardcoded to ten, maybe something worth making configurable?