From ef189d6139099d60c34dbce0d48eb25dc1ed56de Mon Sep 17 00:00:00 2001 From: PurHur Date: Mon, 27 Jul 2026 09:58:54 +0200 Subject: [PATCH] Perf: skip the ++/-- resource guard when the value cannot be a resource (#23483) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- lib/JIT.php | 15 +- lib/JIT/IncDecResourceProvenance.php | 162 ++++++++++++++++++ .../cases/g07_incdec_resource_provenance.php | 55 ++++++ 3 files changed, 229 insertions(+), 3 deletions(-) create mode 100644 lib/JIT/IncDecResourceProvenance.php create mode 100644 test/differential/cases/g07_incdec_resource_provenance.php diff --git a/lib/JIT.php b/lib/JIT.php index d8f28301c24..20dbb0ef81e 100644 --- a/lib/JIT.php +++ b/lib/JIT.php @@ -15398,7 +15398,7 @@ private function compileIncDecOp(Block $block, OpCode $op, bool $increment, bool Variable::TYPE_VALUE === $read->type && (Variable::KIND_VARIABLE === $read->kind || $read->functionStaticGlobal) ) { - $this->guardIncDecResourceOperand($read, $increment); + $this->guardIncDecResourceOperand($read, $increment, $readOp); $readPtr = JIT\JitValueBox::valuePtrFromVariable($this->context, $read); $cur = $this->readIncDecValueBoxLong($read, $readPtr, $increment); $one = $cur->typeOf()->constInt(1, false); @@ -15435,7 +15435,7 @@ private function compileIncDecOp(Block $block, OpCode $op, bool $increment, bool } if (Variable::TYPE_NATIVE_LONG === $read->type && Variable::KIND_VARIABLE === $read->kind) { - $this->guardIncDecResourceOperand($read, $increment); + $this->guardIncDecResourceOperand($read, $increment, $readOp); $cur = $this->context->helper->loadValue($read); $one = $cur->typeOf()->constInt(1, false); $newLong = $increment @@ -15528,11 +15528,20 @@ private function readIncDecValueBoxLong( } /** Reject ++/-- on stream/dir handles (issue #6396, zend_operators.c). */ - private function guardIncDecResourceOperand(JIT\Variable $read, bool $increment): void + private function guardIncDecResourceOperand( + JIT\Variable $read, + bool $increment, + ?Operand $readOp = null + ): void { if (JIT\NestedJitCompileScope::isActive()) { return; } + // A value that provably came from a literal or from arithmetic cannot be a resource handle, + // so the guard is dead code — and it is expensive enough to dominate hot loops (#23483). + if (JIT\IncDecResourceProvenance::cannotBeResource($readOp)) { + return; + } $longVal = null; if (JIT\Variable::TYPE_NATIVE_LONG === $read->type) { $longVal = $this->context->helper->loadValue($read); diff --git a/lib/JIT/IncDecResourceProvenance.php b/lib/JIT/IncDecResourceProvenance.php new file mode 100644 index 00000000000..f2d78d37264 --- /dev/null +++ b/lib/JIT/IncDecResourceProvenance.php @@ -0,0 +1,162 @@ + 11ms on build/micro/m_loop.php once elided). It also blocks + * every LLVM optimisation, because an opaque call in the loop body stops the counter allocas from + * being promoted out of memory. + * + * Resources are only ever *introduced* by a small set of builtins (fopen, opendir, proc_open, ...). + * A value flowing from an integer literal or from arithmetic therefore cannot be one, and the guard + * is dead code there. This walks the php-cfg producers of the read operand to prove exactly that. + * + * The analysis is deliberately conservative: anything it does not recognise — a call, a parameter, + * a property or array read — answers "unknown" and keeps the guard. Only the listed + * value-producing ops, which cannot yield a resource under any input, allow it to be dropped. + */ +final class IncDecResourceProvenance +{ + /** + * Ops whose result is a number, string, bool, array or object — never a resource handle. + * + * Coalesce is deliberately absent: `$x ?? fopen(...)` produces whatever the right side does. + * Assign and Phi are handled separately because they forward another operand's provenance. + */ + private const NON_RESOURCE_OPS = [ + Op\Expr\BinaryOp\Plus::class => true, + Op\Expr\BinaryOp\Minus::class => true, + Op\Expr\BinaryOp\Mul::class => true, + Op\Expr\BinaryOp\Div::class => true, + Op\Expr\BinaryOp\Mod::class => true, + Op\Expr\BinaryOp\Pow::class => true, + Op\Expr\BinaryOp\ShiftLeft::class => true, + Op\Expr\BinaryOp\ShiftRight::class => true, + Op\Expr\BinaryOp\BitwiseAnd::class => true, + Op\Expr\BinaryOp\BitwiseOr::class => true, + Op\Expr\BinaryOp\BitwiseXor::class => true, + Op\Expr\BinaryOp\Concat::class => true, + Op\Expr\BinaryOp\Equal::class => true, + Op\Expr\BinaryOp\NotEqual::class => true, + Op\Expr\BinaryOp\Identical::class => true, + Op\Expr\BinaryOp\NotIdentical::class => true, + Op\Expr\BinaryOp\Greater::class => true, + Op\Expr\BinaryOp\GreaterOrEqual::class => true, + Op\Expr\BinaryOp\Smaller::class => true, + Op\Expr\BinaryOp\SmallerOrEqual::class => true, + Op\Expr\BinaryOp\Spaceship::class => true, + Op\Expr\BinaryOp\LogicalXor::class => true, + Op\Expr\UnaryMinus::class => true, + Op\Expr\UnaryPlus::class => true, + Op\Expr\BitwiseNot::class => true, + Op\Expr\BooleanNot::class => true, + Op\Expr\PreInc::class => true, + Op\Expr\PostInc::class => true, + Op\Expr\PreDec::class => true, + Op\Expr\PostDec::class => true, + Op\Expr\ConcatList::class => true, + Op\Expr\Array_::class => true, + Op\Expr\Cast\Int_::class => true, + Op\Expr\Cast\Double::class => true, + Op\Expr\Cast\String_::class => true, + Op\Expr\Cast\Bool_::class => true, + Op\Expr\Cast\Array_::class => true, + ]; + + /** Bound on the producer walk, so a pathological CFG cannot cost more than the guard saves. */ + private const MAX_VISITS = 64; + + public static function cannotBeResource(?Operand $op): bool + { + if (!$op instanceof Operand) { + return false; + } + $seen = []; + $budget = self::MAX_VISITS; + + return self::operandIsSafe($op, $seen, $budget); + } + + /** + * @param array $seen + */ + private static function operandIsSafe(Operand $op, array &$seen, int &$budget): bool + { + if (--$budget < 0) { + return false; + } + $id = spl_object_id($op); + if (isset($seen[$id])) { + // Loop back-edge. Resource-ness can only be *introduced* by a producing op, and a cycle + // introduces nothing, so this edge carries no counter-example: the remaining edges of + // the phi still have to prove the property on their own. + return true; + } + $seen[$id] = true; + + if ($op instanceof Operand\Literal) { + // Literals are parser-level scalars; a resource has no literal syntax. + return true; + } + + $producers = $op->ops ?? []; + if ([] === $producers) { + // No producer in this CFG — a parameter, a bound closure var, or an operand written + // somewhere the analysis cannot see. Keep the guard. + return false; + } + + foreach ($producers as $producer) { + if (!self::producerIsSafe($producer, $seen, $budget)) { + return false; + } + } + + return true; + } + + /** + * @param array $seen + */ + private static function producerIsSafe(Op $producer, array &$seen, int &$budget): bool + { + if (--$budget < 0) { + return false; + } + if (isset(self::NON_RESOURCE_OPS[$producer::class])) { + return true; + } + if ($producer instanceof Op\Phi) { + foreach ($producer->vars as $var) { + if (!$var instanceof Operand || !self::operandIsSafe($var, $seen, $budget)) { + return false; + } + } + + return true; + } + if ($producer instanceof Op\Expr\Assign) { + $expr = $producer->expr ?? null; + + return $expr instanceof Operand && self::operandIsSafe($expr, $seen, $budget); + } + + return false; + } +} diff --git a/test/differential/cases/g07_incdec_resource_provenance.php b/test/differential/cases/g07_incdec_resource_provenance.php new file mode 100644 index 00000000000..2233f4226c8 --- /dev/null +++ b/test/differential/cases/g07_incdec_resource_provenance.php @@ -0,0 +1,55 @@ + phi -> ++ -> phi +function counted(): int +{ + $acc = 0; + for ($i = 0; $i < 5; ++$i) { + ++$acc; + } + + return $acc; +} +echo counted(), "\n"; + +// arithmetic provenance, post/pre forms, and a decrement below zero +$x = 10 - 7; +$x++; +$y = 2 * 3; +$y--; +echo "$x $y\n"; + +$n = 0; +$n--; +$n--; +echo $n, "\n"; + +// resources still usable after all that incrementing +fwrite($fh, 'alpha'); +rewind($fh); +echo fread($fh, 5), "\n"; +fwrite($fh2, 'beta'); +rewind($fh2); +echo fread($fh2, 4), "\n"; +fclose($fh); +fclose($fh2); +echo "done\n";