Store the result cache in a framed serialize() format instead of a var_export'd PHP file - #5982
Conversation
|
I just merged #5981, please try out latest 2.2.x-dev on real-world projects. Please note this needs bleeding edge enabled. |
|
2.2.4's streaming save (ee9fe9e) addresses the save-side half of this: the peak from building the whole var_export string in memory is gone. This PR overlaps that half, so it needs rebasing, and the two approaches are mutually exclusive (the read format has to match the write format). Where they differ is the read side. So this is really a format choice: keep var_export + include (streamed on write), or switch to serialize + unserialize (lower read-side retention, no streaming needed on write since there is no giant string to build). It is your call which direction you prefer for this file, especially given you are actively working on it. If you want to pursue the serialize direction I will rebase onto 2.2.4 and re-measure both sides on current code; if you would rather keep the var_export format, I will close this. The transition either way is safe without a cache-version bump (old and new formats each fall through the existing corrupted-cache path), and the downgrade case is handled by prefixing the payload so an older PHPStan does not echo it. |
|
#5981 was reverted meanwhile, as it did not improve much and having more than 1 cache file might make trouble in exisiting setups which do not persist the whole temp-folder but just the single result cache file we have today |
ee5f26c to
80d339d
Compare
|
Thanks for the heads-up on #5981. Worth clarifying how this PR relates, since it's a different change: it keeps the single result cache file exactly as today, and only changes that one file's format from a var_export'd PHP file to The motivation is the read side. On current 2.2.x The trade-off against 2.2.4's streaming save: |
80d339d to
c13bc67
Compare
c13bc67 to
a8151ff
Compare
|
Rebased onto current Re-verified rather than assuming the rebase was inert:
The perf numbers in the description are from early July and I have not re-measured them on current Heads-up on an overlap: #6190 also rewrites paths inside |
|
after php/php-src#23189 landed, we should remeasure |
a8151ff to
c27d505
Compare
| // A process killed while the result cache is being written leaves a partial file at the final | ||
| // path, because the cache is streamed there rather than written atomically. Cutting the file | ||
| // inside the first section's payload is the shape that used to be read as a half-populated | ||
| // cache instead of a damaged one. | ||
| $file = __DIR__ . '/tmp/resultCache.php'; | ||
| $contents = file_get_contents($file); | ||
| if ($contents === false) { | ||
| throw new RuntimeException('No result cache at ' . $file); | ||
| } |
There was a problem hiding this comment.
is this a bug/case independent of the format change and needs a separate PR?
|
@staabm update, and a defect I found while re-checking it. On the remeasure gatephp/php-src#23189 is still open, so there is nothing to remeasure against yet. Worth knowing how much it can move this decision, though: I built and measured that PR while reviewing it, and it makes This PR's pitch is the read side - I am happy to remeasure whenever you want, either when it lands or against a locally patched build. Say which and I will run it. A defect, now fixedRe-checking the format handling turned up a real regression against base. The cache file is streamed to its final path, so a process killed during the save leaves a truncated file. With the framed reader, a truncation inside a value frame was read as far as it went and the missing frames came back as Base degrades gracefully for the same corruption, because an incomplete Every format violation now throws, so the file lands in that same handler. Eight truncation points from 100 bytes to 99.5% of the file all report, for example,
Re-verified rather than assumed, on current 2.2.x
What I did not doI have not re-run the memory and CPU table in the description; those figures predate today's rebase. I deliberately took no timings today because this machine is heavily contended and the numbers would not be worth publishing. That is the one open item, and it is the same one 23189 would want a rerun for anyway. CI note: the two red Symplify integration jobs before my push failed on a composer 404 for the The #6190 overlap still stands - whichever of the two lands first, the other needs a rebase. |
|
Followed up on the one open item - here is the fresh measurement, on current SetupA vendor tree built only from public packages, so you can rebuild it: Warm - the case this PR is about
The Note base's peak is the same 393.1 MB whether it reanalyses a file or not: on base the restore is the peak. On this branch it is not, which is why the changed-file figure sits above the pure-restore one. Cold - and a correction to the description
The description's -18% / -21% cold-peak claim does not reproduce here. On this corpus the cold peak is analysis-dominated (2.2 GB against a 60 MB cache), so the save is a rounding error and cold comes out flat. Those older figures came from projects where the cold peak was much closer to the cache size, so the honest version of that claim is "corpus-dependent, and flat where analysis dominates". I would rather say that here than leave a number in the description that does not hold generally. The warm figures, which are the point of the change, hold and are slightly better than what is written there. Cache on disk: 63,013,832 -> 77,148,189 bytes, +22.4%. Output identical: 4819 error lines on both, sorted diff clean, both warm runs confirmed as restoring rather than reanalysing. What this means for the php-src#23189 gateI instrumented the save phase on both sides:
23189 makes Numbers were taken with the machine otherwise idle for this work; the three-round repetition is there because it is a shared machine. |
|
this needs a review from ondrej. my feeling is that this will not land as is. maybe result-cache handling would see some measurable real world benefits by implementing it in turbo. |
|
the "truncate" part and the format change are 2 separate concerns, which I think should have separate PRs... wdyt? |
|
I'm getting warm to this idea. OPCache which we now always enable will have less work and less things to hold in memory if we switch away from the .php format. |
de278eb to
f49b495
Compare
|
Rebased onto current 2.2.x, and the rebase found a real break: the cache file now carries So the frames now decode on demand rather than up front, which the format is a better fit for than a PHP file: each section is one frame, so the reader walks past its entries and seeks back when a callback asks. Eagerly wrapping them instead cost +7.5 MB, so this part is the design rather than an adaptation. @ondrejmirtes on the OPCache point, measured on the same 5400 entries with OPCache on: including the 41.9 MB End to end the warm peak drops 61% on a 4524-file project and 56% on a 2485-file one. The honest exception is this repository's own config, where the peak is collector processing rather than the restore and goes 339 to 346.52 MB. The file on disk is 12 to 35% bigger. All of it is in the description now. @staabm on splitting: the separable half is up as #6349, which renames the cache into place so a killed save stops leaving a partial file at all, in either format. It stands on its own and I would take it first. The guard here cannot go with it, because it only touches the framed reader this PR introduces and shipping the format without it would regress against the |
f49b495 to
85fe784
Compare
|
The callbacks had a reason. I suspect if you unserialize the serialized string, everything in it is going to instantiated, right? And it might corrupt the objects if their internal properties meanwhile changed. I don't know how it's going to behave, whether it will crash outright or whether the objects report an error once we start asking things. Please test it. Also, |
|
Tested, and you were right: it does change behaviour, and it needed a fix. Same fixture, a cache written before the class changed, then read back after:
Appending one without a default breaks the analyser itself, since no So
Fixed in the last commit: the four callbacks are invoked inside On cleaning up
Say which one and I will do it in this PR. |
would be great to have this covered by a test |
b742cd7 to
dca415a
Compare
|
Covered now, and rebased onto current 2.2.x.
re-analyses, and the cache written in its place is usable again. With the change reverted the same fixture dies on the uninitialized property instead, so the test fails for the right reason. One limit worth stating: the reconstruction is only exercised for the sections whose absolutizing reads the objects' properties, which is |
|
Let's walk back a bit - are you actually sure that changing the Error class declaration making the serialized Error object no longer match the declaration would lead to an error? Because with every new PHPStan version, the result cache is thrown away (PHPStan version is part of |
dca415a to
16f09f6
Compare
|
The closures are not in the file, and that is my comment's fault - I have pushed a clearer one. A lazy section is written as plain frames: and On whether it can actually break: it does, and the Errors are not "never accessed". But you are right that it is out of reach for a released version: there So it protects contributors rather than users. Happy to revert it; that would take |
16f09f6 to
b87b09c
Compare
…r_export'd PHP file The cache is a var_export'd PHP file hydrated with include. Including a multi-megabyte PHP source retains its compiled op_arrays and interned strings for the process lifetime, and where OPCache is on it also occupies shared memory: on a 2485 file project the file claims 39.9 MB of it as a single 41,739,216 byte script entry, against a 128 MB default opcache.memory_consumption. The file is now framed serialize() payloads: `<?php return; ?>` on the first line, then `name length` or `name* count` headers, each followed by length-prefixed payloads. Written frame by frame, and the array sections entry by entry, because serializing a section in one call holds it in memory twice over - exportedNodes alone is 39 MB on a 4524 file project - which is the trap the var_export writer avoided the same way. errors, locallyIgnoredErrors, collectedData and exportedNodes are the sections restore() hands back as callbacks. The var_export format defers them by writing one closure per section, which still leaves PHP compiling the array literal inside it. Here each section is one frame, so the reader remembers where it starts, walks past its entries without unserializing them, and decodes on demand. Walking rather than seeking wholesale keeps the framing of the whole file validated up front, so a damaged cache is discarded by restore() rather than throwing later inside a callback. Measured, main process, interleaved, every figure repeated across rounds: opening the cache 0.39s and 325 MB against 0.01s and 14 MB, or 124 MB with all four lazy sections materialised; warm peak -61% at 4524 files and -56% at 2485. The file on disk is 12 to 35% bigger. A format that can be read half way has to refuse to, so every frame violation throws. fseek() past the end of a file succeeds, which is why the skip walk compares the position with the file size after every entry - without that a truncated section is handed out as a callback pointing past the end. No cache version bump is needed: a cache in the old PHP format fails to unserialize and is discarded like any other unreadable file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
unserialize() has no reconstruction hook, so a cache written by a PHPStan whose classes have since changed can fail while the objects are rebuilt: a property the payload does not carry stays uninitialized and reading it throws. var_export absorbed this because __set_state() reconstructs through the constructor, which applies the declared defaults - renaming Error::$tip is harmless there and aborted the run here. cacheVersion and phpstanVersion keep a released version away from another release's objects, but phpstanVersion comes from Composer's installed.php rather than the working tree, so a source checkout keeps one value across every edit of these classes. That is where it is reachable, and it is also where PHPStan is developed. The four callbacks are now invoked inside restore()'s failure path, so a cache that cannot be read back is discarded and everything re-analysed, the same as for a damaged file. Before this the same shape aborted the command with an uncaught error and a usage dump - on both formats, since the var_export closures are evaluated at the same point. e2e/result-cache-stale-objects renames Error::$message inside the cache file, which is that shape without needing two PHPStan versions installed, keeping the byte length so the frame headers stay valid. The run reports the cause and re-analyses; without the change it dies on the uninitialized property. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b87b09c to
25e7493
Compare
|
Thank you! |
What
The result cache is a
var_export'd PHP file hydrated withinclude. Including a multi-megabyte PHP source retains its compiled op_arrays and interned strings, and where OPCache is on it also occupies shared memory. This writes the file as framedserialize()payloads instead.Format:
<?php return; ?>on the first line, thenname lengthorname* countheaders, each followed by length-prefixed serialized payloads. Written frame by frame, and the array sections entry by entry, because serializing a section in one call holds it in memory twice over -exportedNodesalone is 39 MB on the larger project below. The read side is framed for the same reason.errors,locallyIgnoredErrors,collectedDataandexportedNodesare the sectionsrestore()hands back as callbacks. Thevar_exportformat defers them by writing one closure per section, which still leaves PHP compiling the array literal inside it. Here each section is a single frame, so the reader remembers where it starts, walks past its entries without unserializing them, and decodes on demand.What loading the cache costs
Isolated, same 5400 entries, both formats read with OPCache enabled:
Default
opcache.memory_consumptionis 128 MB, so on this project the cache file alone claims a third of the buffer, leaving that much less for PHPStan's own code; a project three times the size does not fit in it at all.End to end
Warm peak of the main process, interleaved A/B, every figure repeated across rounds:
Cold main-process peak on the first: 120 MB against 92 MB, worker peak flat (334.1 against 336.1 MB). Cold and warm wall are unchanged or slightly better.
One exception worth naming: analysing this repository with its own
phpstan.neon.dist(bleeding edge, strict rules, collectors) the warm peak goes the other way, 339.05 to 346.52 MB. There the peak is set by collector processing rather than by the restore, and I have not chased the remaining 7 MB.The file on disk is bigger: +11.9% at 2485 files, +34.7% at 4524.
Truncated caches
The separable half of this is now #6349, which renames the cache into place so a killed run stops leaving a partial file at the path the next run reads, in either format.
The guard here stays, because a format that can be read halfway has to refuse to: every frame violation throws and
restore()discards the file, which is what thevar_exportformat got for free from aParseError. Two e2e cases cover it, one cutting inside a lazily-read section -fseek()past the end of a file succeeds, so that one is only caught by comparing the position with the file size.Transition
No cache version bump is needed. A cache in the old PHP format fails to unserialize and is discarded like any other unreadable file.