Repository navigation
Avoid quadratic work when combining many same-shaped constant arrays - #6970
Merged
Merged
Conversation
optimizeConstantArrays() collapsed each group of constant arrays with the
same keys by merging them one by one into the result, so every merge
unioned the growing value union with one more value - quadratic in the
number of variants. At a key where every variant holds only constant
scalars (or plain unions of them), one union of all the values gives the
same type, members in the same order, as that chain; the chain now merges
only the rest of the shape, with a placeholder at those keys.
bug-12671 merges ~750 list{code, name, null} variants this way.
Amp-Thread-ID: https://ampcode.com/threads/T-01a1b134-d1e7-7395-b029-dd3c2510fac2
reduceArrays() enumerated every pair of arrays sharing a key, then skipped
the pairs whose single constant scalar values rule out a keys-superset.
For n arrays with the same keys that is n^2/2 pairs, nearly all of them
skipped - bug-12671 unions ~790 list{code, name, null} arrays.
Among arrays with the same number of keys (two or more) and the same keys
holding a single constant scalar, a pair is skipped when it differs in two
or more of them, so the pairs that can get past the check are found
through buckets of those values with one key left out. The pairs with
other arrays, and with arrays that lost their single constant scalar
values by a merge, are still all visited, and every pair is visited in the
same order as before, so the merges and the result stay the same.
Amp-Thread-ID: https://ampcode.com/threads/T-01a1b134-d1e7-7395-b029-dd3c2510fac2
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.
Makes
tests/bench/data/bug-12671.phpmuch cheaper again without giving up the precision that e606ca2 ("Merge unions of constant arrays with many keys instead of degrading") added. Analysis results are unchanged.Cause
Before e606ca2,
processArrayTypes()turned a union of constant arrays with more than 62 distinct keys into a general array. Now the union is kept. In bug-12671 theSTATESconstant produces a union of about 790list{code, name, null}arrays, and that union now goes down the precise path. That path had two quadratic algorithms inTypeCombinator:optimizeConstantArrays(), stage 1: each group of constant arrays with the same keys (about 750 variants here) was merged into the result one at a time withmergeWith(). Every step unioned the growing value union with one more value, so the cost grew with the square of the number of variants.reduceArrays(): it went through every pair of arrays that share a key (about 312k pairs here) and then skipped nearly all of them inhaveDisjointConstantScalarValues().Changes
mergeConstantArraysWithSameKeys()handles keys where every variant holds only constant scalars, or plain unions of them (Template and Benevolent unions are excluded). At those keys, all the values are unioned in one step. That gives the same type, with members in the same order, as the old chain. ThemergeWith()chain still builds the rest of the shape, with a placeholder at those keys.reduceArrays()(3dd322c): take arrays with the same number of keys (two or more) whose keys each hold a single constant scalar. A pair of them can only pass the disjointness check if their values differ at no more than one of those keys. The candidate pairs are therefore found through buckets of the values with one key left out. Pairs involving other arrays, and arrays whose scalar values were lost in an earlier merge, are still all visited. Pairs are visited in the same order as before, so the merges happen the same way.I rejected two alternatives because they change results (optional keys, and the order of union members): a tree-shaped pairwise merge, and one union over all values regardless of their kind.
Both changes are mirrored in
turbo-ext/src/TypeCombinator.cpp, andturbo-ext/src/generated/TypeCombinator.his regenerated.Instruction counts
Measured with cachegrind: the whole process at level 8 with bleedingEdge and opcache, without JIT, on PHP 8.5. Net is the file's count minus the count for an empty file at the same commit. Base is 2.3.x at bcca2b0. The table lists every bench file that changed by more than 0.5%. The other 78 files are within ±0.5%.
Before e606ca2 (at 408f6e7), bug-12671 was 4.43 G net. Part of that old number is not comparable: once the overflow path passed 62 keys it stopped collecting values, so the old type kept only about 89 of the roughly 1000 state lists. The rest of today's gap is mostly the cost of the now-precise result, plus the separate 9b24ee5 regression, which is handled elsewhere. On 9b24ee5's parent, these commits take bug-12671 from 9.17 G to 6.07 G.
The small increases come from the extra bookkeeping in
reduceArrays(). They don't show up in wall-clock time (see below).Wall-clock times
Command:
bin/phpstan analyse -c <level 8 + bleedingEdge config> --debug --no-progress <file>, run with hyperfine, 1 warm-up run plus 7 measured runs per side. The table shows the median. Machine: orb, Intel Xeon @ 2.60 GHz, 16 vCPUs. PHP 8.5.12 NTS, without the turbo extension.Tests
make tests: passes both in pure PHP and withphpstan_turbo.soloaded (23,472 tests).make phpstan: no errors.bin/side-by-side.php,tests/smoke.php(ALL OK) andtests/signature-parity.php(18,468 members) all pass. A build with strict compiler flags also passes.