From 0d8a4d4292dac3fceaa40e73f1bbae038d7de30f Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Mon, 8 Jun 2026 19:09:47 -0400 Subject: [PATCH] gpui: Free atlas tile space when removing tiles (#58874) Partially addresses #54659 `PlatformAtlas::remove` removed the tile from `tiles_by_key` and decremented the texture's refcount, but never returned the tile's rectangle to the etagere allocator. Removed tiles' space could only be reclaimed if the entire texture was dropped, which doesn't happen if glyph/emoji share the texture. Because we don't have correct glyph/emoji removal I added regression tests on all three platforms to prevent this from occurring as well. Note: this doesn't fix glyph eviction or the nil-texture panic from #54659; those are follow ups. Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Closes #ISSUE Release Notes: - gpui: Fix GPUI memory growing unboundedly when images are repeatedly loaded and released --- crates/gpui_macos/src/metal_atlas.rs | 32 ++++++++- crates/gpui_wgpu/src/wgpu_atlas.rs | 48 +++++++++++++- crates/gpui_windows/src/directx_atlas.rs | 84 +++++++++++++++++++++++- 3 files changed, 161 insertions(+), 3 deletions(-) diff --git a/crates/gpui_macos/src/metal_atlas.rs b/crates/gpui_macos/src/metal_atlas.rs index 5bedf9df8cd..ce5664d778a 100644 --- a/crates/gpui_macos/src/metal_atlas.rs +++ b/crates/gpui_macos/src/metal_atlas.rs @@ -61,9 +61,10 @@ impl PlatformAtlas for MetalAtlas { fn remove(&self, key: &AtlasKey) { let mut lock = self.0.lock(); - let Some(id) = lock.tiles_by_key.remove(key).map(|v| v.texture_id) else { + let Some(tile) = lock.tiles_by_key.remove(key) else { return; }; + let id = tile.texture_id; let textures = match id.kind { AtlasTextureKind::Monochrome => &mut lock.monochrome_textures, @@ -80,6 +81,7 @@ impl PlatformAtlas for MetalAtlas { }; if let Some(mut texture) = texture_slot.take() { + texture.allocator.deallocate(tile.tile_id.into()); texture.decrement_ref_count(); if texture.is_unreferenced() { textures.free_list.push(id.index as usize); @@ -338,6 +340,34 @@ mod tests { let _texture = atlas.metal_texture(tile_a2.texture_id); } + #[test] + fn test_remove_deallocates_tile_space_for_reuse() { + let Some(atlas) = create_atlas() else { + return; + }; + + let small = Size { + width: DevicePixels(64), + height: DevicePixels(64), + }; + let big = Size { + width: DevicePixels(700), + height: DevicePixels(700), + }; + + let keeper_key = make_image_key(1, 0); + let big_key_a = make_image_key(2, 0); + let big_key_b = make_image_key(3, 0); + + let keeper_tile = insert_tile(&atlas, &keeper_key, small); + let tile_a = insert_tile(&atlas, &big_key_a, big); + assert_eq!(keeper_tile.texture_id, tile_a.texture_id); + + atlas.remove(&big_key_a); + let tile_b = insert_tile(&atlas, &big_key_b, big); + assert_eq!(tile_b.texture_id, keeper_tile.texture_id); + } + #[test] fn test_remove_nonexistent_key_is_noop() { let Some(atlas) = create_atlas() else { diff --git a/crates/gpui_wgpu/src/wgpu_atlas.rs b/crates/gpui_wgpu/src/wgpu_atlas.rs index 94b4b561c00..3cf71ac98cf 100644 --- a/crates/gpui_wgpu/src/wgpu_atlas.rs +++ b/crates/gpui_wgpu/src/wgpu_atlas.rs @@ -130,15 +130,17 @@ impl PlatformAtlas for WgpuAtlas { fn remove(&self, key: &AtlasKey) { let mut lock = self.0.lock(); - let Some(id) = lock.tiles_by_key.remove(key).map(|tile| tile.texture_id) else { + let Some(tile) = lock.tiles_by_key.remove(key) else { return; }; + let id = tile.texture_id; let Some(texture_slot) = lock.storage[id.kind].textures.get_mut(id.index as usize) else { return; }; if let Some(mut texture) = texture_slot.take() { + texture.allocator.deallocate(tile.tile_id.into()); texture.decrement_ref_count(); if texture.is_unreferenced() { lock.pending_uploads @@ -461,6 +463,50 @@ mod tests { Ok(()) } + #[test] + fn remove_deallocates_tile_space_for_reuse() -> anyhow::Result<()> { + let (device, queue) = test_device_and_queue()?; + let atlas = WgpuAtlas::new(device, queue, wgpu::TextureFormat::Bgra8Unorm); + + let small = Size { + width: DevicePixels(64), + height: DevicePixels(64), + }; + let big = Size { + width: DevicePixels(700), + height: DevicePixels(700), + }; + + let make_key = |image_id: usize| { + AtlasKey::Image(RenderImageParams { + image_id: ImageId(image_id), + frame_index: 0, + }) + }; + let insert = |key: &AtlasKey, size: Size| { + let byte_count = (size.width.0 as usize) * (size.height.0 as usize) * 4; + atlas + .get_or_insert_with(key, &mut || { + Ok(Some((size, Cow::Owned(vec![0u8; byte_count])))) + }) + .expect("allocation should succeed") + .expect("callback returns Some") + }; + + let keeper_key = make_key(1); + let big_key_a = make_key(2); + let big_key_b = make_key(3); + + let keeper_tile = insert(&keeper_key, small); + let tile_a = insert(&big_key_a, big); + assert_eq!(keeper_tile.texture_id, tile_a.texture_id); + + atlas.remove(&big_key_a); + let tile_b = insert(&big_key_b, big); + assert_eq!(tile_b.texture_id, keeper_tile.texture_id); + Ok(()) + } + #[test] fn swizzle_upload_data_preserves_bgra_uploads() { let input = vec![0x10, 0x20, 0x30, 0x40]; diff --git a/crates/gpui_windows/src/directx_atlas.rs b/crates/gpui_windows/src/directx_atlas.rs index a6642dc7dc6..d5b1a674309 100644 --- a/crates/gpui_windows/src/directx_atlas.rs +++ b/crates/gpui_windows/src/directx_atlas.rs @@ -98,9 +98,10 @@ impl PlatformAtlas for DirectXAtlas { fn remove(&self, key: &AtlasKey) { let mut lock = self.0.lock(); - let Some(id) = lock.tiles_by_key.remove(key).map(|tile| tile.texture_id) else { + let Some(tile) = lock.tiles_by_key.remove(key) else { return; }; + let id = tile.texture_id; let textures = match id.kind { AtlasTextureKind::Monochrome => &mut lock.monochrome_textures, @@ -113,6 +114,7 @@ impl PlatformAtlas for DirectXAtlas { }; if let Some(mut texture) = texture_slot.take() { + texture.allocator.deallocate(tile.tile_id.into()); texture.decrement_ref_count(); if texture.is_unreferenced() { textures.free_list.push(texture.id.index as usize); @@ -318,3 +320,83 @@ fn etagere_point_to_device(value: etagere::Point) -> Point { y: DevicePixels::from(value.y), } } + +#[cfg(test)] +mod tests { + use super::*; + use gpui::{ImageId, RenderImageParams}; + use std::borrow::Cow; + use windows::Win32::{ + Foundation::HMODULE, + Graphics::{ + Direct3D::D3D_DRIVER_TYPE_WARP, + Direct3D11::{D3D11_CREATE_DEVICE_BGRA_SUPPORT, D3D11_SDK_VERSION, D3D11CreateDevice}, + }, + }; + + fn create_atlas() -> Option { + let mut device: Option = None; + let mut device_context: Option = None; + unsafe { + D3D11CreateDevice( + None, + D3D_DRIVER_TYPE_WARP, + HMODULE::default(), + D3D11_CREATE_DEVICE_BGRA_SUPPORT, + None, + D3D11_SDK_VERSION, + Some(&mut device), + None, + Some(&mut device_context), + ) + } + .ok()?; + Some(DirectXAtlas::new(&device?, &device_context?)) + } + + fn make_image_key(image_id: usize) -> AtlasKey { + AtlasKey::Image(RenderImageParams { + image_id: ImageId(image_id), + frame_index: 0, + }) + } + + fn insert_tile(atlas: &DirectXAtlas, key: &AtlasKey, size: Size) -> AtlasTile { + atlas + .get_or_insert_with(key, &mut || { + let byte_count = (size.width.0 as usize) * (size.height.0 as usize) * 4; + Ok(Some((size, Cow::Owned(vec![0u8; byte_count])))) + }) + .expect("allocation should succeed") + .expect("callback returns Some") + } + + #[test] + fn test_remove_deallocates_tile_space_for_reuse() { + let Some(atlas) = create_atlas() else { + return; + }; + + let small = Size { + width: DevicePixels(64), + height: DevicePixels(64), + }; + let big = Size { + width: DevicePixels(700), + height: DevicePixels(700), + }; + + let keeper_key = make_image_key(1); + let big_key_a = make_image_key(2); + let big_key_b = make_image_key(3); + + let keeper_tile = insert_tile(&atlas, &keeper_key, small); + let tile_a = insert_tile(&atlas, &big_key_a, big); + assert_eq!(keeper_tile.texture_id, tile_a.texture_id); + + atlas.remove(&big_key_a); + + let tile_b = insert_tile(&atlas, &big_key_b, big); + assert_eq!(tile_b.texture_id, keeper_tile.texture_id); + } +}