Skip to content

Commit ce365fd

Browse files
jrfnljrfnl
committed
Generic/LowerCaseType: improve performance
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.
1 parent 38c2caf commit ce365fd

File tree

2 files changed

+56
-28
lines changed

2 files changed

+56
-28
lines changed

‎CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,8 @@ The file documents changes to the PHP_CodeSniffer project.
9999
- Thanks to Juliette Reinders Folmer (@jrfnl) for the patch
100100
- Runtime performance improvement for PHPCS CLI users. The improvement should be most noticeable for users on Windows.
101101
- Thanks to Juliette Reinders Folmer (@jrfnl) for the patch
102+
- The following sniffs have received performance related improvements:
103+
- Generic.PHP.LowerCaseType
102104
- The -e (explain) command will now list sniffs in natural order
103105
- Thanks to Juliette Reinders Folmer (@jrfnl) for the patch
104106
- Tests using the PHPCS native test framework with multiple test case files will now run the test case files in numeric order.

‎src/Standards/Generic/Sniffs/PHP/LowerCaseTypeSniff.php

Lines changed: 54 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,10 @@ class LowerCaseTypeSniff implements Sniff
5151
public function register()
5252
{
5353
$tokens = Tokens::$castTokens;
54+
$tokens += Tokens::$ooScopeTokens;
5455
$tokens[] = T_FUNCTION;
5556
$tokens[] = T_CLOSURE;
5657
$tokens[] = T_FN;
57-
$tokens[] = T_VARIABLE;
5858
return $tokens;
5959

6060
}//end register()
@@ -90,40 +90,66 @@ public function process(File $phpcsFile, $stackPtr)
9090
* Check property types.
9191
*/
9292

93-
if ($tokens[$stackPtr]['code'] === T_VARIABLE) {
94-
try {
95-
$props = $phpcsFile->getMemberProperties($stackPtr);
96-
} catch (RuntimeException $e) {
97-
// Not an OO property.
93+
if (isset(Tokens::$ooScopeTokens[$tokens[$stackPtr]['code']]) === true) {
94+
if (isset($tokens[$stackPtr]['scope_opener'], $tokens[$stackPtr]['scope_closer']) === false) {
9895
return;
9996
}
10097

101-
if (empty($props) === true) {
102-
// Parse error - property in interface or enum. Ignore.
103-
return;
104-
}
98+
for ($i = ($tokens[$stackPtr]['scope_opener'] + 1); $i < $tokens[$stackPtr]['scope_closer']; $i++) {
99+
// Skip over potentially large docblocks.
100+
if ($tokens[$i]['code'] === \T_DOC_COMMENT_OPEN_TAG
101+
&& isset($tokens[$i]['comment_closer']) === true
102+
) {
103+
$i = $tokens[$i]['comment_closer'];
104+
continue;
105+
}
105106

106-
// Strip off potential nullable indication.
107-
$type = ltrim($props['type'], '?');
107+
// Skip over function declarations and everything nested within.
108+
if ($tokens[$i]['code'] === \T_FUNCTION
109+
&& isset($tokens[$i]['scope_closer']) === true
110+
) {
111+
$i = $tokens[$i]['scope_closer'];
112+
continue;
113+
}
108114

109-
if ($type !== '') {
110-
$error = 'PHP property type declarations must be lowercase; expected "%s" but found "%s"';
111-
$errorCode = 'PropertyTypeFound';
115+
if ($tokens[$i]['code'] !== \T_VARIABLE) {
116+
continue;
117+
}
112118

113-
if ($props['type_token'] === T_TYPE_INTERSECTION) {
114-
// Intersection types don't support simple types.
115-
} else if (strpos($type, '|') !== false) {
116-
$this->processUnionType(
117-
$phpcsFile,
118-
$props['type_token'],
119-
$props['type_end_token'],
120-
$error,
121-
$errorCode
122-
);
123-
} else if (isset($this->phpTypes[strtolower($type)]) === true) {
124-
$this->processType($phpcsFile, $props['type_token'], $type, $error, $errorCode);
119+
try {
120+
$props = $phpcsFile->getMemberProperties($i);
121+
} catch (RuntimeException $e) {
122+
// Not an OO property.
123+
continue;
125124
}
126-
}
125+
126+
if (empty($props) === true) {
127+
// Parse error - property in interface or enum. Ignore.
128+
return;
129+
}
130+
131+
// Strip off potential nullable indication.
132+
$type = ltrim($props['type'], '?');
133+
134+
if ($type !== '') {
135+
$error = 'PHP property type declarations must be lowercase; expected "%s" but found "%s"';
136+
$errorCode = 'PropertyTypeFound';
137+
138+
if ($props['type_token'] === T_TYPE_INTERSECTION) {
139+
// Intersection types don't support simple types.
140+
} else if (strpos($type, '|') !== false) {
141+
$this->processUnionType(
142+
$phpcsFile,
143+
$props['type_token'],
144+
$props['type_end_token'],
145+
$error,
146+
$errorCode
147+
);
148+
} else if (isset($this->phpTypes[strtolower($type)]) === true) {
149+
$this->processType($phpcsFile, $props['type_token'], $type, $error, $errorCode);
150+
}
151+
}
152+
}//end for
127153

128154
return;
129155
}//end if

0 commit comments

Comments
 (0)