Perf: skip the ++/-- resource guard when the value cannot be a resource (#23483) - #23781
Conversation
…ce (#23483) `for ($i = 0; $i < 1000000; ++$i) { ++$a; }` ran 4.4x slower than Zend. The generated code was not the problem — it is already ideal native code (`alloca i64`, `add i64`, `icmp slt i64`, no boxing anywhere). The problem was one call per `++`. Resource handles are stored *as* native longs here, and php-types has no resource type at all — `Type::TYPE_LONG` covers both an `int` and an open `fopen()` handle. So `guardIncDecResourceOperand()` (added for #6396) cannot tell a loop counter from a stream handle, and conservatively guards every single ++/-- with `__compiler_is_resource`. That lands in StreamLifecycleJitHelper::isResourceArgv(), which walks up to four handle registries. Two such calls per iteration here. Measured on build/micro/m_loop.php, best of 5, output verified identical (1000000) in every column: Zend 8.2 25 ms master 135 ms (4.4x slower than Zend) with this change 8 ms (3.1x FASTER than Zend) That guard was ~92% of the loop's runtime. It also blocked LLVM: an opaque call in the loop body stops the counter allocas being promoted out of memory, which is why the (opt-in, unused) PHP_COMPILER_OPT_LEVEL pipeline bought only 6% before and 36% after. The claim in Context::runModuleOptimizationPasses()'s docblock — that missing IR optimisation is "the shape behind an untyped ++$a loop running ~12x slower than Zend" — is not what the measurement shows; the opaque call was. Rather than drop the guard, IncDecResourceProvenance proves when it is dead. Resources are only ever *introduced* by a few builtins, so a value flowing from a literal or from arithmetic cannot be one. It walks the php-cfg producers of the read operand, recursing through Phi and Assign, and answers "unknown" — keeping the guard — for calls, parameters, properties and array reads. Phi back-edges are treated as safe because a cycle introduces no new resource-ness; the other edges still have to carry the proof themselves. The walk is bounded so a pathological CFG cannot cost more than the guard saves. Gates (no CI on lib/, per AGENTS.md): - script/differential-sweep.sh --dir test/differential/cases: 53/53 match Zend, exit 0 - script/differential-sweep.sh --aot: failing-case NAMES identical to master, 27 both sides, set difference empty in both directions. Those 27 are pre-existing and now filed as #23779. - g07 (new) covers the shape this could break: ++/-- across loop phis and arithmetic while two fopen() handles are live, with integers colliding with plausible handle ids. Passes VM and AOT. Not fixed here, and deliberately not credited to this change: - #23777 ++ on a real resource silently succeeds at top level (pre-existing; verified by rebuilding the case with lib/JIT.php stashed back to master) - Ack(3,n) is fully typed and still ~3x slower — a different cause, not yet diagnosed
Correction: the
|
lib/ at |
result |
|---|---|
544d1dca9 (before this PR) |
rc=134, SIGABRT, no output at all |
| this PR and later | partial output, rc=0, still wrong |
So the case was already catastrophically broken under AOT; eliding the guard lets it get further.
The change is not a regression here. But "not a regression" is not the same as the "passes AOT"
I claimed, and the merged commit message carries that false claim.
Real defects this exposed
The partial output is worth two follow-ups, both of which are the same design flaw this PR is
about (resource handles are stored as native longs, and php-types has no resource type) showing up
in paths I did not touch:
Resource id #2for the integer2. String interpolation of a long consults the resource
registry, so an ordinary integer whose value collides with a livefopen()handle renders as a
resource.$ais2here only because++$aran on$a = 1.$n = 0; $n--; $n--;prints0, not-2, and output truncates immediately after.
Filing both separately. The performance measurements in this PR (135ms -> 8ms on
build/micro/m_loop.php) are unaffected by any of this — that program opens no resources and was
verified by output comparison.
Maintainer merge: docs-only bench table regenerate + honest reading guide after #23781. No lib/ changes; pillar-1 gates green on host (inventory 6588/6588, user_release_ready=yes).
) (#23844) #23781 elided the resource guard via IncDecResourceProvenance::cannotBeResource() inside guardIncDecResourceOperand(), which made post-decrement a silent no-op for TYPE_VALUE KIND_VALUE locals that fell through to generic binaryOp lowering. Route scoped boxed locals through the value-box inc/dec path (isIncDecValueBoxLvalue) and remove the in-guard cannotBeResource early return so guard CFG matches Zend. cannotBeResourceForString() remains for int→string echo (#23811). Verified: php bin/compile.php -o /tmp/t '<?php $n=5; $n--; echo $n;' && /tmp/t # 4 script/differential-sweep.sh --aot --dir test/differential/cases (g08) vendor/bin/phpunit --filter incdec_post_dec_local Co-authored-by: PurHur <PurHur@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…3840, #23841) #23781 elided the ++/-- resource guard for values that provably cannot be resources. That bailed out of guardIncDecResourceOperand() before its two ensureLinked() calls, and the script-scope store path depends on those units being linked. Result: ++/-- silently stopped taking effect at script scope, in every AOT binary since it merged. $n = 5; $n--; kept 5 $acc=0; for ($i=0;$i<5;++$i){++$acc;} yielded 0 Silent wrong output on the most basic loop in PHP. Mine. Fix: keep ensureLinked() unconditional, and fold the *expensive* part — nativeLongIsResource(), which calls __compiler_is_resource and walks four handle registries — to a constant false when the operand provably cannot be a resource. LLVM then deletes the unreachable error arm. The mechanism took elimination to find, because the obvious candidates are all wrong: - not the load's materialisation — bailing out after loadValue() still broke it - not the basic block split — keeping the split with a constant-false branch but skipping ensureLinked() still broke it - not the call site — restricting to the typed sites did not help; script-scope operands take the TYPE_VALUE+functionStaticGlobal site - not resource interaction — cases with no fopen() fail identically Folding rather than gating on scope also fixes #23841, which an earlier version of this fix would have reintroduced: gating meant the real check ran on plain counters, so with a live fopen() handle `++$acc` raised a false "Cannot increment resource" once a counter's value matched a handle id. Gates: - VM sweep: 57/57 match Zend, exit 0 - 13 standalone reproducers all match Zend: plain --, pre/post forms, loops, with and without a live resource, and the #23841 collision shape (d6, g07a) which now passes - g08 (new, top-level) and g07a both ok under AOT - m_loop.php correct at 7ms (135ms unfixed), Ack(3,9) correct — #23483's win intact New case g08_toplevel_incdec.php is deliberately top-level: EVERY pre-existing ++/-- differential case declares its variables inside a function, which is why this shipped. It records what it excludes and why — two script-scope variables plus interpolation still intermittently heap-corrupts as #23842, which reproduces with the guard force-disabled and so predates all of this. How this shipped: #23781's gates were VM 53/53 and an AOT failing-name diff, both clean and both blind here. g07, the one case with top-level ++/--, was never actually run by the sweep I cited — corrected on #23781 — and is skip-aot pending #23811. A false gate claim plus a coverage gap.
…ollow-on) #23840 made every script-scope ++/-- a no-op. The cases added with its fix gate two shapes: plain post-decrement (g08_incdec_post_dec) and ++ inside a script-scope loop (g08_toplevel_incdec_echo). Neither covers: --$n pre-decrement at script scope $n++; $n++; $n--; a run mixing both forms on one variable $z--; past zero the sign flip --$p vs $q-- both forms storing the same value All of those were equally broken while #23840 was live, and nothing exercises them today. Kept top-level deliberately: the regression only ever appeared at script scope, because every pre-existing ++/-- differential case declares its variables inside a function. That gap is what let it ship. Verified on master f98ba35, both backends explicitly: VM — run directly against bin/vm.php: matches Zend (4 / 2 / 3 / -2 / 9 9) AOT — ok in script/differential-sweep.sh --aot Checked VM by hand rather than reading it off a sweep: the sweep run that covered AOT had already globbed its case list before this file existed, so its VM section does not include it. Inferring a pass from absence is exactly the mistake that let #23840 through (see the correction on #23781). Test only, no lib/ changes.
…ollow-on) (#23867) #23840 made every script-scope ++/-- a no-op. The cases added with its fix gate two shapes: plain post-decrement (g08_incdec_post_dec) and ++ inside a script-scope loop (g08_toplevel_incdec_echo). Neither covers: --$n pre-decrement at script scope $n++; $n++; $n--; a run mixing both forms on one variable $z--; past zero the sign flip --$p vs $q-- both forms storing the same value All of those were equally broken while #23840 was live, and nothing exercises them today. Kept top-level deliberately: the regression only ever appeared at script scope, because every pre-existing ++/-- differential case declares its variables inside a function. That gap is what let it ship. Verified on master f98ba35, both backends explicitly: VM — run directly against bin/vm.php: matches Zend (4 / 2 / 3 / -2 / 9 9) AOT — ok in script/differential-sweep.sh --aot Checked VM by hand rather than reading it off a sweep: the sweep run that covered AOT had already globbed its case list before this file existed, so its VM section does not include it. Inferring a pass from absence is exactly the mistake that let #23840 through (see the correction on #23781). Test only, no lib/ changes. Co-authored-by: PurHur <tedyyyyy@gmail.com>
What
++/--emit a__compiler_is_resourcecall on every native long. This drops it where thevalue provably cannot be a resource.
Why it is slow
Resource handles are stored as native longs here, and php-types has no resource type —
Type::TYPE_LONGcovers both anintand an openfopen()handle. SoguardIncDecResourceOperand()(#6396) genuinely cannot tell a loop counter from a stream handle,and guards every ++/--. The guard calls
StreamLifecycleJitHelper::isResourceArgv(), which walksup to four handle registries — twice per iteration in a
forloop with a counter and an accumulator.The generated code was never the problem.
loop()already lowers to ideal native code:...it is just preceded by
call i32 @__compiler_is_resource(i64 %8)every time.Measured
build/micro/m_loop.php, best of 5, inphp-compiler:22.04-dev/ LLVM 9. Output verifiedidentical (
1000000) in every column:~92% of the loop's runtime. The guard also blocked LLVM — an opaque call in the loop body stops the
allocas being promoted — so
PHP_COMPILER_OPT_LEVEL=3was worth 6% before and 36% after.Related:
Context::runModuleOptimizationPasses()'s docblock says missing IR optimisation is "theshape behind an untyped
++$aloop running ~12x slower than Zend". The measurement does notsupport that; the opaque call was the cause. Left alone here to keep this diff scoped.
How the elision stays sound
Resources are only ever introduced by a few builtins, so a value flowing from a literal or from
arithmetic cannot be one.
IncDecResourceProvenancewalks the php-cfg producers of the readoperand and returns "cannot be a resource" only for a whitelist of ops that cannot yield one,
recursing through
PhiandAssign. Everything else — calls, parameters, properties, arrayreads,
AssignRef,Coalesce— answers "unknown" and keeps the guard.Phi back-edges are treated as safe: resource-ness can only be introduced by a producing op, and a
cycle introduces none, so the remaining edges still have to carry the proof. The walk is bounded
(64 visits) so a pathological CFG cannot cost more than the guard saves.
Gates
There is no CI on
lib/(AGENTS.md §1), so these were run locally:script/differential-sweep.sh --dir test/differential/cases→ 53/53 match Zend, exit 0sides, set difference empty in both directions — no regressions, no accidental fixes. Those
27 are pre-existing and now filed as AOT differential sweep: 27 of 53 cases fail on master (half the least-covered path is untested-broken) #23779.
g07covers exactly what this could break: ++/-- across loop phis and arithmeticwhile two
fopen()handles are live, with integers colliding with plausible handle ids(1, 2, 3). Passes VM and AOT.
Explicitly not claimed
++on a real resource silently succeeds at top level. Pre-existing; verified byrebuilding that case with
lib/JIT.phpstashed back to master, which reproduces identically.Filed separately so this change is not credited with fixing it.
Ack(3,n)is fully typed (function Ack(int $m, int $n): int) and still ~3× slower thanZend. It contains no
++/--, so this change does not touch it. Different cause, not yetdiagnosed — which also corrects an earlier framing of Perf: generated code is 4-14x SLOWER than Zend on basic loops and calls #23483 as purely "typed vs untyped".