sync: stop re-reading a file the scan cache already settled - #59
Merged
Conversation
Issue 56. materialize_tracked read and hashed EVERY present file on EVERY pass. On a converged tree that answer is always "unchanged", so the read and the hash were pure waste. Measured on the live Silber daemon, version 0.2.0+fff659e. One entry re-read and re-hashed 70,157,702 bytes every 0.51 seconds, which is 136 MB/s. blake3_hash_many_neon was 14.22 percent of one core, the largest single consumer. 99.6 percent of those bytes were three files that never change. This asks the same question scan_folder asks, from the same evidence: size and both mtime components byte-identical to what THIS MACHINE last observed. Trusting that is the existing contract, not a new one. scan_folder already runs first in every pass and already trusts the cache, so re-deriving the opposite answer here, from the same disk, was incoherent rather than safe. materialization_does_not_manufacture_an_mtime_collision already pins that a local cache entry is trusted by design, and that only this machine can create one, so no peer can manufacture a hit. Anything unknown, differing, or unreadable takes the full read-and-hash path exactly as before. The new test makes the read CHANGE THE ANSWER, because a read cannot be observed directly. The disk carries bytes that differ from the recorded hash while wearing the same size and mtime. A materialization that reads sees drift and republishes it; one that trusts the cache leaves it alone. I ran that test against the pre-fix code and it fails, on the republish assertion. A control asserts a file the cache does NOT cover is still read and hashed, so the fast path cannot quietly become "never check anything". The control passes both before and after the fix, so it is a control and not a mirror. This does not change the number of passes. The declarations entry still walks 14,368 files about twice a second, which is roughly 8 percent of a core in getdirentries and glob matching. That is issue 57 and it is separate.
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.
Closes #56.
The fault
materialize_trackedread and hashed EVERY present file on EVERY pass. On aconverged tree that answer is always "unchanged", so the read and the hash were
pure waste.
Measured on the live Silber daemon, version 0.2.0+fff659e, before #57 landed:
one entry re-read and re-hashed 70,157,702 bytes every 0.51 s, which is
136 MB/s.
blake3_hash_many_neonwas 14.22 percent of one core, the largestsingle consumer. 99.6 percent of those bytes belonged to three files that never
change.
Read this after #58, because it changes what this PR is worth
#58 fixed the cause: a converged tree no longer full-scans twice a second. At
the expected one pass per 300 s, this entry's re-hash cost falls from 136 MB/s
to about 0.23 MB/s on its own.
So this is no longer the fix. It is defense in depth, and it still earns its
place for two reasons:
st2-bus-defaultentry re-reads 14,247 files per pass. The bytes aresmall at 12.5 MB, but it is 14,247 reads and hashes each time.
reduces the number of passes; it does not make a pass cheap.
I would not have merged this before #58, and cos sequenced it that way.
The change
This asks the same question
scan_folderasks, from the same evidence: size andboth mtime components byte-identical to what THIS MACHINE last observed.
Trusting that is the existing contract, not a new one.
scan_folderalreadyruns first in every pass and already trusts the cache, so re-deriving the
opposite answer here, from the same disk, was incoherent rather than safe.
materialization_does_not_manufacture_an_mtime_collisionalready pins that alocal cache entry is trusted by design, and that only this machine can create
one, so no peer can manufacture a hit.
Anything unknown, differing, or unreadable takes the full read-and-hash path
exactly as before.
The behaviour change I am not hiding
Drift that preserves both size and mtime is no longer repaired by
materialization.
scan_folderwould not have noticed it either, so that repairwas never coherent. That is the argument for the change, not a caveat I found
afterwards.
Was the bypass deliberate?
cos asked me to check this before writing anything, and to stop if the bypass
was load bearing. It is not.
There is no comment, no test and no rationale for materialize re-reading. The
documented incident in that code, Linux CI run 30814462024 attempt 4 with 24
republishes from v44 to v67, was the SCAN trusting a REPLICATED manifest. PR 47
fixed that by keying the cache locally, and its test states the resulting
position outright: "a local cache entry is trusted by design; only its ORIGIN
changed".
Tests
A read cannot be observed directly, so the test makes the read CHANGE THE
ANSWER. The disk carries bytes that differ from the recorded hash while wearing
the same size and mtime. A materialization that reads sees drift and republishes
it; one that trusts the cache leaves it alone.
Run against the pre-fix code it fails, on the republish assertion.
A control asserts a file the cache does NOT cover is still read and hashed, so
the fast path cannot quietly become "never check anything". The control passes
both before and after the fix, so it is a control and not a mirror.
Verification
macOS on the rebased branch: 231 lib, 3 sync_slice, 21 local_slice.
engine.rsis rustfmt clean and clippy matches
mainwith an empty set difference bothways.
I also tested this together with #58 on a throwaway branch before either merged,
because a clean
merge-treesays nothing about behaviour and this fast pathdepends on the cache freshness that #58 makes rare. It holds:
sync_oncealwaysscans immediately before materializing under one operation guard, and the
standalone
materialize_entrypath is stat-validated, so stalenessself-corrects.
That is a macOS claim. This PR's CI run is the Linux face and I read the job
steps before calling it green.