Track scanned files as file dependencies instead of invalidating the whole result cache - #6331
Track scanned files as file dependencies instead of invalidating the whole result cache#6331phpstan-bot wants to merge 3 commits into
Conversation
|
/cc @SanderMuller @staabm Would be nice if you could test this and review this. Thanks. |
SanderMuller
left a comment
SanderMuller
left a comment
There was a problem hiding this comment.
Tested and reviewed on 42e6a75c2. It does what it says and I could not break it. The merge call is yours, but from my side this looks good.
What I verified
All five e2e fixtures pass on the PR head. The more useful direction: I kept the PR's e2e fixtures and reverted only the three src/ files to bdb4580dd, and four of them fail. result-cache-autoloaded fails in the way that matters, reporting Result cache restored. 0 files will be reanalysed. and no error at all, which is the silent staleness you describe.
Probes beyond the fixtures, all on the PR head:
- Two-hop inheritance through scanned files (analysed
A, scannedB extends C, editC): 1 file re-analysed, error caught. The graph records the grandparent. - Adding a new scanned file that defines a previously unknown class: 1 file re-analysed and the
class.notFounderrors clear, so the$scannedFileAddedOrEditedpath does what it claims. - A global constant and a function declared in a scanned file: both dependents re-analysed, both errors caught. That covers the two paths you probed but kept no test for.
- Editing a scanned file and running again with nothing further changed settles to
0 files will be reanalysed, so the hash written back for a not-analysed file is the fresh one rather than the cached one.
Gate on the PR head: full suite 21249 tests / 96329 assertions green, self-analysis clean, phpcs clean on the three touched files. None of the three classes carries #[ShadowedByTurboExtension] and turbo-ext/ does not reference them, so there is no native mirror to keep in sync.
Performance
A 4524-file corpus at level 5, cold, base against PR, 2 interleaved rounds:
- CPU flat: 137.6s and 138.8s against 136.6s and 139.5s.
resultCache.phpgrows from 39.76MB to 40.88MB, +2.8%. The same size to the byte in both rounds, so that one is a real cost rather than noise. Whole tmpDir 194MB to 195MB.
I would treat +2.8% as a floor rather than a typical figure. On that corpus nearly every non-analysed dependency resolves to an installed Composer package and lands in the packages bucket, so few new file edges get recorded. A monorepo scanning sibling project code, which is the case the issue describes, will record considerably more.
The red CI
None of it reproduces here.
- The six extension and integration reds (doctrine, Larastan, Rector, Symplify, phpstan-laravel, symplify/phpstan-rules) are the same set #6328 carries.
Tests with old PHPUnitalso fails on #6332, #6330 and #6326. The three failures areIntersectionTypeTest::testIsAcceptedBy, Maybe against No, which references none of the classes touched here and passes locally, 106/106.Test (PHP 7.4)is the phpbench gate against the committed baseline. Its@Assertexpression only asserts on time, so the redmem_peakcells in that table are diff colouring rather than assertion failures. I A/B'd base against PR locally on the three flagged variants:nullsafe-chain-walkcame out at 166.578mb against 166.577mb, and on the other two the PR never used more than base. Time stayed inside run-to-run noise. That runner also reported rstdev diffs up to +471%, which is the usual tell.
What I did not check
Everything above is PHP 8.5 on macOS. I did not reproduce the downgraded 7.4 source that the red bench job actually runs, and I tested neither Windows nor a run with the turbo extension loaded.
|
Those were leftover polling tasks for the test run that already finished — no new information, and I've stopped the remaining ones. The work stands as reported: |
…whole result cache - `NodeDependencies::getNonAnalysedDependencies()` replaces `getPackageDependencies()`: one pass over the reflections returns both the Composer packages of the non-analysed dependency files and the remaining project files (scanFiles/scanDirectories, files excluded from the analysis, files only reachable through the autoloader). Files inside a PHAR are left out - they belong to the running PHPStan. - `FileAnalyserCallback` records those project files as regular file dependencies (and as used-trait dependencies for traits), so the cached dependency graph knows which analysed files depend on which scanned file. - `ResultCacheManager::restore()` no longer throws the cache away when only the `scannedFiles` metadata differs: the graph entries of files that are not analysed but still on disk are carried over, their dependents (including used-trait dependents) are re-analysed only when the file's hash changed, and a scanned file that appeared or was edited also re-analyses the files with errors, the same way a new analysed file does. - A changed scanned file that `getNonAnalysedDependencies()` does not track - one belonging to an installed Composer package (tracked at package granularity) or living inside a PHAR - still falls back to a full re-analysis, so no stale results are kept. - The `composerLocks`/`composerInstalled` handling is generalized into `PARTIALLY_INVALIDATING_META_KEYS`, so a run where both the Composer metadata and the scanned files changed is handled incrementally by both mechanisms. - Bumped the result cache version to `v15-scannedFileDependencies`. - Same fix applies to the analogous cases: `scanFiles`, `scanDirectories`, files excluded from the analysis inside an analysed directory, traits and functions/constants declared in scanned files, deleted and re-added scanned files, and project files reachable only through the Composer autoloader (previously not tracked at all, which left stale results behind).
The scanned file fixtures exercise editing and deleting a scanned file, but not one appearing: a new file in a scanDirectories directory may define a symbol that is reported as unknown somewhere, which is what the $scannedFileAddedOrEdited branch re-analyses the files with errors for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A bootstrapFile is executed, not just read, so a change in one of them can affect anything about the analysis. They stay in the fully invalidating executedFilesHashes metadata entry rather than being tracked per file like the scanned ones - say so where PARTIALLY_INVALIDATING_META_KEYS is declared, and cover it with an e2e test so it cannot regress unnoticed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
e9b6bc8 to
5fc697b
Compare
|
needs rebase |
| continue; | ||
| } | ||
|
|
||
| if (str_starts_with($dependencyFile, 'phar://')) { |
There was a problem hiding this comment.
I guess this is a perf optimization because phar's are readonly and cannot change?
if so, should have a comment
There was a problem hiding this comment.
would it also make sense for .phar-ending paths?
4 participants
Summary
Editing a single scanned file (
scanFiles,scanDirectories, or a file excluded from the analysis but living in an analysed directory) invalidated the entire result cache, because the SHA-256 hashes of all scanned files are part of the cache metadata and any metadata difference meant a full re-analysis. In a monorepo where several PHPStan configs scan each other's code, every edit made all the other caches useless.Scanned files are now tracked the same way third-party packages are (#5933): an analysed file records which non-analysed project files it depends on, and editing one of them re-analyses only the files depending on it.
Changes
src/Dependency/NodeDependencies.php:getPackageDependencies()is replaced bygetNonAnalysedDependencies(), which walks the reflections once and splits the dependency files thatgetFileDependencies()drops intopackages(files of an installed Composer package, resolved to the package name - the previous behaviour) andfiles(the remaining project files, by path). Files inside a PHAR are left out of both: they belong to the running PHPStan, whose version is already part of the metadata.src/Analyser/FileAnalyserCallback.php: thefileshalf is appended to the file dependencies (and to the used-trait dependencies forresolveUsedTraitDependencies()), so the cached dependency graph gains entries for scanned files. Merging the two calls into one keeps the per-node work at the same two traversals as before (measured on this repo's self-analysis: 73.0s before, 71.6s after).src/Analyser/ResultCache/ResultCacheManager.php:PARTIALLY_INVALIDATING_META_KEYS(composerLocks,composerInstalled,scannedFiles) generalizes the existing Composer-only special case; a difference limited to those keys no longer forces a full re-analysis, and a run where both changed is handled by both mechanisms.getChangedScannedFiles()diffs the cached and the currentscannedFilesmetadata into "changed file => still scanned?".deletedFilesloop is now thenotAnalysedFilesloop: an entry that is not analysed but still on disk keeps its edges in the graph for the next run, and its dependent files (plus used-trait dependent files) are re-analysed only when its hash differs from the cached one. A missing file keeps the old "deleted file" behaviour.v15-scannedFileDependencies..github/workflows/e2e-tests.ymlpluse2e/result-cache-scanned-2,e2e/result-cache-scanned-trait,e2e/result-cache-scanned-vendorande2e/result-cache-autoloaded: new and updated end-to-end tests (below).Root cause
The result cache tracked scanned files only as a hash map inside the metadata, an all-or-nothing signal:
restore()compared the whole metadata array and returned a full analysis on any difference. There was no per-file link between an analysed file and the scanned file it depends on, becauseNodeDependencies::getFileDependencies()deliberately dropped every dependency file that is not analysed.The same gap had a second, quieter symptom: a project file that is neither analysed nor scanned - only reachable through the Composer autoloader (an
autoload-devnamespace, a directory outsidepaths) - was not tracked anywhere at all, so editing it re-analysed nothing and the cache silently kept stale results. Recording those files as ordinary file dependencies fixes both: the pinpointed invalidation for scanned files, and the missing invalidation for the rest.Test
E2E tests, in the shape of the existing result-cache tests (patch a file, observe what the cache does):
e2e/result-cache-scanned(updated): editing a file excluded from the analysis but living in an analysed directory now restores the cache and re-analyses 1 file, instead of reportingmetadata do not match: scannedFiles. The reported error is unchanged.e2e/result-cache-scanned-2(new): a project with bothscanFilesandscanDirectoriesand three analysed files. Editing the scanned class re-analyses only the file depending on it; undoing the edit clears the error again; editing thescanFilesentry re-analyses only its dependent; deleting the scanned file re-analyses its dependent and reports the unknown class.e2e/result-cache-scanned-trait(new): a trait declared in a scanned file - covers the used-trait dependency path.e2e/result-cache-autoloaded(new): a project file that is neither analysed nor scanned, reachable only through the Composer autoloader. Before this change the run re-analysed 0 files and reported no error at all.e2e/result-cache-scanned-vendor(new): a scanned directory inside an installed Composer package - asserts the full-invalidation fallback still kicks in, so this case cannot go stale.Also probed and found to work through the same mechanism (no separate test kept, they share the single code path): functions and constants declared in a scanned file, and a scanned file being re-added after deletion.
make tests(21198 tests),make phpstan,make cs-fixand all 72 result-cache E2E scripts pass.e2e/result-cache-relative-pathfails locally only because it checks outHEADinto a git worktree, which does not contain the uncommitted change; it passes once the change is committed.Fixes phpstan/phpstan#15149