Repository navigation
[perf] Decide named constructor from node visibility before reflection - #8721
Merged
Merged
Conversation
This file contains hidden or 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
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.
What
ClassMethodManipulator::isNamedConstructor()resolved the class reflection up front for every__construct, then looked at visibility. But visibility is local to the node:__constructis always a named constructor,__constructis never one (the only reflection-dependent branch,isFinalByKeyword, can only returnfalsefor it anyway),__constructstill needs reflection, to read the classisFinalByKeyword().So the check now answers private and public (the common cases) from the node and resolves reflection only for the protected case.
Bench
bench/named_constructor_visibility_bench.php- 150 classes with ordinary public constructors,isNamedConstructor()driven over each (9000 calls), CPU time (user+sys), warmup excluded.Modest - the resolve is scope-backed, not full native reflection - but a real, repeatable CPU drop on the common path.
Behaviour
Equivalent for all valid code. The one edge: a private
__constructwhose enclosing class cannot be resolved previously returnedfalse(via the null-reflection bail) and now returnstrue. A__constructalways has a resolvable enclosing class, so this only differs for degenerate input. The sole caller (RemoveEmptyClassMethodRector) test suite is green (33 tests).