Repository navigation
fix(vk): tie texel buffer views to their imported backing - #1706
Conversation
6942add to
4621ada
Compare
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Looks good. The cache is now keyed by the real VkBuffer plus format/offset/range, gpu_buffer is resolved on every call (so remaps and the pool use stamp are handled), and failed creations are no longer cached. Every vkDestroyBuffer in vk_memory.cpp now goes through destroy_import_buffer, so views are released right before the buffer goes. I also checked lock order: texel_views_mu is only taken after gpu_buffer returns, and release takes it under pools_mu, so there's no inversion. Test is registered in run_all.py and CI is green.
Nit: the texel_registry_failed_frame / failed_frame bookkeeping is a bit elaborate for the OOM-in-map-insert case, but harmless.
4621ada to
7dcbdb4
Compare
|
Heads-up: this conflicts with main now (a lot landed today). Could you merge main into the branch (or rebase) and resolve the conflicts? Thanks! |
Resolve and stamp each backing before cache reuse, check creation and metadata commit failures, and release owned views at the existing import destruction gates. Assisted-by: GPT-6 (OpenAI)
Arm the throwing allocator after successful view creation so MSVC sentinel allocation cannot consume the intended cache insertion fault. Keep every rollback assertion and production source unchanged. Assisted-by: GPT-6 (OpenAI)
7dcbdb4 to
12f2a62
Compare
|
Rebased onto main Head
Full-suite/current API13 native validation remains separate and pending. |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Re-checked after the rebase. The production changes (vk_texture.cpp/.h, vk_memory.cpp) and the fixture are identical to what I approved; the only delta is the runner migration: the test now declares RUN_ALL_ARGS (--cxx {cxx} --headers {vulkan_headers}) instead of adding a line to run_all.py, which matches the new #1822 convention. The source-scraping asserts still hold against current main (one vkDestroyBuffer, nine destroy_import_buffer calls in vk_memory.cpp).
CI: test_texel_buffer_ownership passes on both Linux and Windows in the fork run. The only failures there are nx2/test_draw_parameters (Linux) and nx2/test_nup_out_of_space (Windows), which fail on main too and aren't from this PR. Upstream source checks are still pending. Still good to go from my side.
Current main integration: head
12f2a6265fd24a59c1534e7a1612f8f38879adc9, based224dc0680ad418b1ab111555c7d19cc483dcfdc. The runner migration is integrated and focused package/declaration checks pass. Production patch ID is unchanged. Complete local suite: 278 passed, 3 failed, 20 skipped in 139 s (windows, 2 at once). Exact main d224dc0: 277 passed, 3 failed, 20 skipped in 131 s (windows, 2 at once). Same failures: nx2/test_nup_out_of_space.py, nx2/test_nup_paths.py, tests/test_precompile_compute_guard.py. LLVM-Mingw and UTF-8 explicitly pinned; initial collector failures remain preserved separately.Fresh exact-head CI dispatches (do not infer success from this list):
Historical native coverage below stays tied to its recorded build; no new API13 native run claimed.
A formatted-buffer cache hit currently skips the backing lookup and its imported-pool use stamp. After a guest remap it can return a view of the old buffer; creation errors also enter the cache, and cached views are retained when their imported backing is destroyed.
Resolve and stamp the backing before every lookup, key views by the actual
VkBufferplus exact format/offset/clamped range, and release them immediately before the backing's existing completed destruction. All eight raw import-destruction paths share that release wrapper. Retirement thresholds, keep-retired policy, completion values, mirrors and the host/renderer API are unchanged; no additional GPU wait is introduced.Creation failure stays retryable on the next present, with one failed attempt per backing per present and no suppression of existing valid views. Undefined error-output handles are not admitted or destroyed. Throwing cache insertion rolls back only the new view. Storage write tracking and alignment/device-limit guards remain in place. A separate registry mutex covers background drops of unpublished imports; lookup never holds it while acquiring the pool lock.
Validation at
2caed98e3aa181d9ba831ec130761e1f3cee95b7, based on mainf1f7ac8db22a2384207b10a473c9446269d1ef48:The before/after failure evidence is game-free binding/allocation/retirement execution, not a reproduced native OOM, device loss, import-pressure eviction or performance gain. Native runs cover the healthy checkpoint path, not a campaign or physical-monitor migration. Transient resize capture images settle after the resize; no claim of a temporal-rendering fix is made here. No game-derived data or binaries are included.
Overlap checked against current open heads: #1439 changes retirement selection/grace policy, which this PR preserves; #1377 changes mirror selection. #1688 concerns texture-owned image views, a different cache/owner. This PR does not import or depend on those pending patches.
Fixture portability follow-up at
c8333cdff19ccadd9e64939e61ea1f04f88d0766: GPU/host/lifter trees are byte-identical to validated2caed98. The allocator fault is armed after successful Vulkan view creation, targeting cache commit independently of STL sentinel allocation. All rollback assertions remain;23 cases pass LLVM/GCC/ASan/UBSan again and fourteen behavioral mutants are rejected. Native/build evidence above remains tied to2caed98, not relabeled. Earlier Windows source CI failed at the allocator-timing assertion; its Windows build passed.New exact-head workflows are pending:
Implementation and test scope are documented in
docs/texel-buffer-ownership.md. Only authored source, fixtures and documentation are included.Main BC retry/staging fallback changes introduced a shared source-fixture compilation regression. At
06a353a7af1187d4b48dc200bf84c2f12445a7a7, the two staging-publication fixture files are integrated fromfe1c22ceb699830965d75168bfe13efc9db690ef: existing assertions remain, mapped fallback/unmapped cleanup/reuse and multi-atom ASTC visibility are exercised. GPU/host/lifter/third-party source trees are identical to the previous head; earlier native results retain their actual hashes. This CI repair is tracked separately and adds no production behavior to this contribution. Exact-head fork workflows were refreshed.Current-head validation
Current head
12f2a62, based on maind224dc0. Earlier local/runtime results describe their recorded source hashes.Fork runs are separate from upstream Actions approval; no maintainer approval gate is bypassed.