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

Detect issues in parallel #421

Closed
aik099 opened this issue Jan 5, 2015 · 10 comments
Closed

Detect issues in parallel #421

aik099 opened this issue Jan 5, 2015 · 10 comments
Milestone

Comments

@aik099
Copy link
Contributor

aik099 commented Jan 5, 2015

Maybe a speed at which large files are processed can be improved by:

  1. tokenizing file in main process
  2. invokening sniffs on token list in parallel in forked processed
  3. reporting back to main process with results

I'm not sure what amount of change is required to do this and what performance can be gained through that.

Of course fixing still should happen sequentially or we maybe can fix several files in parallel.

@gsherwood
Copy link
Member

This is something I am planning to look at for version 3. I've used pcntl_fork a fair bit for CLI programming, so I'd probably look into using it for this.

@aik099
Copy link
Contributor Author

aik099 commented Jan 6, 2015

I guess PEAR packages (as PHP_CodeSniffer) are not allowed to use Composer. Since otherwise using Symfony Process component would surely speed up the development flow and interprocess communications.

The tricky part of how process forking works in PHP is that it copies over all open resources (e.g. open file pointers, db connections, etc.) to forked process. Then if forked process decides to close them this would result in same pointer closed in parent process as well.

So we better create all these forks early when no resources were allocated and then each process will go it's own way.

@orlangur
Copy link

orlangur commented Jan 6, 2015

Why do this inside PHP_CodeSniffer instead of file/directory lists preparation + forking separate phpcs processes? Something like https://github.com/zendframework/zf2/blob/master/.travis.yml#L20 or https://github.com/brianium/paratest if phpcs is used as a part of PHPUnit based test.

Such parallelization + HHVM involvement would give performance acceptable for any purpose I believe.

@aik099
Copy link
Contributor Author

aik099 commented Jan 6, 2015

To my knowledge the PHP_CodeSniffer is standalone tool and as such parallelization is supposed to speed up coding standard violation finding process.

We're not trying to speed up own unit tests on Travis here 😉

@orlangur
Copy link

orlangur commented Jan 6, 2015

Yeah, but it's already not so hard from outside the PHP_CodeSniffer. If parallelization is built-in, some additional configuration is needed, or auto-detection of amount of CPU cores, which complicates things a bit. To go this way benefit should be obvious.

ParaTest is developed this way - from the inside of PHPUnit - and you can see that implementation for PHP_CodeSniffer will duplicate the same wheels a bit. The other example is developed from outside and may work pretty well for our case.

@aik099
Copy link
Contributor Author

aik099 commented Jan 6, 2015

PHP_CodeSniffer is PEAR library, so I guess using Composer libraries to ease the job isn't an option (unless there is PEAR library with same purpose, which is also available from Composer).

I don't know any plug-n-play solution to parallelize any script and then produce united output from it.

@orlangur
Copy link

orlangur commented Jan 6, 2015

Really? Thanks for pointing out, PHPUnit already abandoned sharing via PEAR channel and I thought it's a common trend.

So, there is an acceptance criteria to use only something PEAR-available? With no Composer, no apt-get.

@gsherwood
Copy link
Member

PHPCS is still a PEAR package and will remain one for a while I imagine. But that doesn't really affect what code I can use. I just do not want to depend on any external library for something this core. Having said that, I'm not planning on devoting any time to this task for quite a while.

@gsherwood
Copy link
Member

I've added experimental support for this in 3.0 now. To use, add --parallel=n to your command, where n is the number of concurrent processes you want running.

For example: phpcs --parallel=100 /path/to/code seems to process code the fastest for my setup and the checks I am doing, but anything above 1 makes a difference.

Not all reports are currently working because they need to be changed over to not assume they are using a shared memory space.

@gsherwood gsherwood added this to the 3.0 milestone Oct 6, 2015
@gsherwood
Copy link
Member

I've finished adding support for all reports. Everything seems to be working well.

jrfnl referenced this issue in Yoast/whip Jun 10, 2018
jrfnl added a commit to jrfnl/PHP_CodeSniffer that referenced this issue Jul 29, 2024
…dproperties-bugfix-skip-closure-use

File::getMethodProperties(): skip over closure use statements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants