Serve offloaded collector data from one bundle file - #427
Open
ondrejmirtes wants to merge 1 commit into
Open
Conversation
With offloadCollectorData enabled, pack() writes one md5-named .dat file
per class per collector. That is 6k files on phpstan-src and 81k files /
677 MB on a ~16k-file codebase, and DeadCodeRule reads every one back in
the CollectedDataNode run, single threaded, after the parallel analysis
is done.
The bytes are not the problem, the file count is. Reading the same 6217
files twice inside one process takes 1437 ms and then 245 ms: on APFS a
cold open+read+close is ~230 us against ~40 us warm. 81k of those is
~19 seconds of syscall and metadata work.
gc() now merges the entries that survived into bundle.dat plus a
bundle.idx of 32 char hashes and pack('J') positions, and unpack() serves
them with fseek/fread on one descriptor. Loose files stay the write path
for workers - they need no locking and dedupe for free - and are removed
once merged. The bundle is rewritten only when more than a fifth of it
became garbage, otherwise new entries are appended.
Entries are laid out in the order they were read, which is close to the
order the next run will ask for them, so the reads go front to back
instead of seeking around: 1045 ms for the loose files, 343 ms bundled in
directory order, 49 ms bundled in read order.
The descriptor is guarded by getmypid() because a forked child inherits
it together with its file offset.
Also replaces the serialize() memoization key in getAlternativeMemberKeys
with a plain string built from the ref, which is 2.2x cheaper and is
called once per collected usage, and returns early from recordUsage()
when no debug members are configured.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G7xhBWRXCTYyc336QVj13s
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is slow
I profiled the
CollectedDataNoderun of PHPStan'sAnalyserResultFinalizer, which is single threaded and happens after the parallel analysis is finished.DeadCodeRuleis essentially all of it — every other CollectedDataNode rule on phpstan-src combined takes 25 ms,DeadCodeRuletakes 1.7 s. On a ~16k-file codebase with a warm result cache it takes ~40 s.Phase breakdown on that codebase (1.59M collected usages, 81,348 cache files, 677 MB):
Why
With
offloadCollectorDataenabled (the default),pack()writes one md5-named.datfile per class per collector. The finalizer then reads all of them back one by one.The bytes are not the problem, the file count is. A probe reading the same 6,217 files twice inside one PHPStan process:
On APFS a cold
open+read+closeis ~230 µs against ~40 µs once the path is in the process's realpath cache. 81k of those is ~19 seconds of pure syscall and metadata work. Reading the same 34 MB when warm takes 235 ms, so the data volume is irrelevant.What this changes
gc()already walks the whole cache directory to drop entries the run did not read. It now also merges the survivors intobundle.datplus abundle.idx(a block of 32-char hashes followed by a block ofpack('J')positions, where offset and length share one int).unpack()serves them withfseek/freadon a single descriptor.Design points worth calling out:
Loose files stay the write path.
pack()runs in parallel workers; content-addressed files need no locking and dedupe for free. They are removed once merged into the bundle.The bundle is laid out in read order.
readHashesis insertion-ordered, so writing the bundle in that order means the next run reads it roughly front to back rather than seeking all over it. This is most of the win:Compaction is amortized. The bundle is rewritten only when more than a fifth of its entries became garbage; otherwise new entries are appended and only the (much smaller) index is rewritten.
The descriptor is guarded by
getmypid(), because a forked child inherits it together with its file offset.A truncated bundle or index is detected and falls back to the loose files, which produces the existing "clear the result cache" message rather than a half-parsed line.
Two smaller things in the same commit, both in the second-hottest phase:
getAlternativeMemberKeys()usedserialize([$member, $accessType])as its memoization key. It is called once per collected usage — 1.59M times on the codebase above.ClassMemberRef::toCacheKey()builds the same identity as a plain string from the key prefixes, the descendant flag andclass::member; 2.2× cheaper per call, and the resulting array keys are ~60 bytes instead of ~200.DebugUsagePrinter::recordUsage()returns early when no debug members are configured, which is the normal case.Numbers
Measured against
master(05054d4), timingDeadCodeRule::processNode()only.phpstan-src (2,467 files, 6,214 cache files, 139k usages) — three variants run interleaved, order reversed every round, 6 rounds:
6/6 rounds favour the PR, paired t = 26.8. −50.5%. For context, on a run where nothing changed this rule was ~45% of phpstan-src's total wall time; every other CollectedDataNode rule combined is 25 ms.
~16k-file codebase (81,348 cache files / 677 MB, 1.59M usages), warm result cache:
DeadCodeRule::processNode()−62.6%. The cache directory goes from 81,348 files across 677 MB to a single ~470 MB
bundle.datplus a 3.2 MB index.The one run that pays extra is the first one after upgrading, which reads the old loose files and writes the bundle: 38.7 s once, then 7.5 s from there on.
Correctness
I dumped every error group the rule produces (message, file, line, identifier, tips) on both codebases and compared master against this PR, across a fresh full analysis, a warm run, and a full re-analysis on top of an existing bundle. All identical — with one caveat that turned out to be a pre-existing bug: the order of tips within one error varies between runs on master, because
buildError()callsksort()on a list, which does nothing. Two runs of unmodified master produce different tip order. Once tip order is normalized, all dumps match exactly. That is fixed separately in #426; this PR does not depend on it.composer checkpasses: 858 tests, PHPStan, phpcs, dependency and collision checks.Known limitation (unchanged from master)
Two PHPStan runs sharing one
tmpDirstill interfere — master'sgc()already deletes files the other run is about to read. The bundle does not make this worse (rename()is atomic, and an open descriptor keeps reading the old inode), but it does not fix it either.Possible follow-ups, not in this PR
CollectedUsage::deserialize()is now the biggest remaining cost — 4.5 s on the large codebase, onejson_decodeplus four object constructions per usage. A compact line format would likely halve it, but it changes the on-disk format.gc()then unlinks them (~3 s). Sorting the index hash block would letpack()binary-search membership on a 2.6 MB string without loading the map into every worker.