Generic/LowerCaseType: improve performance #3839
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Someone reported a performance issue with the `Generic.PHP.LowerCaseType` sniff to me.
Running the Performance report (PR 3810) over a number of codebases, confirmed that the sniff ranked in the top 10 of "slow" sniffs.
As it was, the sniff would examine all variables it comes across and disregard them if they are not properties or not typed.
The "disregard when not a property" was done by catching an exception thrown by the `File::getMemberProperties()` method.
As the majority of `T_VARIABLE` tokens in the average file are not property declarations, the `File::getMemberProperties()` method would be triggered lots and lots of times, with the majority of those times resulting in the need for creating and then catching and throwing away the above mentioned exception.
By changing the logic for the sniff to only look within OO constructs and skip over anything non-property, thus avoiding the unnecessary exception creation, I can see a significant difference in the sniff run time.
Using the test file which was originally shared with me and running the below command on PHP 7.4:
```bash
phpcs -ps db.php --standard=Generic --report=source -vvv --sniffs=Generic.PHP.LowercaseType
```
... yielded the following difference in runtime:
Base time:
```
*** START SNIFF PROCESSING REPORT ***
PHP_CodeSniffer\Standards\Generic\Sniffs\PHP\LowerCaseTypeSniff: 0.3802 secs
*** END SNIFF PROCESSING REPORT ***
```
Time with the performance tweak included in this PR:
```
*** START SNIFF PROCESSING REPORT ***
PHP_CodeSniffer\Standards\Generic\Sniffs\PHP\LowerCaseTypeSniff: 0.0113 secs
*** END SNIFF PROCESSING REPORT ***
```
Using the performance report to benchmark the improvement with a larger number of files, I see improvement across the board as well:
Command used: `phpcs -ps . --extensions=php --ignore=/vendor/ --report=performance --standard=psr12`
Output for the `Generic.PHP.LowercaseType` sniff:
Result | PHPCS itself | Set of Projects A | Set of Projects B | Set of Projects C |
------ | ------------------ | ------------------ | ------------------ | ----------------- |
Nr of Files Scanned | 614 | 4115 | 25546 | 2250 |
Before | 0.131587 ( 3.9 %) | 1.514729 ( 3.0 %) | 5.390167 ( 3.4 %) | 0.359674 ( 4.2 %)
After | 0.029166 ( 0.9 %) | 0.449517 ( 0.9 %) | 1.917077 ( 1.2 %) | 0.181097 ( 2.2 %)
---
I've also had a quick look at all other PHPCS native sniffs using the `File::getMemberProperties()` method. As those are all based on the `AbstractVariableSniff`, they don't seem to suffer from the same issue, or at least, nowhere near as badly.
I also considered changing the setup of the sniff to start using the `AbstractVariableSniff`, but considering this particular sniff is also examining functions and type casts, I believe that would make the sniff more complex than necessary.
jrfnl
force-pushed
the
feature/generic-lowercasetype-improve-performance
branch
from
3089c41
to
a219206
Compare
|
Closing as replaced by PHPCSStandards/PHP_CodeSniffer#63 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Someone reported a performance issue with the
Generic.PHP.LowerCaseTypesniff to me.Running the Performance report (PR #3810) over a number of codebases, confirmed that the sniff ranked in the top 10 of "slow" sniffs.
As it was, the sniff would examine all variables it comes across and disregard them if they are not properties or not typed. The "disregard when not a property" was done by catching an exception thrown by the
File::getMemberProperties()method.As the majority of
T_VARIABLEtokens in the average file are not property declarations, theFile::getMemberProperties()method would be triggered lots and lots of times, with the majority of those times resulting in the need for creating and then catching and throwing away the above mentioned exception.By changing the logic for the sniff to only look within OO constructs and skip over anything non-property, thus avoiding the unnecessary exception creation, I can see a significant difference in the sniff run time.
Using the test file which was originally shared with me and running the below command on PHP 7.4:
... yielded the following difference in runtime:
Base time:
Time with the performance tweak included in this PR:
Using the performance report to benchmark the improvement with a larger number of files, I see improvement across the board as well:
Command used:
phpcs -ps . --extensions=php --ignore=/vendor/ --report=performance --standard=psr12Output for the
Generic.PHP.LowercaseTypesniff:I've also had a quick look at all other PHPCS native sniffs using the
File::getMemberProperties()method. As those are all based on theAbstractVariableSniff, they don't seem to suffer from the same issue, or at least, nowhere near as badly.I also considered changing the setup of the sniff to start using the
AbstractVariableSniff, but considering this particular sniff is also examining functions and type casts, I believe that would make the sniff more complex than necessary.Suggested changelog entry
Generic.PHP.LowercaseType: improved time-to-result for the sniff
Related issues/external references
N/A
Types of changes