Block cache, coalescing, and single-flight - #12
Merged
Conversation
Phase 3, and the first release that changes the recorded numbers on purpose.
libs/usd-asset-cache sits between the resolver and a backend as an AssetReader
that wraps an AssetReader, so nothing above it knows it is there and nothing
below it changes: block alignment, coalescing, single-flight, and eviction, over
a CacheKey that carries the validator from the first commit.
What it moves, against the table v0.2.0 recorded as the definition of success
(docs/reference/BASELINE.md, "What the next release has to move"):
header and index read 18 requests -> 3
parallel readers 152 requests -> 25
bounded query amplification 1.000000 -> 4.543210, and
bytesOverFetched now non-zero and honest about it
full sequential read 33 requests, amplification 1.000000 -- unchanged,
which is the row that had to not regress
The fourth line is the one worth stating twice. A cache that improves the first
three and quietly damages the worst case has not improved anything, and the
bypass threshold is what keeps it: a read above 1 MiB goes straight to the
backend rather than through a store it would evict itself out of.
**The constants are measured, and the measurement is recorded rather than
implied.** tests/cache-tuning sweeps four access patterns across five block
sizes and four coalescing gaps -- sixty-six runs, each against a fresh store so
no row is warmed by the row above it -- and docs/reference/BLOCK_POLICY.md is
the record. 64 KiB blocks and a gap of one block are chosen from request and
byte counts alone, because those are exact on every machine and a wall clock on
loopback is a fact about the runner. Two of the five constants were not measured
and say so in the table: the request ceiling is a safety bound and the budget is
a residency policy. A number in a table under a heading about measurement, which
was not measured, is the failure that file exists to prevent.
The premise the harness cannot measure is stated outright rather than assumed
quietly: on any link this project targets, one round trip costs more than one
block of bytes. That is CACHE.md §2 as an assumption, and it is what makes fewer
requests and more bytes the better trade. It gets its own measurement in v0.5.0,
when a consumer puts real distance between the reader and the origin. Until then
the direction of the trade is assumed and only its magnitude is measured.
The cache enters the shared boundary suite as a row rather than as a suite, for
the third time: boundary_cached_local, the cached backend over the local one,
which is the configuration where the oracle can still say what every byte should
be. A cache that returns a byte from the wrong block fails there against the
same naive oracle usdAssetLocal has been checked against since v0.1.0.
Cache counters join the metrics that already existed, so the block cache reports
hits, misses, over-fetch, and requests saved by single-flight in the same dump
every other counter appears in.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ebuild Eight findings from the review of this branch. Two are defects that a test cannot reach, three are counters that were wrong, and three are a comment or a bound saying something the code does not. **The store's rebuild raced its own admission check.** `ConfigureProcess` took `identityMutex`, read `liveBindings == 0`, released it, and *then* called `Reset`, which clears and rebuilds `shards` with no lock held. A `Bind` that had already passed the same check could hand back a binding whose stripe index pointed into the vector the rebuild was about to free. The check and the rebuild are one critical section now; `Reset` split into a locking wrapper and a `ResetLocked` the configure path calls with the lock already held. What makes it sufficient is the binding destructor's ordering, which was already right: `liveBindings` drops as its last act, after the binding has finished touching every stripe, so zero under this lock means nobody is inside `shards`. **An exception left blocks pending for the life of the process.** `Abandon` documents the invariant -- every `Owned` acquisition ends in exactly one `Publish` or one `Abandon` -- and the release paths kept it across every `return` and across no `throw`. Both allocations on that path can throw: the transfer buffer is sized by the run and may be `maxRequestBytes`, and `Publish` copies the block. A `bad_alloc` out of either left every unpublished block of that read pending forever, and `Await` has no deadline, so the next reader of one of those blocks waits forever too -- a hang, in a process that never saw an error. Ownership is a destructor now. `Publish` allocates before it touches the store, so a throw from it leaves the block still owned and the guard still correct. **`bytesOverFetched` did not count the gap, which is most of what it exists to count.** It was charged as the bytes of a run falling outside the caller's byte range. Coalescing across a block that is already resident re-transfers that block, and those bytes sit *inside* the range -- so the counter called them wanted and charged nothing, while the wire moved them and the caller read that block from the store. It is charged against what the caller took out of each transfer now, which counts both halves: the alignment slack outside the range and the gap inside it. The recorded numbers move with it, in exactly one row of one table, and that row is the one the coalescing constant is chosen from. BLOCK_POLICY.md's interleaved re-read at 4 KiB blocks recorded a gap of one block as costing 0 bytes; it costs 28672. The prose two sections below it already said 28672 -- it was worked out by hand -- so the file has been disagreeing with itself since it was written, and the generated half was the wrong half. The trade the constant is chosen on is unchanged and now legible: seven fewer round trips for 28 KiB. **A read served entirely by single-flight was invisible.** Blocks obtained through `Await` bumped `bytesFromCache` and `requestsSavedBySingleFlight` and neither of the two counters that classify a read, so such a read landed in none of `blockHits`, `blockMisses`, or `partialHits`. They count as what they are -- blocks this reader did not fetch -- which is what `blockMisses` deliberately excludes them from. BASELINE.md's parallel row moves with it: `blockHits` 16 to 127, and `requestsSavedBySingleFlight` 153 to 156, which is that row's documented run-to-run variance rather than the fix. **The gap ceiling was off by one.** Merging across G blocks puts G + 2 in the request, so a ceiling of N blocks admits a gap of N - 2, not N - 1. With a 4 KiB block and a 16 KiB ceiling the normalizer resolved the gap to 3, and a gap of 3 needs 20 KiB -- a resolved option `PlanRuns` can never take, which is the exact thing the cap exists to prevent. The shipped defaults are unaffected: 128 blocks per request against a gap of 1. **Two clamps were silent and one comment described a bound that is not there.** The budget's floor is `kMinBlockSize`, not one block of the configured size, so a large block size and a small budget passed the parse and were then raised by the normalizer with nothing reported -- unlike the block size and the gap, which both report. The budget and the request ceiling report now, and the comment says what the floor is. **The resolver's comment named the guarded entry point and the code called the unguarded one.** `WrapAsset` checks `supportsRandomAccess` and passes such a reader through undecorated; `Wrap` cannot, because it returns a `CachedAssetReader` and has nothing to pass through -- and its own doc comment claimed the check anyway, which is a third statement of the same confusion. The resolver calls `WrapAsset`, and `Wrap`'s doc says what it does. Latent rather than live: ADR-0002 makes range support a hard error at open, so every reader reaching that line has it, which is how long a missing guard stays invisible. **And `ParseByteRange` was pasted into the middle of `TestRangeRead`'s doc comment**, leaving §4's sentence attached to the parser and "out of an asset costs the window." dangling above the test. Reunited. Verified: 29/29 on Windows with the bundle, and 25/25 under ASan+UBSan and 25/25 under TSan in WSL under GCC 15.2 -- the lanes that are the only real check on the first two findings. The over-fetch fix is pinned by an assertion in the gap test that the old accounting fails, and the recorded tables were regenerated and diffed rather than edited by hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The code for the block cache landed in the two commits before this one; what was left is every document that had said the cache was planned, and one that turned out to be asserting something narrower than the contract it is the executable form of. That one is worth naming. The boundary suite's mid-read revision case asserted `AssetChanged` on a re-read of a range the reader had already read. A block cache answers that read from bytes it captured under the same binding, observes nothing, and returns the revision the reader is bound to -- which is the guarantee rather than an exception to it: ASSET_READER.md §2.1 already says *observes*, and CACHE.md §6 already admits in-memory caching for the reader's lifetime. The case now asserts `AssetChanged` at an offset the reader has not read, and for a repeated range asserts either `AssetChanged` or byte-for-byte what the first read returned, never the new revision. It is a strengthening: the byte comparison is new, and the old case would have passed a backend that rebound *and* reported `AssetChanged`. The rest is bookkeeping that the invariants require: - WORKSPACE.md gains the fifth reverse edge onto the fixture server, for tests/cache-tuning, and says why that one needs a server for a reason of its own -- the constants it measures are about round trips, and a sweep over a local file would be a sweep over a cost that does not exist there. - CONFIGURATION.md fills in the four cache defaults with the measured values and states the two rules the code follows: a block size that is not a power of two is rounded down and the rounding is reported, and a value outside the bounds is refused rather than clamped. - libs/usd-asset-cache/README.md, which invariant 10 requires and which states what the module refuses to own -- no transport, no revalidation, no validator interpretation, no read-ahead. - METRICS.md records the rule the decorator forced: a decorated stack has one counter set, because the two ends disagree about what `bytesRequested` means. - CAPABILITY_MATRIX.md, the roadmap, the design policy's assessment, the changelog, and the CI comment that counted three libraries. Both lanes are green on this tree: 25 of 25 under core-msvc, 29 of 29 in the plugin lane including httpResolver_stage, and 25 of 25 under each of core-asan and core-tsan on GCC 15.2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two bundle cells have been failing since the cache landed, and the two local lanes and core-ci.yml have been green throughout, which is the whole shape of the defect. A bundle declares its library edges twice: to the build graph, which resolves them as in-tree targets, and in openstrata.plugin.yaml, which is the list `ost plugin build` installs into the workspace prefix before it configures the bundle standalone. usdAssetCache was added to the first and not the second. usdAssetHttp carries usdAssetIo transitively, so the bundle never had to name it and the descriptor's one-line list had never been wrong before. usdAssetCache is that library's sibling over usdAssetIo rather than anything above it -- the cache is a decorator and knows no transport concept -- so nothing carries it and it has to be named. Reproduced and fixed against the path the cells take, not against the in-tree build: `ost plugin build plugins/http-resolver` now installs usdAssetCache into the workspace prefix and configures, and `ost plugin test --up-to 1` is 8 pass, 0 fail, 4 skip. The Windows leg of that still needs CMAKE_PREFIX_PATH in the environment for libcurl, which is blocking item 3 and unrelated. `verify: graph` is the rung that would have caught this, and it counts the edge: it now reports 5 library edges where it reported 4. Both places that state the edge list say so -- WORKSPACE.md §2 next to the list a reader would otherwise trust, and the cell's comment in openstrata.ci.yaml, which had the old count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Phase 3.
libs/usd-asset-cachesits between the resolver and a backend as anAssetReaderwrapping anAssetReader, so nothing above it knows it is thereand nothing below it changes.
What it moves
Against the table
v0.2.0recorded as thisrelease's definition of success — BASELINE.md § What the next release has to
move:
v0.2.0amplificationbytesOverFetchednon-zero and honest about itThe last row is the point. A cache that improves the first three and quietly
damages the worst case has not improved anything; the 1 MiB bypass threshold is
what keeps it, by sending a large read straight to the backend rather than
through a store it would evict itself out of.
The constants are measured
tests/cache-tuningsweeps four access patterns across five block sizes andfour coalescing gaps — sixty-six runs, each against a fresh store so no row is
warmed by the one above it — and
docs/reference/BLOCK_POLICY.mdis therecord. 64 KiB blocks and a gap of one block are chosen from request and byte
counts alone, because those are exact on every machine and a loopback wall clock
is a fact about the runner.
Two of the five constants were not measured and say so in the table: the
request ceiling is a safety bound, the budget is a residency policy. And the one
premise the harness cannot measure is stated outright rather than assumed
quietly — that one round trip costs more than one block of bytes — which is what
makes "fewer requests, more bytes" the better trade. It gets its own measurement
in
v0.5.0.How it is checked
The cache enters the shared boundary suite as a row, for the third time:
boundary_cached_local, the cached backend over the local one, which is theconfiguration where the naive oracle can still say what every byte should be. A
cache that returns a byte from the wrong block fails there.
Verified locally on Windows, MSVC, Release, with the bundle: 29 of 29 tests
pass, up from 22 — the new ones being the cache's own unit tests, the
single-flight test, the cached boundary row, and the block-policy sweep.
Not in this PR
CHANGELOG.mdanddocs/roadmap/implementation-status.mdare not yet updatedfor phase 3.
single-flight is exactly the code TSan exists for.
tests/cache-tuning/tuning_main.cppemits MSVC C4819: it has an em dashinside a
printfformat string, and unlike the non-ASCII elsewhere in thistree — which lives in comments — a string literal has to be converted to the
execution charset. Cosmetic, and left alone here.
🤖 Generated with Claude Code