Skip to content

fix(macos): the sweep reopened its path list once per changed file - #166

Open
h1d3mun3 wants to merge 1 commit into
topic/virtiofs-cachefrom
perf/macos-sweep-list-accumulation
Open

fix(macos): the sweep reopened its path list once per changed file#166
h1d3mun3 wants to merge 1 commit into
topic/virtiofs-cachefrom
perf/macos-sweep-list-accumulation

Conversation

@h1d3mun3

@h1d3mun3 h1d3mun3 commented Aug 5, 2026

Copy link
Copy Markdown
Owner

The loop that accumulates the NUL-separated path list held its >> "$list" redirect on the printf, so it opened, wrote and closed the list file once per changed file. That reopen is what ADR-0016 §Cost's "~130 µs per changed file, i.e. ~99% of the host-side cost" measured, and it is the term the --share-refresh dial exists to work around.

Moving the redirect onto the enclosing while … done — one open per root instead of one per file — is the entire functional change. Two lines.

Measured

Through _refresh_macos_shares_locked itself, with ssh_macos stubbed so the reading is the host half alone; 12,628 files across four roots, bash 3.2, best of three:

before after
Host-side term (find + accumulation) 122.0 µs/changed file 64.7 µs
Full blind sweep of a 12,628-file tree 1541 ms 817 ms
End to end, with the guest's 0.23 ms ~0.35 ms/file ~0.29 ms
Crossing point vs the 5 s interval ~14,200 files ~17,000

The 122.0 µs "before" corroborates the snapshot's ~130 µs from a different machine and method, which is what makes the comparison usable at all.

It does not retire the dial

Every doc this touches now says so rather than leaving a reader to infer it. The guest's own 0.23 ms per file is untouched by anything host-side and is now ~78% of the total, so halving the host half moved the crossing point ~20% and no further. ADR-0016 §4's argument for --share-refresh stands unchanged.

It also does not help the common case — a realistic incremental sweep is a few dozen files, where the saving is well under a millisecond. What it helps is the large sweep: a branch switch, a build, the first blind sweep on a fresh marker.

Why the inner loop and not the outer

Load-bearing, not stylistic. warn writes to stdout (augur:123), so a redirect spanning the outer body would put any warning added there into the NUL path list, and the guest would be handed a path built from a sentence — the exact trap macos_share_roots documents at its own error-not-warn line. One open per root is already almost all of the win; the remaining three opens do not buy that risk.

Correctness

Byte-equivalence with the per-printf form was checked against paths containing spaces, tabs, embedded newlines, quotes, non-ASCII and a leading dash: identical bytes, identical NUL count. tests/41's existing "a filename containing a NEWLINE crosses as exactly one path" arm covers the same property against the real sweep and stays green.

Tests

tests/41 gains four arms, because a revert is invisible to every other arm in the file — the output is byte-identical, so the whole suite stays green while the sweep silently costs twice what it should. They pin that the printf carries no redirect, that the inner done does, and — the control that guards more than cost — that the outer one does not.

Mutation-checked both directions against a 148/0 control: reverting the redirect onto the printf turns two arms red; hoisting it onto the outer loop turns two red.

Stale figures corrected wherever they were quoted as current: augur's --share-refresh comment and --help, the project-settings header, README's operator section, tests/43's header, and ADR-0016 §Cost — which gains a dated amendment and keeps the superseded reasoning, marked, because it is the argument the amendment reversed.

Measured in a Virtualization.framework guest on local APFS, not on a real host. That caveat is recorded in ADR-0016; the ratio is a property of the shell loop and does not depend on it.

Full suite: ALL GREEN (30 scripts).

🤖 Generated with Claude Code

The loop that accumulates the NUL-separated path list held its `>> "$list"`
redirect on the `printf`, so it opened, wrote and closed the list file once
per CHANGED FILE. That reopen is what ADR-0016 §Cost's "~130 µs per changed
file, i.e. ~99% of the host-side cost" measured, and it is the term the
`--share-refresh` dial exists to work around.

Moving the redirect onto the enclosing `while … done` — one open per ROOT
instead of one per FILE — is the entire functional change. Two lines.

MEASURED through `_refresh_macos_shares_locked` itself, with `ssh_macos`
stubbed so the reading is the host half alone; 12,628 files across four
roots, bash 3.2, best of three:

    host-side term      122.0 → 64.7 µs per changed file
    full blind sweep    1541  → 817  ms for a 12,628-file tree

The 122.0 µs "before" corroborates the snapshot's ~130 µs from a different
machine and method, which is what makes the comparison usable at all.

IT DOES NOT RETIRE THE DIAL, and every doc this touches now says so rather
than leaving a reader to infer it. The guest's own 0.23 ms per file is
untouched by anything host-side and is now ~78% of the total, so halving the
host half moved the crossing point where one sweep outlasts the 5 s interval
by ~20% — ~14,200 → ~17,000 changed files — and no further. §4's argument
for `--share-refresh` stands unchanged. It also does not help the common
case: a realistic incremental sweep is a few dozen files, where the saving is
well under a millisecond. What it helps is the large sweep — a branch switch,
a build, the first blind sweep on a fresh marker.

THE REDIRECT IS ON THE INNER LOOP, NOT THE OUTER ONE, and that is
load-bearing rather than stylistic. `warn` writes to STDOUT (augur:123), so a
redirect spanning the outer body would put any warning added there INTO the
NUL path list, and the guest would be handed a path built from a sentence —
the exact trap `macos_share_roots` documents at its own `error`-not-`warn`
line. One open per root is already almost all of the win; the remaining three
opens do not buy that risk.

Byte-equivalence with the per-`printf` form was checked against paths
containing spaces, tabs, embedded newlines, quotes, non-ASCII and a leading
dash: identical bytes, identical NUL count. tests/41's existing
"a filename containing a NEWLINE crosses as exactly one path" arm covers the
same property against the real sweep and stays green.

tests/41 gains four arms, because a revert is INVISIBLE to every other arm in
the file — the output is byte-identical, so the whole suite stays green while
the sweep silently costs twice what it should. They pin that the `printf`
carries no redirect, that the inner `done` does, and — the control that
guards more than cost — that the OUTER one does not. Mutation-checked both
directions against a 148/0 control: reverting the redirect onto the `printf`
turns two arms red, hoisting it onto the outer loop turns two red.

Stale figures corrected wherever they were quoted as current: augur's
`--share-refresh` comment and `--help`, the project-settings header, README's
operator section, tests/43's header, and ADR-0016 §Cost — which gains a dated
amendment and keeps the superseded reasoning, marked, because it is the
argument the amendment reversed.

Measured in a Virtualization.framework guest on local APFS, not on a real
host. That caveat is recorded in ADR-0016; the ratio is a property of the
shell loop and does not depend on it.

Full suite: ALL GREEN (30 scripts).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

🧭 Egress drift reminder

This PR changes the egress core (augur / augur-proxy/ / gvproxy/ / augur.conf / augur-vm/) but does not touch docs/security-reviews/.

If this change alters the egress posture, please:

  • run /egress-survey (the bounded drift audit), and
  • if it drifted: add a new dated snapshot under docs/security-reviews/ and/or update docs/security-reviews/INVARIANTS.md.

Non-blocking reminder. It does not verify correctness (that is the egress fail-closed E2E + swift test). If no doc update is needed, ignore this.

@h1d3mun3
h1d3mun3 changed the base branch from main to topic/virtiofs-cache August 31, 2026 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant