Run indentation detection, node cloning and node replacing in a single AST traversal and cache per-file fixing data#6101
Conversation
|
Done — pushed as Change: removed all four files the PR added under
The PR is now source-only: Verification:
One thing worth flagging for your call: the merged-traversal behaviour is still covered end to end by the existing fix tests ( |
|
The last monitor has ended. Everything is finished: the review request is addressed by |
|
before this PR after this PR |
…e AST traversal and cache per-file fixing data * `RuleErrorTransformer::transform()` used to run three separate `NodeTraverser` passes over the whole file AST for every fixable error (`PhpPrinterIndentationDetectorVisitor`, `CloningVisitor`, `ReplacingNodeVisitor`). They now share one traversal. * `ReplacingNodeVisitor` does the replacement in `leaveNode()` instead of `enterNode()`, so the callable still receives a node whose children have already been cloned by `CloningVisitor` - keeping the produced diffs byte-identical while allowing both visitors in one pass. * `PhpPrinterIndentationDetectorVisitor` no longer returns `STOP_TRAVERSAL` (which would have aborted the shared traversal); it exposes a `found` flag and skips further nodes once the indentation is known. * The fixed file is read, parsed and tokenized once per file instead of once per error. `FileReader::read()`, `Parser::parse()`, the sha256 hash and the detected indentation are kept in a small FIFO cache bounded by `PARSED_FILES_LIMIT`. * `PhpPrinter` instances are reused per indentation string (bounded by `PRINTERS_LIMIT`) - `printFormatPreserving()` memoizes eight sizeable lookup tables on first use, which were rebuilt for every single error before. * Fixing is skipped early when no file nodes are available (`AnalyserResultFinalizer` and `ConstantConditionInTraitHelper` pass an empty array), which previously still read and parsed the file just to find nothing to replace. * Probed `PHPStan\Fixable\PhpDoc\PhpDocEditor`, which has the same two-pass clone-then-callback shape over the PHPDoc AST. Left unchanged on purpose: merging the passes would flip the user callback from pre-order to post-order on an `@api` class, and the PHPDoc ASTs involved are tiny. Closes phpstan/phpstan#14996
The behaviour is already covered end to end by the existing fix tests (FixIssetRuleTest, OverridingMethodRuleTest::testFixWithTabs, ...). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
100 methods wrongly marked with #[\Override] produce 100 fixable errors in a single ~1000 line file, which is the shape that makes RuleErrorTransformer::transform() dominate the analysis. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6a094cb to
3747595
Compare
staabm
left a comment
staabm
left a comment
There was a problem hiding this comment.
Also worth recording for a follow-up (intentionally not changed here): with the traversals fixed, the remaining cost of transform() is dominated by SebastianBergmann\Diff\Differ::diffToArray(), whose getArrayDiffParted() builds the common suffix with $end = [$k => $v] + $end — quadratic in the number of lines after the change (154ms for a change near the top of a 5k-line file vs 14ms near the bottom)
@phpstan-bot suggest how getArrayDiffParted() can be improved for performance, without changing any files in the PR. just send a diff in a comment.
|
Nothing in the PR changed — the reviewer asked for a proposal only. Working tree is clean, vendor restored to pristine. Here's what I found and verified: Comment for the PR
Two independent O(n²) loops1. Suffix loop — 2. Prefix loop — subtler and previously unnoticed.
Both loops are quadratic in the length of the unchanged region — which for a one-node fix is essentially the whole file. That's the asymmetry I measured: a change near the top pays it in the suffix loop, near the bottom in the prefix loop. Proposed patchCompute the common prefix/suffix lengths by integer index and slice once. No internal array pointer, no incremental array unions. --- a/src/Differ.php
+++ b/src/Differ.php
@@ -12,17 +12,15 @@
use const PHP_INT_SIZE;
use const PREG_SPLIT_DELIM_CAPTURE;
use const PREG_SPLIT_NO_EMPTY;
+use function array_keys;
use function array_shift;
+use function array_slice;
use function array_unshift;
use function array_values;
use function count;
-use function current;
-use function end;
use function is_string;
-use function key;
use function min;
use function preg_split;
-use function prev;
use function reset;
use function str_ends_with;
use function substr;
@@ -207,41 +205,40 @@
private static function getArrayDiffParted(array &$from, array &$to): array
{
- $start = [];
- $end = [];
+ $fromKeys = array_keys($from);
+ $toKeys = array_keys($to);
+ $fromLength = count($fromKeys);
+ $toLength = count($toKeys);
+ $shortest = min($fromLength, $toLength);
- reset($to);
-
- foreach ($from as $k => $v) {
- $toK = key($to);
+ $prefixLength = 0;
- if ($toK === $k && $v === $to[$k]) {
- $start[$k] = $v;
+ while ($prefixLength < $shortest) {
+ $key = $fromKeys[$prefixLength];
- unset($from[$k], $to[$k]);
- } else {
+ if ($toKeys[$prefixLength] !== $key || $from[$key] !== $to[$key]) {
break;
}
- }
- end($from);
- end($to);
+ $prefixLength++;
+ }
- do {
- $fromK = key($from);
- $toK = key($to);
+ $suffixLength = 0;
- if (null === $fromK || null === $toK || current($from) !== current($to)) {
+ while ($suffixLength < $shortest - $prefixLength) {
+ if ($from[$fromKeys[$fromLength - $suffixLength - 1]] !== $to[$toKeys[$toLength - $suffixLength - 1]]) {
break;
}
- prev($from);
- prev($to);
+ $suffixLength++;
+ }
- $end = [$fromK => $from[$fromK]] + $end;
- unset($from[$fromK], $to[$toK]);
- } while (true);
+ $start = array_slice($from, 0, $prefixLength, true);
+ $end = array_slice($from, $fromLength - $suffixLength, $suffixLength, true);
+ $from = array_slice($from, $prefixLength, $fromLength - $prefixLength - $suffixLength, true);
+ $to = array_slice($to, $prefixLength, $toLength - $prefixLength - $suffixLength, true);
return [$from, $to, $start, $end];
}
}
Numbers
Scaling (change on line 0) — clean O(n²) → O(n):
Full PHPStan end-to-end, on top of this PR:
The gain scales with file length, so it's small on the 1k-line bench case and dominant on real large files. Correctness checks
Happy to open the PR against Note on scope: no commits — the review asked for a suggestion without touching the PR. The |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
3 participants
Summary
RuleErrorTransformer->transform()walked the whole file AST three times and re-read, re-parsed and re-tokenized the file for every single fixable error. On files with many fixable errors this dominated the run.The three
NodeVisitorimplementations now share a singleNodeTraverserpass, and everything that only depends on the file being fixed (contents, tokens, hash, detected indentation, printer) is computed once per file instead of once per error.Measured on a stress case (200 fixable errors in
src/Analyser/MutatingScope.php, ~5k lines): 59.8s → 41.4s, with the AST-traversal portion dropping from ~22s to 3.3s. On a normal-sized file (src/Rules/Methods/OverridingMethodRule.php, 200 errors): 2.31s → 1.20s.Changes
src/Analyser/RuleErrorTransformer.phpfix()method.CloningVisitorandReplacingNodeVisitorare registered on oneNodeTraverserinstead of three.PARSED_FILES_LIMIT) holding the file contents, its sha256 hash, its tokens and the detected indentation, soFileReader::read()andParser::parse()run once per file.PhpPrintercache keyed by the indentation string (PRINTERS_LIMIT).$fileNodesis empty — nothing can be replaced, so reading and parsing the file was pure waste.src/Fixable/ReplacingNodeVisitor.php— replacement moved fromenterNode()toleaveNode().src/Fixable/PhpPrinterIndentationDetectorVisitor.php— replacedNodeVisitor::STOP_TRAVERSALwith a publicfoundflag and an early return.Analogous cases probed
src/Fixable/PhpDoc/PhpDocEditor::edit()— structurally the same two-pass shape (CloningVisitortraversal followed by aCallbackVisitortraversal), but over a PHPDoc AST. Deliberately left unchanged: the sameleaveNode()trick would turn the third-party callback from pre-order into post-order on an@api-tagged class, and the ASTs involved are a handful of nodes, so there is nothing to gain.src/Fixable/Patcher::applyDiffs()— also does repeated splitting/diffing, but it runs once per file at the end of a--fixrun, not once per error, so it is not on the hot path.src/Parser/RichParser,src/Node/DeepNodeCloner,src/Dependency/ExportedNodeFetcher— checked; their traversals are either already merged into one pass or ordered by necessity, and none of them run per error.Also worth recording for a follow-up (intentionally not changed here): with the traversals fixed, the remaining cost of
transform()is dominated bySebastianBergmann\Diff\Differ::diffToArray(), whosegetArrayDiffParted()builds the common suffix with$end = [$k => $v] + $end— quadratic in the number of lines after the change (154ms for a change near the top of a 5k-line file vs 14ms near the bottom). Working around it would mean trimming the input and rewriting the@@hunk headers, which is too risky to bundle into this change since those diffs get applied to users' files.Root cause
The pattern is per-error work that only depends on the file. Every call to
transform()for aFixableNodeRuleErrorredid all of it:FileReader::read($fixingFile)— file I/O.$this->parser->parse($oldCode)— a full re-parse of the file, only to getgetTokens().new TokenStream($oldTokens, ...)— an indentation map over every token.new PhpPrinter(...)—printFormatPreserving()memoizes eight lookup tables (fixup map, removal map, insertion maps, modifier change map, …) on the printer instance, so a fresh instance per error rebuilt all of them.Only steps 5 and 6 genuinely depend on the individual error, and even those can share one traversal.
Merging 5 and 6 naively is unsafe:
CloningVisitor::enterNode()clones a node before its children are cloned, so a replacement callable running inenterNode()would see un-cloned children, and any brand-new node returned by the callable would get its children cloned and tagged with anorigNodeattribute pointing at nodes with no token positions — which breaks format-preserving printing. Doing the replacement inleaveNode()avoids this entirely: by then the traverser has already descended into the node and cloned its whole subtree, which is exactly the state the callable used to receive from the separate second pass.Merging 4 in required dropping
STOP_TRAVERSALfrom the indentation detector, since stopping would abort cloning as well.Test
tests/PHPStan/Analyser/RuleErrorTransformerTest.php(new), withtests/PHPStan/Analyser/CountingParser.php(aPhpParser\Parserdecorator countingparse()calls) and data files undertests/PHPStan/Analyser/data/rule-error-transformer/:testFileIsParsedOnceForMultipleFixableErrors()— 10 fixable errors in one file must parse the file once. Fails before the fix (10 parses).testNoFileNodesDoesNotReadTheFile()— a fixable error transformed with no file nodes must not touch the parser at all. Fails before the fix (1 parse).testCachingDoesNotChangeProducedDiffs()— every diff produced by a cache-warm transformer is byte-identical (hash and diff) to the one produced by a fresh transformer, for both a space-indented and a tab-indented file.testInterleavedFilesDoNotShareIndentation()— alternating between a space-indented and a tab-indented file keeps both files' diffs correct, guarding the new per-file indentation cache.FixIssetRuleTest,NamedArgumentsRuleTest,AttributeNamedArgumentsRuleTest,FinalClassRuleTest,MemoizationPropertyRuleTest,OrChainIdenticalComparisonToInArrayRuleTest,OverridingMethodRuleTest(testFixOverrideAttribute,testFixWithTabs),OverridingPropertyRuleTest,BacktickRuleTest.make tests, 17628 tests) andmake phpstanare green.make name-collisionfails ontests/PHPStan/Build/data/final-class-rule-pipe.phpboth with and without this change (the collision detector's parser does not understand the pipe operator).Fixes phpstan/phpstan#14996