Conversation
… an error `ConstantArrayType::checkOurKeys()` (used by `ConstantArrayType::accepts()`) calls `VerbosityLevel::getRecommendedLevelByType($valueType, $otherValueType)` for every key it checks. That call walks both value types completely, but its result is only used to describe the two types in the error message of a key whose value is rejected. For every accepted key the walk is wasted, and when a key's value is itself an array shape, the walk covers that whole nested shape too. This PR moves the computation so it only runs when a message is actually built. In PHP it becomes a closure called from the two places that build the message. The C++ twin in turbo-ext (`turbo-ext/src/ConstantArrayType.cpp`) gets the same treatment, with the call moved into `offsetReason()`. Error messages are unchanged. I added a phpbench scenario, `tests/bench/data/nested-array-shape-accepts.php`, with a 150-key shape where 40 keys refer to a nested 150-key shape, passed to a parameter of the same shape. Results (wall time, 3 alternating runs after a discarded warm-up): | Benchmark | Before | After | |---|---|---| | phpbench `nested-array-shape-accepts.php` | 231.2-231.7 ms | 207.6-208.8 ms | | [reproducer](https://github.com/Kocal/sf-ux-css-phpstan-reproducer) app, `phpstan-nested.neon`, without turbo extension | 23.03-23.25 s | 18.80-19.09 s | | same app, with turbo extension loaded | 8.16-8.24 s | 7.12-7.15 s | The reproducer is a small Symfony app that analyses, at level 6, one method taking the symfony/ux-css `CssStyles` shape (847 keys, 144 of them referring to a nested copy of the same shape) and passing it to one `css()` call. Same errors before and after (none). The rest of the phpbench suite stays within its thresholds, the largest slowdown being +1.3% (bug-14869.php, 259 ms vs 256 ms). `tests/PHPStan/Rules` and `tests/PHPStan/Type` pass with and without the turbo extension loaded, and `turbo-ext/tests/smoke.php` plus `turbo-ext/tests/signature-parity.php` pass too. This is independent from phpstan#6643 and phpstan#6645, which fix other redundant walks over the same kind of shape. ``` XDEBUG_MODE=off tests/vendor/bin/phpbench run --variant=nested-array-shape-accepts.php --report=aggregate ```
Author
|
#6649 seems to be a better version |
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.
ConstantArrayType::checkOurKeys()(used byConstantArrayType::accepts()) callsVerbosityLevel::getRecommendedLevelByType($valueType, $otherValueType)for every key it checks. That call walks both value types completely, but its result is only used to describe the two types in the error message of a key whose value is rejected. For every accepted key the walk is wasted, and when a key's value is itself an array shape, the walk covers that whole nested shape too.This PR moves the computation so it only runs when a message is actually built. In PHP it becomes a closure called from the two places that build the message. The C++ twin in turbo-ext (
turbo-ext/src/ConstantArrayType.cpp) gets the same treatment, with the call moved intooffsetReason(). Error messages are unchanged.I added a phpbench scenario,
tests/bench/data/nested-array-shape-accepts.php, with a 150-key shape where 40 keys refer to a nested 150-key shape, passed to a parameter of the same shape.Results (wall time, 3 alternating runs after a discarded warm-up):
nested-array-shape-accepts.phpphpstan-nested.neon, without turbo extensionThe reproducer is a small Symfony app that analyses, at level 6, one method taking the symfony/ux-css
CssStylesshape (847 keys, 144 of them referring to a nested copy of the same shape) and passing it to onecss()call. Same errors before and after (none).The rest of the phpbench suite stays within its thresholds, the largest slowdown being +1.3% (bug-14869.php, 259 ms vs 256 ms).
tests/PHPStan/Rulesandtests/PHPStan/Typepass with and without the turbo extension loaded, andturbo-ext/tests/smoke.phpplusturbo-ext/tests/signature-parity.phppass too.This is independent from #6643 and #6645, which fix other redundant walks over the same kind of shape.