Improve performance of function restriction sniffs #779

Merged
merged 1 commit into from Jan 14, 2017

Projects

None yet

3 participants

@JDGrimes
Contributor
JDGrimes commented Jan 10, 2017 edited

This abstract sniff is extended by quite a few of our sniffs, and so it has a major impact on the performance of the ruleset as a whole.

Here we make two improvements:

  1. We use isset() instead of in_array().
  2. We don't call findPrevious() a second time unless we actually need the information. We only need to check if the namespace is top-level if the function is prefixed with \\, the namespace separator.

This seems to improve the performance of these sniffs by as much as 25%.

Profiled with Blackfire (free edition, so the links will only be retained for 1 day), by running over a sizable plugin (using a custom ruleset that includes Core, Extra, Docs, and VIP, but with some exclusions):

Before:

untitled_profile_-blackfire-_2017-01-10_10 09 59

After:

untitled_profile_-blackfire-_2017-01-10_10 27 02

Comparison:

comparison_from_untitled_to_untitled_-blackfire-_2017-01-10_10 39 36

@JDGrimes JDGrimes added this to the 0.11.0 milestone Jan 10, 2017
@grappler

Like the use of Blackfire.

@@ -150,24 +150,32 @@ public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) {
if ( false !== $prev ) {
// Skip sniffing if calling a same-named method, or on function definitions.
- if ( in_array( $tokens[ $prev ]['code'], array( T_FUNCTION, T_DOUBLE_COLON, T_OBJECT_OPERATOR ), true ) ) {
+ $skipped = array(
+ T_FUNCTION => true,
@grappler
grappler Jan 10, 2017 Contributor

I think you could do T_FUNCTION => T_FUNCTION. It would be nice if the values were at the same height,

- return;
+ if ( T_NS_SEPARATOR === $tokens[ $prev ]['code'] ) {
+ $pprev = $phpcsFile->findPrevious( PHP_CodeSniffer_Tokens::$emptyTokens, ( $prev - 1 ), null, true );
+ if ( false !== $pprev && T_STRING === $tokens[ $pprev ]['code'] ) {
@grappler
grappler Jan 10, 2017 Contributor

There is a double space before &&

@jrfnl
jrfnl approved these changes Jan 10, 2017 View changes

I like @grappler's spacing suggestions, other than that the code looks solid.

@JDGrimes JDGrimes Improve performance of function restriction sniffs
This abstract sniff is extended by quite a few of our sniffs, and so it has a major impact on the performance of the ruleset as a whole.

Here we make two improvements:

1. We use `isset()` instead of `in_array()`.
2. We don't call `findPrevious()` a second time unless we actually need the information. We only need to check if the namespace is top-level if the function is prefixed with `\\`, the namespace separator.

This seems to improve the performance of these sniffs by as much as 25%.
4e2d298
@JDGrimes
Contributor

I've implemented @grappler's suggestions in 4e2d298, and in addition added $skipped to the list of unset() vars at the end of that if block.

@jrfnl
Contributor
jrfnl commented Jan 13, 2017

Related: #782 will actually make this PR unit testable for the change to the handling of exclude

@jrfnl
Contributor
jrfnl commented Jan 14, 2017

@JDGrimes Shall I merge this or do you want to look at #782 before I do ?

@jrfnl jrfnl merged commit f2c7614 into develop Jan 14, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jrfnl jrfnl deleted the performance/function-sniff branch Jan 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment