fix(gpu): drain resolved GPU timestamps at stop/teardown (#11)#20
Merged
Conversation
The per-frame DrainGpu() runs at different points on the two sides -- pre drains after qpc_exit (outside its bracket), post drains inside its bracket -- so when monitoring stops (ApplyToggle(false)) or the layer is torn down (~OpenXrLayer), each side can still be holding GPU timestamps the timer has ALREADY resolved but the writer never picked up. The sinks were closed without a final drain, so those samples were dropped and the last few frames' target_gpu_us went blank every session. Add a one-shot DrainResolvedGpuOnce() (best-effort, non-blocking) called right before g_gpuCsv.Stop() on both paths. It recovers the resolved tail; frames the GPU has not finished yet are still not recovered (that would need a blocking fence wait, which would add a stop hitch -- out of scope here). Scope: finding #11 only. The other GPU edge-cases are intentionally left as-is: #10 (D3D11 per-window vs D3D12 fixed timestamp frequency) is correct per-backend behavior already documented in the README; #9 (the bounded fence wait in Reset()/destructor) is a deliberate safety drain that returns as soon as the GPU catches up; #12 (a wedged writer after a failed thread join) degrades gracefully and is logged. Like the rest of the stop/teardown glue, this is covered by the build + review rather than a unit test (it needs a live layer + GPU timer). Co-Authored-By: Claude Opus 4.8 <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
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.
Finding #11 (code review): GPU tail samples systematically blank
The per-frame
DrainGpu()runs at different points on the two sides — pre drains afterqpc_exit(outside its bracket), post drains inside its bracket — so when monitoring stops (ApplyToggle(false)) or the layer is torn down (~OpenXrLayer), each side can still be holding GPU timestamps the timer has already resolved but the writer never picked up. The sinks were closed without a final drain, so those samples were dropped and the last few frames'target_gpu_uswent blank every session.Fix
A one-shot, non-blocking
DrainResolvedGpuOnce()called right beforeg_gpuCsv.Stop()on both stop paths. It recovers the resolved tail; frames the GPU hasn't finished yet are still not recovered (that would need a blocking fence wait → a stop hitch, deliberately out of scope). +26 lines,layer.cpponly.Scope — the other GPU edge-cases are intentionally left as-is
After investigating #9/#10/#12, only #11 is a clean correctness win; the rest are non-bugs or accepted tradeoffs:
Reset()/destructor) — a deliberate safety drain that returns as soon as the GPU catches up (~1 frame in practice); the destructor wait is required to free resources safely.Verification
Like the rest of the stop/teardown glue (and
MergeIntoOutput), this needs a live layer + GPU timer, so it's covered by the build + review rather than a unit test. No lines >120 cols.🤖 Generated with Claude Code