Repository navigation
Memoize Scope::getPhpVersion() per scope - #7014
Merged
Merged
Conversation
Since rules and expression handlers ask the scope for the PHP version (8cf6b64), PropertyFetchHandler asks on every property fetch and isReadonlyPropertyFetch() on every readonly check. On tests/bench/data/nullsafe-chain-walk.php that is 53,911 calls on 51 distinct scopes, each looking up PHP_VERSION_ID among the expression types and building a new PhpVersions. The answer depends only on the scope's expression types, so the scope remembers it with the array it was computed from. While the expression types are untouched both properties hold the same array and the check is a pointer comparison; a write in place (specifyExpressionTypeInPlace() and co. on a working copy) separates them and the answer is computed again. The native MutatingScope mirrors it. Amp-Thread-ID: https://ampcode.com/threads/T-01a1976c-1bde-72f8-b96c-072279109d59
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.
Since rules and expression handlers ask the scope for the PHP version (8cf6b64),
PropertyFetchHandlercallsScope::getPhpVersion()on every property fetch, andisReadonlyPropertyFetch()calls it on every readonly check. Each call looks upPHP_VERSION_IDamong the expression types and builds a newPhpVersions. Ontests/bench/data/nullsafe-chain-walk.phpthat is 53,911 calls on only 51 distinct scope objects.This PR memoizes the answer per scope, together with the expression-types array it was computed from:
===check is a pointer comparison.specifyExpressionTypeInPlace()and co. on a working copy): the scope's array is separated, the arrays no longer match, and the answer is computed again.So the memo doesn't rely on scopes never being written to after being read. The native
MutatingScopemirrors it: two new property slots, the same check (Z_ARRpointer equality, thenzend_is_identical()), and regeneratedMutatingScope.h/NodeCallbackScope.h(generated with PHP 8.5, as CI'sside-by-side.phpuses).Measurements
Excimer CPU samples,
--debug, bleeding edge, level 8, 3 interleaved rounds:nullsafe-chain-walk.php, whole rungetPhpVersion()On the whole
tests/bench/datacorpus the change is within noise (user CPU 49.6/52.7/53.0 s vs 50.5/53.2/51.0 s). The gain is in property-fetch-heavy code.Two alternatives I measured and left out:
PhpVersionsinConfiguredPhpVersionRangeHelper: about −1%, and redundant once the scope memoizes. In ordinary code there are about 2 calls per distinct scope, andgetPhpVersion()is negligible there.PhpVersions::supports*(): about −0.8%, not worth editing 67 methods.Verification
ScopePhpVersionTest::testPhpVersionFollowsExpressionTypesWrittenInPlace(new) passes. A plain memo without the array check fails it.ResultCacheManagerCollectedDataTest::testFileChangedAndChangedBackDuringTheAnalysisIsAnalysedAgain, which also fails 1 in 15 on unmodified 2.3.x and passed 15/15 with this change. With the extension loaded, 23,562 tests, 0 failures.tests/smoke.phpALL OK (scope-family.phpalready asksgetPhpVersion()twice per scope on both sides),signature-parity.phpOK,bin/side-by-side.phpOK on PHP 8.5,walk-trace.phpidentical (703,814 lines), clang-tidy 21 clean onMutatingScope.cpp.--error-format=rawontests/bench/data(3,021 lines) is byte-identical for HEAD, this PR in PHP mode, and this PR with the extension.make phpstanand phpcs pass.No code shipped in the PHAR uses syntax that needs the downgrade to handle anything new.