From 8a32f0d5453d8587b501cefa57c65e06b15df577 Mon Sep 17 00:00:00 2001 From: catbot Date: Wed, 17 Jun 2026 17:40:39 +0000 Subject: [PATCH 1/2] perf(decompress): release compressed staging after submit, not for the resource's life MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The compressed Mesh::Build / ImageVulkan::Update paths kept their host-visible `compressedStaging` (holding the GDeflate streams) alive for the whole life of the mesh/image, pinning host-visible memory long after the single GPU decompress that reads it has retired. Release it via VulkanBuffer::DeferredClear() right after recording the decompress. The recorded vkCmdDecompressMemoryEXT still references compressedStaging.address, so it must outlive the submit — exactly what the fence-keyed deletion queue (#101/#102) guarantees: it retires the allocation only after framesInFlight frames have elapsed, by which point the submit's fence has cleared. Between Builds/Updates the handle is null; the next call's Resize re-creates it. Co-Authored-By: Claude Opus 4.8 --- implementations/Crafter.Graphics-Mesh.cpp | 10 ++++++++++ interfaces/Crafter.Graphics-ImageVulkan.cppm | 19 ++++++++++++++++--- interfaces/Crafter.Graphics-Mesh.cppm | 13 +++++++------ 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/implementations/Crafter.Graphics-Mesh.cpp b/implementations/Crafter.Graphics-Mesh.cpp index 203e028..570511d 100644 --- a/implementations/Crafter.Graphics-Mesh.cpp +++ b/implementations/Crafter.Graphics-Mesh.cpp @@ -261,6 +261,16 @@ void Mesh::Build(const CompressedMeshAsset& asset, VkCommandBuffer cmd, RTBuildO VK_PIPELINE_STAGE_2_ACCELERATION_STRUCTURE_BUILD_BIT_KHR, VK_ACCESS_2_ACCELERATION_STRUCTURE_READ_BIT_KHR); + // The compressed staging is only read by the decompress recorded above; the + // subsequent BLAS build reads the decompressed vertex/index buffers, never + // this. So hand it to the fence-keyed deletion queue (#101/#102) now rather + // than pinning host-visible memory for the mesh's whole life. The recorded + // vkCmdDecompressMemoryEXT still references compressedStaging.address, so it + // must outlive this submit — which the queue guarantees: it retires the + // allocation only after framesInFlight frames have elapsed, by which point + // the decompress submit's fence has cleared. + compressedStaging.DeferredClear(); + allowUpdate = options.allowUpdate; builtInputCount = asset.vertexCount; RecordBLASBuild(*this, asset.vertexCount, asset.indexCount, BlasFlags(options), /*update*/ false, cmd); diff --git a/interfaces/Crafter.Graphics-ImageVulkan.cppm b/interfaces/Crafter.Graphics-ImageVulkan.cppm index 6630e2c..2c40a01 100644 --- a/interfaces/Crafter.Graphics-ImageVulkan.cppm +++ b/interfaces/Crafter.Graphics-ImageVulkan.cppm @@ -75,9 +75,12 @@ export namespace Crafter { VkImage image; VkDeviceMemory imageMemory; VulkanBuffer buffer; - // Lives until the compressed Update path's cmd buffer completes. - // Same lifetime contract as Mesh::compressedStaging — caller must - // not destroy / re-Update before the submit fence is signaled. + // Transient host-visible staging for the compressed Update path. Same + // lifetime contract as Mesh::compressedStaging: the compressed Update + // releases it via DeferredClear() right after recording the decompress, + // so the fence-keyed deletion queue (#101/#102) frees it once that + // submit's frame has cleared instead of pinning it for the image's life. + // Between Updates the handle is null; the next Update's Resize re-creates it. VulkanBuffer compressedStaging; VkImageView imageView; VkDescriptorImageInfo descriptor; @@ -299,6 +302,16 @@ export namespace Crafter { VK_PIPELINE_STAGE_2_COPY_BIT, VK_ACCESS_2_TRANSFER_READ_BIT); + // Compressed staging is read only by the decompress recorded above; + // the buffer→image copy below reads `buffer` (the decompress dst), + // never this. Release it to the fence-keyed deletion queue + // (#101/#102) now instead of pinning host-visible memory for the + // image's whole life. The recorded vkCmdDecompressMemoryEXT still + // references compressedStaging.address, so it must outlive this + // submit — which the queue guarantees by retiring the allocation + // only after framesInFlight frames (i.e. after the submit's fence). + compressedStaging.DeferredClear(); + // Continue with the existing buffer→image upload + layout transitions. // We've already inserted the decompress→transfer-read barrier, // so we skip the FlushDevice host-write barrier the regular Update diff --git a/interfaces/Crafter.Graphics-Mesh.cppm b/interfaces/Crafter.Graphics-Mesh.cppm index c04f819..1fc126e 100644 --- a/interfaces/Crafter.Graphics-Mesh.cppm +++ b/interfaces/Crafter.Graphics-Mesh.cppm @@ -77,12 +77,13 @@ export namespace Crafter { // Lifetime contract matches vertexBuffer/indexBuffer: must stay // alive until the build submitted on `cmd` completes. VulkanBuffer aabbBuffer; - // Lives until the cmd buffer issued by the compressed Build path - // completes execution. Kept as a member so the recorded - // vkCmdDecompressMemoryEXT references valid memory until the queue - // submit signals — caller must not re-Build or destroy the Mesh - // before that submit's fence is signaled (same contract as the - // existing uncompressed path). + // Transient host-visible staging for the compressed Build path. Kept as + // a member only so the recorded vkCmdDecompressMemoryEXT has a stable + // address to reference; the compressed Build releases it via + // DeferredClear() right after recording the decompress, so it is freed + // by the fence-keyed deletion queue (#101/#102) once that submit's frame + // has cleared — no longer pinned for the mesh's life. Between Builds the + // handle is null; the next Build's Resize re-creates it. VulkanBuffer compressedStaging; VkAccelerationStructureGeometryTrianglesDataKHR blasData; VkAccelerationStructureGeometryKHR blas; From b29b8f03717372fe57284a7519cace46abf5d974 Mon Sep 17 00:00:00 2001 From: catbot Date: Wed, 17 Jun 2026 17:40:39 +0000 Subject: [PATCH 2/2] test(decompress): cover compressed-staging release on the real GPU path MeshDecompressStagingRelease drives the real VK_EXT_memory_decompression / GDeflate path on a headless device and asserts the staging is handed to the deferred-deletion queue (not pinned), the build is validation-clean with byte-correct decompressed data, and the entry retires only after framesInFlight frames. Skips the GPU-path assertions when the extension is absent (CPU fallback never allocates the staging). Co-Authored-By: Claude Opus 4.8 --- project.cpp | 34 ++++ tests/MeshDecompressStagingRelease/main.cpp | 197 ++++++++++++++++++++ 2 files changed, 231 insertions(+) create mode 100644 tests/MeshDecompressStagingRelease/main.cpp diff --git a/project.cpp b/project.cpp index 879108f..97d51b5 100644 --- a/project.cpp +++ b/project.cpp @@ -617,6 +617,40 @@ extern "C" Configuration CrafterBuildProject(std::span a dc.GetInterfacesAndImplementations(ifaces, deferredImpls); cfg.tests.push_back(std::move(deferredTest)); + // Issue #67: the compressed Mesh::Build path no longer pins its + // host-visible `compressedStaging` for the mesh's life — it releases it + // via DeferredClear() right after recording the GPU decompress, so the + // fence-keyed deletion queue (#101/#102) frees it once that submit's + // frame has cleared. Drives the real VK_EXT_memory_decompression / + // GDeflate path on a headless device (no swapchain/window — a decompress + // + BLAS build only needs the queue + command pool) and asserts the + // staging is enqueued (not pinned), the build is validation-clean with + // correct decompressed data, and the entry retires only after + // framesInFlight frames. Shares the native build settings; needs the + // asset pipeline for SaveCompressed/LoadCompressedMesh (cfg.dependencies + // already carries Crafter.Asset). + Test meshStagingTest; + Configuration& msc = meshStagingTest.config; + msc.path = cfg.path; + msc.name = "MeshDecompressStagingRelease"; + msc.outputName = "MeshDecompressStagingRelease"; + msc.type = ConfigurationType::Executable; + msc.target = cfg.target; + msc.march = cfg.march; + msc.mtune = cfg.mtune; + msc.debug = cfg.debug; + msc.sysroot = cfg.sysroot; + msc.dependencies = cfg.dependencies; + msc.externalDependencies = cfg.externalDependencies; + msc.compileFlags = cfg.compileFlags; + msc.linkFlags = cfg.linkFlags; + msc.defines = cfg.defines; + msc.cFiles = cfg.cFiles; + std::vector meshStagingImpls(impls.begin(), impls.end()); + meshStagingImpls.emplace_back("tests/MeshDecompressStagingRelease/main"); + msc.GetInterfacesAndImplementations(ifaces, meshStagingImpls); + cfg.tests.push_back(std::move(meshStagingTest)); + // Issue #89: Device::PreferDirectDeviceWrite chooses the upload strategy // for a CPU-written, GPU-read buffer — direct HOST_VISIBLE|DEVICE_LOCAL // map+write on ReBAR/UMA vs. staged-into-pure-DEVICE_LOCAL on a small diff --git a/tests/MeshDecompressStagingRelease/main.cpp b/tests/MeshDecompressStagingRelease/main.cpp new file mode 100644 index 0000000..9f93e77 --- /dev/null +++ b/tests/MeshDecompressStagingRelease/main.cpp @@ -0,0 +1,197 @@ +/* +Crafter®.Graphics +Copyright (C) 2026 Catcrafts® +catcrafts.net + +This library is free software; you can redistribute it and/or +modify it under the terms of the GNU Lesser General Public +License version 3.0 as published by the Free Software Foundation; + +This library is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +Lesser General Public License for more details. + +You should have received a copy of the GNU Lesser General Public +License along with this library; if not, write to the Free Software +Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +*/ + +// Issue #67: the compressed Mesh::Build path used to keep `compressedStaging` +// — a per-mesh host-visible staging buffer holding the GDeflate streams — +// alive for the whole life of the mesh, pinning host-visible memory long after +// the one decompress that reads it has retired. Build now releases it via +// VulkanBuffer::DeferredClear() right after recording the decompress, so the +// fence-keyed deletion queue (#101/#102) frees it once that submit's frame has +// cleared instead of pinning it forever. +// +// This drives the real hardware GPU-decompress path (VK_EXT_memory_decompression, +// GDeflate 1.0) on a headless device — no swapchain/window needed, a decompress + +// BLAS build only touches the queue + command pool — and asserts: +// - After Build, mesh.compressedStaging owns no handle (released). +// - Exactly that one allocation was handed to Device's deletion queue, tagged +// with the current frameCounter (so it retires on a later frame, not now). +// - The decompress + BLAS build still complete with ZERO validation errors and +// a non-zero BLAS address — proof the staging genuinely outlived the submit +// (the queue had not yet retired it), so this is no use-after-free. +// - The GPU-decompressed vertex/index data read back byte-equal to the source, +// so releasing the staging didn't corrupt the result. +// - The deferred entry actually retires (and frees) once framesInFlight frames +// elapse, and not before — the whole point of routing through the queue. +// +// On hardware without VK_EXT_memory_decompression the compressed Build takes the +// CPU-decode fallback, which never allocates compressedStaging, so the new +// behavior is moot — the test reports that and skips the GPU-path assertions. + +#include "vulkan/vulkan.h" +#include + +import Crafter.Graphics; +import Crafter.Asset; +import Crafter.Math; +import std; + +using namespace Crafter; +namespace fs = std::filesystem; + +namespace { + +int failures = 0; + +void Check(bool ok, std::string_view what) { + std::println("{} {}", ok ? "PASS" : "FAIL", what); + if (!ok) ++failures; +} + +// One-time command buffer helpers — record, submit, block. Mirrors the other +// headless RT tests (BLASBuildOptions). +VkCommandBuffer BeginCmd() { + VkCommandBufferAllocateInfo allocInfo { + .sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_ALLOCATE_INFO, + .commandPool = Device::commandPool, + .level = VK_COMMAND_BUFFER_LEVEL_PRIMARY, + .commandBufferCount = 1, + }; + VkCommandBuffer cmd = VK_NULL_HANDLE; + Device::CheckVkResult(vkAllocateCommandBuffers(Device::device, &allocInfo, &cmd)); + VkCommandBufferBeginInfo beginInfo { + .sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO, + .flags = VK_COMMAND_BUFFER_USAGE_ONE_TIME_SUBMIT_BIT, + }; + Device::CheckVkResult(vkBeginCommandBuffer(cmd, &beginInfo)); + return cmd; +} + +void SubmitWait(VkCommandBuffer cmd) { + Device::CheckVkResult(vkEndCommandBuffer(cmd)); + VkSubmitInfo submitInfo { + .sType = VK_STRUCTURE_TYPE_SUBMIT_INFO, + .commandBufferCount = 1, + .pCommandBuffers = &cmd, + }; + Device::CheckVkResult(vkQueueSubmit(Device::queue, 1, &submitInfo, VK_NULL_HANDLE)); + Device::CheckVkResult(vkQueueWaitIdle(Device::queue)); + vkFreeCommandBuffers(Device::device, Device::commandPool, 1, &cmd); +} + +// A unit cube (8 verts, 36 indices) — small enough to fit one GDeflate tile, +// enough topology for a real BLAS. +MeshAsset MakeCubeMesh() { + MeshAsset mesh; + mesh.vertexes = { + {-1.f, -1.f, -1.f}, { 1.f, -1.f, -1.f}, + { 1.f, 1.f, -1.f}, {-1.f, 1.f, -1.f}, + {-1.f, -1.f, 1.f}, { 1.f, -1.f, 1.f}, + { 1.f, 1.f, 1.f}, {-1.f, 1.f, 1.f}, + }; + mesh.indexes = { + 0,1,2, 0,2,3, 4,6,5, 4,7,6, + 0,4,5, 0,5,1, 3,2,6, 3,6,7, + 1,5,6, 1,6,2, 0,3,7, 0,7,4, + }; + return mesh; +} + +} // namespace + +int main() { + Device::Initialize(); + Device::validationErrorCount = 0; + + // Build → compress → load a cube via the real asset pipeline. + MeshAsset srcMesh = MakeCubeMesh(); + const fs::path meshPath = + fs::temp_directory_path() / "crafter_meshdecompress_staging_release.cmesh"; + srcMesh.SaveCompressed(meshPath); + CompressedMeshAsset asset = LoadCompressedMesh(meshPath); + fs::remove(meshPath); + + Check(asset.vertexCount == srcMesh.vertexes.size() + && asset.indexCount == srcMesh.indexes.size(), + "LoadCompressedMesh round-trips the cube header"); + + if (!Device::memoryDecompressionSupported) { + // No GPU codec → compressed Build takes the CPU-decode fallback, which + // never allocates compressedStaging. The release behavior under test + // only exists on the GPU path, so there is nothing to assert here. + std::println("VK_EXT_memory_decompression absent — GPU decompress path " + "not exercised; skipping staging-release assertions."); + return failures == 0 ? 0 : 1; + } + + // Control the deferred-deletion clock: enqueue at frame 0, retire after 2. + Device::framesInFlight = 2; + Device::frameCounter = 0; + Device::deletionQueue.clear(); + + Mesh mesh; + VkCommandBuffer cmd = BeginCmd(); + mesh.Build(asset, cmd); + + // ── The fix: staging is released to the queue at record time, not pinned. ── + Check(mesh.compressedStaging.buffer == VK_NULL_HANDLE, + "Build releases compressedStaging (no handle pinned to the mesh)"); + Check(Device::deletionQueue.size() == 1, + "exactly one allocation (the staging) handed to the deletion queue"); + const bool taggedNow = !Device::deletionQueue.empty() + && Device::deletionQueue.front().retireAfter == 0 + && Device::deletionQueue.front().buffer != VK_NULL_HANDLE; + Check(taggedNow, + "deferred staging is tagged with the enqueue frame and still a live handle"); + + // The decompress recorded above still references the staging's address; it + // must outlive THIS submit. The queue hasn't retired it (frame 0, retires at + // 2), so the GPU reads valid memory. Zero validation errors + a real BLAS + // address prove there's no use-after-free. + SubmitWait(cmd); + Check(mesh.blasAddr != 0, "compressed Build produced a non-zero BLAS address"); + Check(Device::validationErrorCount == 0, + "decompress + BLAS build raised no validation errors"); + + // The GPU decompress wrote into the host-visible vertex/index buffers; + // invalidate and read them back — releasing the staging must not corrupt + // the decompressed result. + mesh.vertexBuffer.FlushHost(); + mesh.indexBuffer.FlushHost(); + const bool vertsMatch = std::memcmp( + mesh.vertexBuffer.value, srcMesh.vertexes.data(), + srcMesh.vertexes.size() * sizeof(srcMesh.vertexes[0])) == 0; + const bool idxMatch = std::memcmp( + mesh.indexBuffer.value, srcMesh.indexes.data(), + srcMesh.indexes.size() * sizeof(srcMesh.indexes[0])) == 0; + Check(vertsMatch, "GPU-decompressed vertices are byte-equal to the source"); + Check(idxMatch, "GPU-decompressed indices are byte-equal to the source"); + + // ── Retire timing: the staging frees once framesInFlight frames elapse, + // and not a frame before. We've already wait-idled, so freeing is safe. ── + Device::ReclaimDeletions(); + Check(Device::deletionQueue.size() == 1, + "staging is NOT freed before framesInFlight frames have elapsed"); + Device::frameCounter = 2; // retireAfter(0) + framesInFlight(2) <= 2 + Device::ReclaimDeletions(); + Check(Device::deletionQueue.empty(), + "staging is freed once its retire frame is reached"); + + std::println("{}", failures == 0 ? "ALL PASS" : "FAILURES PRESENT"); + return failures == 0 ? 0 : 1; +}