Skip to content

Improve texture caching by allowing weak refs and more formats - #4447

Open
timon-schelling wants to merge 8 commits into
fix-vello-texture-leaks-prfrom
texture-cache-pr
Open

Improve texture caching by allowing weak refs and more formats#4447
timon-schelling wants to merge 8 commits into
fix-vello-texture-leaks-prfrom
texture-cache-pr

Conversation

@timon-schelling

Copy link
Copy Markdown
Member

Allow nodes (that use gpu pipelines) to hold weak refs to textures.
If a texture is only referenced weakly the cache is allowed to GC it when under memory pressure.
Nodes should use this when they still have use for a texture (can be used to speed up rendering for example) but can also just rerender the content (with a slowdown).

Also includes fixes for GPU out of memory issues on web.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 issues found across 13 files

Confidence score: 2/5

  • In node-graph/libraries/wgpu-executor/src/shader_runtime/per_pixel_adjust_runtime.rs, the pipeline cache can keep targeting Rgba8UnormSrgb while the render target is created as Rgba8Unorm, and wgpu will reject the render pass due to format mismatch; this is the highest immediate regression risk for affected textures — key the cached pipeline by the actual attachment format (or rebuild on format change).
  • In node-graph/libraries/wgpu-executor/src/lib.rs, the WebGPU texture-cache cap is large enough to retain ~512 MiB of unused textures, which can exceed browser GPU budgets and lead to device loss/OOM in real sessions — lower the web cap to a conservative default or derive it from adapter limits.
  • In node-graph/libraries/wgpu-executor/src/lib.rs (request_texture_with_format), using texture_cache.lock().unwrap() means a panic while holding the mutex can poison it, turning a transient GPU failure into persistent cache failures on later calls — handle lock poisoning/recovery explicitly instead of unwrapping.
  • In node-graph/libraries/wgpu-executor/src/texture_cache.rs, compressed-texture accounting uses block_copy_size as if it were bytes-per-texel, which overestimates usage and can evict textures too aggressively, hurting cache effectiveness/perf — compute usage from block dimensions and block counts for compressed formats.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="node-graph/libraries/wgpu-executor/src/shader_runtime/per_pixel_adjust_runtime.rs">

<violation number="1" location="node-graph/libraries/wgpu-executor/src/shader_runtime/per_pixel_adjust_runtime.rs:206">
P1: When the input texture uses `Rgba8Unorm`, this line creates an `Rgba8Unorm` render target, but the cached pipeline still targets `Rgba8UnormSrgb`. wgpu rejects the render pass because the attachment format must match the pipeline; make the pipeline format-aware and cache pipelines by format, or keep the output format compatible with the pipeline.</violation>
</file>

<file name="node-graph/libraries/wgpu-executor/src/texture_cache.rs">

<violation number="1" location="node-graph/libraries/wgpu-executor/src/texture_cache.rs:33">
P2: For block-compressed formats, `block_copy_size` is bytes per compressed block, not bytes per texel. This overestimates cache usage and causes premature eviction; count blocks using the format's block dimensions in both calculations.</violation>
</file>

<file name="node-graph/libraries/wgpu-executor/src/lib.rs">

<violation number="1" location="node-graph/libraries/wgpu-executor/src/lib.rs:39">
P1: On WebGPU, this cap allows 512 MiB of unused textures to remain strongly cached, which can exceed browser GPU budgets and trigger device loss/OOM. Keep the cache cap within a conservative web budget or derive it from the available device budget.</violation>

<violation number="2" location="node-graph/libraries/wgpu-executor/src/lib.rs:119">
P2: `request_texture_with_format` calls `texture_cache.lock().unwrap()` on the new `std::sync::Mutex`. If anything panics while the lock is held (e.g. `device.create_texture` on device-lost or GPU OOM — the scenario this PR is fixing), the mutex is poisoned and every later `request_texture` call panics, taking down the app. The previous `futures::lock::Mutex::lock().await` did not poison, so this is a regression. Prefer handling the poisoned-lock case gracefully instead of `unwrap()`.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

usage: wgpu::TextureUsages::TEXTURE_BINDING | wgpu::TextureUsages::COPY_DST | wgpu::TextureUsages::COPY_SRC | wgpu::TextureUsages::RENDER_ATTACHMENT,
view_formats: &[format],
});
let tex_out = executor.request_texture_with_format(UVec2::new(tex_in.width(), tex_in.height()), format);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: When the input texture uses Rgba8Unorm, this line creates an Rgba8Unorm render target, but the cached pipeline still targets Rgba8UnormSrgb. wgpu rejects the render pass because the attachment format must match the pipeline; make the pipeline format-aware and cache pipelines by format, or keep the output format compatible with the pipeline.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/wgpu-executor/src/shader_runtime/per_pixel_adjust_runtime.rs, line 206:

<comment>When the input texture uses `Rgba8Unorm`, this line creates an `Rgba8Unorm` render target, but the cached pipeline still targets `Rgba8UnormSrgb`. wgpu rejects the render pass because the attachment format must match the pipeline; make the pipeline format-aware and cache pipelines by format, or keep the output format compatible with the pipeline.</comment>

<file context>
@@ -203,16 +203,7 @@ impl PerPixelAdjustGraphicsPipeline {
-					usage: wgpu::TextureUsages::TEXTURE_BINDING | wgpu::TextureUsages::COPY_DST | wgpu::TextureUsages::COPY_SRC | wgpu::TextureUsages::RENDER_ATTACHMENT,
-					view_formats: &[format],
-				});
+				let tex_out = executor.request_texture_with_format(UVec2::new(tex_in.width(), tex_in.height()), format);
 
 				let view_out = tex_out.create_view(&TextureViewDescriptor::default());
</file context>

#[cfg(not(target_family = "wasm"))]
const TEXTURE_CACHE_SIZE: u64 = 1024 * 1024 * 1024; // 1GB
#[cfg(target_family = "wasm")]
const TEXTURE_CACHE_SIZE: u64 = 512 * 1024 * 1024; // 512MB

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: On WebGPU, this cap allows 512 MiB of unused textures to remain strongly cached, which can exceed browser GPU budgets and trigger device loss/OOM. Keep the cache cap within a conservative web budget or derive it from the available device budget.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/wgpu-executor/src/lib.rs, line 39:

<comment>On WebGPU, this cap allows 512 MiB of unused textures to remain strongly cached, which can exceed browser GPU budgets and trigger device loss/OOM. Keep the cache cap within a conservative web budget or derive it from the available device budget.</comment>

<file context>
@@ -30,7 +33,10 @@ pub use wgpu_sync::Instance as WgpuInstance;
+#[cfg(not(target_family = "wasm"))]
+const TEXTURE_CACHE_SIZE: u64 = 1024 * 1024 * 1024; // 1GB
+#[cfg(target_family = "wasm")]
+const TEXTURE_CACHE_SIZE: u64 = 512 * 1024 * 1024; // 512MB
 
 #[derive(dyn_any::DynAny, Clone)]
</file context>
Suggested change
const TEXTURE_CACHE_SIZE: u64 = 512 * 1024 * 1024; // 512MB
const TEXTURE_CACHE_SIZE: u64 = 256 * 1024 * 1024; // 256 MiB

}

let incoming_bytes = size.x as u64 * size.y as u64 * 4;
let incoming_bytes = size.x as u64 * size.y as u64 * format.block_copy_size(None).unwrap_or(4) as u64;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: For block-compressed formats, block_copy_size is bytes per compressed block, not bytes per texel. This overestimates cache usage and causes premature eviction; count blocks using the format's block dimensions in both calculations.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/wgpu-executor/src/texture_cache.rs, line 33:

<comment>For block-compressed formats, `block_copy_size` is bytes per compressed block, not bytes per texel. This overestimates cache usage and causes premature eviction; count blocks using the format's block dimensions in both calculations.</comment>

<file context>
@@ -17,49 +16,53 @@ impl TextureCache {
 		}
 
-		let incoming_bytes = size.x as u64 * size.y as u64 * 4;
+		let incoming_bytes = size.x as u64 * size.y as u64 * format.block_copy_size(None).unwrap_or(4) as u64;
 		self.evict_until_fits(incoming_bytes);
 
</file context>

}

pub fn request_texture_with_format(&self, size: UVec2, format: wgpu::TextureFormat) -> Texture {
self.inner.texture_cache.lock().unwrap().request_texture(&self.context().device, size, format)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: request_texture_with_format calls texture_cache.lock().unwrap() on the new std::sync::Mutex. If anything panics while the lock is held (e.g. device.create_texture on device-lost or GPU OOM — the scenario this PR is fixing), the mutex is poisoned and every later request_texture call panics, taking down the app. The previous futures::lock::Mutex::lock().await did not poison, so this is a regression. Prefer handling the poisoned-lock case gracefully instead of unwrap().

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/wgpu-executor/src/lib.rs, line 119:

<comment>`request_texture_with_format` calls `texture_cache.lock().unwrap()` on the new `std::sync::Mutex`. If anything panics while the lock is held (e.g. `device.create_texture` on device-lost or GPU OOM — the scenario this PR is fixing), the mutex is poisoned and every later `request_texture` call panics, taking down the app. The previous `futures::lock::Mutex::lock().await` did not poison, so this is a regression. Prefer handling the poisoned-lock case gracefully instead of `unwrap()`.</comment>

<file context>
@@ -109,8 +111,20 @@ impl WgpuExecutor {
+	}
+
+	pub fn request_texture_with_format(&self, size: UVec2, format: wgpu::TextureFormat) -> Texture {
+		self.inner.texture_cache.lock().unwrap().request_texture(&self.context().device, size, format)
+	}
+
</file context>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants