sync: stat each file once per scan, not twice - #66
Merged
Conversation
`scan_folder` called `child.metadata()` twice for every file it keeps.
`DirEntry::metadata` is a fresh `lstat` on each call, so the second one
bought nothing:
let (mtime_secs, mtime_nanos) = mtime_of(&child); // lstat
let disk = child.metadata().ok(); // lstat again
`mtime_of_metadata` already existed and takes a `&Metadata`, so the
single-call shape was in the file the whole time. `mtime_of` was a
`DirEntry` wrapper around it that forced the extra call. It has no other
caller and is removed.
MEASURED, AND IT IS A SMALL WIN. Do not read it as a large one.
The watched tree on Silber holds 17,063 files. One `lstat` pass over all
of them costs 36 ms wall on this machine, warm, best of three. The bus
entry ran 12.2 scans a minute over a 419 second window, so the redundant
call costs about 0.43 s of CPU a minute, or 0.72 percent of one core.
Treat 0.72 percent as an UPPER bound. I measured the syscall from Python,
whose per-call interpreter overhead is included in the 2.08 us per file
and would not be present in this loop.
WHAT THIS DOES NOT FIX, SO NOBODY READS IT AS THE ANSWER. The dominant
idle cost on Silber is that one changed file triggers a full walk of the
whole tree, about 21 times a minute. Halving the stats per file makes the
walk cheaper. It does not stop the walk from happening.
Agent: Silber.fabric
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
1 participant
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 this changes
scan_folderstat'ed every file it keeps twice, once insidemtime_ofandagain on the next line.
DirEntry::metadatais a freshlstaton each call, sothe second one bought nothing.
mtime_of_metadataalready existed and takes the&Metadata, so the one-callshape was in the file the whole time.
mtime_ofwas theDirEntrywrapper thatforced the extra call, it has no other caller, and it is removed.
Without this, every scan pays one redundant
lstatfor every file in the tree.What was verified, and what was not
Platform: macOS 15 on arm64 (Silber). Not run on Linux by me — CI covers that.
cargo test --lib— 121 sync tests pass, 0 failed.cargo build— clean, and it introduces no new warning.No new test, and I want to be honest about why. This change removes a
duplicate syscall and changes no behaviour, so a test that passes only after it
would be a mirror of the change rather than a check on it. The existing scan
tests are the control: they cover mtime, size and the executable bit, which are
the three things read from the metadata this touches, and they pass unchanged.
Measured, and it is a SMALL win.
The watched tree on Silber holds 17,063 files. One
lstatpass over all of themcosts 36 ms wall, warm, best of three. The bus entry ran 12.2 scans/min over a
419 second window, so the redundant call costs about 0.43 s of CPU per minute,
which is 0.72% of one core.
Read 0.72% as an upper bound. I timed the syscall from Python, so its
per-call interpreter overhead is inside the 2.08 us/file figure and would not be
present in this loop.
What I did not measure: syscalls per pass on a deployed daemon carrying this
change. That needs a release and a roll, and I did not want to cut one for a
sub-1% fix on its own.
What this does not fix
The dominant idle cost on Silber is that one changed file triggers a full walk
of the whole tree, about 21 times a minute. This makes the walk cheaper. It
does not stop the walk. That is the next piece of work and it is a much larger
number than this one.