Skip reading source files on a warm cache hit via mtime + length - #59
Draft
perryqh wants to merge 1 commit into
Draft
Skip reading source files on a warm cache hit via mtime + length#59perryqh wants to merge 1 commit into
perryqh wants to merge 1 commit into
Conversation
`process_files_with_cache` is the largest phase of `pks check`, and half of it is work we can avoid: for every file we open it, read it in full, and MD5 it, purely to compare a digest against the cache entry. Record (mtime_ns, len) alongside the digest and settle the common case -- nothing changed since the last run -- with one `stat`. The digest remains the authority: if no stat is recorded, or the stat moved, we fall back to reading and hashing exactly as before. An entry whose contents match but whose stat moved (a git checkout, a `touch`) is repaired in place so the next run takes the fast path. MEASURED on a 51,513-file application, A/B against main in one hyperfine run: main 4.945s +/- 0.118 this branch 3.450s +/- 0.117 1.43x faster user time 7.534s -> 6.877s system time 12.927s -> 8.495s (-34%) An earlier batch on a busier machine measured the same pair at 1.30x. Both are valid single-batch comparisons; the ratio moves with load because the phases this does not touch are a larger share when the machine is quiet. The system-time drop is the stable signal, and it is the mechanism: this phase is syscall-bound, not CPU-bound. An earlier attempt to speed the same phase up by parsing JSON faster (from_reader -> from_slice) changed nothing measurable, which is what pointed at the syscalls. ## Coarse filesystems are detected, not assumed away Trusting (mtime, len) is only sound where the filesystem timestamps finely enough to notice a write. At one-second granularity -- some Docker bind mounts on macOS, NFS, SMB, FAT -- a same-length edit inside the same second keeps both fields, and a stat-only check would serve the stale entry. Rather than probe or assume a platform, `SourceStat::of` reads the value it already has: a non-zero sub-second component proves the filesystem tracks sub-second time, so an edit at any other instant would have moved the mtime. A zero component means it cannot tell us, so the stat is discarded and the digest carries the entry. This needed no new branches at the call sites -- `None` already meant "no usable stat" -- and both ways of being wrong fail safe: - Coarse filesystem: nothing is trusted, the fast path never engages. Correct, just not faster. - Fine filesystem, mtime landing exactly on a second boundary: a 1-in-10^9 coincidence costing one extra hash. Measured across 20,003 files of a real Rails application: zero occurrences. Cost of the check itself: 1.00x +/- 0.02 against the same branch without it. It narrows rather than closes the window, and the type's docs say so. A millisecond-granularity filesystem is trusted, so two same-length writes inside one millisecond would still be missed -- six orders of magnitude tighter, and needing machine-speed edits to reach. Also documented: mtimes that are copied rather than set by writing (rsync -t, tar -p, cp -p) can carry a timestamp from elsewhere; every mtime-driven cache shares that hole, which is why they all document `touch` as the way to force a rebuild. ## packwerk compatibility Verified against packwerk 3.3.0, and the concern turned out to be misplaced: - Its `Cache::CacheContents.deserialize` uses plain hash access and never enumerates keys, so an unknown key is invisible to it. Ran its logic against a packwerk-format entry carrying `source_stat`: reads fine. - The tools do not share a directory. packwerk reads `tmp/cache/packwerk/<md5>`; pks writes `tmp/cache/packwerk/zeitwerk/<md5>`. - The formats were never interchangeable. Feeding packwerk what pks writes today raises `NoMethodError: undefined method 'map' for nil`. That predates this change, so `test_compatible_with_packwerk` does not test what its name claims; it round-trips pks's own format. Left alone, but it is not a guarantee. Regardless, `source_stat` is `#[serde(default, skip_serializing_if)]`, so an entry without one still deserializes and is still honored via the digest. ## Failure modes closed by construction - `EmptyCacheEntry` holds `Option<String>` rather than an empty string meaning "not computed", private behind `digest()`. `write` errors instead of persisting a placeholder, which would have produced an entry that never matches -- making that file permanently uncacheable and silently slow. - The in-place repair warns on failure rather than discarding the error. The result stays correct either way, but a persistent failure (unwritable cache dir, full disk) would otherwise leave every run re-hashing with no clue why. - That repair only fires when there is a stat worth recording. Without the guard, a filesystem yielding `None` every run would never match and would rewrite the entire cache every time. ## Tests tests/cache_stat_fastpath_test.rs, ten cases. Note that before this change *no test in the repo exercised a warm cache at all* -- every fixture ships `cache: false` -- so these paths were untested rather than under-tested. The fast path: stats are recorded; warm output matches cold; an edit invalidates; a *same-length* edit invalidates (the case a length-only check would miss); a whole-second mtime is not trusted. Fallback and repair: a stat-less packwerk-style entry is honored then upgraded; a stale stat with a matching digest is repaired in place; a malformed `source_stat` (five shapes) degrades to the digest without panicking. Other commands: `pks update` on a warm cache -- the highest-consequence path, since update *writes* package_todo.yml and a stale entry persists a wrong answer rather than printing one -- and the experimental parser, which uses a different cache subdirectory but shares this implementation. Each was verified to fail rather than assumed to pass: injecting a bug that makes the cache always hit fails 7 of the 10, and the granularity and repair guards were separately confirmed to fail with their own checks removed. Uses `common::Fixture` from #57 rather than a local copy helper. Verified: `check` and `check --no-cache` produce identical output on the 51k-file application, as do this branch and main. Across the 30 fixture apps with a packwerk.yml, 29 are byte-identical; the 30th is app_with_monkey_patches, which trips the pre-existing nondeterministic duplicate-constant panic in both binaries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
perryqh
force-pushed
the
perf/mtime-cache-fastpath
branch
from
August 22, 2026 01:00
289e08c to
7da7d5c
Compare
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.
1.43× faster on
pks check, measured against currentmainin a single hyperfine run.What it does
process_files_with_cacheis the largest phase ofpks check, and half of it is work we can avoid: for every file we open it, read it in full, and MD5 it, purely to compare a digest against the cache entry.This records
(mtime_ns, len)alongside the digest and settles the common case — nothing changed since the last run — with onestat.The digest stays the authority. If no stat is recorded, or the stat moved, we fall back to reading and hashing exactly as before. An entry whose contents match but whose stat moved (a
git checkout, atouch) is repaired in place so the next run takes the fast path.Measured
51,513-file application, warm cache, both binaries in one hyperfine run:
Phase breakdown:
process_files_with_cacheAn earlier batch on a busier machine measured the same pair at 1.30×. Both are valid single-batch A/B runs; the ratio moves with load because the phases this doesn't touch are a larger share of the total when the machine is quiet.
The system-time drop is the stable signal, and it's the mechanism: this phase is syscall-bound, not CPU-bound. It removes 51,513 file opens and full reads per run. An earlier attempt to speed the same phase up by parsing JSON faster (
from_reader→from_slice) changed nothing measurable, which is what pointed at the syscalls.Coarse filesystems are detected, not assumed away
Trusting
(mtime, len)is only sound where the filesystem timestamps finely enough to notice a write. At one-second granularity — some Docker bind mounts on macOS, NFS, SMB, FAT — a same-length edit inside the same second keeps both fields, and a stat-only check would serve the stale entry.Rather than probe the filesystem or assume a platform,
SourceStat::ofreads the value it already has: a non-zero sub-second component proves the filesystem tracks sub-second time, so an edit at any other instant would have moved the mtime. A zero component means it can't tell us, so the stat is discarded and the digest carries the entry.No new branches were needed at the call sites —
Nonealready meant "no usable stat, use the digest". Both ways of being wrong fail safe:Cost of the check:
1.00 ± 0.02against the same branch without it — unmeasurable.I did consider git's "racily clean" rule (record when the entry was written; re-hash anything whose mtime falls in a granularity window of it). It doesn't transfer cleanly — git's index timestamp comes from the same filesystem as the files, whereas our write time would come from a fine-grained clock and the mtime from a coarse one, comparing incommensurable units. The sub-second check is cheaper and self-calibrating.
The hole that remains
The check proves the filesystem would have moved the mtime. It can't prove nobody moved it back. Tools that preserve timestamps —
rsync -t,tar -p,cp -p, unzip, some backup and container-image flows — can install different content carrying an mtime from elsewhere; if both that mtime and the length match what was cached, the fast path serves a stale entry.Reaching it needs the replacement to match in mtime and byte length, which in practice means restoring a near-identical copy of what was already there. It also isn't specific to this design —
make,ccacheand every other mtime-driven cache share it, which is why they all documenttouchas the way to force a rebuild. Closing it would mean always hashing, i.e. giving up the entire win.Documented on
SourceStatrather than left implicit, along with the escape hatches that already exist:--no-cacheandpks delete-cache.packwerk cache compatibility
This adds a key to the cache JSON, so I treated "does this break packwerk?" as blocking. It doesn't. Checked against packwerk 3.3.0:
source_statNoMethodErrorCache::CacheContents.deserializeuses plain hash access and never enumerates keys — there's noT::Struct.from_hash, which would have been strict.tmp/cache/packwerk/<md5>; pks writestmp/cache/packwerk/zeitwerk/<md5>. On the app I measured: 51,708 files underzeitwerk/, and the only top-level file isconstant_resolver.json, also pks's.unresolved_referencesunderprocessed_fileand names the fields differently. That predates this PR.So
test_compatible_with_packwerkdoesn't test compatibility with packwerk; it round-trips pks's own format. Left alone — renaming it is a separate change — but it shouldn't be read as a guarantee.Belt and braces regardless:
source_statis#[serde(default, skip_serializing_if = "Option::is_none")], so an entry without it still deserializes and is still honored via the digest.Note
Pre-existing interaction, not caused by this PR: packwerk's
bust_cache!doesFileUtils.rm_rf(tmp/cache/packwerk), which deletes pks'szeitwerk/subdirectory too. Running packwerk after apackwerk.ymlor inflections change forces a full pks cold rebuild. Performance only, not correctness.Failure modes closed by construction
EmptyCacheEntryholdsOption<String>, not an empty string meaning "not computed", and it's private behinddigest().writeerrors rather than persisting a placeholder — which would produce an entry that never matches, making that file permanently uncacheable and silently slow.Noneevery run would never match and would rewrite the entire cache every time — turning a read-mostly cache into a full rewrite of itself.Tests
tests/cache_stat_fastpath_test.rs, ten cases.Before this branch, no test in the repo exercised a warm cache at all — every fixture ships
cache: false. These paths weren't under-tested, they were untested.The fast path: stats are recorded · warm output matches cold · an edit invalidates · a same-length edit invalidates (the case a length-only check would miss) · a whole-second mtime is not trusted.
Fallback and repair: a stat-less packwerk-style entry is honored then upgraded in place · a stale stat with a matching digest is repaired · a malformed
source_stat(five shapes: not an object, wrong type, missing field,null, negative) degrades to the digest without panicking.Other commands:
pks updateon a warm cache — the highest-consequence path, sinceupdatewritespackage_todo.yml, so a stale entry persists a wrong answer rather than printing one · the experimental parser, which usestmp/cache/packwerk/experimentalbut shares this implementation, so a regression confined there would be invisible.Uses
common::Fixturefrom #57 rather than a local copy helper.Each test was verified to fail
A test never seen failing isn't yet evidence of anything. Injecting a bug that makes the cache always hit fails 7 of the 10:
The granularity guard and the repair path were separately confirmed to fail with their own checks removed.
Verification
cargo test— 268 passing (258 + 10 new)cargo clippy --all-targets --all-featuresandcargo fmt --all -- --check— cleancargo doc --no-deps— cleancheckandcheck --no-cacheproduce identical output, and so do this branch andmainpackwerk.yml: 29 byte-identical. The 30th isapp_with_monkey_patches, which trips the pre-existing nondeterministic duplicate-constant panic in both binaries — the unmodified binary names a different constant run to run.🤖 Generated with Claude Code