Repository navigation
Describe array shapes once while sorting a union, memoize a union's loosely equal types - #6973
Merged
Merged
Conversation
usort() compares a member with several others, and the comparator described both sides of every comparison. Constant arrays are ordered by their description, so sorting a union of many array shapes described each shape O(log n) times, which dominated describe() of such unions. The comparator now reads those descriptions from a table filled once per shape for the sort. Amp-Thread-ID: https://ampcode.com/threads/T-01a1b134-e5a2-75fe-9eeb-fa489813ac89
A loose comparison asks the other side for the values it can, or always does, loosely equal, once for its type and once per narrowed branch, and again in every pass over a loop body. A union like the int|string key of an array is the same instance in all those places, so the result is now kept on the union. Amp-Thread-ID: https://ampcode.com/threads/T-01a1b134-e5a2-75fe-9eeb-fa489813ac89
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.
tests/bench/data/bug-7581.php got 47 % slower with 9d55d96 ("Narrow loose comparisons by the values each side can loosely equal"). Counting only its own analysis (net instructions, see below): 3.98 G on 4563fe2, 5.82 G on 9d55d96. This PR takes it to 4.99 G and keeps the inferred types and errors unchanged.
Cause
The benchmark is a
switch ($tag)over 33case 'Ax':arms insideforeach ($parsed as $tag => $val). Since 9d55d96,$tag == 'Ax'narrows$tag(int|string) to'Ax'inside each arm; before,$tagstayedint|string. That is correct and more precise, but it changes what the branch merges do:$tagand$datain both directions:$tag='Ak'⇒$data, and$data=…⇒$tag. It creates 325 conditional expressions instead of 54. Each pair runsisSuperTypeOf()andTypeCombinator::remove()over unions of about 33 array shapes.ConditionalExpressionHolder::getKey()also describes such a union.UnionTypeHelper::sortTypes(), whichdescribe()uses, orders constant arrays by their description. Its comparator described both sides of every comparison, so each shape was described O(log n) times. This was the largest single part of the added time.getPossiblyLooselyEqualType()/getAlwaysLooselyEqualType()of theint|stringkey were rebuilt from scratch for the same union instance: 99 times inIdenticalNarrowingHelper::createLooselyEqualTypes()(both branches, three loop passes) and again inresolveEqualType().Changes
sortTypes()keeps the descriptions of constant arrays in a table for the duration of the sort. The other branches still calldescribe()directly, because memoizing their cheap descriptions cost more than it saved (+0.7 % on and-chain-resolve-type-blowup). The native mirror inturbo-ext/src/UnionTypeHelper.cppmakes the samedescribe()calls.UnionTypekeeps both results per version id, like its other memos ($cachedDescriptions,$isNull, …).UnionType.cppmirrors it, with the two new property slots in the generated headers ofUnionType,TemplateUnionTypeandTemplateBenevolentUnionType.For reviewers: the first commit memoizes
describe()insideUnionTypeHelper::sortTypes(). It keeps the existing describe-based ordering of constant arrays, which CLAUDE.md discourages ("never sort, compare, or transform types viadescribe()"). It does not add a new use ofdescribe()for ordering, but it does make that ordering cheaper to keep. You may prefer to key the sort by the types instead, for example by aConstantArrayTypecomparison of key and value types. That would change the order union members are described in, so it would be a separate change.The remaining ~1 G over 4563fe2 is the extra conditional-expression work that comes with the more precise narrowing. That is mostly
ConstantArrayType::isSuperTypeOf()pairs insideUnionType::isSuperTypeOf()/tryRemove(). Cutting it means making comparisons between unions of array shapes sub-quadratic, which is out of scope here.Instruction counts
Measured with Valgrind cachegrind through
sweep.sh: PHP 8.5.12 with OPcache, no turbo extension, a level 8 config including bleedingEdge.neon. Net = instructions for the file minus instructions for an empty file at the same commit, so boot is excluded. All 86 files intests/bench/datawere measured onbcca2b08a(2.3.x) and this branch. Every file not listed changed by less than ±0.2 %.For reference, bug-7581.php was 3.977 G on 4563fe2 and 5.821 G on 9d55d96. By commit: the sortTypes change takes it to 5.36 G and the union memo to 4.99 G. The +0.3–0.4 % files come from the per-sort memo table; none of them changed more than that.
Wall-clock times
php8.5 bin/phpstan analyse -c <config> --debug --no-progress --memory-limit=-1 <file>, run from each side's own checkout.tests/bench/phpstan.neondoes not exist on 2.3.x, so the config is the onesweep.shgenerates (bleedingEdge.neon, level 8,inferPrivatePropertyTypeFromConstructor, a separate tmpDir per side). Each side got two warm-up runs, then 11 runs per side alternating base/branch, and the table shows the median. A first pass with hyperfine (2 warm-ups, 7 runs, all base runs then all branch runs) drifted by up to ±5–10 % between the two blocks on this VM, so these are the interleaved numbers. The machine is an Amp orb: Intel Xeon @ 2.60 GHz, 16 vCPUs, PHP 8.5.12 CLI without OPcache, no turbo extension.hash-key-lookup's +2.8 % is larger than its +0.4 % instruction change and within the run-to-run spread seen on this machine.
Tests
make tests: green exceptDirectoryWalkerTest::testListingsSeeAddedAndRemovedFiles, which fails intermittently on unchanged code and passed on rerun.PHP_INI_SCAN_DIR): green exceptResultCacheManagerCollectedDataTest::testFileChangedAndChangedBackDuringTheAnalysisIsAnalysedAgain. It also fails intermittently on unchanged 2.3.x (3 of 20 runs under load) and passed on rerun.make phpstanandmake cs: green.turbo-ext/tests/smoke.php: ALL OK.turbo-ext/tests/signature-parity.php: OK.php8.5 turbo-ext/bin/side-by-side.php: OK. The generated headers are current under PHP 8.5.