[Merged by Bors] - Compute Pipeline Specialization

[kurtkuehnert]

Objective

• Fixes Compute Shader Integration #3970
• To support Bevy's shader abstraction(shader defs, shader imports and hot shader reloading) for compute shaders, I have followed carts advice and change the PipelinenCache to accommodate both compute and render pipelines.

Solution

• renamed RenderPipelineCache to PipelineCache
• Cached Pipelines are now represented by an enum (render, compute)
• split the SpecializedPipelines into SpecializedRenderPipelines and SpecializedComputePipelines
• updated the game of life example

Open Questions

• should SpecializedRenderPipelines and SpecializedComputePipelines be merged and how would we do that?
• should the get_render_pipeline and get_compute_pipeline methods be merged?
• is pipeline specialization for different entry points a good pattern

[kurtkuehnert]

Clippy complains, that the newly introduced PipelineDescriptor has two variants with a large size difference. https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
I think it is fine to just ignore it since there wont be too many pipelines for it to really matter, but what do you think?

[mockersf]

error: large size difference between variants
  --> crates/bevy_render/src/render_resource/pipeline_cache.rs:21:5
   |
21 |     RenderPipelineDescriptor(RenderPipelineDescriptor),
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this variant is 432 bytes
   |
   = note: `-D clippy::large-enum-variant` implied by `-D warnings`
note: and the second-largest variant is 160 bytes:
  --> crates/bevy_render/src/render_resource/pipeline_cache.rs:22:5
   |
22 |     ComputePipelineDescriptor(ComputePipelineDescriptor),
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
help: consider boxing the large fields to reduce the total size of the enum
   |
21 |     RenderPipelineDescriptor(Box<RenderPipelineDescriptor>),
   |                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

That means each ComputePipelineDescriptor will waste 272 bytes...

[superdump]

So I guess we should try Boxing them and see if it has any negative impact on performance?

[kurtkuehnert]

So I guess we should try Boxing them and see if it has any negative impact on performance?

Ok, will do that later. 😃

[kurtkuehnert]

I did something wrong while rebasing. ...again

[superdump]

This looks generally in the right ballpark. I wonder if there is a way to duplicate less of the code for render vs compute. I have a mild wonder about whether it impacts performance of render stuff in any way. Does it affect bevymark performance at all?

[cart]

Yup I like the general direction of a consolidated RenderPipeline type, although I think we might want to have separate CachedRenderPipelineId and CachedComputePipelineId (and internally remove and "switching" on compute and render pipelines). That would eliminate a class of user error where they pass a "render pipeline" CachedPipelineId into get_compute_pipeline (or vice versa). Likewise, it would eliminate all of the "internal switching" thats going on right now. It would mean duplicating some code, but I think its worth it.

[kurtkuehnert]

I will test performance and experiment with the distinct IDs tomorrow. 👍

[kurtkuehnert]

I have done some form of a split, but I don't really think it is much of an improvement. I still believe there is some way to make all of this pretty with some rust magic, but I am not experienced enough to see how.

[cart]

I think your changes made a good compromise between code reuse and "separate code paths". Splitting out the remaining enums would start bleeding in to the shader caching logic, which would make things pretty nasty.

I removed specialization from the game of life example as I think it hindered legibility. Imo specialization should only be used if you really need it. In this case it added over 30 lines of code / extra type complexity for no real benefit.

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-android
• build (nightly, ubuntu-latest)
• build (stable, macos-latest)
• build (stable, ubuntu-latest)
• build (stable, windows-latest)
• build-wasm (nightly, ubuntu-latest)
• build-wasm (stable, ubuntu-latest)
• check-benches
• check-doc
• check-markdown-links
• check-missing-examples-in-docs
• check-unused-dependencies
• ci
• markdownlint
• run-examples