Feature/parallelization #414

Jibbarth · 2020-06-21T11:21:21Z

Q	A
Bug fix?	no
New feature?	yes
Fixed tickets	#390

This PR add parallelization feature.

No external dependencies added, I create a new hidden command 'internal:processors' that launch code sniffer and CS fixer on a list of files.

The runner create as much as it can be new processes for this new command by retrieving number of CPU core, and launch them in parallel.

The new command store all details and fixed issues in cache, then the runner load issues from cache.

Please note that is do only for CodeSniffer and CSFixer processors.

I cannot add Insights instances in cache because there is a problem to cache SplFileInfo (not serializable), so each process need to reinstance them.

Let's talk about performance now.

Performance on Master without Cache : https://blackfire.io/profiles/c161aa31-3081-449c-b359-e481eb2be04c/graph => ~ 8s
Performance on Master with Cache : https://blackfire.io/profiles/793dd7c1-0fcb-44aa-a564-731f02818f3e/graph => ~ 1.24s
Performance on this branch without cache : https://blackfire.io/profiles/60f40fe3-c798-404e-a1e3-35d0f8cb34e3/graph => ~ 2,12s
Performance on this branch with cache : https://blackfire.io/profiles/bd5f7ac3-060b-4671-af73-41607ff7cc2f/graph => ~ 1.56s

Gain without cache : https://blackfire.io/profiles/compare/21eebd42-e83c-4bb4-99ad-8e717f42e6e9/graph
=>

When cache is already set, I loose slight performance: https://blackfire.io/profiles/compare/6dd977d1-c140-4016-a536-f03274744eca/graph

Maybe should we didn't launch subprocess in this case.
Edit : We bypass now launching subprocess for files that are already in cache 👍

TODO :

Fix coding standards (phpstan etc.)
Add more tests ?
Document the new phpinsights.threads option (in stubs)
Test on laravel application, launched with artisan insights
Test on Windows (if someone can do the test for me ? 🤗)
...

nunomaduro · 2020-06-21T11:27:43Z

@50bhan Wanna review this one?

50bhan · 2020-06-21T13:17:47Z

@nunomaduro Sure, I'll check it out tomorrow.

50bhan · 2020-06-22T10:46:36Z

Perfect work @Jibbarth 👍 Really like the approach and implementation is clean as always. I have some comments though.

src/Domain/Runner.php

50bhan · 2020-06-22T10:57:39Z

Regarding performance, currently we are looking in cache for files at InternalProcessorCommand class. So if we move this check to Runner and before creation of processes, we don't need to create even useless processes. The code can be something like below image:

I did a test and you can check the result: https://blackfire.io/profiles/8dcc20f6-2435-4161-896e-92513fa2ece6/graph .

Jibbarth · 2020-06-23T18:00:11Z

@50bhan I had in mind to check if first file and last file had cache, but your idea is more clever 👍

In case phpinsights is launched by artisan

Jibbarth · 2020-09-06T11:44:38Z

Anyone who use Windows can help me to test this PR ?

It's pretty simple :

On an other project

Launch this command to add the fork repository in composer

composer config repositories.phpinsights vcs https://github.com/jibbarth/phpinsights

Then require phpinsights on correct branch :

composer require nunomaduro/phpinsights:dev-feature/parallelization

On phpinsights repo

git remote add jibbarth [email protected]:Jibbarth/phpinsights.git
git fetch jibbarth
git checkout feature/parallelization

ekvedaras · 2020-09-06T17:17:25Z

I can test tomorrow on a bigger project. Will reply with the results

ekvedaras · 2020-09-07T06:49:51Z

Stable version

Finished in about 2 minutes

I was unable to run parallelized version though.

In Process.php line 338:

[Symfony\Component\Process\Exception\RuntimeException]
Unable to launch a new process.

Exception trace:

at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\symfony\process\Process.php:338
Symfony\Component\Process\Process->start() at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\symfony\process\Process.php:232
Symfony\Component\Process\Process->run() at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\nunomaduro\phpinsights\src\Domain\Insights\SyntaxCheck.php:52
NunoMaduro\PhpInsights\Domain\Insights\SyntaxCheck->process() at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\nunomaduro\phpinsights\src\Domain\Runner.php:161
NunoMaduro\PhpInsights\Domain\Runner->run() at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\nunomaduro\phpinsights\src\Domain\Insights\InsightFactory.php:98
NunoMaduro\PhpInsights\Domain\Insights\InsightFactory->runInsightCollector() at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\nunomaduro\phpinsights\src\Domain\Insights\InsightFactory.php:69
NunoMaduro\PhpInsights\Domain\Insights\InsightFactory->makeFrom() at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\nunomaduro\phpinsights\src\Domain\Insights\InsightCollectionFactory.php:67
NunoMaduro\PhpInsights\Domain\Insights\InsightCollectionFactory::NunoMaduro\PhpInsights\Domain\Insights\{closure}() at n/a:n/a
array_map() at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\nunomaduro\phpinsights\src\Domain\Insights\InsightCollectionFactory.php:68
NunoMaduro\PhpInsights\Domain\Insights\InsightCollectionFactory->get() at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\nunomaduro\phpinsights\src\Application\Console\Analyser.php:36
NunoMaduro\PhpInsights\Application\Console\Analyser->analyse() at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\nunomaduro\phpinsights\src\Application\Console\Commands\AnalyseCommand.php:65
NunoMaduro\PhpInsights\Application\Console\Commands\AnalyseCommand->__invoke() at n/a:n/a
call_user_func() at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\nunomaduro\phpinsights\src\Application\Console\Commands\InvokableCommand.php:34
NunoMaduro\PhpInsights\Application\Console\Commands\InvokableCommand->execute() at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\symfony\console\Command\Command.php:255
Symfony\Component\Console\Command\Command->run() at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\symfony\console\Application.php:912
Symfony\Component\Console\Application->doRunCommand() at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\symfony\console\Application.php:264
Symfony\Component\Console\Application->doRun() at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\symfony\console\Application.php:140
Symfony\Component\Console\Application->run() at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\nunomaduro\phpinsights\bin\phpinsights:35
{closure}() at C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\nunomaduro\phpinsights\bin\phpinsights:36

ekvedaras · 2020-09-07T07:05:55Z

I guess that project is just too big 😀 Tried to dump the syntax check command and run it manually.

Program 'php.exe' failed to run: The filename or extension is too longAt line:1 char:1
+ C:\php\php.exe "C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\nu ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~.
At line:1 char:1
+ C:\php\php.exe "C:\Users\ekvedaras\AppData\Roaming\Composer\vendor\nu ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : ResourceUnavailable: (:) [], ApplicationFailedException
    + FullyQualifiedErrorId : NativeCommandFailed

ekvedaras · 2020-09-07T07:10:18Z

Might be the culprit https://stackoverflow.com/a/29359348/3805289 . There is a limit on command length. Even when I try to paste the command to windows terminal it already warns about the command being longer then 5KiB.

Jibbarth · 2020-09-07T07:28:36Z

@ekvedaras Thanks you so much for testing this ! 🤗

If I understand, the bug came from the SyntaxCheck insight which was added in #373 ? Do you reproduce it on dev-master ?

Can you exclude the insight NunoMaduro\PhpInsights\Domain\Insights\SyntaxCheck in your phpinsights.php config file and recheck ?

You are probably right about command lenght, that's something we should care about 😕

ekvedaras · 2020-09-07T07:36:03Z

I tested again on a smaller project.

Stable version:

Parallel version:

Not a supper big difference. The cached version ran instantly of course :)) My laptop was not completely idle, but I tried to close most of the apps.
Btw, not sure if relevant. But I saw it fetches the number of cores as 4. Would it make sense to fetch threads instead which is 8 ? Although it seemed like all threads were receiving load, so maybe that works.

Jibbarth · 2020-09-19T13:10:58Z

@ekvedaras You are right, we used wmic cpu get NumberOfCores to retrieve number of threads available, but I found wmic cpu get NumberOfLogicalProcessors that return the correct result (same results as on my Linux)

I also reduce size of args by passing array of files to analyze by thread in the cache, so we should no longer had problem about command size.

ekvedaras · 2020-09-21T07:03:09Z

Tried to run it again. Seems a lot faster actually. But I'm hitting this issue now. Event with --disable-security-check

ekvedaras · 2020-09-30T11:34:52Z

CPU

Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz

Base speed:	2,80 GHz
Sockets:	1
Cores:	4
Logical processors:	8

v1.14.0

Time: 1m 48s

Parallel

Time: 28s

That's more like it 👍

Jibbarth · 2020-10-12T16:02:17Z

@ekvedaras Thanks you so much for helping me to test this feature 🙏

@50bhan @olivernybroe I think this PR is ready for review 😇

olivernybroe

Amazing work @Jibbarth!

Just tested it out locally and it worked perfectly fine on my mac.

I'll say this is ready now 🥇

And thanks to @ekvedaras for helping out on this 👍

50bhan · 2020-10-13T07:55:47Z

Great job @Jibbarth 👍 I think everything looks great and ready to merge!

olivernybroe · 2020-10-13T07:57:33Z

Alright.

@Jibbarth You got green light to merge it in 👍

Jibbarth · 2020-10-13T11:08:59Z

Thank you all 🙌

Jibbarth added 3 commits June 20, 2020 13:59

Add number of thread to use in Configuration

25ae00f

Handle parallelization

863ec78

Fix tests

0ed828e

Jibbarth added the enhancement New feature or request label Jun 21, 2020

50bhan reviewed Jun 22, 2020

View reviewed changes

src/Domain/Runner.php Outdated Show resolved Hide resolved

Jibbarth added 4 commits June 23, 2020 20:26

Avoid launching subprocessing for already cached files

53ec880

Fix Phpstan issues

be4b3ee

Merge branch 'master' into feature/parallelization

8dd87f4

Fix dataprovider for invalid thread tests

50933c2

50bhan mentioned this pull request Aug 27, 2020

When is 2.0 coming out, why its stated in the docs, but there are no signs of it #431

Closed

Jibbarth added 6 commits September 6, 2020 12:20

Lazy retrieve fileProcessors and insightsLoaders

a55f45a

Fix 0 sizeChunk

ba5bf47

Fix phpstan error on getMaximum return

727ec51

Merge branch 'master' into feature/parallelization

ecbcf7f

Document thread option in stubs

795911e

Fix retrieve binary path

8bd075a

In case phpinsights is launched by artisan

Jibbarth marked this pull request as ready for review September 6, 2020 11:35

Jibbarth added 2 commits September 6, 2020 13:55

Merge branch 'master' into feature/parallelization

525e649

Merge branch 'master' into feature/parallelization

1bae9ad

Jibbarth added 2 commits September 19, 2020 11:49

Fix retrieve number of processors on Windows

b48635e

Avoid long args command on windows by saving in cache files to process

12a5b79

Jibbarth mentioned this pull request Sep 19, 2020

Fix SyntaxCheck launch with windows on large project #437

Merged

Jibbarth added 2 commits September 19, 2020 17:29

Fix phpstan issue on getArgument

5c03688

Merge branch 'master' into feature/parallelization

0643879

Jibbarth mentioned this pull request Sep 26, 2020

Exception when no internet #438

Closed

Merge branch 'master' into feature/parallelization

9b9da02

austinheap approved these changes Oct 9, 2020

View reviewed changes

olivernybroe approved these changes Oct 13, 2020

View reviewed changes

Jibbarth merged commit 8517087 into nunomaduro:master Oct 13, 2020

Jibbarth deleted the feature/parallelization branch October 13, 2020 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/parallelization #414

Feature/parallelization #414

Jibbarth commented Jun 21, 2020 •

edited

Loading

nunomaduro commented Jun 21, 2020

50bhan commented Jun 21, 2020

50bhan commented Jun 22, 2020 •

edited

Loading

50bhan commented Jun 22, 2020

Jibbarth commented Jun 23, 2020

Jibbarth commented Sep 6, 2020 •

edited

Loading

ekvedaras commented Sep 6, 2020

ekvedaras commented Sep 7, 2020 •

edited

Loading

ekvedaras commented Sep 7, 2020

ekvedaras commented Sep 7, 2020

Jibbarth commented Sep 7, 2020 •

edited

Loading

ekvedaras commented Sep 7, 2020

Jibbarth commented Sep 19, 2020

ekvedaras commented Sep 21, 2020

ekvedaras commented Sep 30, 2020

Jibbarth commented Oct 12, 2020

olivernybroe left a comment

50bhan commented Oct 13, 2020

olivernybroe commented Oct 13, 2020

Jibbarth commented Oct 13, 2020

Feature/parallelization #414

Feature/parallelization #414

Conversation

Jibbarth commented Jun 21, 2020 • edited Loading

nunomaduro commented Jun 21, 2020

50bhan commented Jun 21, 2020

50bhan commented Jun 22, 2020 • edited Loading

50bhan commented Jun 22, 2020

Jibbarth commented Jun 23, 2020

Jibbarth commented Sep 6, 2020 • edited Loading

On an other project

On phpinsights repo

ekvedaras commented Sep 6, 2020

ekvedaras commented Sep 7, 2020 • edited Loading

Stable version

I was unable to run parallelized version though.

ekvedaras commented Sep 7, 2020

ekvedaras commented Sep 7, 2020

Jibbarth commented Sep 7, 2020 • edited Loading

ekvedaras commented Sep 7, 2020

Jibbarth commented Sep 19, 2020

ekvedaras commented Sep 21, 2020

ekvedaras commented Sep 30, 2020

v1.14.0

Parallel

Jibbarth commented Oct 12, 2020

olivernybroe left a comment

Choose a reason for hiding this comment

50bhan commented Oct 13, 2020

olivernybroe commented Oct 13, 2020

Jibbarth commented Oct 13, 2020

Jibbarth commented Jun 21, 2020 •

edited

Loading

50bhan commented Jun 22, 2020 •

edited

Loading

Jibbarth commented Sep 6, 2020 •

edited

Loading

ekvedaras commented Sep 7, 2020 •

edited

Loading

Jibbarth commented Sep 7, 2020 •

edited

Loading