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

Generic/LowerCaseType: improve performance #63

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Nov 10, 2023

Description

Recreation of upstream PR squizlabs/PHP_CodeSniffer#3839:

Someone reported a performance issue with the Generic.PHP.LowerCaseType sniff to me.

Running the Performance report (PR squizlabs/PHP_CodeSniffer#3810/PR #60) 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:

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.


Other useful info from the original PR:

This PR will likely conflict with PR squizlabs/PHP_CodeSniffer#3662 and/or squizlabs/PHP_CodeSniffer#3833 and either one of those PRs may need rebasing if one of others gets merged first.

👆🏻 This translated to PRs #49 and #62 in this repo.

Suggested changelog entry

Generic.PHP.LowercaseType: improved time-to-result for the sniff

@jrfnl jrfnl added this to the 3.8.0 milestone Nov 10, 2023
@jrfnl jrfnl force-pushed the feature/generic-lowercasetype-improve-performance branch from 6b82714 to 8387c5d Compare November 11, 2023 03:41
@jrfnl jrfnl force-pushed the feature/generic-lowercasetype-improve-performance branch from 8387c5d to fbecde7 Compare December 5, 2023 23:12
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 jrfnl force-pushed the feature/generic-lowercasetype-improve-performance branch from fbecde7 to ce365fd Compare December 5, 2023 23:15
@jrfnl jrfnl merged commit b2464f7 into master Dec 5, 2023
63 checks passed
@jrfnl jrfnl deleted the feature/generic-lowercasetype-improve-performance branch December 5, 2023 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant