[perf][MEDIUM] Descriptor heaps force HOST_VISIBLE|DEVICE_LOCAL with no fallback #58

New issue

Closed

opened 2026-06-16 16:51:33 +02:00 by jorijnvdgraaf · 1 comment

jorijnvdgraaf commented

2026-06-16 16:51:33 +02:00

Owner

Subsystem: Device / DescriptorHeap
Location: interfaces/Crafter.Graphics-DescriptorHeapVulkan.cppm:86-89 (root cause implementations/Crafter.Graphics-Device.cpp:814-827)
Impact: MEDIUM (robustness/portability) · Effort: Small · Setup-time
Related: #59 (the fallback mechanism this needs), #89 (upload-strategy helper — generalizes the "don't over-commit the small BAR window" budget logic referenced below)

Problem

Initialize allocates 3 heap copies requesting HOST_VISIBLE|DEVICE_LOCAL. GetMemoryType returns the first match and throws if none exists. On a discrete non-ReBAR GPU the only such type is the ~256 MB pinned BAR heap, so the bindless heaps compete for scarce BAR memory — or the app hard-throws.

Proposed fix

Make GetMemoryType prefer-but-not-require DEVICE_LOCAL for host-visible heaps: try HOST_VISIBLE|DEVICE_LOCAL, fall back to plain HOST_VISIBLE. Keep HOST_VISIBLE a hard requirement.

Correctness caveat

The per-frame triplication is correct by design (each frame-slot binds its own heap copy; collapsing to one shared region is a write-after-read race — keep it). If the fallback type lacks HOST_COHERENT, correctness then depends on the existing FlushDevice/FlushHost paths being called.

**Subsystem:** Device / DescriptorHeap **Location:** `interfaces/Crafter.Graphics-DescriptorHeapVulkan.cppm:86-89` (root cause `implementations/Crafter.Graphics-Device.cpp:814-827`) **Impact:** MEDIUM (robustness/portability) · **Effort:** Small · Setup-time **Related:** #59 (the fallback mechanism this needs), #89 (upload-strategy helper — generalizes the "don't over-commit the small BAR window" budget logic referenced below) ### Problem `Initialize` allocates 3 heap copies requesting `HOST_VISIBLE|DEVICE_LOCAL`. `GetMemoryType` returns the first match and **throws** if none exists. On a discrete non-ReBAR GPU the only such type is the ~256 MB pinned BAR heap, so the bindless heaps compete for scarce BAR memory — or the app hard-throws. ### Proposed fix Make `GetMemoryType` prefer-but-not-require DEVICE_LOCAL for host-visible heaps: try `HOST_VISIBLE|DEVICE_LOCAL`, fall back to plain `HOST_VISIBLE`. Keep HOST_VISIBLE a hard requirement. ### Correctness caveat The per-frame triplication is **correct by design** (each frame-slot binds its own heap copy; collapsing to one shared region is a write-after-read race — keep it). If the fallback type lacks HOST_COHERENT, correctness then depends on the existing `FlushDevice`/`FlushHost` paths being called.

jorijnvdgraaf referenced this issue

2026-06-16 16:59:22 +02:00

[perf][MEDIUM] TLAS instance buffer is HOST_VISIBLE, read directly as AS build input #65

jorijnvdgraaf referenced this issue

2026-06-16 17:02:03 +02:00

[perf] RT shader binding table is HOST_VISIBLE, GPU-read every traceRays #72

jorijnvdgraaf added the

claude:ready

label

2026-06-16 18:48:37 +02:00

catbot added

claude:in-progress

and removed

claude:ready

labels

2026-06-16 18:49:29 +02:00

jorijnvdgraaf referenced this issue

2026-06-16 18:55:02 +02:00

[perf] Runtime upload-strategy helper: direct HOST_VISIBLE|DEVICE_LOCAL vs staging (ReBAR-aware) #89

catbot commented

2026-06-16 18:59:50 +02:00

Member

Investigated — this is already fully resolved by #59 (commit 294c137, merged via PR #87). #58 and #59 are duplicate findings from the same audit, and #59 implements the exact fix proposed here, touching the same files and lines.

What is already in master:

Device::GetMemoryType (implementations/Crafter.Graphics-Device.cpp:814-843) now takes a preferred mask distinct from required. It tries required | preferred first, falls back to required alone, and throws only when even required cannot be satisfied. HOST_VISIBLE stays a hard requirement; DEVICE_LOCAL is a preference.
The descriptor heaps (interfaces/Crafter.Graphics-DescriptorHeapVulkan.cppm:86-93 — the exact lines this issue cites) now call Resize(..., VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, size, VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT), i.e. host-visible required + device-local preferred. On a discrete non-ReBAR GPU they get a host-visible-only allocation instead of competing for BAR memory or hard-throwing.
Correctness caveat handled: the per-frame triplication is preserved (one heap copy per frame-slot), and descriptor writes are followed by FlushDevice() (implementations/Crafter.Graphics-UI.cpp:69,77,304,327,344), so a fallback memory type without HOST_COHERENT is still safe.
A regression test (tests/MemoryTypeFallback) exercises selection, preference, fallback, and the unsatisfiable-required throw. It passes: crafter-build test → 8 passed, including MemoryTypeFallback.

No code change is needed. Recommend closing this as a duplicate of #59. No PR opened (there would be no diff).

Investigated — **this is already fully resolved by #59** (commit `294c137`, merged via PR #87). #58 and #59 are duplicate findings from the same audit, and #59 implements the exact fix proposed here, touching the same files and lines. What is already in `master`: - **`Device::GetMemoryType`** (`implementations/Crafter.Graphics-Device.cpp:814-843`) now takes a `preferred` mask distinct from `required`. It tries `required | preferred` first, falls back to `required` alone, and throws only when even `required` cannot be satisfied. `HOST_VISIBLE` stays a hard requirement; `DEVICE_LOCAL` is a preference. - **The descriptor heaps** (`interfaces/Crafter.Graphics-DescriptorHeapVulkan.cppm:86-93` — the exact lines this issue cites) now call `Resize(..., VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, size, VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT)`, i.e. host-visible required + device-local preferred. On a discrete non-ReBAR GPU they get a host-visible-only allocation instead of competing for BAR memory or hard-throwing. - **Correctness caveat handled:** the per-frame triplication is preserved (one heap copy per frame-slot), and descriptor writes are followed by `FlushDevice()` (`implementations/Crafter.Graphics-UI.cpp:69,77,304,327,344`), so a fallback memory type without `HOST_COHERENT` is still safe. - A regression test (`tests/MemoryTypeFallback`) exercises selection, preference, fallback, and the unsatisfiable-required throw. It passes: `crafter-build test` → 8 passed, including `MemoryTypeFallback`. No code change is needed. Recommend closing this as a duplicate of #59. No PR opened (there would be no diff).

catbot added

claude:blocked

and removed

claude:in-progress

labels

2026-06-16 19:00:02 +02:00

jorijnvdgraaf closed this issue

2026-06-16 19:09:31 +02:00

catbot referenced this issue from a commit

2026-06-16 19:23:28 +02:00

feat(device): runtime upload-strategy helper PreferDirectDeviceWrite (#89)

jorijnvdgraaf referenced this issue