Skip to content

fix(vk): a recorder packet bigger than its room is always made directly (#2251 follow-up) - #2490

Merged
hckrman101 merged 1 commit into
NotRexed:mainfrom
hckrman101:fix/recorder-oversize-direct
Nov 11, 2026
Merged

hckrman101 merged 1 commit into
NotRexed:mainfrom
hckrman101:fix/recorder-oversize-direct

Conversation

@hckrman101

Copy link
Copy Markdown
Collaborator

tests/test_recorder_sync.py still fails now and then on the Windows "Python source checks" job after #2251 (for example on #2325 and #2447), with FAIL thread oversize: 0 calls made directly, 1 expected. The cause is in vk_recorder.h, not in the test: whether a packet bigger than its room was made directly depended on how far the recorder thread had got.

Why it flaked

The failure is not a late read of g_rec.direct. The producer thread bumps that counter itself, before the vkCmd* returns. Also, the failing process never logged did not fit in the room a packet is given, so the 4 KB vkCmdUpdateBuffer never took the direct path at all. It was written into the ring.

Recorder::reserve() waits until room bytes are free, but then lets the packet use everything free and contiguous ahead of it, min(tail, cap - (w - r_seen)). In the oversize scenario (8 KB ring, 2 KB room) that means:

  • If the recorder is behind, only about the room is free, so the 4 KB packet does not fit and is made directly. This is the usual case, and what the test expects.
  • If the recorder has caught up and the producer has just re-read R (a loaded runner that preempts the producer), up to 8 KB is free. The packet then fits, is packed, and direct stays at 0.

The log matched either way, which is why only the counter check failed. The header comment, g_ring_room's comment and the stats line all say a packet bigger than its room is made directly.

What changes

  • reserve() hands out exactly room bytes (lim = base + off + room). Those bytes are free and contiguous because wait_room() already waited for them, so the packet's size alone decides. In the renderer the room is 80 KB, sized so a 64 KB vkCmdUpdateBuffer fits. A larger packet now always takes the direct path, where before it was sometimes packed.
  • recorder_sync_test.cpp runs the oversize packet a second time, as the first packet in an empty ring: the recorder is stopped and its counters are moved to the ring's start. With the old reserve() the packet is packed there on every run, so the check no longer depends on timing to catch this.
  • test_recorder_sync.py adds a broken variant with the old limit, which must fail (caught). The "no backpressure" variant now matches the new line.

How it was verified

  • python -I -B tests/test_recorder_sync.py --headers <Vulkan-Headers> passes on Linux (clang++), with every broken variant caught, the new one included.
  • The new test against the old header (--only thread) failed 50 of 50 runs with FAIL thread oversize, ring empty: 0 calls made directly, 1 expected. That is the CI message, now deterministic.
  • Loaded runs of --only thread, 200 runs each for the old and new code, in two setups: lanes pinned to 1 CPU with a busy-loop on the same CPU, and lanes of 2 CPUs with a busy-loop on one of them. Every run passed. I could not reproduce the old flake on Linux, so the deterministic check above is the evidence for the cause. The 1e6-packet stress check still passes with every frame packet on the recorder.
  • The test compiles warning-free (-Werror) with llvm-mingw (x86_64-w64-mingw32-clang++) before and after the change. It was not run on Windows.
  • The change has no effect on calls that fit in the room, shown from the code. A packet is one vkCmd* call: each packer opens one Enc, which reserves once and emits once. publish() moves w by the packet's own size, not by lim, so the cap never ends an encoder early or splits a call. reserve()'s waits (wait_room(room), or tail + room at the wrap) and its r_seen refreshes are unchanged, and the old lim was never below room (tail >= room, and wait_room() had made room bytes free). So for every packet of room bytes or less, the same bytes go to the same place, with the same publishes, ring waits and no direct call, exactly as before. Only a single call whose packet is bigger than the room behaves differently: it is now always made directly (counted in the stats line's "calls made directly (no room)"), instead of being packed when the recorder happened to be far enough ahead. In the renderer the room is 80 KB. The test's frame still runs all 267 of its vkCmd* on the recorder thread, and the 1e6-packet stress check passes. I did not do a game run.

…ly (NotRexed#2251 follow-up)

test_recorder_sync's "FAIL thread oversize: 0 calls made directly, 1 expected"
on the Windows runners was not a late read of g_rec.direct: the counter is
bumped on the producer thread itself, before the call returns, and the
failing process never logged "did not fit": the 4 KB vkCmdUpdateBuffer was
written into the ring instead. Recorder::reserve() let a
packet use everything free and contiguous ahead of it,
min(tail, cap - (w - r_seen)), not the room it is given; so when the recorder
had caught up and the producer had just re-read R, an oversize packet fit in
the 8 KB ring and was packed, and whether it was made directly depended on
scheduling. The header (RX_VK_RECORDER=thread, g_ring_room) and the stats line
say a packet bigger than its room is made directly.

reserve() now hands out exactly `room` bytes (free and contiguous: wait_room()
guaranteed them), so the size alone decides. In the renderer the room is
80 KB (sized so a 64 KB vkCmdUpdateBuffer fits); a packet above it now always
takes the direct path the header describes, where it sometimes was packed.

The test runs the oversize packet a second time first in an empty ring (the
recorder stopped, its counters moved to the ring's start), where the old
reserve() packs it every time, and a broken variant with the old limit must
fail it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@hckrman101
hckrman101 marked this pull request as ready for review November 11, 2026 21:30
@hckrman101
hckrman101 merged commit 15646d1 into NotRexed:main Nov 11, 2026
@hckrman101
hckrman101 deleted the fix/recorder-oversize-direct branch November 11, 2026 21:33
dazemc added a commit to dazemc/Hookshot-rs that referenced this pull request Nov 12, 2026
Whether a packet bigger than its room was made directly depended on how far
the recorder thread had got. reserve() waited for  bytes, then let
the packet use everything free and contiguous ahead of it, min(tail,
cap - (w - r_seen)); with the recorder caught up, an 8 KB ring held a 4 KB
packet that its 2 KB room should have sent direct, so tests' direct-call
counter saw 0 where the header, the ring-room comment and the stats line all
promise a packet bigger than its room takes the direct path.

reserve() now hands out exactly  bytes: they are free and contiguous
because wait_room() waited for them, so a packet's size alone decides.

recorder_sync_test runs the oversize packet a second time as the first
packet in an empty ring (stopped, counters moved to the ring's start),
where the old limit packed it on every run.

Ported from upstream 15646d1 (NotRexed#2490).
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