Merge pull request 'perf(decompress): release compressed staging after submit, not for the resource's life (#67)' (#106) from claude/issue-67 into master

implementations/Crafter.Graphics-Mesh.cpp

@@ -278,6 +278,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);

interfaces/Crafter.Graphics-ImageVulkan.cppm

@@ -75,9 +75,12 @@ export namespace Crafter {
         VkImage image;
         VkDeviceMemory imageMemory;
         VulkanBuffer<PixelType, true> 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<std::byte, true> 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

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<RTAabb, true> 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<std::byte, true> compressedStaging;
         VkAccelerationStructureGeometryTrianglesDataKHR blasData;
         VkAccelerationStructureGeometryKHR blas;

project.cpp

@@ -617,6 +617,40 @@ extern "C" Configuration CrafterBuildProject(std::span<const std::string_view> 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<fs::path> 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

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 <cstring>
+
+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<void> MakeCubeMesh() {
+    MeshAsset<void> 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<void> 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;
+}