Repository navigation
Do the per-key work of array shapes once per shared value type - #6987
Open
ondrejmirtes wants to merge 2 commits into
Open
ondrejmirtes wants to merge 2 commits into
ondrejmirtes wants to merge 2 commits into
Conversation
Writing through a union offset gives an array shape one optional key per member of the offset, all holding the same value type. With 250 keys and a 250-member value type, every operation that walked the keys did the same work 250 times: - mergeWith() and legacyMergeWith() unioned each pair of value types once per key; they now union each distinct pair once, so the merged keys keep sharing one value type - isSuperTypeOf() against another shape called getOffsetValueType() and isSuperTypeOf() once per key; it now compares each pair of value types once - countConstantArrayValueTypes() traversed a shared value type once per key; it now traverses it once and counts it per occurrence - reduceArrays()' single-constant-scalar lookup inspected the shared value type once per key - getValuesArray() and getKeysArray() unioned every window of possible source values from scratch, quadratic in the number of keys; the window unions are now kept as a two-stack queue, linear in the number of keys The inferred types do not change. The turbo-ext mirrors get the same changes. Amp-Thread-ID: https://ampcode.com/threads/T-01a1b24f-e4cc-7457-9324-5d29caf3c481
isSuperTypeOf() records a comparison only after the `no` early return, and getConstantScalarValuesByKey() keeps its original checks and memoizes their outcome, so the lines carrying the pre-existing TrinaryLogic checks are unchanged. Mutation testing had flagged the moved `$typeUnsealedness->yes()` check: `$bothDefinite` rules out the maybe case there, so its `!no()` mutant cannot be told apart. Amp-Thread-ID: https://ampcode.com/threads/T-01a1b24f-e4cc-7457-9324-5d29caf3c481
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.
Writing through a union offset gives an array shape one optional key per member of the offset, all holding the same value type. In the reproducers below that is 250 keys holding the same 250-member union, and every operation that walked the keys repeated the same work 250 times. This keeps the shape (2f66c45's design) and makes those operations do the work once per distinct value type:
ConstantArrayType::mergeWith()/legacyMergeWith()union each distinct pair of value types once, so merged keys keep sharing one value type (and its memory).ConstantArrayType::isSuperTypeOf()against another shape compares each pair of value types once instead of callinggetOffsetValueType()+isSuperTypeOf()per key (remove()increateConditionalExpressionshit this).TypeCombinator::countConstantArrayValueTypes()traverses a shared value type once and counts it per occurrence.TypeCombinator::getConstantScalarValuesByKey()(inreduceArrays()) inspects a shared value type once.ConstantArrayType::getValuesArray()/getKeysArray()unioned every window of possible source values from scratch, quadratic in the number of keys. The window unions are now a two-stack queue: linear.No new limits. Inferred types don't change: the reproducers' dumped types are byte-identical to 2.3.x, and to each other with turbo on and off. The turbo-ext mirrors (
ConstantArrayType.cpp,TypeCombinator.cpp) get the same changes.Measurements
Level 8, bleedingEdge,
phpVersion: 80500. Per-file time and memory fromanalyse --debug -vvv("took" / "consumed"), best of 5 runs, PHP 8.4 without the turbo extension (release PHARs don't load it):$missing[$value] = Code::from($value)$values[$code->value] = $code->value, ksort + array_valuesWithout bleedingEdge (the
legacyMergeWith()path): A 1.02 s → 0.27 s, B 2.60 s → 0.65 s. With the turbo extension loaded, 2.3.x-dev and this PR are the same (A 0.15 s, B 0.27–0.29 s): its union cache already absorbed most of the repeated unions.The private real-world file from the report was not measured.
What is left
A's memory stays at about 52 MB, compared with 20 MB on 2.1.34. The shape's precise description is about 1 MB, and
ConditionalExpressionHolder::getKey()embeds the guard type's description, so each conditional expression guarded by$missingretains a 1 MB key string (about 26 MB in A). TheIntersectionTypedescription cache holds another 8 MB or so. This comes from the shape representation and can't be memoized away. The rest of the remaining time is mostly that describe, plus traversals that may not be memoized (TemplateTypeHelper::resolveTemplateTypes()distinguishes occurrences by identity). Getting all the way back to 2.1.34 would mean not building the shape for this case, which reverses 2f66c45. I left that out of this PR.Tests
tests/bench/data/enum-value-keyed-map.php: reproducer A. Reproducer B is already covered byarray-values-literal-keyed-map.php.nsrt/array_values.php:array_values()of a shape with optional keys between required ones, covering both the window refill and its growth.getValuesArray()against the old algorithm over 3,000 shapes matched every value type and optional key; the same check fails when the window tail is dropped.make testspasses with and without the turbo extension, apart from one parallel-load flake (DirectoryWalkerTest/ResultCacheManagerCollectedDataTest, both green when run alone).make phpstanis green, andturbo-ext/tests/smoke.phpandsignature-parity.phppass with the strict build.