Build only needed shaders with `--cpu` #383

Zoxc · 2023-10-12T10:18:33Z

This changes shaders to have an optional GPU and an optional CPU shader. This allows us to only compile shaders which will actually get used, helping startup times.

The separate install_cpu_shaders function is removed and the CPU shaders are passed with add_shader.

armansito

A couple of minor notes and suggestions. I think this is overall OK for now but I'd like you to be aware that there are a few aspects of the shaders that are in flux and will change quite a bit:

As I noted inline, src/shaders.rs will get rewritten to use crates/shaders which is our solution for exporting all the code for consumption in external/custom renderers.
Currently there are a lot of type definitions for CPU shaders and CPU vs GPU-side resource management that live in wgpu_engine.rs. I would very much like to decouple initialization and management of CPU pipelines/resources and the engine that executes them.

For example we want to add different backend engines that don't use wgpu because we want to research and experiment with features that are not available in wgpu (I'm working on a metal_engine.rs right now). IWBN to avoid having duplicate all the CPU-shader definitions for every engine that wants to have that capability.

This shouldn't impact your PR since you've been working with what exists now. I just wanted to point out that a lot of this is in flux and will gradually evolve.

armansito · 2023-10-17T18:06:55Z

src/wgpu_engine.rs

+pub enum CpuShaderType {
+    Present(fn(u32, &[CpuBinding])),
+    Missing,
+    Ununsed,


Was this meant to be Unused?

Please fix this typo (this should be Unused not Ununsed). Also, following the thread above, I think I prefer to call this Skipped.

armansito · 2023-10-17T18:17:03Z

src/wgpu_engine.rs

+
+pub enum CpuShaderType {
+    Present(fn(u32, &[CpuBinding])),
+    Missing,


I'm wondering why we need two different ways (Missing and Unused) to disable a CPU shader. I think we could just have two modes instead: a CPU shader is either present or it's not. Then at execution time we either use the GPU version or we use the CPU one. I think for now, this can be represented by two variants rather than three. WDYT?

Unused exists because neither the CPU or GPU shader is needed when passing use-cpu, so we can skip compiling the GPU shader even if the CPU shader is missing.

I see, Unused represents a stage that gets skipped when running in CPU mode. I think this only makes sense in the current global "run everything on the CPU setting" but once we support a combination of CPU and GPU pipelines, we will likely want to have the pipelines available regardless.

I think what I would prefer is to have a way to defer pipeline creation for an unused pipeline rather than skip it using a global setting (similarly to what we have with buffer resources and "proxies" in our engine abstraction). But that should happen as future work.

armansito · 2023-10-17T18:38:05Z

src/shaders.rs

+
+    let mut force_gpu = false;
+
+    let force_gpu_from: Option<&str> = None;


I think IWBN to decide this dynamically at render time. I think I'd like to move towards a mode where the presence of the CPU shaders and the decision on their execution are decoupled from each other.

A legitimate production case for this is small scenes: when rasterizing a handful of draw objects, we may want to process them quickly on the CPU to avoid the GPU upload and dispatch overhead and only run the coarse+fine raster stages on the GPU. We may want to make the CPU vs GPU decision dynamically at render time based on the size of the workload. In that world we may want to compile all the pipelines up front even though we'll sometimes use the CPU versions.

Ideally I'd like FullShaders to hold all available pipelines and have an option we give to engine that tells it which stages should run on the CPU.

Another thing I'd like you to be aware of is that we want to rewrite src/shaders.rs to be in terms of crates/shaders which automatically preprocesses the WGSL and the pipeline layouts. The CPU shaders will need to integrate with that somehow and they may move to the shaders crate (though we haven't put a lot of thought into this yet).

What you have is fine for now but be aware that this is all in flux and will change eventually.

I would be nicer if we could run a set of shaders on the CPU, but we'll need readback hooked up on wasm first. This is a bit of a hack until then.

I think the most common case will involve CPU stages that are strictly ordered before any GPU stages (rather than a mix of them), which shouldn't require read-back. I don't know if/when we'd want to run them in combination except maybe for testing.

armansito

Approved with a couple of comments. Please address them before merging.

Thank you for the improvements to the CPU pipeline management!

armansito · 2023-10-30T18:15:53Z

src/shaders.rs

+
+    let mut force_gpu = false;
+
+    let force_gpu_from: Option<&str> = None;


I think the most common case will involve CPU stages that are strictly ordered before any GPU stages (rather than a mix of them), which shouldn't require read-back. I don't know if/when we'd want to run them in combination except maybe for testing.

armansito · 2023-10-30T18:16:17Z

src/wgpu_engine.rs

+pub enum CpuShaderType {
+    Present(fn(u32, &[CpuBinding])),
+    Missing,
+    Ununsed,


Please fix this typo (this should be Unused not Ununsed). Also, following the thread above, I think I prefer to call this Skipped.

armansito · 2023-10-30T18:24:46Z

src/wgpu_engine.rs

+
+pub enum CpuShaderType {
+    Present(fn(u32, &[CpuBinding])),
+    Missing,


I see, Unused represents a stage that gets skipped when running in CPU mode. I think this only makes sense in the current global "run everything on the CPU setting" but once we support a combination of CPU and GPU pipelines, we will likely want to have the pipelines available regardless.

I think what I would prefer is to have a way to defer pipeline creation for an unused pipeline rather than skip it using a global setting (similarly to what we have with buffer resources and "proxies" in our engine abstraction). But that should happen as future work.

Zoxc mentioned this pull request Oct 13, 2023

Get the fine CPU shader running #386

Open

Zoxc force-pushed the opt_shader branch from 84103ac to d322789 Compare October 13, 2023 21:31

armansito reviewed Oct 17, 2023

View reviewed changes

Zoxc force-pushed the opt_shader branch from d322789 to a878684 Compare October 28, 2023 02:48

armansito approved these changes Oct 30, 2023

View reviewed changes

Build only needed shaders with --cpu

8d372bf

Zoxc force-pushed the opt_shader branch from a878684 to 8d372bf Compare November 1, 2023 02:04

Zoxc merged commit ddca7c5 into linebender:main Nov 1, 2023
4 checks passed

Zoxc deleted the opt_shader branch November 1, 2023 02:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build only needed shaders with `--cpu` #383

Build only needed shaders with `--cpu` #383

Zoxc commented Oct 12, 2023

armansito left a comment

armansito Oct 17, 2023

armansito Oct 30, 2023

armansito Oct 17, 2023

Zoxc Oct 18, 2023

armansito Oct 30, 2023

armansito Oct 17, 2023

Zoxc Oct 18, 2023

armansito Oct 30, 2023

armansito left a comment

armansito Oct 30, 2023

armansito Oct 30, 2023

armansito Oct 30, 2023


		let mut force_gpu = false;

		let force_gpu_from: Option<&str> = None;

Build only needed shaders with --cpu #383

Build only needed shaders with --cpu #383

Conversation

Zoxc commented Oct 12, 2023

armansito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armansito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Build only needed shaders with `--cpu` #383

Build only needed shaders with `--cpu` #383