MagicBeanDev/Hookshot

Archived

Watch

Fork 0

perf(vk): suballocate untouched tails of in-flight staging arenas #2469

Open

Erfboom wants to merge 1 commit from Erfboom/perf/vk-staging-suballocation into main

Erfboom commented

2026-11-11 20:29:31 +00:00

(Migrated from github.com)

Problem

When staging texture uploads, uncompleted in-flight arenas (value < rec_value) with unused tail capacity were skipped, forcing immediate allocation of fresh 64 MB arenas and pinning tens of megabytes of staging memory.

Solution

Untouched Tail Suballocation: Check in-flight arenas (a.value < rec_value) for remaining unused tail space if no completed arena is available, aligning past a.used to at least nonCoherentAtomSize and advancing the arena's timeline stamp to rec_value.
Test Coverage: Add inflight-tail case to tests/texture_staging_publication_test.cpp and tests/test_texture_staging_publication.py to verify that in-flight arenas are suballocated without allocating a new buffer and flush independently.

Verification

python3 -I -B tests/test_texture_staging_publication.py --headers /usr/include (passes all cases across coherent and noncoherent caches)

## Problem When staging texture uploads, uncompleted in-flight arenas (`value < rec_value`) with unused tail capacity were skipped, forcing immediate allocation of fresh 64 MB arenas and pinning tens of megabytes of staging memory. ## Solution 1. **Untouched Tail Suballocation**: Check in-flight arenas (`a.value < rec_value`) for remaining unused tail space if no completed arena is available, aligning past `a.used` to at least `nonCoherentAtomSize` and advancing the arena's timeline stamp to `rec_value`. 2. **Test Coverage**: Add `inflight-tail` case to `tests/texture_staging_publication_test.cpp` and `tests/test_texture_staging_publication.py` to verify that in-flight arenas are suballocated without allocating a new buffer and flush independently. ## Verification - `python3 -I -B tests/test_texture_staging_publication.py --headers /usr/include` (passes all cases across coherent and noncoherent caches)

Joey-Magnus (Migrated from github.com) requested changes

2026-11-11 21:24:06 +00:00

Joey-Magnus (Migrated from github.com) left a comment

I ran the suite merged into main: tests/test_texture_staging_publication.py fails because its stubs lack env_on and create_buffer_cached, which vk_texture.cpp:468 now needs.

Beyond that:

It changes defaults for everyone with resizable BAR. Staging arenas, including the ones the ASTC decoder reads as storage buffers, move from VRAM behind the BAR to cached host memory (:468), with no numbers on NVIDIA, AMD or Deck.
On 6 GB or smaller cards, idle staging goes from 64 to 128 MB (:3806), and one arena never expires. That's pinned system RAM, which main's comments describe running 16 GB laptops out of memory.

The tail reuse (:449) looks correct. I'd want the host-cached part opt-in or measured.

I ran the suite merged into main: `tests/test_texture_staging_publication.py` fails because its stubs lack `env_on` and `create_buffer_cached`, which `vk_texture.cpp:468` now needs. Beyond that: - It changes defaults for everyone with resizable BAR. Staging arenas, including the ones the ASTC decoder reads as storage buffers, move from VRAM behind the BAR to cached host memory (`:468`), with no numbers on NVIDIA, AMD or Deck. - On 6 GB or smaller cards, idle staging goes from 64 to 128 MB (`:3806`), and one arena never expires. That's pinned system RAM, which main's comments describe running 16 GB laptops out of memory. The tail reuse (`:449`) looks correct. I'd want the host-cached part opt-in or measured.

hckrman101 (Migrated from github.com) reviewed

2026-11-11 22:20:44 +00:00

hckrman101 (Migrated from github.com) left a comment

Thanks. I agree with Joey-Magnus's review: the suite failure he found is real (tests/test_texture_staging_publication.py cuts staging() out of the source, and its fixture tests/texture_staging_publication_test.cpp stubs create_buffer only, so it needs env_on and create_buffer_cached stubs in this PR), and the default change and the 64 to 128 MB keep on small cards are his points, not repeated here. No CI checks have run on the branch.

What I checked on top of that:

The tail reuse is sound. The new slice starts past a.used, aligned to at least nonCoherentAtomSize (and the ASTC caller's storage offset alignment), so neither the CPU writes nor the flush touch bytes an older submission still reads, and bumping value to rec_value just holds the arena until the newest reader completes. It would be worth a case in the fixture: an arena in flight (value < rec_value, not completed) gets a second slice at an aligned offset past the first, with no new arena made.
Host-cached arenas are correct even where that memory is not coherent: both staging() callers already flush their range (vk_texture.cpp:994, :1711). The open question is only speed: the ASTC decoder then reads its blocks over PCIe instead of from BAR VRAM. The existing vk: a staging arena of N MB took X ms to make log line gives the before/after the description's "100+ ms" claim needs.
Nit: vram is read with env_on three times (two statics plus once more inside the keep lambda, which could just use the static above it).

My suggestion: split it. The tail reuse is a small, clear win I'd approve on its own once the test passes; keep the host-cached arenas and the warm arena behind a switch (or bring numbers from a ReBAR card and a 6 GB one) in a follow-up.

Other open PRs by the same author: #2462 and #2467 also edit vk_texture.cpp, but in other functions (readback ring, eviction pacing), so no overlap with the staging code here. #2482, #2487 and #2491 are in other files.

Thanks. I agree with Joey-Magnus's review: the suite failure he found is real (`tests/test_texture_staging_publication.py` cuts `staging()` out of the source, and its fixture `tests/texture_staging_publication_test.cpp` stubs `create_buffer` only, so it needs `env_on` and `create_buffer_cached` stubs in this PR), and the default change and the 64 to 128 MB keep on small cards are his points, not repeated here. No CI checks have run on the branch. What I checked on top of that: - The tail reuse is sound. The new slice starts past `a.used`, aligned to at least `nonCoherentAtomSize` (and the ASTC caller's storage offset alignment), so neither the CPU writes nor the flush touch bytes an older submission still reads, and bumping `value` to `rec_value` just holds the arena until the newest reader completes. It would be worth a case in the fixture: an arena in flight (`value < rec_value`, not completed) gets a second slice at an aligned offset past the first, with no new arena made. - Host-cached arenas are correct even where that memory is not coherent: both `staging()` callers already flush their range (`vk_texture.cpp:994`, `:1711`). The open question is only speed: the ASTC decoder then reads its blocks over PCIe instead of from BAR VRAM. The existing `vk: a staging arena of N MB took X ms to make` log line gives the before/after the description's "100+ ms" claim needs. - Nit: `vram` is read with `env_on` three times (two statics plus once more inside the `keep` lambda, which could just use the static above it). My suggestion: split it. The tail reuse is a small, clear win I'd approve on its own once the test passes; keep the host-cached arenas and the warm arena behind a switch (or bring numbers from a ReBAR card and a 6 GB one) in a follow-up. Other open PRs by the same author: #2462 and #2467 also edit `vk_texture.cpp`, but in other functions (readback ring, eviction pacing), so no overlap with the staging code here. #2482, #2487 and #2491 are in other files.

gpu/vk/vk_texture.cpp

This flips every ReBAR user to cached host memory, including the buffers the ASTC compute decoder reads as storage. Correctness is fine (both callers flush), but I'd make this opt-in (e.g. RX_VK_STAGING_HOST=1) until there are numbers, rather than opt-out with RX_VK_STAGING_VRAM. The fixture also needs env_on / create_buffer_cached stubs because of this line.

This flips every ReBAR user to cached host memory, including the buffers the ASTC compute decoder reads as storage. Correctness is fine (both callers flush), but I'd make this opt-in (e.g. `RX_VK_STAGING_HOST=1`) until there are numbers, rather than opt-out with `RX_VK_STAGING_VRAM`. The fixture also needs `env_on` / `create_buffer_cached` stubs because of this line.

gpu/vk/vk_texture.cpp

This part looks right to me (see the body); a fixture case for an in-flight arena's tail would pin it down.

github-sync commented

2026-11-12 06:23:14 +00:00

Owner

@Erfboom commented on GitHub · 2026-11-12 04:15 UTC · original

Rebased onto main and updated following the review recommendations:

Split out host-cached arenas and idle keep changes: Reverted the default memory type change and warm arena retention, keeping staging memory defaults exactly as upstream. (We can explore host-cached staging with ReBAR benchmark data in a separate opt-in PR).
Narrowed strictly to in-flight tail suballocation: When no completed arena is available, staging() checks uncompleted in-flight arenas (a.value < rec_value) for remaining tail capacity, aligning past a.used to at least nonCoherentAtomSize and advancing a.value to rec_value.
Added test coverage: Added the inflight-tail case to tests/texture_staging_publication_test.cpp and tests/test_texture_staging_publication.py ensuring an in-flight arena gets suballocated past its first slice without creating a new arena, bumps its timeline stamp, and flushes independently.
Verified test suite: python3 -I -B tests/test_texture_staging_publication.py --headers /usr/include passes cleanly for both coherent and noncoherent caches.

> [@Erfboom](https://github.com/Erfboom) commented on GitHub · 2026-11-12 04:15 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2469#issuecomment-6814318448) Rebased onto `main` and updated following the review recommendations: 1. **Split out host-cached arenas and idle keep changes**: Reverted the default memory type change and warm arena retention, keeping staging memory defaults exactly as upstream. (We can explore host-cached staging with ReBAR benchmark data in a separate opt-in PR). 2. **Narrowed strictly to in-flight tail suballocation**: When no completed arena is available, `staging()` checks uncompleted in-flight arenas (`a.value < rec_value`) for remaining tail capacity, aligning past `a.used` to at least `nonCoherentAtomSize` and advancing `a.value` to `rec_value`. 3. **Added test coverage**: Added the `inflight-tail` case to `tests/texture_staging_publication_test.cpp` and `tests/test_texture_staging_publication.py` ensuring an in-flight arena gets suballocated past its first slice without creating a new arena, bumps its timeline stamp, and flushes independently. 4. **Verified test suite**: `python3 -I -B tests/test_texture_staging_publication.py --headers /usr/include` passes cleanly for both `coherent` and `noncoherent` caches.

github-sync commented

2026-11-12 14:42:44 +00:00

Owner

@Joey-Magnus approved on GitHub · 2026-11-12 14:33 UTC · original

Narrowing it resolves my first review: the host-cached default and the 128 MB keep on small cards are gone, and the fixture needs no new stubs.

The tail reuse in staging() (gpu/vk/vk_texture.cpp:454) is correct. The slice starts past a.used, aligned to at least nonCoherentAtomSize, so neither the CPU writes nor the flush touch bytes an older submission still reads, and raising value to rec_value only keeps the arena until its newest reader completes. Completed arenas are still reused from offset 0 first, so this only replaces making a new 64 MB arena, and the inflight-tail case covers it.

I merged it into current main and ran tests/run_all.py: all pass except test_lift_cache.py (fails on main here too) and test_recorder_sync.py, which timed out under machine load and doesn't touch this code. The CI reds are main's own failures from early Nov 12 and pass on current main, so a re-run should clear them. The description's "pinning tens of megabytes" has no number from the game; the staging arena ... took X ms to make log line would show it, but I wouldn't hold the merge on that.

> [@Joey-Magnus](https://github.com/Joey-Magnus) approved on GitHub · 2026-11-12 14:33 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2469#pullrequestreview-5876213223) Narrowing it resolves my first review: the host-cached default and the 128 MB keep on small cards are gone, and the fixture needs no new stubs. The tail reuse in `staging()` (`gpu/vk/vk_texture.cpp:454`) is correct. The slice starts past `a.used`, aligned to at least `nonCoherentAtomSize`, so neither the CPU writes nor the flush touch bytes an older submission still reads, and raising `value` to `rec_value` only keeps the arena until its newest reader completes. Completed arenas are still reused from offset 0 first, so this only replaces making a new 64 MB arena, and the `inflight-tail` case covers it. I merged it into current main and ran `tests/run_all.py`: all pass except `test_lift_cache.py` (fails on main here too) and `test_recorder_sync.py`, which timed out under machine load and doesn't touch this code. The CI reds are main's own failures from early Nov 12 and pass on current main, so a re-run should clear them. The description's "pinning tens of megabytes" has no number from the game; the `staging arena ... took X ms to make` log line would show it, but I wouldn't hold the merge on that.

Commenting is not possible because the repository is archived.

No reviewers

Joey-Magnus