From d9c998fa1712086a6b2de841f16473e7580e0155 Mon Sep 17 00:00:00 2001 From: Panos <25664832+mechpanos@users.noreply.github.com> Date: Sat, 5 Sep 2026 16:43:23 +0300 Subject: [PATCH] fix: never cache a buffer the device rejected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After an out-of-memory the viewport stopped updating for the rest of the session — both Model and Layout frozen, the UI still responsive. The log shows why: `block-wire-geometry` prints *after* the errors it provoked. The failed uploads were cached. A rejected allocation still yields a `Buffer`. It is invalid, every later use reports again, and it is indistinguishable from a good one at the cache key. So the entry keeps hitting, nothing ever rebuilds, and the frame stays empty even once memory frees up. Sharing `block_geometry` across slots in the previous commit made that worse, not better: one poisoned entry now serves every viewport. The device's uncaptured-error counter was already there, private to the handler that throttles the log. It is now readable, and the three caches that can retain a bad upload — block definition geometry, the shared wire buffer cache, and the arena's content id — snapshot it across their build and decline to record what it condemns. The degraded frame still draws; the next frame tries again. Best effort by construction: wgpu surfaces some errors asynchronously, so a moved counter means something failed, while an unmoved one is not proof that nothing did. That asymmetry is the right way round here — believing a bad buffer costs a frozen viewport, rebuilding a good one costs a frame. `block-wire-geometry` gains `poisoned=` so a run says whether this happened rather than leaving it to be inferred. Co-Authored-By: Claude Opus 5 --- src/scene/pipeline/mod.rs | 21 +++++++++++++++++---- src/scene/pipeline/wire_gpu.rs | 24 +++++++++++++++++++++--- src/scene/view/render.rs | 28 ++++++++++++++++++++++++---- 3 files changed, 62 insertions(+), 11 deletions(-) diff --git a/src/scene/pipeline/mod.rs b/src/scene/pipeline/mod.rs index a98627f59..441704005 100644 --- a/src/scene/pipeline/mod.rs +++ b/src/scene/pipeline/mod.rs @@ -4956,12 +4956,25 @@ impl MultiPipeline { /// `Pipeline::new`, which runs again for every pane `ensure_len` adds. If iced /// ever rebuilds the device it builds a new `MultiPipeline` too, so the fresh /// device is covered (and the throttle restarts with it). +static GPU_ERRORS_SEEN: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); + +/// How many uncaptured device errors have been reported since this device was +/// created. +/// +/// Callers snapshot it around an allocation to find out whether what they just +/// built is usable. Best effort: wgpu surfaces some errors asynchronously, so a +/// move means "something failed", while no move is not proof that nothing did. +/// That is the right asymmetry here — the cost of believing a bad buffer is a +/// frozen viewport, the cost of rebuilding a good one is a frame. +pub(crate) fn gpu_errors_seen() -> usize { + GPU_ERRORS_SEEN.load(std::sync::atomic::Ordering::Relaxed) +} + fn install_gpu_error_handler(device: &wgpu::Device) { - use std::sync::atomic::{AtomicUsize, Ordering}; - static SEEN: AtomicUsize = AtomicUsize::new(0); - SEEN.store(0, Ordering::Relaxed); + use std::sync::atomic::Ordering; + GPU_ERRORS_SEEN.store(0, Ordering::Relaxed); device.on_uncaptured_error(std::sync::Arc::new(|e: wgpu::Error| { - let n = SEEN.fetch_add(1, Ordering::Relaxed) + 1; + let n = GPU_ERRORS_SEEN.fetch_add(1, Ordering::Relaxed) + 1; if n.is_power_of_two() { eprintln!("[gpu] uncaptured wgpu error #{n} (frame degraded, session kept alive): {e}"); } diff --git a/src/scene/pipeline/wire_gpu.rs b/src/scene/pipeline/wire_gpu.rs index 722452b87..8774ed251 100644 --- a/src/scene/pipeline/wire_gpu.rs +++ b/src/scene/pipeline/wire_gpu.rs @@ -778,6 +778,8 @@ impl BlockWireGpu { // path's saving is visible rather than assumed. let mut uploaded_bytes = 0usize; let mut uploaded_segments = 0usize; + // Definitions whose upload the device rejected, so they were not cached. + let mut poisoned = 0usize; for (mesh_edge, group) in groups { let Some(&source) = group.first() else { continue; @@ -803,6 +805,12 @@ impl BlockWireGpu { push_block_batches(&mut out, device, queue, &chunks, &instances, mesh_edge); continue; } + // A failed allocation still produces a `Buffer`; it is simply + // invalid, and every later use of it reports again. Cached, it + // freezes the viewport for the rest of the session because the key + // keeps hitting and nothing ever rebuilds. Watch the device's error + // count across the build and refuse to cache what it condemns. + let errors_before = super::gpu_errors_seen(); let (segments, mut constant) = emit_wire_native(source, 0, color, 0.0); if segments.is_empty() { continue; @@ -906,8 +914,17 @@ impl BlockWireGpu { * std::mem::size_of::() * if mode.uses_storage() { 1 } else { 6 }; let chunks = std::sync::Arc::new(chunks); - if let Some(cache) = cache.as_deref_mut() { - cache.insert(cache_key, std::sync::Arc::clone(&chunks)); + let built_clean = super::gpu_errors_seen() == errors_before; + if built_clean { + if let Some(cache) = cache.as_deref_mut() { + cache.insert(cache_key, std::sync::Arc::clone(&chunks)); + } + } else { + // Draw this frame with what we have — a degraded frame beats a + // blank one — but leave the cache clean so the next frame tries + // again. `live_keys` still holds the key, so the retain below + // does not evict a good entry from an earlier frame. + poisoned += 1; } let instances = block_instances(&group, base, mesh_edge, depth_map); push_block_batches(&mut out, device, queue, &chunks, &instances, mesh_edge); @@ -919,12 +936,13 @@ impl BlockWireGpu { if crate::perf::enabled() && uploaded_segments > 0 { crate::perf_record!( "[perf] block-wire-geometry mode={} definitions={} segments={} bytes={} \ -bytes_if_packed={}", +bytes_if_packed={} poisoned={}", if mode.uses_storage() { "storage" } else { "packed" }, live_keys.len(), uploaded_segments, uploaded_bytes, uploaded_segments * std::mem::size_of::() * 6, + poisoned, ); } out diff --git a/src/scene/view/render.rs b/src/scene/view/render.rs index 768f20ce4..7d5d9eded 100644 --- a/src/scene/view/render.rs +++ b/src/scene/view/render.rs @@ -714,6 +714,9 @@ impl shader::Primitive for Primitive { }) }) }; + // Snapshot before any arena allocation, so a rejected + // upload cannot claim this content id below. + let arena_errors_before = crate::scene::pipeline::gpu_errors_seen(); let reg_ok = base_ok && if let Some(arena) = inner.wire_arena.as_mut() { let patch = patch.unwrap(); @@ -868,7 +871,15 @@ impl shader::Primitive for Primitive { inner.wire_handle_index = wire_arena::build_handle_index(&vp_wires[..]); } - inner.wire_arena_id = vp.wire_content_id; + // Only claim the content id when the device accepted + // the upload; otherwise the slot believes it holds this + // content and never rebuilds it. + inner.wire_arena_id = + if crate::scene::pipeline::gpu_errors_seen() == arena_errors_before { + vp.wire_content_id + } else { + u64::MAX + }; arena_served = true; } else { inner.wire_arena = None; @@ -922,6 +933,8 @@ impl shader::Primitive for Primitive { ); } let t_upload = _perf.then(iced::time::Instant::now); + let errors_before = + crate::scene::pipeline::gpu_errors_seen(); let entry = inner.build_wire_buffers(device, queue, &vp_wires[..], &draw_depths); if let Some(start) = t_upload { @@ -933,9 +946,16 @@ impl shader::Primitive for Primitive { ); } - pipeline - .wire_buffer_cache - .insert(vp.wire_content_id, entry.clone()); + // Buffers the device rejected are still `Buffer`s. + // Cached under a content id that keeps matching, + // they render nothing for the rest of the session + // and nothing ever rebuilds them. Draw the degraded + // frame, but let the next one try again. + if crate::scene::pipeline::gpu_errors_seen() == errors_before { + pipeline + .wire_buffer_cache + .insert(vp.wire_content_id, entry.clone()); + } entry } };