Repository navigation
Decide sealed array shapes with disjoint required keys before comparing values - #6986
Open
ondrejmirtes wants to merge 2 commits into
Open
ondrejmirtes wants to merge 2 commits into
ondrejmirtes wants to merge 2 commits into
Conversation
…ng values
ConstantArrayType::isSuperTypeOf() walks the keys of $this in order and
only reports that the other shape lacks a required key when it reaches
that key - after asking the other shape for the offset and comparing the
value of every key before it. Two sealed shapes where one has a required
key the other lacks share no value, and that is now decided from the key
sets first. The answer is the same No; when a value mismatch also comes
earlier in key order, the reason is now the sealed-shapes one instead of
the offset's.
hasOffsetValueType() and getOffsetValueType() also scanned every key with
isSuperTypeOf() for a constant offset the key index has no entry for,
which can only ever find nothing. A non-compound constant int or string
offset missing from the index now skips that scan.
A chain of `if ($pN !== null) { $data['kN'] = $pN; }` builds a sealed
shape under bleedingEdge, and every if-merge rechecks the growing list of
conditional holders guarded by earlier shapes of $data against the
branch's new shape (preserveVacuousConditionalExpressions()). With 90
ifs that took 1.8 s in those checks alone; each check is now a few key
lookups.
Amp-Thread-ID: https://ampcode.com/threads/T-01a1b24f-ec49-7194-bbd2-0f4a43ab9b8f
The shortcut in ConstantArrayType::isSuperTypeOf() only applies when both shapes are sealed: a shape of unknown sealedness accepts extra keys, so a required key it lacks decides nothing. isKeyIndexMiss() no longer asks isConstantScalarValue() first - a non-compound type has a single int or string constant scalar value only when it is that constant. Amp-Thread-ID: https://ampcode.com/threads/T-01a1b24f-ec49-7194-bbd2-0f4a43ab9b8f
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.
Fixes the bleedingEdge slowdown of long
if ($pN !== null) { $data['kN'] = $pN; }chains (introduced with the if/elseifpreserveVacuousConditionalExpressions()from #5056; #5848 only fixed the non-bleedingEdge side).With sealed array shapes (bleedingEdge), each of these ifs leaves conditional holders guarded by
$data's earlier, sealed shape (e.g.$p5 is null when $data is array{k0?: string, …, k4?: string}). The holders are sound and still useful (if ($data === [])narrows every$pNtonull), so every if-merge keeps them: the branch that writes$dataalways holds the new required key that the guard lacks. That makes O(N) holders to recheck at each of N merges, and each recheck,ConstantArrayType::isSuperTypeOf()between two sealed shapes, was itself quadratic:isSuperTypeOf()walks$this's keys in order and only says No when it reaches the required key the other shape lacks. Here that's the last one, after ahasOffsetValueType()/getOffsetValueType()and a value comparison for every key before it. Two sealed shapes where one has a required key the other lacks share no value, and that is now decided from the key sets first.hasOffsetValueType()andgetOffsetValueType()fell back to anisSuperTypeOf()scan over all keys for a constant offset with no entry in the key index, a scan that can only find nothing. A non-compound constant int/string offset missing from the index now skips it.Behavior: every Yes/Maybe/No answer is unchanged. In one case the reported reason changes: two sealed shapes that differ in a required key and also have a value mismatch earlier in key order now get the "Sealed array shapes … cannot be intersected" reason instead of the offset's. The turbo-ext mirror is updated the same way. The type-family differential gets sealed subjects covering that case; disabling the native check makes
tests/smoke.phpfail on exactly those pairs. Smoke test, signature parity,side-by-side.php(PHP 8.5),make testswith and without the extension, and raw output identity with the extension on/off all pass.make phpstanis green.Regression bench:
tests/bench/data/sealed-shape-if-chain.php(the 90-key reproducer).Timings
Best of 5, wall time / peak RSS;
analyse -l 8 --debug,phpVersion: 80500, PHP 8.4.27 without the turbo extension. 2.1.34 and 2.3.0 are the release PHARs; 2.3.x-dev is 8af6dd7 run from source. The empty-file row shows the startup cost, which differs between the PHARs and source.?stringkeys, bleedingEdge?stringkeys, bleedingEdge?stringkeys, no bleedingEdge$params[] = 'pN'ifs (list), bleedingEdgeThe list case doesn't depend on bleedingEdge (0.92 s without it on 2.3.x-dev) and is a separate cost that this PR doesn't touch: about 0.25 s of conditional-expression work across its 80 merges, which end with only 4 holders.