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

Improve performance for get_namespaced_classname #2054

Closed
kkmuffme opened this issue May 22, 2022 · 4 comments
Closed

Improve performance for get_namespaced_classname #2054

kkmuffme opened this issue May 22, 2022 · 4 comments

Comments

@kkmuffme
Copy link

kkmuffme commented May 22, 2022

Just profiled phpcbf and the get_namespaced_classname function is extremely slow.

It's easy enough to cache this with a static though.

if findNext( \T_NAMESPACE, $search_from ) returns false, we can store (and apply) the current namespace for all subsequent calls.

This means we do not need to call findPrevious everytime, but can just use the namespace we already determined for all code in this file.

I quickly put my idea together (not tested yet, also this assumes that the function can be called on earlier lines after it was called on later lines. if this is not the case/possible in phpcs, then this code can be simplified a lot instead of storing it in an array)

	protected function get_namespaced_classname( $classname, $search_from ) {
		static $namespace_cache = array();
		// Don't do anything if this is already a fully qualified classname.
		if ( empty( $classname ) || '\\' === $classname[0] ) {
			return $classname;
		}

		// Remove the namespace keyword if used.
		if ( 0 === strpos( $classname, 'namespace\\' ) ) {
			$classname = substr( $classname, 10 );
		}
		
		// asc array keys
		$cached_namespace = false;
		foreach ( array_keys( $namespace_cache ) as $cached_search_from ) {
			if ( $cached_search_from <= $search_from ) {
				$cached_namespace = $namespace_cache[ $cached_search_from ];
				break;
			}
		}

		// nothing in cache yet
		if ( false === $cached_namespace ) {
			$namespace_keyword = $this->phpcsFile->findPrevious( \T_NAMESPACE, $search_from );	
		} elseif ( true === $cached_namespace ) {
			// global namespace
			$namespace_keyword = false;
		} else {
			// just to skip the first if, since there is a namespace
			$namespace_keyword = true;
		}
		
		if ( false === $namespace_keyword ) {
			// No namespace keyword found at all, so global namespace.
			$classname = '\\' . $classname;
		} else {
			if ( false === $cached_namespace ) {
				$namespace = Namespaces::determineNamespace( $this->phpcsFile, $search_from );
			} else {
				// don't need to check the elseif "true" here, since we would have already taken the global namespace in above "if" condition
				$namespace = $cached_namespace;
			}

			if ( ! empty( $namespace ) ) {
				$classname = '\\' . $namespace . '\\' . $classname;
			} else {
				// No actual namespace found, so global namespace.
				$classname = '\\' . $classname;
			}
		}
		
		// got data from cache, so we don't need to check again
		if ( false !== $cached_namespace ) {
			return $classname;
		}
		
		$next_namespace_keyword = $this->phpcsFile->findNext( \T_NAMESPACE, $search_from );
		// there are no subsequent namespace declarations, so all later code will have the given namespace
		if ( $next_namespace_keyword === false ) {
			if ( false === $namespace_keyword && empty( $namespace ) ) {
				// global namespace
				$namespace_cache[ $search_from ] = true;
			} else {
				$namespace_cache[ $search_from ] = $namespace;
			}
			ksort( $namespace_cache, SORT_NUMERIC );
		}

		return $classname;
	}

If phpcs calls lines in correct order (e.g. this function will not be called on line 100 and then on line 50), code can be simplified to:

	protected function get_namespaced_classname( $classname, $search_from ) {
		static $cached_namespace = false;
		// Don't do anything if this is already a fully qualified classname.
		if ( empty( $classname ) || '\\' === $classname[0] ) {
			return $classname;
		}

		// Remove the namespace keyword if used.
		if ( 0 === strpos( $classname, 'namespace\\' ) ) {
			$classname = substr( $classname, 10 );
		}

		// nothing in cache yet
		if ( false === $cached_namespace ) {
			$namespace_keyword = $this->phpcsFile->findPrevious( \T_NAMESPACE, $search_from );	
		} elseif ( true === $cached_namespace ) {
			// global namespace
			$namespace_keyword = false;
		} else {
			// just to skip the first if, since there is a namespace
			$namespace_keyword = true;
		}
		
		if ( false === $namespace_keyword ) {
			// No namespace keyword found at all, so global namespace.
			$classname = '\\' . $classname;
		} else {
			if ( false === $cached_namespace ) {
				$namespace = Namespaces::determineNamespace( $this->phpcsFile, $search_from );
			} else {
				// don't need to check the elseif "true" here, since we would have already taken the global namespace in above "if" condition
				$namespace = $cached_namespace;
			}

			if ( ! empty( $namespace ) ) {
				$classname = '\\' . $namespace . '\\' . $classname;
			} else {
				// No actual namespace found, so global namespace.
				$classname = '\\' . $classname;
			}
		}
		
		// got data from cache, so we don't need to check again
		if ( false !== $cached_namespace ) {
			return $classname;
		}
		
		$next_namespace_keyword = $this->phpcsFile->findNext( \T_NAMESPACE, $search_from );
		// there are no subsequent namespace declarations, so all later code will have the given namespace
		if ( $next_namespace_keyword === false ) {
			if ( false === $namespace_keyword && empty( $namespace ) ) {
				// global namespace
				$cached_namespace = true;
			} else {
				$cached_namespace = $namespace;
			}
		}

		return $classname;
	}

I'm happy to provide a PR, just wanted to get general feedback on this before I start testing and PRing it.

@jrfnl
Copy link
Member

jrfnl commented May 22, 2022

@kkmuffme Thanks for the suggestion. This is something which is already in the planning and will be solved by a NamespaceTracker class which is lined up to be added to PHPCSUtils. Once that (and some other planned features) are available in PHPCSUtils, the whole AbstractClassRestrictions class will be deprecated and the classes which use it in WPCS will switch to using an improved version from PHPCSUtils.

@jrfnl jrfnl added Status: Requirement not met On hold until WPCS supports a particular version of a dependency (PHP, PHP_CodeSniffer, etc.). Upstream: PHPCSUtils labels May 22, 2022
@kkmuffme
Copy link
Author

Thanks for your feedback.
Is there any ETA for the PHPCSUtils?

Is there any possibility to create a new WPCS release with just this and perhaps #1583 (comment) as this would massively improve WPCS's performance and probably save multiple working days per year in dev waiting & server processing time per year in even the smallest of companies.

@jrfnl
Copy link
Member

jrfnl commented May 22, 2022

No and no.

@jrfnl jrfnl added this to the 3.0.0 milestone Aug 15, 2023
@jrfnl
Copy link
Member

jrfnl commented Aug 15, 2023

Runtime caching of the results various functions was added to PHPCSUtils in the 1.0.0-alpha4 release and the PHPCSUtils functions are used in the AbstractClassRestrictionsSniff since PR #2237. Closing as fixed.

In the future, this will be further improved with the abstract from PHPCSUtils, but in the mean time, the performance issues should already be gone.

@jrfnl jrfnl closed this as completed Aug 15, 2023
@jrfnl jrfnl removed the Status: Requirement not met On hold until WPCS supports a particular version of a dependency (PHP, PHP_CodeSniffer, etc.). label Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants