Free emptied terrain buffer pages a few frames later, off the render thread

Arena::trim dropped an emptied page's buffer (up to 256 MB) on the render
thread; giving it back to the driver took that frame 8-14 ms of CPU on Mesa's
lavapipe (spans, RD 12 flights). A dimension change (clear_sections) dropped
every page at once the same way.

Arena::trim and Arena::clear now give the buffers back; the renderer keeps them
in a Graveyard for four frames, so the GPU is done with them and the bind
groups naming them (flat draws' page tables) are gone, then drops them on a
short-lived thread, as chunk::free_columns does for unloaded chunks. The
driver frees them as before.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

crates/horizon-client-render/src/gfx.rs

@@ -924,6 +924,8 @@ pub struct Renderer {
     /// last asked (`take_dropped_sections`).
     sections_dropped: bool,
     sorted_indices: arena::Arena,
+    /// Buffers of released arena pages, freed a few frames later on a thread of their own.
+    graveyard: arena::Graveyard<wgpu::Buffer>,
     translucent_sorter: translucency::Sorter,
     translucency_cursor: usize,
     /// `DrawIndexedIndirectArgs` for this frame's solid then translucent draws.
@@ -3695,6 +3697,7 @@ impl Renderer {
             translucent_batching,
             sections_dropped: false,
             sorted_indices,
+            graveyard: Default::default(),
             translucent_sorter: translucency::Sorter::default(),
             translucency_cursor: 0,
             indirect,
@@ -4443,14 +4446,15 @@ impl Renderer {
         self.lod_pending = None;
         self.detail_columns.clear();
         self.detail_revision += 1;
-        self.arena.clear();
+        // A dimension change drops every page at once: freed later, off this thread.
+        self.graveyard.add(self.arena.clear());
         if let Some(arena) = &mut self.translucent_arena {
-            arena.clear();
+            self.graveyard.add(arena.clear());
         }
         if let Some(flat) = &mut self.flat {
             flat.clear();
         }
-        self.sorted_indices.clear();
+        self.graveyard.add(self.sorted_indices.clear());
         self.translucency_cursor = 0;
         if layout == self.layout {
             self.occlusion.clear();
@@ -5341,12 +5345,14 @@ impl Renderer {
         if !frame.title {
             self.follow_translucent_layout();
         }
-        self.arena.trim();
+        let released = self.arena.trim();
+        self.graveyard.add(released);
         let left = self.compact_arena();
         if let Some(arena) = &mut self.translucent_arena {
-            arena.trim();
+            self.graveyard.add(arena.trim());
         }
-        self.sorted_indices.trim();
+        self.graveyard.add(self.sorted_indices.trim());
+        arena::free_later(self.graveyard.advance());
         self.shrink_translucent(left);
         if let Some(flat) = &mut self.flat {
             flat.trim(|page| self.arena.has_page(page));

crates/horizon-client-render/src/gfx/arena.rs

@@ -4,7 +4,7 @@
 //! with pages up to `LARGE_PAGE_BYTES`, and their sorted indices too
 //! (`Arena::translucent_indices`): their back-to-front list then stays in one
 //! page of each and draws in a few multi-draws.
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, VecDeque};
 
 /// Ranges are rounded up to this many vertices to limit fragmentation, and
 /// start at multiples of it (`flat` names sections by these blocks).
@@ -545,14 +545,17 @@ impl Arena {
         old.free.give(previous, capacity - previous);
         true
     }
-    /// Reclaim after an upload batch, so replacing a section can reuse its range.
-    pub fn trim(&mut self) {
+    /// Reclaim after an upload batch, so replacing a section can reuse its range. Gives back
+    /// the buffers of the pages it emptied, for a [`Graveyard`].
+    pub fn trim(&mut self) -> Vec<wgpu::Buffer> {
+        let mut released = Vec::new();
         for page in &mut self.pages {
-            if let Some(p) = page
-                && p.free.0.len() == 1
-                && p.free.0.get(&0) == Some(&p.capacity)
+            if page
+                .as_ref()
+                .is_some_and(|p| p.free.0.len() == 1 && p.free.0.get(&0) == Some(&p.capacity))
+                && let Some(p) = page.take()
             {
-                *page = None;
+                released.push(p.buffer);
             }
         }
         if self.closing.is_some_and(|page| !self.has_page(page)) {
@@ -562,13 +565,67 @@ impl Arena {
         while self.pages.last().is_some_and(Option::is_none) {
             self.pages.pop();
         }
+        released
     }
-    pub fn clear(&mut self) {
-        self.pages.clear();
+    /// Drops every page; gives back their buffers, for a [`Graveyard`].
+    pub fn clear(&mut self) -> Vec<wgpu::Buffer> {
         self.closing = None;
+        self.pages.drain(..).flatten().map(|p| p.buffer).collect()
     }
 }
 
+/// Frames a released page waits before it is freed: by then the GPU is done with the frames that
+/// used it, and the bind groups naming it (flat draws' page tables) are gone, so the freeing
+/// thread holds its last reference.
+const GRAVE_FRAMES: usize = 4;
+
+/// Buffers of released pages, freed [`GRAVE_FRAMES`] frames later on a thread of their own
+/// ([`free_later`]): giving a page of up to 256 MB back to the driver on the render thread took
+/// that frame 8-14 ms of CPU (Mesa's lavapipe), when flight or a teleport emptied a page.
+pub struct Graveyard<T> {
+    graves: VecDeque<Vec<T>>,
+}
+
+impl<T> Default for Graveyard<T> {
+    fn default() -> Self {
+        Self {
+            graves: VecDeque::new(),
+        }
+    }
+}
+
+impl<T> Graveyard<T> {
+    /// Released this frame.
+    pub fn add(&mut self, items: impl IntoIterator<Item = T>) {
+        if self.graves.is_empty() {
+            self.graves.push_back(Vec::new());
+        }
+        if let Some(grave) = self.graves.back_mut() {
+            grave.extend(items);
+        }
+    }
+    /// A frame went by: what was released `GRAVE_FRAMES` frames ago, to free.
+    pub fn advance(&mut self) -> Vec<T> {
+        self.graves.push_back(Vec::new());
+        if self.graves.len() > GRAVE_FRAMES {
+            self.graves.pop_front().unwrap_or_default()
+        } else {
+            Vec::new()
+        }
+    }
+}
+
+/// Drops `items` on a short-lived thread of its own, as `chunk::free_columns` does for
+/// unloaded chunks; should the thread not start, they drop here.
+pub fn free_later<T: Send + 'static>(items: Vec<T>) {
+    if items.is_empty() {
+        return;
+    }
+    let _ = std::thread::Builder::new()
+        .name("Buffer freeing".into())
+        .spawn(move || drop(items));
+}
+
 /// `create_buffer` with an out-of-memory failure returned instead of raised
 /// (raised errors are fatal).
 fn try_create(device: &wgpu::Device, size: u64, usage: wgpu::BufferUsages) -> Option<wgpu::Buffer> {
@@ -587,6 +644,24 @@ fn try_create(device: &wgpu::Device, size: u64, usage: wgpu::BufferUsages) -> Op
 mod tests {
     use super::*;
 
+    /// A released page is handed back for freeing `GRAVE_FRAMES` frames later, in order.
+    #[test]
+    fn graveyard_frees_after_its_frames() {
+        let mut graves = Graveyard::default();
+        graves.add([1, 2]);
+        let mut freed = Vec::new();
+        for frame in 0..GRAVE_FRAMES + 2 {
+            if frame == 1 {
+                graves.add([3]);
+            }
+            freed.push(graves.advance());
+        }
+        let mut expected = vec![Vec::new(); GRAVE_FRAMES + 2];
+        expected[GRAVE_FRAMES - 1] = vec![1, 2];
+        expected[GRAVE_FRAMES] = vec![3];
+        assert_eq!(freed, expected);
+    }
+
     fn device() -> Option<(wgpu::Device, wgpu::Queue)> {
         device_with(wgpu::Features::empty())
     }