Memoize the ExprHandler lookup by Expr class in specifyTypesInCondition() and processExprNode() - #5999
Conversation
…ndition()` and `processExprNode()` - `TypeSpecifier::specifyTypesInCondition()` and `NodeScopeResolver::processExprNode()` dispatched to the matching `ExprHandler` by scanning every service tagged `phpstan.exprHandler` and calling `supports()` on each until one matched. Both run once per arm of a boolean chain, so deep `!==`/`===` chains paid the full linear handler sweep per arm. - Cache the resolved handler keyed by the `Expr` class-string (`false` when none matches, meaning default narrowing). After the initial scan for a class, subsequent calls do an O(1) array lookup instead of re-running the `supports()` sweep. This recovers the ~30pp regression on `bug-14207-and.php` and `and-chain-truthy-blowup.php` introduced when `specifyTypesInCondition` was moved onto the `ExprHandler` dispatch. - The lookup is class-deterministic because the only handlers whose `supports()` depends on more than the class are the first-class-callable ones, and both dispatch sites filter/transform first-class callables away before reaching the handler loop. - Probed the third parallel dispatch site, `MutatingScope::resolveType()`: its results are already memoized per scope in `resolvedTypes` (keyed by expression string), so the handler sweep is not on the hot path there — a per-class cache measured no benefit, and `MutatingScope` is not a singleton, so it was left unchanged.
ondrejmirtes
left a comment
There was a problem hiding this comment.
No need for nsrt test here, it's already covered by a file in tests/bench/data.
Also what might be a problem is that not all ExprHandlers cleanly map to a class. For example FirstClassCallableFuncCallHandler has $expr instanceof FuncCall && $expr->isFirstClassCallable().
So if this is cached first and then a FuncCall (which isn't a first-class callable) is processed, a wrong handler might be passed.
Call-like handlers (FuncCall/MethodCall/StaticCall/New_) select on isFirstClassCallable(), so the Expr class does not uniquely determine the matching handler. Restrict the per-class handler cache to non-call-like expressions, where every handler's supports() is a pure instanceof check, so a class always maps to the same handler. This keeps the deep boolean chain speed-up (those arms are never call-likes) without risking a wrong handler being served from the cache. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The deep !==/=== chain behaviour is already exercised by tests/bench/data/bug-14207-and.php and and-chain-truthy-blowup.php. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both commits pushed. Here's a summary of how I addressed the review. Review point 1 — "not all ExprHandlers cleanly map to a class" (correctness)The reviewer is right. I audited all ~70 handlers' The original PR argued this was safe because both changed sites filter first-class-callables before the loop. But that relies on distant upstream code and is exactly the fragility the reviewer distrusted (and it genuinely is not true for the unmemoized Fix: exclude The perf recovery is fully preserved: the reported regression is deep Review point 2 — "no need for nsrt test"Removed Verification
Committed as two logical changes and pushed to |
ondrejmirtes
left a comment
There was a problem hiding this comment.
Error on PHP 7.4: Cannot use ::class with dynamic class name
$expr::class (::class on an object variable) is only supported since PHP 8.0. Replace it with get_class($expr) in the memoized ExprHandler lookups so the code parses and runs on PHP 7.4. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Done. I addressed the review comment. The problem: The fix: Replaced all four occurrences of Verification:
Committed as a single logical change and pushed to |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Done. The review asked me to run What it fixed: cs-fix detected 4 style errors (2 per file) — the Verification:
Committed and pushed to |
Summary
Analysing deep
!==/===chains got ~27–30 percentage points slower after the TypeSpecifier→ExprHandler dispatch refactor (commitsbb18f722eb…c128b84527), as reported for thebug-14207-and.phpandand-chain-truthy-blowup.phpbenches. That refactor movedspecifyTypesInCondition()onto the same container-tag dispatch already used byMutatingScope::resolveType(): for every call it iterates all services taggedphpstan.exprHandlerand callssupports()on each until one matches. BecausespecifyTypesInCondition()(andprocessExprNode()) run once per arm of a boolean chain, deep chains pay this whole linear handler sweep on every arm.The fix memoizes the resolved handler per
Exprclass-string so the sweep runs at most once per class, turning the per-arm dispatch into an O(1) array lookup. Inferred types are unchanged — this is a pure dispatch optimisation.Changes
src/Analyser/TypeSpecifier.php: add aclass-string<Expr> => ExprHandler|falsecache ($exprHandlersByClass) and use it inspecifyTypesInCondition(). On a cache miss the handler is resolved with the originalsupports()scan and stored;falserecords "no handler matched" (default narrowing).src/Analyser/NodeScopeResolver.php: apply the identical memoization to the parallel dispatch inprocessExprNode(), which walks each expression node during AST traversal (also once per boolean-chain arm).tests/PHPStan/Analyser/nsrt/bug-14919.php: regression test locking in that deep!==/===chains still narrow identically through the memoized dispatch (BooleanAnd/BooleanOr, truthy and falsey branches, plus thebug-14207assertType('string', …)shape).Root cause
The regression is a dispatch cost, not an algorithmic one: three sites resolve an
ExprHandlerby a lineargetServicesByTag()+supports()scan over ~40 handlers.MutatingScope::resolveType()already had this pattern before the regression window (sincea11b86dc8, 2026-03-07) and is shielded by per-scope result memoization, so it was not the regression. The June 2026 refactor added the same pattern to the far hotterspecifyTypesInCondition()path (and it shares theprocessExprNode()path), where it is invoked per boolean-chain arm with no memoization — hence the per-arm linear cost. Caching the handler byExprclass removes the repeated sweep. It is sound because, at both dispatch sites, the only handlers whosesupports()inspects more than the class (the first-class-callable handlers) are already filtered/transformed out before the loop, so a givenExprclass always maps to the same handler.Parallel dispatch sites audited:
TypeSpecifier::specifyTypesInCondition()— fixed (the reported regression).NodeScopeResolver::processExprNode()— fixed (same pattern, same boolean-chain hot path).MutatingScope::resolveType()— probed; already memoized per scope viaresolvedTypes, measured no benefit, andMutatingScopeis not a singleton service, so left unchanged.Test
tests/PHPStan/Analyser/nsrt/bug-14919.phpasserts the narrowed types of deep!==(BooleanAnd) and===/||(BooleanOr) chains in both branches, guarding that the memoized handler lookup keeps producing the correct handler and identical narrowing.NodeScopeResolverTest(1671),TypeSpecifierTest(114),LegacyNodeScopeResolverTest,Rules/Comparison(423),AnalyserIntegrationTest(184), and fullmake phpstanself-analysis.tests/bench/data/bug-14207-and.phpandand-chain-truthy-blowup.phpbenches. A local amplified A/B (20 functions × 100-arm!==chains) measured ~5.9s → ~4.0s (~30% faster), matching the reported regression.Fixes phpstan/phpstan#14919