Rendering foundation hardening: real RenderGraph sort, GpuAllocator pools, RRM hot-reload - #748
Merged
Conversation
JeanPhilippeKernel
added
area-linux
BuildTopology was a flat insertion-order loop despite RGPass already recording per-pass Reads/Writes — no actual dependency sort ever ran. Binds each resource's readers directly to its sole writer regardless of declared order, which is what lets a pass registered before its producer get fixed instead of just reproducing declaration order (a single forward scan can never do this, since it can only emit edges to a later index — checked this against a first design that compiled fine but, per its own tests, could never reorder anything or detect a cycle). Falls back to declaration order and logs on a cycle instead of crashing. Also fixes the resulting Compile() ordering bug: BuildLifetimes must run after BuildTopology now, since it indexes by sorted execution order, and one unrelated pre-existing bug found in the same function (a resource index compared against a pass index, which could never match). 7 new device-free tests in RenderGraphTest.cpp.
Pools[5] was declared but never populated — every allocation went through VMA's default pool regardless of domain. Adds real vmaCreatePool calls for DeviceGeometry/DeviceTexture/HostUniform/HostStaging (RenderTarget intentionally stays unpooled — it benefits from VMA's automatic dedicated-allocation promotion, which pooling would work against). Wired into AllocateBuffer/AllocateImage with a fallback retry against the default pool on exhaustion. Building and testing this against a real headless Vulkan device (no window/surface needed — GpuAllocator only touches raw Vulkan handles) caught three real bugs no amount of review would have found: - VK_ERROR_OUT_OF_POOL_MEMORY doesn't exist in this VMA version at all — it's a different Vulkan concept (VkDescriptorPool exhaustion). VMA signals VmaPool exhaustion via VK_ERROR_OUT_OF_DEVICE_MEMORY instead. - A fixed-blockSize pool sized exactly equal to an allocation's byte count fails — a block needs alignment headroom beyond its raw size. Fixed HostStaging (built around the ring's exact-size buffer) and DeviceTexture (single large textures can exceed a fixed block) with blockSize=0 (auto-sized), which also preserves dedicated-allocation fallback for oversized textures. - Every custom pool must be destroyed before the VmaAllocator, or vmaDestroyAllocator hits VMA's own internal assert. 4 new tests against a real headless Vulkan device in GpuAllocatorTest.cpp.
…ools ScheduleSwap(BufferHandle,...) and ScheduleSwap(ImageHandle,...) were both literal no-op stubs — neither ever constructed a SwapEntry or incremented m_swap_count, so EndFrame's swap-processing loop never ran for either kind (the Image branch was correctly written but permanently unreachable, since the queue it drained was always empty). ScheduleSwap now enqueues onto a mutex-guarded m_pending_swaps queue — a dedicated mutex, not m_pending_mutex, which SubmitTextureFile holds across file I/O and a GPU call. A new FlushPendingSwaps (render-thread only, called from BeginFrame) drains it and applies each swap immediately: no frame-in-flight delay, since tracing every consumer of RRM handles in this engine found none that needs one (the old SwapSafeFrame gating was independently broken anyway — it compared against a wrapped 0..2 swapchain slot index that could never satisfy frame_index + FRAMES_IN_FLIGHT). Mesh swaps repoint the slot's offsets at freshly-appended data via a new shared AppendMeshData helper (no GPU free needed — the global buffer is append-only); image swaps re-upload and DeferFree the old image. SwapEntry/m_swaps/EndFrame's old drain loop are removed entirely — the design collapsed to one stage, not two. Also fixes the ABA bug: AllocMeshSlot/AllocImageSlot/AllocGBufSlot assigned a deterministic idx+1 generation on slot reuse, so a stale handle from a released slot could alias whatever got allocated into it next. Now assigns a never-reset-by-Release monotonic counter per slot. Also fixes DoUploadTexture silently overwriting AllocImageSlot's already-correct generation with the old idx+1 scheme right after allocating it — found while extending AllocImageSlot for the ABA fix. New tests are honestly skip-gated in RenderResourceManagerHotReloadTest.cpp: no test in this codebase constructs a real RenderResourceManager today, and doing so needs a fully-initialized VulkanDevice (Arena, GpuMem, a DeviceSwapchain with a real timeline semaphore, ThreadPoolHelper::Pool, CommandPool/CommandBuffer) — a materially bigger fixture than GpuAllocatorTest's raw-Vulkan-handle approach, left as a documented follow-up rather than distorting the class for testability. Fixes #740.
JeanPhilippeKernel
force-pushed
the
feat/rendering-foundation-hardening
branch
from
September 4, 2026 06:02
80e9269 to
465f355
Compare
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
1 participant
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.
Summary
Three-track foundation-hardening pass, planned and executed as one initiative this month, per the approved plan — foundation over new visual features.
BuildTopologynow does a real Kahn's-algorithm sort over RAW/WAW/WAR hazard edges (previously a flat insertion-order loop despite already recording per-pass Reads/Writes). Fixes the resultingCompile()ordering bug plus one unrelated pre-existing resource/pass-index bug found in passing. 7 new tests.DeviceGeometry/DeviceTexture/HostUniform/HostStaging(RenderTargetintentionally stays unpooled). Testing against a real headless Vulkan device caught 3 real bugs: a nonexistent VMA error code being checked, a fixed-blockSize pool that can't fit an allocation exactly equal to its own size, and pools needing explicit destruction before the allocator. 4 new tests.ScheduleSwap) was a complete no-op for both buffers and images, not just buffers as originally filed in RenderResourceManager: SwapKind::Buffer silently dropped on hot-reload; no ABA protection on handle pool #740. Implemented via a dedicated mutex-guarded pending-swap queue drained onBeginFrame, applied immediately (traced every consumer of RRM handles in this engine — none needs a frame-in-flight delay). Also fixes the ABA bug on the handle pools (monotonic generation counters instead of deterministicidx+1) and one bug found while extending it (DoUploadTexturesilently overwriting its own correct generation).Full technical rationale, the three rounds of adversarial design verification, and the "explicitly out of scope this month" list are in the commit messages (one per track) and the corrected docs.
Note: this branch originally included its own fix for #746 (
Logger::Logcrashing on calls beforeInitialize/afterDispose), found incidentally while testing the GpuAllocator work. @BetterAndBetterII independently found and fixed the same bug in #747 — merged separately, with better-targeted regression tests than what I'd written. Rebased this branch ontodevelopafter that merge and dropped the now-redundant Logger.cpp commit; the RenderGraph/GpuAllocator/RRM work here doesn't depend on it.Test plan
cteston the rebased branch — 100% passed, 0 failed, out of 549 (545 passing + 4 honestly skip-gated — no headless-VulkanDevicefixture exists in this codebase yet; building one is a separate, larger undertaking, documented inRenderResourceManagerHotReloadTest.cpp)ctest's per-test-process execution is unaffected by a real MoltenVK/Vulkan cross-test side effect discovered in the raw single-process test binary (documented inGpuAllocatorTest.cpp)Closes #740. Comments on #312 with a deferral rationale (not closing — full memory aliasing is out of scope this month).