| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
…in scope-only walks - `NodeScopeResolver::processClosureNode()` walked a `use (&$x)` closure's body twice on every call: once in the by-ref convergence loop (deep context, `NoopNodeCallback`) and once more as the result walk (top-level context, gathering callback). With nested by-ref closures that doubling compounds - `tests/bench/data/bug-11283.php` turned its 10 closure nodes into 513 `processClosureNode()` calls, 212 of them for the innermost closure alone (now 254 and 70). - When the walk is scope-only (the node callback is a `NoopNodeCallback`, possibly behind `GatheringNodeCallback` layers) it emits no rule or collector output, so the convergence pass that settles the by-ref uses is already the result walk from the same entry scope. Such passes now run with the gathering callback and the settled one is reused instead of walking the body again. - Added `NodeScopeResolver::isScopeOnlyWalk()`, which unwraps `GatheringNodeCallback` layers the same way `FiberNodeScopeResolver` does. - Gathering is restarted at the top of every gathering walk, so a non-converging closure that falls back to the result walk does not gather the same returns, yields, execution ends, impure points and invalidate expressions twice. - `tests/bench/data/bug-11283.php` goes from ~2.25s to ~0.99s locally; the analysis output is unchanged for it, for the whole bench corpus and for PHPStan's own source. - Probed the sibling constructs on the same "throwaway pass then result walk" axis: the foreach/while/do-while/for convergence loops and `resolveBackwardGotoScope()` have the same shape, but reuse there is unsafe behind an enclosing gatherer (a loop body, unlike a closure body, shares the enclosing anonymous function reflection, so it would be gathered once per pass) and, once made safe by matching the result walk's statement context, it is a wash on the bench corpus - left unchanged. Arrow functions and closures without by-ref uses already walk their body exactly once.
|
on my local machine I can see this PR improves from 1,39s to 0,89s which is 0,1s faster than what #5857 would achieve. @ondrejmirtes are you fine with this PR, or should I wait for #5857 ? |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Summary
CI's phpbench run reported tests/bench/data/bug-11283.php regressing by +75%. The
benchmark's baseline was recorded while a closure-type cache still lived in
phpstanCachedTypes node attributes: because the parser cache retains ASTs, that
cache survived from one analyse() call to the next inside the same phpbench process,
so the measured iteration reused the warm-up iteration's closure types.
07e5f78 moved that cache into ClosureTypeResolver with a per-file reset — correct
for real runs, where every file is analysed once — which made the benchmark measure the
honest cold cost for the first time.
So the number to fix is the cold cost, and profiling it showed where it goes: the file's
10 closures accounted for 513 NodeScopeResolver::processClosureNode() calls — 212 of
them for the innermost closure alone — and 58% of the analysis was spent inside closure
body walks. The
driver is that a closure with by-ref use walks its body twice per call — once in
the convergence loop, once as the result walk — which multiplies with each level of
closure nesting.
This PR removes that doubling for walks that produce no rule output.
Changes
collector output. It unwraps GatheringNodeCallback layers before testing for
NoopNodeCallback, the same unwrapping FiberNodeScopeResolver::callNodeCallback()
already does.
with the gathering callback ($closureStmtsCallback) on the caller's storage, and
the pass that settles the convergence — either because processClosureScope()
reproduced the previous entry scope, or because the closure is immediately invoked —
is reused as $statementResult instead of walking the body one more time.
so the arrays hold exactly one walk's worth of returns, yields, execution ends,
impure points and invalidate expressions. This matters for the fallback path: a
closure that does not converge within LOOP_SCOPE_ITERATIONS still runs the result
walk, and it must not append to what the last convergence pass already gathered.
Probed and left unchanged:
"throwaway passes, then a result walk" shape, and 444 of them run under a scope-only
walk across the bench corpus (all reached through
tryProcessUnrolledConstantArrayForeach()). Reuse is not safe here in general: a
loop body, unlike a closure body, is not separated from an enclosing gatherer by its
own anonymous function reflection, so running the passes with the enclosing callback
would gather the body's nodes once per pass. Restricted to a bare NoopNodeCallback
it is safe, but making the settled pass match the result walk requires walking the
passes in the result walk's statement context, and that costs more than the saved
walk: measured over the bench corpus it is a wash (two files 1.5–2x faster,
bug-14996.php 17% slower, total unchanged). Not worth the risk.
convergence loop, nothing to reuse. PHP arrow functions capture by value, so the
by-ref convergence has no arrow-function counterpart at all.
Root cause
processClosureNode() splits a by-ref closure into a convergence loop that walks the
body with a NoopNodeCallback in deep statement context, and a result walk that walks
it again with the gathering callback in top-level context. The second walk exists only
to emit node callbacks and to gather the engine-facing data; the scope it starts from is
identical to the settled convergence pass's entry scope.
Nothing about that was wrong for a single closure, but the two walks are both full
walks of the body: a closure nested d levels deep is walked 2^d times. In
bug-11283.php that is 10 closure nodes turning into 513 processClosureNode() calls,
254 after this change (the innermost closure: 212 → 70). Walking is deterministic in the
entry scope — the same assumption
ab88714 already relies on to skip loop verification passes — so when a walk emits no
rule output, one of the two walks is pure duplication and the settled pass can stand in
for the result walk.
Test
closures with assertType() on the by-ref captures inside each level and after the
outermost one, plus nested by-ref closures inside a foreach and inside an
immediately-invoked closure (the second reuse path). It pins the inference this
optimization must not change; it produces identical types with and without the change,
which is the point — a reuse that picked the wrong pass, or gathered a body twice,
would move these types.
it goes from ~2.25s to ~0.99s (2.3x), with byte-identical analysis output.
green, and PHPStan's own source analyses to the same 0 errors in the same wall-clock
time as before.
Fixes phpstan/phpstan#15089