Memoize file contents by path in CachedParser to skip redundant reads - #5928
Memoize file contents by path in CachedParser to skip redundant reads#5928SanderMuller wants to merge 1 commit into
Conversation
|
How to reproduce/measure the performance improvement? |
|
Here's the A/B I used to measure it. Swap only # in the phpstan checkout, swap just the one file between the two versions:
git show <ver>:src/Parser/CachedParser.php > src/Parser/CachedParser.php # <ver> = this branch, then bad7874ec
# from the target project, cold + single-process, 3 reps:
rm -rf <tmpDir> && /usr/bin/time php <phpstan>/bin/phpstan analyse -l 8 --no-progress <paths>The mechanism: So the win tracks how much cross-file re-read redundancy a codebase has, which makes it corpus-dependent. Single-process, cold, 3 reps, median:
So it's a real win on high-fan-in codebases and neutral on low-redundancy ones. The content memo costs about 4MB regardless of whether the project benefits. If that always-paid cost is the concern, a bounded variant (memoize only files that get read more than once) would make the memory track the benefit; happy to do that if you'd prefer it. The red CI check is the flaky |
4fd4ed1 to
78d7299
Compare
|
Reworked and re-measured on top of the current 2.2.x (the LRU and source-byte-cap changes rewrote this file under the PR, so the numbers below are fresh, from instrumented builds, cold runs). The call volume that motivates it: a large Laravel project does 143,613 Changes since your question:
Measured effect on the current base (cold, single process, interleaved): sys 7.3 s to 5.4 s (-26%) on the Laravel project, every run with the change below every run without; total CPU and wall within noise. So this is a bounded syscall/IO reduction in the spirit of the recent duplicate-work PRs, not a latency win. Output stays byte-identical; the full test suite passes (17,565 tests). |
78d7299 to
cbf5fd4
Compare
cbf5fd4 to
1922b3f
Compare
| clearstatcache(true, $file); | ||
| $mtime = @filemtime($file); | ||
| $size = @filesize($file); | ||
| if ($mtime === false || $size === false) { | ||
| return FileReader::read($file); |
There was a problem hiding this comment.
I think this parts adds a additional cost for the common path (namely a file which does not contain a trait and will likely only be parsed a few times gets a perf penality).
the cache itself is mostly useful for traits. maybe we can optimize this cache for the trait case, without taking a perf hit for the common path which does not involve traits (I have no idea yet how this can/should work).
running this PR on phpstan-src does not yield a meaningful improvement yet in analysis time in my testing
There was a problem hiding this comment.
in addition my testing using make phpstan somehow shows, that this path is only taken for .stub files, when adding a echo "building cache $file \n"; in CachedParser->readFile
There was a problem hiding this comment.
The ".stub only" is a parallel-mode artifact, not the real picture. make phpstan runs the analysis in worker child processes, and only the main process's stdout reaches your terminal. The main process parses almost only stubs; the source files are parsed in the workers, whose stdout the parallel runner captures rather than prints. Counting parseFile calls per process on a cold run of phpstan-src:
- main process: 10 .php, 186 .stub
- all processes together: 71,978 .php, 586 .stub
So source files do go through readFile (~72k times), the workers' echo just isn't shown. Run with --debug (single process) and you'll see it directly: 68,174 .php reads in the one visible process.
On the common-path cost, fair point: for a file parsed once, the clearstatcache + filemtime + filesize is pure overhead. It's easy to only start memoizing (and doing the stat) from a path's second read, so single-parse files pay nothing. Happy to do that if you think it's worth keeping.
On who benefits, to be precise: the redundancy is high wherever many classes pull the same traits/base files, so the same file gets re-parsed a lot in one cold run: phpstan-src re-reads the average file ~31x, a fresh Laravel ~40x, a doctrine/symfony app ~3x. But it's a sys/IO reduction, not a CPU/latency win. Reads are only ~0.6-3.4% of cold CPU in my measurements, so dropping ~94% of them is a real cut in sys time and syscalls on cold runs of large high-fan-in projects, but it won't move total analysis time on phpstan-src meaningfully, and it does nothing for warm runs. That's the honest scope: a bounded cold-run IO win, largest on big high-fan-in codebases.
1922b3f to
acaf364
Compare
|
Rebased onto current Gates: full suite green (21308), The measurement in the description is from early July. This one is a cold-run IO win, so it is worth re-measuring on current |
|
Please check whether/how much impact has this PR on analyzing the shopware codebase (which also uses traits heavily) |
|
Measured it on Shopware. Short answer: -12.7% CPU, and the output is byte-identical. It is the biggest win I have measured for this PR, and your hunch about traits is exactly why. Setup
-12.7% (1.15x), and every PR run beat every base run. JSON output is identical between the two (same sha256, 423,440 bytes, 839 file errors + 2 errors), so nothing is being skipped. Why it is so large here — the numbers behind itShopware is 107 trait definitions against 2322
So analysing a 9205-file project currently reads 15 GB off disk; with the memoization it reads 217 MB. That is 36x fewer read calls and 70x fewer bytes, which is where the 12.7% comes from. The PR still does 54,877 reads against roughly 20,800 distinct paths, because the memo is capped at Caveats
For contrast, the same PR is roughly flat on a Doctrine/Symfony application I benchmark with, so trait-heavy is indeed the distinguishing factor. If you want, I can rerun with the CI |
acaf364 to
5db34aa
Compare
how can I measure the number of syscalls a PHPStan run needs on macos to reproduce the before/after PR syscall counts? |
|
Good questions — the third one especially, because the honest answer was "convention, not measurement". I re-measured everything on Shopware ( First, a correction to my own description: "total CPU and wall are within noise" is out of date. That was measured on a Laravel project in July. On Shopware it is not noise:
I will update the description. 1. Why does it improve performance?
Per avoided read (1,916,564 of them): 25.9 µs of sys time and 27.7 µs of user time. Sys time nearly halves, which is the syscall side; the user-time half is PHP's stream layer, the 2. Why doesn't the OS page cache make this free?It already does the part it can — and the measurement says so directly: What the page cache cannot remove is the cost of asking for it: the That cost is dominated by the per-call overhead rather than the file size. Re-reading a 929-byte file 100k times, warm: An 11.8 KB file costs 15363 ns/op — 13x the bytes for 14% more time. So it is ~13 µs of fixed cost per call whatever the file, against ~0.8 µs for the stat pair the memo replaces it with. (That 13 µs is a floor measured in a tight loop where every cache is hot; in a real run, interleaved across 20k files, the observed cost is the ~54 µs above.) 3. Why 512 KB?Honestly: because the surrounding cache work had just introduced byte caps with LRU eviction and I matched it. So I swept it. Read counts are deterministic, so they are the clean signal; CPU across a single run at each cap is noisy and I would not read anything into differences of a few seconds.
So the knee is at or below 64 KB, and everything from there up is the same plateau — 512 KB is comfortably on it but there is nothing special about that number. It is 0.06% of the ~800 MB the run uses anyway, so the cap is not what bounds memory here. I am happy to drop it to 64 KB (same result, smaller promise) or raise it; tell me which you prefer and I will change it and re-measure. One thing the sweep exposed that I did not expect: even unbounded, reads stay at ~2.6x the distinct-path count (52,601 vs 20,374 summed over the workers). So something re-reads files beyond the memo's reach — most likely more than one 4. Measuring syscalls on macOSThe two I actually used here, neither needing root:
For real syscall counts there is |
CachedParser::parseFile() read the whole file via FileReader::read() on every call, before the content-keyed node cache. The same file is parsed many times (a trait file once per class that uses it - on Tempest, 73 498 parseFile calls for 2 327 distinct files, 96.8% redundant reads; the 256-entry content cache thrashes on hot traits), so the read is repeated even when nothing changed. Memoize the contents by path, keyed by mtime, and skip the re-read when the file is unchanged. clearstatcache() before the mtime check keeps this correct in long-running processes (PHPStan Pro, fixer worker) where a file may be edited between calls, so an edited file is always re-read and re-parsed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5db34aa to
e68c94f
Compare
CachedParser::parseFile()reads the whole file viaFileReader::read()on every call, because the contents are the key of the AST cache. The same file is parsed many times (a trait file once per class that uses it), so the read repeats even when nothing changed. Measured on current 2.2.x with an instrumented build, cold: a large Laravel project does 143,613parseFile()reads for 6,624 distinct files, with one hot trait file read 94,007 times; a large doctrine/symfony project does 24,155 reads for 6,251 distinct files.This memoizes the contents by path, keyed by mtime and size, and skips the re-read when the file is unchanged. Following the direction of the recent cache work (LRU eviction, source-byte caps), the memo is bounded: total memoized source is capped at 512 KB (
MEMOIZED_SOURCE_BYTES_LIMIT) with least-recently-used eviction, and files larger than the cap are never memoized. Hot trait files stay resident by definition, so the bound costs little: the read counts drop to 8,444 on the Laravel project (94% fewer) and 8,266 on the doctrine/symfony one (66% fewer).Keying by size as well as mtime catches same-second edits that change the length in long-running processes (PHPStan Pro, fixer worker);
filesize()is served from the stat cache populated byfilemtime(), so it costs no extra syscall. A same-second, same-length edit is the remaining undetectable case, pinned in a test.Effect, measured on Shopware
v6.7.6.2(9205 files, 9 workers, cold result cache), phars built from2.2.xand from this branch:FileReader::read()callsOutput is byte-identical.
block input operationsis 0 in both runs, so none of that 15 GB reaches the disk - the page cache serves it, and what the memo removes is the syscall round trips, the copy into a fresh userspace buffer and PHP's stream/string overhead. Re-reading a warm 929-byte file costs ~13.5 us per call against ~0.8 us for the stat pair the memo replaces it with, so the cost is per call rather than per byte.A trait-light project sees much less: the same PR is flat on a Doctrine/Symfony application, and was within noise on the Laravel project this PR was originally measured against (sys 7.3 s -> 5.4 s, total CPU unchanged).
Memory: at most 512 KB per process (the earlier revision of this PR held contents unbounded, about +4 MB; that is gone). Sweeping the cap on Shopware shows the knee is at or below 64 KB - 64 KB already captures 98.0% of the achievable read reduction and every larger cap up to unbounded lands on the same 98.3% plateau - so 512 KB is on the plateau rather than at a cliff, and is 0.06% of the run's footprint.
Tests cover: unchanged file not re-read, size change detected with unchanged mtime, newer mtime re-read, oversized files never memoized, and LRU eviction at the byte cap.