PHPCS 3.x takes a --parallel=# argument that allows for processing in parallel, which should make it all run faster. The main question is how to start using it...do we hardcode a value in composer.json? Or should we be somehow injecting it just for Wikimedia Jenkins to use? Or put it in phpcs.xml (probably not?)?
I'm leaning towards just putting it as part of the "test" command in composer.json for ease of implementation and that way developers also see the benefits on their machines too.
It is possible to set args in the ruleset.xml as in phpcs.xml?
Would be nice when the args are used on each repo, than the parallel could be set and overridden in each phpcs.xml individually.
Or is only the list of rules is used from the ruleset.xml?
What is written on https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-ruleset.xml creates the impression that <arg name="parallel" value="#" /> is possible. Just put this line into the base rule set and it will be enabled for everybody using it.
Change 362325 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/tools/codesniffer@master] Set --parallel=2 by default
Change 362325 abandoned by Legoktm:
Set --parallel=2 by default
Reason:
Not worth it.
I read the comments in the gerrit patch, but they seem to be about HHVM quirks that are presumably no longer relevant. There's also this:
I have sent a follow up change that further bump the value to 64: https://gerrit.wikimedia.org/r/#/c/362359/ The job takes about the same time and on the machine there is a single hhvm process running.
Again, I suspect this might have been an HHVM thing. On my local, adding --parallel=32 to the PHPCS invocation cuts the runtime against MW core from something in the 2-4 minutes range to just 16 seconds. I guess for core this is not an issue in CI thanks to the "run only for changed files" feature, but it would still be really useful for local development, and more generally every repo that does not have the "lint only what changed" thing; and also for MW core commits that modify the PHPCS configuration, although I guess that's fairly rare.
So my question is: is there any reason not to do this?
Correction: 36 seconds. 16 seconds is for linting includes/ only.
I should also mention that when running in parallel, the normal progress bar with the file count is replaced by a progress bar that counts how many processes have completed. But given the speed up, I think a progress bar also becomes less useful.
Change 362325 restored by Krinkle:
[mediawiki/tools/codesniffer@master] Set --parallel=2 by default
Baseline:
https://gerrit.wikimedia.org/r/c/mediawiki/tools/codesniffer/+/969755: […] Example result against MediaWiki core in 5m 43s
https://gerrit.wikimedia.org/r/c/mediawiki/tools/codesniffer/+/967562: […] Example result against MediaWiki core in 2m 29s
Two runs with "Set --parallel=2 by default" https://gerrit.wikimedia.org/r/362325
Example result against MediaWiki core in 1m 45s
Example result against MediaWiki core in 1m 48s
Two runs with "Increase --parallel to 64" https://gerrit.wikimedia.org/r/362359
Example result against MediaWiki core in 57s
Example result against MediaWiki core in 58s
Seems to work as expected. Indeed, the exactly benefit depends greatly on the resources your system actually has available, but operating systems are good at sharing resources in this way, so over-allocating should be fine. For a process that is mostly CPU bound, I would probably suggest 16 at the most. But it seems this is largely I/O bound, so optimising for CPU-sharing seems more optimal.
Change 362359 had a related patch set uploaded (by Krinkle; author: Hashar):
[mediawiki/tools/codesniffer@master] Increase --parallel to 64
Change 362325 merged by jenkins-bot:
[mediawiki/tools/codesniffer@master] Set --parallel=2 by default
Change 362359 merged by jenkins-bot:
[mediawiki/tools/codesniffer@master] Increase --parallel to 64
I'm not entirely sure about that. I ran a few experiments on my local, and the sweet spot is 32. Some more results for comparison:
| parallel | time (s) |
|---|---|
| 16 | 43.56 |
| 28 | 40.31 |
| 32 | 32.7 |
| 36 | 37.85 |
| 64 | 40.52 |
And since my laptop CPU has 8 cores / 16 threads, that seems to match the num_threads * 2 I was expecting. I don't know how many cores the CPUs used in CI have, but according to this test change, parallel=32 doesn't seem to make much difference:
https://integration.wikimedia.org/ci/job/mw-tools-codesniffer-mwcore-testrun/2956/console : FAILURE Example result against MediaWiki core in 1m 01s (non-voting)
https://integration.wikimedia.org/ci/job/mw-tools-codesniffer-mwcore-testrun/2957/console : FAILURE Example result against MediaWiki core in 1m 05s (non-voting)
Still, I think for local environments 16 threads might be more common than 32?
For me locally:
$ vendor/bin/phpcs -p -s ............................................................ 60 / 64 (94%) .... 64 / 64 (100%) Time: 13.86 secs; Memory: 10MB $ vendor/bin/phpcs -p -s ................ 16 / 16 (100%) Time: 15.51 secs; Memory: 10MB
Each varies by about 1 second in either direction. But adding more processes does not seem to slow it down. It seems to have a slight edge for me.
Note that this is without the --cache that we set in composer phpcs in MediaWiki, which normally speeds it up to 1s or less in total, given few or no changed files between runs.
In any case, 16 is fine by me. Feel free to patch it further.
Interesting! What are you running PHPCS on? Is it MW core?
Note that this is without the --cache that we set in composer phpcs in MediaWiki, which normally speeds it up to 1s or less in total, given few or no changed files between runs.
Yep, I should've mentioned that my runs were also without cache: vendor/bin/phpcs -p -s --parallel=32 etc.
In any case, 16 is fine by me. Feel free to patch it further.
I think 64 is fine, if the slowdown I saw is specific to my local.
Change 975445 had a related patch set uploaded (by Jforrester; author: Jforrester):
[mediawiki/tools/codesniffer@master] Follow-up c12564765 & db9563150: Add blended HISTORY.md entry
Change 975445 merged by jenkins-bot:
[mediawiki/tools/codesniffer@master] Follow-up c12564765 & db9563150: Add blended HISTORY.md entry
Our HHVM configuration had hhvm.stats.enable_hot_profiler enabled and in order to collect profiling it was binding all threads to the same CPU using sched_setaffinity which in turns mostly defeat the purpose of parallelization. See T191921#4555667 for my discovery and the following comments debugging it. The fix was easy: disable the hot profiler, but given we were starting to migrate out of HHVM toward PHP 7, the easy fix never got applied.
I see... Then I think it's fine to leave it as-is. It might be interesting for me to go figure out what's causing such a big difference, but for the default value that this task is about, I think we can call this resolved.
At least for me, trying to kick off 64 processes all at once just causes my development VM to lock up. Is it possible to automatically determine and scale the value based on the number of CPUs/threads available?