Repository navigation
metal: stop waiting for the GPU to read GPU-written indices and DCC keys - #515
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b79de00218
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const bool skipScan = !cpuNeedsIndices && timeline != nullptr && | ||
| timeline->MayHaveWritten(draw.indexAddress, draw.indexAddress + bytes) && | ||
| WholeVertexRangesBindable(vertex->program->vertexAttributes, draw); |
There was a problem hiding this comment.
When the index range cannot use a no-copy host alias—for example, file-backed, unaligned, or Metal-refused memory—DispatchSnapshot creates a private copied buffer. The pending producer then writes its own copy, and guest memory is not updated until its timeline completion runs; meanwhile this draw's resource snapshot has already copied the stale guest bytes. Allowing skipScan based only on borrowability therefore binds stale indices rather than the producer's GPU output. Restrict this path to ranges proven to share a no-copy alias, or drain before creating/binding a copied snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c86ff4a: the scan is skipped only when the index binding is a no-copy alias of the guest bytes (its contents pointer is the borrowed host range). A copied snapshot waits and scans as before. Note that the copy is taken when the resources object is built, so the old drain-then-scan path read the same pre-producer copy too; this path never binds one unscanned.
| const auto type = userData[3] >> 30u; | ||
| const auto format = (userData[3] >> 12u) & 0x7fu; | ||
| if (type != 0 || stride != 16 || swizzled || addTid || dstSel != 0xfacu || format != 0x4bu) return std::nullopt; | ||
| const auto records = std::min<std::uint64_t>(std::uint64_t{groups[0]} * 64u, userData[2]); |
There was a problem hiding this comment.
For a thread-count DISPATCH_DIRECT whose X dimension is not divisible by 64, DecodeComputeDispatch rounds groups[0] up and records the actual limit in compute.partialThreads, but this matcher sees only the rounded group count. If userData[2] exceeds the actual thread count, records claims that the partial group's inactive lanes stored bytes they never touch; PendingBytes can then classify an only-partially-filled DCC range as entirely cleared without waiting. Pass the partial-thread state into this matcher and size or reject such fills, as the Vulkan fill path already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c86ff4a: MatchConstantFill takes compute.partialThreads and rejects a dispatch with a partial last group, as the Vulkan fill path does.
c9af850 to
c86ff4a
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c86ff4a9f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (skipScan && static_cast<const std::byte*>(indexBuffer.buffer.contents) + indexBuffer.offset != | ||
| NativeGuestMemory::ContiguousBorrowedRange(draw.indexAddress, static_cast<std::size_t>(bytes), false).data()) { | ||
| skipScan = false; | ||
| if (cpuReadHook) cpuReadHook(draw.indexAddress, bytes); | ||
| } |
There was a problem hiding this comment.
When skipScan is initially true but the index binding is a copied mirror, this fallback drains only after resources.Buffer has returned that copy. The fresh ordering evidence is that MetalShaderResources is constructed earlier in this function and its constructor immediately builds DispatchSnapshot, whose copied components are populated with memcpy; draining updates the borrowed guest bytes but never refreshes indexBuffer. The subsequent scan therefore still sees pre-producer indices and the draw can use stale data. Drain before constructing the snapshot or rebuild the copied binding after the drain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This one is already on main and this PR does not change it. On main the draw drains (cpuReadHook) after the MetalShaderResources constructor has built the DispatchSnapshot copy, then scans and binds that same pre-drain copy. This PR's fallback for a copied index snapshot follows exactly that path, and the skip-scan path never binds a copy. Refreshing copied snapshot components after a drain would touch every copied binding (vertex buffers too), so it belongs in a separate change. Leaving it out of scope here.
| const auto stride = (attribute.resource.fields[1] >> 16u) & 0x3fffu; | ||
| if (NullVertexDescriptor(attribute) || attribute.fetchIndex != 0 || stride == 0) return VertexBufferReadSize(attribute, 0, instances, firstInstance); | ||
| return VertexBufferExtent(attribute); |
There was a problem hiding this comment.
For a strided per-vertex descriptor with OOB_SELECT 0 or 1, indices at or beyond NUM_RECORDS are valid here: VertexBufferReadSize deliberately permits them and sizes the binding through the actual index. If a pending GPU-produced index, or that index plus baseVertex, exceeds the record count, this new helper instead returns only records * stride; the skip-scan path then binds a shorter Metal buffer and reads out of bounds rather than fetching the guest bytes that the previous scanned path exposed. Only use the whole-range optimization when the descriptor's record count actually bounds every index, or retain the scan for these modes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d336982: WholeVertexRangesBindable refuses a strided per-vertex attribute with OUT_OF_BOUNDS_SELECT 0 or 1, so such draws keep scanning. For select 2 and 3, VertexBufferReadSize already requires every index to be below NUM_RECORDS, so the whole record range bounds the fetch.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d336982d40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const auto fault = CompleteCommittedWork([&] { return bindings->Complete(commands); }); | ||
| completed(fault); | ||
| }, stores, bindings->WrittenGuestRanges()); | ||
| }, stores, bindings->WrittenGuestRanges(), std::move(known)); |
There was a problem hiding this comment.
When a deferred compute dispatch writes a DCC-backed storage image while an earlier key writer is pending, CommittedWithoutWaiting() creates deferred key stores, but this commit publishes only the separately matched fill in known. Because WrittenGuestRanges() still records the compute dispatch's newer key write, PendingBytes() rejects the older fill and the next image bind drains the timeline, reintroducing the GPU wait this path is intended to avoid. Merge bindings->KnownStores() into known here, as the draw commit path already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 96cfde0: the dispatch commit appends bindings->KnownStores() after CommittedWithoutWaiting, as the draw path does.
96cfde0 to
6137755
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6137755424
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const auto bytes = Graphics::WholeVertexReadSize(attribute, draw.instanceCount, draw.firstInstance); | ||
| const auto& fields = attribute.resource.fields; | ||
| const auto address = fields[0] | (std::uint64_t{fields[1] & 0xffffu} << 32u); | ||
| if (bytes > MaximumBytes || NativeGuestMemory::ReadableBorrowedBytes(address, bytes) < bytes) return false; |
There was a problem hiding this comment.
ReadableBorrowedBytes only proves that the descriptor range is borrowed; it does not prove that DispatchSnapshot created a no-copy Metal alias. If pending GPU work writes both generated indices and vertex data, and the index range is aliased but an unaligned, file-backed, or otherwise refused vertex range is copied, skipScan remains enabled and the later resources.Buffer binds the vertex snapshot captured before the producer completes. The draw then consumes fresh indices with stale vertices. Require every whole vertex binding to share its borrowed host range, or drain and rebuild the copied resource snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same pre-existing limit as the copied-index comment above, not one this PR adds. A copied vertex snapshot is made when MetalShaderResources is built, before any drain in the draw, so on main the scanned path also binds a vertex copy taken before the producer, even though it drains first. In both paths the drain never refreshes a copy, so skipping the scan does not make the vertex data any staler. Fixing copied snapshots of GPU-written ranges (re-snapshot after a drain) is a separate change.
Two CPU reads of guest memory that the previous GPU work had just written made the queue worker wait for that work on every frame of the live map. Index scan (MetalDraw::Draw): the scan of an indexed draw's indices only bounds the vertex buffer ranges, unless the CPU needs the indices themselves (fan expansion, strips without primitive restart, mesh fans, bounded fetches). When committed GPU work may still be writing the index buffer (GPU culling output), the index binding is a no-copy alias of the guest bytes, and every vertex descriptor's record count bounds its index (OUT_OF_BOUNDS_SELECT 2 or 3), the draw no longer reads the indices: it binds each vertex buffer over its whole record range (WholeVertexReadSize) and the GPU reads the indices after their producer. DCC keys (MetalShaderResources::textureKeys): games clear DCC keys with a constant fill kernel. The Metal dispatch recognises it (MatchConstantFill, as the Vulkan driver does; partial last groups excluded) and tells the timeline what it stores (MetalTimeline::KnownStore). PendingBytes answers what a range holds once pending work completes when the newest pending store over it is such a known store, no later work writes it and no BDA writes are pending. Every byte the CPU sees there must be one a pending known store writes or found there when committed; anything else (a guest CPU write) makes the read wait as before. A draw or dispatch that writes a DCC image while such a fill is pending stores its "uncompressed" keys in Complete instead of at commit (the fill would land over an earlier CPU store) and reports them as a known store. The timeline now retires a work's written ranges when Reap retires the work, instead of only when nothing is pending. Tests: a GPU-written index buffer (list drawn without a CPU read, strip without restart still read), DCC keys under a pending GPU clear versus after a guest CPU write, the deferred uncompressed store, and the timeline's known-store rules. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DgW4n3qYgi5jAAohEmXRQj
6137755 to
4b17a57
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b17a573ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const auto fill = MatchConstantFill(std::span(snapshot.code).subspan(offset), decoded.userData, decoded.compute.numThreads, decoded.groups, | ||
| decoded.compute.partialThreads); |
There was a problem hiding this comment.
For a GPU-side DISPATCH_INDIRECT, the caller invokes this function with a synthetic 1x1x1 direct packet because the real group counts remain in the indirect argument buffer. This therefore publishes a 64-record KnownStore even when any actual indirect dimension is zero and Metal executes no threads; a subsequent DCC lookup can then accept the predicted fill pattern although the keys were never written, causing the wrong texture representation to be used. Disable fill prediction when indirectArguments is present unless the ordered real counts are available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fa6bbde: no fill prediction when indirectArguments is set.
| const auto bytes = Graphics::WholeVertexReadSize(attribute, draw.instanceCount, draw.firstInstance); | ||
| const auto& fields = attribute.resource.fields; | ||
| const auto address = fields[0] | (std::uint64_t{fields[1] & 0xffffu} << 32u); | ||
| if (bytes > MaximumBytes || NativeGuestMemory::ReadableBorrowedBytes(address, bytes) < bytes) return false; |
There was a problem hiding this comment.
ReadableBorrowedBytes proves contiguous guest-address coverage but deliberately counts adjacent borrowed spans whose host pointers are disjoint, while MetalShaderResources::mirror immediately requires the requested range to pass ContiguousBorrowedRange. For GPU-written indices whose actual maximum stays within the first span, the former scanned path could bind only that prefix, but this check enables the skip path and the subsequent whole-descriptor binding throws. Validate host contiguity here, inside the existing try, before opting out of the scan.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fa6bbde: WholeVertexRangesBindable also calls ContiguousBorrowedRange inside its try, so a range spanning host-disjoint borrowed spans keeps the scan.
…s must be one host run Review fixes: an indirect dispatch's group counts are in its argument buffer, so MatchConstantFill is not used for it. WholeVertexRangesBindable also requires each whole vertex range to be one host run (ContiguousBorrowedRange), as the binding needs, and scans otherwise. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DgW4n3qYgi5jAAohEmXRQj
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa6bbde033
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const auto address = begin + i; | ||
| const auto& store = newest->store; | ||
| out[i] = store.pattern[(address - store.begin) % store.pattern.size()]; | ||
| if (visible[i] == out[i] || visible[i] == store.before[address - store.begin]) continue; |
There was a problem hiding this comment.
If the fill has physically completed but has not yet been reaped, and the guest CPU restores a DCC byte to its pre-fill value (commonly 0xff), this condition treats that write as evidence that the fill has not landed because it equals store.before. PendingBytes then returns the fill pattern (for example 0x00) even though the CPU write is newer, causing textureKeys to select the wrong DCC representation. The same ambiguity exists for values matching an older store's pattern below; detecting post-submit CPU writes needs write/version tracking or a synchronization check rather than value equality.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
By design. The value check is a safety net, not the ordering guarantee. A guest learns that the fill finished only through a label or interrupt the driver stores when it reaps or drains the work (Reap/Drain run completions in commit order, and DeferSignal labels too). The fill's KnownStore is retired at that same point, so a CPU write made after the guest saw the fill complete is never compared against it. A CPU write that is not ordered after the fill races the GPU fill on hardware too, where either value may win. The Metal build has no guest write watching (GuestMemory::WriteWatched is off there), so value comparison is the strongest cheap check available. It still makes every byte outside the expected sets wait and read the real keys.
What
The live 3D map frame profile showed the queue worker blocking on the GPU in two CPU read-after-write drains (cause 0x102) every frame, so encoding and GPU execution ran one after the other:
MetalDraw::Draw): the CPU scanned an index buffer that the previous compute dispatch had just written (GPU culling output).MetalShaderResources::textureKeys): the CPU read DCC keys that the previous dispatch had just written. The producer is the guest's constant fill kernel clearing the keys.Both drains are gone from the live-map frame. A bind trace (
ANYPS5_METAL_BIND_TRACE) shows 5 "drain before CPU read" events per frame on main and 0 with this branch.How
Index scan. The scan only bounds the vertex buffer ranges, except where the CPU needs the indices themselves: fan expansion, strips without primitive restart, mesh fans and bounded fetches. When committed GPU work may still be writing the index buffer, the draw no longer reads it. Instead it binds each vertex buffer over its descriptor's whole record range (
WholeVertexReadSize, the range GPU-indirect draws already bind). The GPU then reads the indices after their producer. Three conditions must hold, or the draw scans as before:DCC keys.
MatchConstantFillrecognises the guest fill kernel. It is the same kernelDriver::matchesFillKernelruns as a buffer fill on Vulkan. Dispatches with a partial last group, and indirect dispatches, are not matched.MetalTimeline::KnownStore.MetalTimeline::PendingBytesgives what a range will hold once pending work completes, when all of these hold:Completeinstead. It also reports them as a known store, so later work learns them without waiting.Timeline. Each work's written ranges are now retired when
Reapretires that work. Before, they stayed until the whole timeline was empty, so a CPU read waited for unrelated work queued behind a writer that had already completed.Tests
GuestDrawReplay:DrawGpuWrittenIndicescovers an index buffer that pending GPU work writes.TextureViews:DccPendingFillcovers a resources object that binds DCC keys under a pending GPU clear.Completeand reports it as a known store.KnownStores(metal_utility_tests) covers the timeline rules: coverage, later writes, unknown writes, CPU-written bytes, chained stores and retirement on reap.Correctness and performance
Base is origin/main
ee4c46f68(#516). Each side was built separately, with the base copy's rpath re-pointed. Each side also ran on its own APFS clone of the capture.Drains. A bind trace (
ANYPS5_METAL_BIND_TRACE) of one live-map frame shows 5 "drain before CPU read" events on main and 0 with this branch.Correctness. The gate is
--expect-png replay-ref-v2.png --max-diff-pixels 200, checked on every timed repeat, 2 processes per side:That is no noise beyond what main shows.
A/B on the live map: 10 interleaved pairs under the replay lock, nothing else replaying,
--repeat 4(1 warm-up + 3 timed). Values are medians of the per-run medians.ANYPS5_NO_SHADER_CACHE=1was set on both sides, because pipeline-archive rewrites in the shared cache added seconds of noise per repeat.ANYPS5_REPLAY_NO_RESTOREdoes not exist on main, so these are normal-mode numbers.The worker does about 140 M more instructions per frame. The timeline holds more pending works now that it is not drained mid-frame, so
MayHaveWrittenscans longer lists.In game (
--map --from-save, 420 s, both runs reached the same map; this pair was measured on a #517 base, before the #516 rebase). The fps is the median of the last 20 frame-profile windows:Review findings
partialThreadsare not matched (as on Vulkan)KnownStores()ContiguousBorrowedRangecheck, scan otherwise🤖 Generated with Claude Code
https://claude.ai/code/session_01DgW4n3qYgi5jAAohEmXRQj