Repository navigation
Speed up the falsey isset() fold over many optional offsets - #6971
Merged
Merged
Conversation
Removing a union peeled its members one at a time. From mixed, each member was first asked whether it is a supertype of the mixed the earlier members left, which walks everything subtracted so far - for a hasOffset() member that is isOffsetAccessible() of a mixed subtracting a whole intersection of hasOffset() types, once per member. The falsey isset() of the bench's 26-key shape removes such unions from the subject's native mixed type, so that cost grew quadratically with the key count. mixed is the top type, so removing a member is exactly subtracting it unless it is subtracted already, and the members of a union are distinct, so only what mixed subtracted before can cover one. The members mixed does not subtract yet are now subtracted together, with no change in the inferred types of the test suite or the bench. Amp-Thread-ID: https://ampcode.com/threads/T-01a1b134-ed02-7078-a72e-a1e527d957a3
The fold of a multi-subject isset() in a non-true context asked every step's narrowing for the step's falsey scope, then the next step wrapped it in a fresh memo and asked it again as its left operand - so every step was computed twice. Each step is now memoized where it is built and the same memo serves both asks. Amp-Thread-ID: https://ampcode.com/threads/T-01a1b134-ed02-7078-a72e-a1e527d957a3
IntersectionType::tryRemove() intersected the members again after
removing the type from each of them, even when no member changed -
re-reducing every pair of members only to arrive at the same
intersection. Removing hasOffset('k') from an intersection of many
other hasOffset() types never changes a member, and the falsey isset()
of many optional offsets does that once per offset.
Amp-Thread-ID: https://ampcode.com/threads/T-01a1b134-ed02-7078-a72e-a1e527d957a3
…ject ObjectType::isOffsetAccessible() is yes for an ArrayAccess object whatever the extra classes say, but it asked about each of them anyway. Every hasOffset() compared with an ArrayAccess object - the default base type of hasOffset() is array|ArrayAccess - paid for it. Amp-Thread-ID: https://ampcode.com/threads/T-01a1b134-ed02-7078-a72e-a1e527d957a3
processArrayAccessoryTypes() turned each hasOffset() member of an array intersection into hasOffsetValue() with the intersection's iterable value type, asking for it once per member. It is the same for every member, and the intersection computes it from all of its members, so a union with an array of n hasOffset() members cost O(n^2). Amp-Thread-ID: https://ampcode.com/threads/T-01a1b134-ed02-7078-a72e-a1e527d957a3
Removing a union from a union first removes the whole of it from each member. When no member changed, doRemove() went on to remove the members of the removed union one at a time anyway - repeating the same removals from every member, and asking each removed member about the whole union in between, which describes it for ObjectType's cache key. Removing a part of what changed no member cannot change one either. The falsey isset() of many optional offsets removes the intersection of their hasOffset() types from the union of shapes each such check leaves. Amp-Thread-ID: https://ampcode.com/threads/T-01a1b134-ed02-7078-a72e-a1e527d957a3
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-15061.phpfast again after 77527df "Narrow falsey isset() on an optional constant-array offset", without changing what that narrowing infers.Cause
!isset($a['k'])on an optional, non-nullable key now removesHasOffsetType('k')from the subject. For the bench's 26-argumentisset()on an all-optional 26-key shape, that gives every step of the falsey conjunction fold a removal for$entity. The removal grows into(non-empty-array&hasOffset(a..i))|(ArrayAccess&hasOffset(a..i)), and it is applied at every step. With bleedingEdge, the branch-union augment also turns the subject into the union of the per-key shapes. Profiling showed that the extra time went mostly into these generic paths, not into that union of shapes:TypeCombinator::doRemove()removed a union frommixedone member at a time. It asked each member about themixedthe earlier members left, which forhasOffset()meansMixedType::isOffsetAccessible()over everything already subtracted, once per member. That is quadratic. Without bleedingEdge this is the dominant cost (the subject's native type ismixed).IntersectionType::tryRemove()intersected the members again even when removing changed none of them.doRemove()peeled the removed union off member by member anyway. That repeats the same removals, andObjectType::isSuperTypeOf()describes the whole union of shapes for its cache key along the way.ObjectType::isOffsetAccessible()asked about the extra offset-accessible classes even for anArrayAccessobject.array|ArrayAccessis the default base type ofhasOffset(), so every such comparison paid for it.TypeCombinator::processArrayAccessoryTypes()asked an intersection for its iterable value type once perhasOffset()member.The extra conditional-expression guards for
$okare about 1% of the cost.Changes
One commit per change, each with its
turbo-extmirror:mixedis the top type, so removing a member is exactly subtracting it, unless it is subtracted already. Members thatmixedalready subtracts are skipped, and the rest are subtracted together. The skip keepsmixed-subtract.phpunchanged: without it, re-unioningarray{}into the subtractedarray<mixed, mixed>describes it asarray<mixed>.isInstanceOf(ArrayAccess)->or(isExtraOffsetAccessibleClass()).TrinaryLogic::or()takes the maximum, and yes is the maximum, so when the first operand is yes the result is yes whatever the second one says.isExtraOffsetAccessibleClass()only reads class reflection and has no side effects, so skipping the call changes nothing else either. The C++ mirror returns early the same way.hasOffset()member, so it is now fetched lazily, once per array type.Overlap with another PR
Commit 1 overlaps with ac73303e0 "Subtract a union from plain mixed at once" in another PR. Both change how
TypeCombinator::doRemove()removes a union frommixed. That version was deliberately narrowed to plain mixed minus a plain union, because a broader one changed a description inmixed-subtract.php. This one applies to any non-templatemixed(with or without subtracted types) and keeps that test unchanged by skipping members the subtracted type already covers. Whichever of the two lands second should be rebased onto the other.Numbers
Net instruction counts are file minus
_empty.php, measured with cachegrind via a sweep of all 86tests/bench/datafiles (bleedingEdge, level 8, opcache on, JIT off, PHP 8.4.27). Before is origin/2.3.x bcca2b0, after is this branch. Only the files listed changed by 1% or more. No file got slower by more than 0.03%.bug-15061.phpbug-14319.phpbug-14972.phpimpure-call-columns.phpwordpress-user.phpbug-6265.phpbug-12159.phpbug-13352.phpbug-14624.phpbug-11283.phpbug-14207.phpPer commit on
bug-15061.php(measured before rebasing onto bcca2b0, same method): 6.53 → 6.53 (commit 1 only matters without bleedingEdge; there it measured 10.55 → 5.48 G) → 5.63 → 5.41 → 4.91 → 4.54 → 4.09 G. For reference, head with 77527df reverted measures 3.23 G, and this branch with 77527df reverted measures 2.56 G. So the narrowing itself still costs about 1.5 G, down from 3.3 G. Most of what remains is TypeCombinator re-normalizing the growinghasOffset()intersection at each fold step, plus the branch-union work the narrowing actually does.Wall-clock times
Median of 7 runs after 2 warm-up runs, using hyperfine with
php bin/phpstan analyse -c <config> --debug <file>.tests/bench/phpstan.neondoes not exist on 2.3.x, so the config includesconf/bleedingEdge.neonwith level 8, likeBenchCase. Machine: an Amp orb (E2B sandbox VM, Intel Xeon @ 2.60GHz, 16 vCPUs), PHP 8.4.27 NTS without the turbo extension. Each time includes about 1 s of boot. The run-to-run spread on this VM is about ±0.1 s, so only thebug-15061.phpdifference is clearly beyond noise. The instruction counts above are the reliable measure.bug-15061.phpbug-14319.phpbug-14972.phpimpure-call-columns.phpwordpress-user.phpbug-6265.phpbug-12159.phpbug-13352.phpbug-14624.phpbug-11283.phpbug-14207.phpTests
turbo-ext/phpstan_turbo.soloaded: green apart from two file-timing tests,DirectoryWalkerTest::testListingsSeeAddedAndRemovedFilesandResultCacheManagerCollectedDataTest::testFileChangedAndChangedBackDuringTheAnalysisIsAnalysedAgain. Each failed once under the 16-process load, in different runs, and passes 3/3 when run alone. They are unrelated to this change.turbo-ext/tests/smoke.php: ALL OK.turbo-ext/tests/signature-parity.php: OK.turbo-ext/bin/side-by-side.php: method parity passes. It also reports 59 generated headers as stale, and an untouched 2.3.x checkout reports the same 59 in this environment.dumpType()/dumpNativeType()checks of the falsey and truthy branches give byte-identical output to 2.3.x, with and without bleedingEdge, and with the turbo extension on and off.phpstan.neon.dist) are clean on the changed files.