From 1553f585a287fd0417b58b3ef9667651adadbb9a Mon Sep 17 00:00:00 2001
From: catbot <catbot@bot.local>
Date: Wed, 17 Jun 2026 17:44:37 +0000
Subject: [PATCH] perf(window): use HOST_CACHED readback staging in SaveFrame
 (#74)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

SaveFrame's readback staging buffer was allocated HOST_VISIBLE |
HOST_COHERENT, whose first matching type is write-combined (uncached) on
most discrete GPUs. The CPU then reads the entire frame back from it —
and reads from write-combined memory are pathologically slow.

Select a HOST_CACHED type instead (preferring a cached-coherent type
when one exists), falling back to any HOST_VISIBLE type if no cached type
is advertised (#59 mechanism). When the chosen type is cached-but-not-
coherent, invalidate the mapped range before the CPU read — the read-path
counterpart of FlushDevice (#60) — gated on the chosen type's actual
coherency, not the requested flags. Whole-range invalidate sidesteps
nonCoherentAtomSize.

Deliberately not routed through the #89 upload-strategy helper, which is
upload-only and steers toward write-combined DEVICE_LOCAL|HOST_VISIBLE —
the worst type for readback.

Verified by driving SaveFrame from the VulkanTriangle example: the
barycentric RGB triangle reads back with correct pixels and channel
order on an RTX 4090.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 implementations/Crafter.Graphics-Window.cpp | 40 +++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/implementations/Crafter.Graphics-Window.cpp b/implementations/Crafter.Graphics-Window.cpp
index 0afe9d8..7be3d4e 100644
--- a/implementations/Crafter.Graphics-Window.cpp
+++ b/implementations/Crafter.Graphics-Window.cpp
@@ -1200,11 +1200,29 @@ void Window::SaveFrame(const std::filesystem::path& path) {
 
     VkMemoryRequirements memReqs;
     vkGetBufferMemoryRequirements(Device::device, stagingBuf, &memReqs);
+
+    // Readback path: the CPU *reads* this whole buffer back. The first
+    // HOST_VISIBLE|HOST_COHERENT type is usually write-combined (uncached) on
+    // discrete GPUs, where CPU reads are pathologically slow. Ask for
+    // HOST_CACHED so reads hit the CPU cache, preferring a cached-coherent type
+    // when one exists. HOST_CACHED is near-universal but not spec-guaranteed,
+    // so fall back to any HOST_VISIBLE type (#59) if absent — that path then
+    // relies on the coherency-gated invalidate below. (Deliberately NOT routed
+    // through the upload-strategy helper, which steers toward write-combined
+    // DEVICE_LOCAL|HOST_VISIBLE — the worst type for readback. See issue #89.)
+    std::uint32_t memTypeIndex;
+    try {
+        memTypeIndex = Device::GetMemoryType(memReqs.memoryTypeBits,
+            VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT | VK_MEMORY_PROPERTY_HOST_CACHED_BIT,
+            VK_MEMORY_PROPERTY_HOST_COHERENT_BIT);
+    } catch (const std::runtime_error&) {
+        memTypeIndex = Device::GetMemoryType(memReqs.memoryTypeBits,
+            VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT);
+    }
     VkMemoryAllocateInfo mai{
         .sType           = VK_STRUCTURE_TYPE_MEMORY_ALLOCATE_INFO,
         .allocationSize  = memReqs.size,
-        .memoryTypeIndex = Device::GetMemoryType(memReqs.memoryTypeBits,
-            VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT | VK_MEMORY_PROPERTY_HOST_COHERENT_BIT),
+        .memoryTypeIndex = memTypeIndex,
     };
     VkDeviceMemory stagingMem = VK_NULL_HANDLE;
     Device::CheckVkResult(vkAllocateMemory(Device::device, &mai, nullptr, &stagingMem));
@@ -1289,6 +1307,24 @@ void Window::SaveFrame(const std::filesystem::path& path) {
     // Read back, swizzle BGRA → RGBA if needed, write PNG.
     void* mapped = nullptr;
     Device::CheckVkResult(vkMapMemory(Device::device, stagingMem, 0, VK_WHOLE_SIZE, 0, &mapped));
+
+    // If the chosen type is cached-but-not-coherent, the transfer's writes may
+    // not yet be visible to the CPU read; invalidate to pull them in. Gate on
+    // the chosen type's actual coherency (not the requested flags) — a coherent
+    // type needs no invalidate. Whole-range invalidate sidesteps
+    // nonCoherentAtomSize. This is the read-path counterpart of FlushDevice (#60).
+    if (!(Device::memoryProperties.memoryTypes[memTypeIndex].propertyFlags &
+          VK_MEMORY_PROPERTY_HOST_COHERENT_BIT)) {
+        VkMappedMemoryRange invalidateRange{
+            .sType  = VK_STRUCTURE_TYPE_MAPPED_MEMORY_RANGE,
+            .memory = stagingMem,
+            .offset = 0,
+            .size   = VK_WHOLE_SIZE,
+        };
+        Device::CheckVkResult(
+            vkInvalidateMappedMemoryRanges(Device::device, 1, &invalidateRange));
+    }
+
     const std::uint8_t* src = static_cast<const std::uint8_t*>(mapped);
 
     std::vector<std::uint8_t> rgba(static_cast<std::size_t>(width) * height * 4);