Interior mutablilty for re_renderer's static resource pools (RenderPi…

…peline/Shader/Layouts/etc.) (#4421) ### What * Prerequisite for #1325 * Followed by #4422 The dynamic resource pool (textures, buffers, bindgroups) was already interior mutable. The static resource pool (mostly append only pool) however, still required mutable access. This PR addresses this. The actual tricky part is the implications this has on the draw step: `wgpu::RenderPass<'a>` expects all passed in references to live for `'a`. This isn't much of a problem we have with resources from the dynamic resource pool since there the handles are actually Arcs to the underlying wgpu resources. However we can't do this with RenderPipelines (the only relevant static resource for this case) though since we want to be able to swap out what the handle points to for shader reloading. This means we need to hold the lock read lock on render pipelines for the entirety of the pass recording. This is quite nice and easy for our ViewBuilder draws (== individual space views), but hard for the "compositing step" where we draw into egui's render pass. This is solved by taking out all render pipelines of the lock and putting them back in at the start of the frame. Once wgpu lifts this restrictions we can simplify things a bit! (to be continued with follow-up PRs!) ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested the web demo (if applicable): * Full build: [app.rerun.io](https://app.rerun.io/pr/4421/index.html) * Partial build: [app.rerun.io](https://app.rerun.io/pr/4421/index.html?manifest_url=https://app.rerun.io/version) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG - [PR Build Summary](https://build.rerun.io/pr/4421) - [Docs preview](https://rerun.io/preview/8e4b0d88d9d01df8121a84850b251c1840494695/docs)  - [Examples preview](https://rerun.io/preview/8e4b0d88d9d01df8121a84850b251c1840494695/examples)  - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) --- Part of series towards more multithreading in the viewer! * #4387 * #4404 * #4389 * You are here ➡️ #4421 * #4422 * #4430
rerun-io · Dec 5, 2023 · a7589a0 · a7589a0
1 parent 9ddf32b
commit a7589a0
Show file tree

Hide file tree

Showing 26 changed files with 429 additions and 189 deletions.
diff --git a/crates/re_renderer/examples/framework.rs b/crates/re_renderer/examples/framework.rs
@@ -276,6 +276,10 @@ impl<E: Example + 'static> Application<E> {
                     );
 
                     {
+                        // Lock render pipelines for the lifetime of the composite pass.
+                        let render_pipelines =
+                            self.re_ctx.gpu_resources.render_pipelines.resources();
+
                         let mut composite_pass =
                             composite_cmd_encoder.begin_render_pass(&wgpu::RenderPassDescriptor {
                                 label: None,
@@ -295,6 +299,7 @@ impl<E: Example + 'static> Application<E> {
                         for draw_result in &draw_results {
                             draw_result.view_builder.composite(
                                 &self.re_ctx,
+                                &render_pipelines,
                                 &mut composite_pass,
                                 draw_result.target_location,
                             );

diff --git a/crates/re_renderer/src/context.rs b/crates/re_renderer/src/context.rs
@@ -9,7 +9,7 @@ use crate::{
     global_bindings::GlobalBindings,
     renderer::Renderer,
     resource_managers::{MeshManager, TextureManager2D},
-    wgpu_resources::WgpuResourcePools,
+    wgpu_resources::{GpuRenderPipelinePoolMoveAccessor, WgpuResourcePools},
     FileResolver, FileServer, FileSystem, RecommendedFileResolver,
 };
 
@@ -116,7 +116,7 @@ impl RenderContext {
         log_adapter_info(&adapter.get_info());
 
         let mut gpu_resources = WgpuResourcePools::default();
-        let global_bindings = GlobalBindings::new(&mut gpu_resources, &device);
+        let global_bindings = GlobalBindings::new(&gpu_resources, &device);
 
         // Validate capabilities of the device.
         assert!(
@@ -187,6 +187,7 @@ impl RenderContext {
         let active_frame = ActiveFrameContext {
             before_view_builder_encoder: Mutex::new(FrameGlobalCommandEncoder::new(&device)),
             per_frame_data_helper: TypeMap::new(),
+            pinned_render_pipelines: None,
             frame_index: 0,
         };
 
@@ -299,9 +300,18 @@ impl RenderContext {
         // Map all read staging buffers.
         self.gpu_readback_belt.get_mut().after_queue_submit();
 
+        // Give back moved render pipelines to the pool if any were moved out.
+        if let Some(moved_render_pipelines) = self.active_frame.pinned_render_pipelines.take() {
+            self.gpu_resources
+                .render_pipelines
+                .return_resources(moved_render_pipelines);
+        }
+
+        // New active frame!
         self.active_frame = ActiveFrameContext {
             before_view_builder_encoder: Mutex::new(FrameGlobalCommandEncoder::new(&self.device)),
             frame_index: self.active_frame.frame_index + 1,
+            pinned_render_pipelines: None,
             per_frame_data_helper: TypeMap::new(),
         };
         let frame_index = self.active_frame.frame_index;
@@ -431,6 +441,13 @@ pub struct ActiveFrameContext {
     /// Utility type map that will be cleared every frame.
     pub per_frame_data_helper: TypeMap,
 
+    /// Render pipelines that were moved out of the resource pool.
+    ///
+    /// Will be moved back to the resource pool at the start of the frame.
+    /// This is needed for accessing the render pipelines without keeping a reference
+    /// to the resource pool lock during the lifetime of a render pass.
+    pub pinned_render_pipelines: Option<GpuRenderPipelinePoolMoveAccessor>,
+
     /// Index of this frame. Is incremented for every render frame.
     frame_index: u64,
 }

diff --git a/crates/re_renderer/src/draw_phases/outlines.rs b/crates/re_renderer/src/draw_phases/outlines.rs
@@ -51,8 +51,8 @@ use crate::{
     view_builder::ViewBuilder,
     wgpu_resources::{
         BindGroupDesc, BindGroupEntry, BindGroupLayoutDesc, GpuBindGroup, GpuBindGroupLayoutHandle,
-        GpuRenderPipelineHandle, GpuTexture, PipelineLayoutDesc, PoolError, RenderPipelineDesc,
-        SamplerDesc, WgpuResourcePools,
+        GpuRenderPipelineHandle, GpuRenderPipelinePoolAccessor, GpuTexture, PipelineLayoutDesc,
+        PoolError, RenderPipelineDesc, SamplerDesc,
     },
     DebugLabel, RenderContext,
 };
@@ -372,11 +372,9 @@ impl OutlineMaskProcessor {
 
     pub fn compute_outlines(
         self,
-        pools: &WgpuResourcePools,
+        pipelines: &GpuRenderPipelinePoolAccessor<'_>,
         encoder: &mut wgpu::CommandEncoder,
     ) -> Result<(), PoolError> {
-        let pipelines = &pools.render_pipelines;
-
         let ops = wgpu::Operations {
             load: wgpu::LoadOp::Clear(wgpu::Color::TRANSPARENT), // Clear is the closest to "don't care"
             store: wgpu::StoreOp::Store,
@@ -396,16 +394,14 @@ impl OutlineMaskProcessor {
                 occlusion_query_set: None,
             });
 
-            let render_pipeline_init =
-                pipelines.get_resource(self.render_pipeline_jumpflooding_init)?;
+            let render_pipeline_init = pipelines.get(self.render_pipeline_jumpflooding_init)?;
             jumpflooding_init.set_bind_group(0, &self.bind_group_jumpflooding_init, &[]);
             jumpflooding_init.set_pipeline(render_pipeline_init);
             jumpflooding_init.draw(0..3, 0..1);
         }
 
         // Perform jump flooding.
-        let render_pipeline_step =
-            pipelines.get_resource(self.render_pipeline_jumpflooding_step)?;
+        let render_pipeline_step = pipelines.get(self.render_pipeline_jumpflooding_step)?;
         for (i, bind_group) in self.bind_group_jumpflooding_steps.into_iter().enumerate() {
             let mut jumpflooding_step = encoder.begin_render_pass(&wgpu::RenderPassDescriptor {
                 label: DebugLabel::from(format!("{} - jumpflooding_step {i}", self.label)).get(),
@@ -429,7 +425,7 @@ impl OutlineMaskProcessor {
     }
 
     fn create_bind_group_jumpflooding_init(
-        ctx: &mut RenderContext,
+        ctx: &RenderContext,
         instance_label: &DebugLabel,
         mask_texture: &GpuTexture,
     ) -> (GpuBindGroup, GpuBindGroupLayoutHandle) {
@@ -466,7 +462,7 @@ impl OutlineMaskProcessor {
 
     fn create_bind_groups_for_jumpflooding_steps(
         config: &OutlineConfig,
-        ctx: &mut RenderContext,
+        ctx: &RenderContext,
         instance_label: &DebugLabel,
         voronoi_textures: &[GpuTexture; 2],
     ) -> (Vec<GpuBindGroup>, GpuBindGroupLayoutHandle) {

diff --git a/crates/re_renderer/src/draw_phases/picking_layer.rs b/crates/re_renderer/src/draw_phases/picking_layer.rs
@@ -19,8 +19,8 @@ use crate::{
     view_builder::ViewBuilder,
     wgpu_resources::{
         BindGroupDesc, BindGroupEntry, BindGroupLayoutDesc, GpuBindGroup, GpuRenderPipelineHandle,
-        GpuTexture, GpuTextureHandle, PipelineLayoutDesc, PoolError, RenderPipelineDesc,
-        TextureDesc, WgpuResourcePools,
+        GpuRenderPipelinePoolAccessor, GpuTexture, GpuTextureHandle, PipelineLayoutDesc, PoolError,
+        RenderPipelineDesc, TextureDesc,
     },
     DebugLabel, GpuReadbackBuffer, GpuReadbackIdentifier, RectInt, RenderContext,
 };
@@ -344,20 +344,23 @@ impl PickingLayerProcessor {
     pub fn end_render_pass(
         self,
         encoder: &mut wgpu::CommandEncoder,
-        pools: &WgpuResourcePools,
+        render_pipelines: &GpuRenderPipelinePoolAccessor<'_>,
     ) -> Result<(), PickingLayerError> {
         let extent = glam::uvec2(
             self.picking_target.texture.width(),
             self.picking_target.texture.height(),
         );
 
-        let readable_depth_texture = if let Some(depth_copy_workaround) =
-            self.depth_readback_workaround.as_ref()
-        {
-            depth_copy_workaround.copy_to_readable_texture(encoder, pools, &self.bind_group_0)?
-        } else {
-            &self.picking_depth_target
-        };
+        let readable_depth_texture =
+            if let Some(depth_copy_workaround) = self.depth_readback_workaround.as_ref() {
+                depth_copy_workaround.copy_to_readable_texture(
+                    encoder,
+                    render_pipelines,
+                    &self.bind_group_0,
+                )?
+            } else {
+                &self.picking_depth_target
+            };
 
         self.readback_buffer.read_multiple_texture2d(
             encoder,
@@ -581,7 +584,7 @@ impl DepthReadbackWorkaround {
     fn copy_to_readable_texture(
         &self,
         encoder: &mut wgpu::CommandEncoder,
-        pools: &WgpuResourcePools,
+        render_pipelines: &GpuRenderPipelinePoolAccessor<'_>,
         global_binding_bind_group: &GpuBindGroup,
     ) -> Result<&GpuTexture, PoolError> {
         // Copy depth texture to a readable (color) texture with a screen filling triangle.
@@ -600,7 +603,7 @@ impl DepthReadbackWorkaround {
             occlusion_query_set: None,
         });
 
-        let pipeline = pools.render_pipelines.get_resource(self.render_pipeline)?;
+        let pipeline = render_pipelines.get(self.render_pipeline)?;
         pass.set_pipeline(pipeline);
         pass.set_bind_group(0, global_binding_bind_group, &[]);
         pass.set_bind_group(1, &self.bind_group, &[]);

diff --git a/crates/re_renderer/src/global_bindings.rs b/crates/re_renderer/src/global_bindings.rs
@@ -56,7 +56,7 @@ pub(crate) struct GlobalBindings {
 }
 
 impl GlobalBindings {
-    pub fn new(pools: &mut WgpuResourcePools, device: &wgpu::Device) -> Self {
+    pub fn new(pools: &WgpuResourcePools, device: &wgpu::Device) -> Self {
         Self {
             layout: pools.bind_group_layouts.get_or_create(
                 device,

diff --git a/crates/re_renderer/src/queuable_draw_data.rs b/crates/re_renderer/src/queuable_draw_data.rs
@@ -1,7 +1,8 @@
 use crate::{
+    context::Renderers,
     draw_phases::DrawPhase,
     renderer::{DrawData, DrawError, Renderer},
-    RenderContext,
+    wgpu_resources::GpuRenderPipelinePoolAccessor,
 };
 
 #[derive(thiserror::Error, Debug)]
@@ -17,7 +18,8 @@ pub enum QueueableDrawDataError {
 }
 
 type DrawFn = dyn for<'a, 'b> Fn(
-        &'b RenderContext,
+        &Renderers,
+        &'b GpuRenderPipelinePoolAccessor<'b>,
         DrawPhase,
         &'a mut wgpu::RenderPass<'b>,
         &'b dyn std::any::Any,
@@ -36,8 +38,7 @@ pub struct QueueableDrawData {
 impl<D: DrawData + Sync + Send + 'static> From<D> for QueueableDrawData {
     fn from(draw_data: D) -> Self {
         QueueableDrawData {
-            draw_func: Box::new(move |ctx, phase, pass, draw_data| {
-                let renderers = ctx.renderers.read();
+            draw_func: Box::new(move |renderers, gpu_resources, phase, pass, draw_data| {
                 let renderer = renderers.get::<D::Renderer>().ok_or(
                     QueueableDrawDataError::FailedToRetrieveRenderer(std::any::type_name::<
                         D::Renderer,
@@ -47,7 +48,7 @@ impl<D: DrawData + Sync + Send + 'static> From<D> for QueueableDrawData {
                     QueueableDrawDataError::UnexpectedDrawDataType(std::any::type_name::<D>()),
                 )?;
                 renderer
-                    .draw(&ctx.gpu_resources, phase, pass, draw_data)
+                    .draw(gpu_resources, phase, pass, draw_data)
                     .map_err(QueueableDrawDataError::from)
             }),
             draw_data: Box::new(draw_data),

diff --git a/crates/re_renderer/src/renderer/compositor.rs b/crates/re_renderer/src/renderer/compositor.rs
@@ -6,8 +6,8 @@ use crate::{
     view_builder::ViewBuilder,
     wgpu_resources::{
         BindGroupDesc, BindGroupEntry, BindGroupLayoutDesc, GpuBindGroup, GpuBindGroupLayoutHandle,
-        GpuRenderPipelineHandle, GpuTexture, PipelineLayoutDesc, RenderPipelineDesc,
-        WgpuResourcePools,
+        GpuRenderPipelineHandle, GpuRenderPipelinePoolAccessor, GpuTexture, PipelineLayoutDesc,
+        RenderPipelineDesc, WgpuResourcePools,
     },
     OutlineConfig, Rgba,
 };
@@ -206,7 +206,7 @@ impl Renderer for Compositor {
 
     fn draw<'a>(
         &self,
-        pools: &'a WgpuResourcePools,
+        render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>,
         phase: DrawPhase,
         pass: &mut wgpu::RenderPass<'a>,
         draw_data: &'a CompositorDrawData,
@@ -216,7 +216,7 @@ impl Renderer for Compositor {
             DrawPhase::CompositingScreenshot => self.render_pipeline_screenshot,
             _ => unreachable!("We were called on a phase we weren't subscribed to: {phase:?}"),
         };
-        let pipeline = pools.render_pipelines.get_resource(pipeline_handle)?;
+        let pipeline = render_pipelines.get(pipeline_handle)?;
 
         pass.set_pipeline(pipeline);
         pass.set_bind_group(1, &draw_data.bind_group, &[]);

diff --git a/crates/re_renderer/src/renderer/debug_overlay.rs b/crates/re_renderer/src/renderer/debug_overlay.rs
@@ -5,8 +5,8 @@ use crate::{
     include_shader_module,
     wgpu_resources::{
         BindGroupDesc, BindGroupEntry, BindGroupLayoutDesc, GpuBindGroup, GpuBindGroupLayoutHandle,
-        GpuRenderPipelineHandle, GpuTexture, PipelineLayoutDesc, RenderPipelineDesc,
-        WgpuResourcePools,
+        GpuRenderPipelineHandle, GpuRenderPipelinePoolAccessor, GpuTexture, PipelineLayoutDesc,
+        RenderPipelineDesc, WgpuResourcePools,
     },
     RectInt,
 };
@@ -239,12 +239,12 @@ impl Renderer for DebugOverlayRenderer {
 
     fn draw<'a>(
         &self,
-        pools: &'a WgpuResourcePools,
+        render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>,
         _phase: DrawPhase,
         pass: &mut wgpu::RenderPass<'a>,
         draw_data: &'a DebugOverlayDrawData,
     ) -> Result<(), DrawError> {
-        let pipeline = pools.render_pipelines.get_resource(self.render_pipeline)?;
+        let pipeline = render_pipelines.get(self.render_pipeline)?;
 
         pass.set_pipeline(pipeline);
         pass.set_bind_group(1, &draw_data.bind_group, &[]);

diff --git a/crates/re_renderer/src/renderer/depth_cloud.rs b/crates/re_renderer/src/renderer/depth_cloud.rs
@@ -22,7 +22,8 @@ use crate::{
     view_builder::ViewBuilder,
     wgpu_resources::{
         BindGroupDesc, BindGroupEntry, BindGroupLayoutDesc, GpuBindGroup, GpuBindGroupLayoutHandle,
-        GpuRenderPipelineHandle, PipelineLayoutDesc, RenderPipelineDesc,
+        GpuRenderPipelineHandle, GpuRenderPipelinePoolAccessor, PipelineLayoutDesc,
+        RenderPipelineDesc,
     },
     Colormap, OutlineMaskPreference, PickingLayerObjectId, PickingLayerProcessor,
 };
@@ -500,7 +501,7 @@ impl Renderer for DepthCloudRenderer {
 
     fn draw<'a>(
         &self,
-        pools: &'a WgpuResourcePools,
+        render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>,
         phase: DrawPhase,
         pass: &mut wgpu::RenderPass<'a>,
         draw_data: &'a Self::RendererDrawData,
@@ -516,7 +517,7 @@ impl Renderer for DepthCloudRenderer {
             DrawPhase::OutlineMask => self.render_pipeline_outline_mask,
             _ => unreachable!("We were called on a phase we weren't subscribed to: {phase:?}"),
         };
-        let pipeline = pools.render_pipelines.get_resource(pipeline_handle)?;
+        let pipeline = render_pipelines.get(pipeline_handle)?;
 
         pass.set_pipeline(pipeline);
 

diff --git a/crates/re_renderer/src/renderer/generic_skybox.rs b/crates/re_renderer/src/renderer/generic_skybox.rs
@@ -7,7 +7,8 @@ use crate::{
     renderer::screen_triangle_vertex_shader,
     view_builder::ViewBuilder,
     wgpu_resources::{
-        GpuRenderPipelineHandle, PipelineLayoutDesc, RenderPipelineDesc, WgpuResourcePools,
+        GpuRenderPipelineHandle, GpuRenderPipelinePoolAccessor, PipelineLayoutDesc,
+        RenderPipelineDesc, WgpuResourcePools,
     },
 };
 
@@ -96,14 +97,14 @@ impl Renderer for GenericSkybox {
 
     fn draw<'a>(
         &self,
-        pools: &'a WgpuResourcePools,
+        render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>,
         _phase: DrawPhase,
         pass: &mut wgpu::RenderPass<'a>,
         _draw_data: &GenericSkyboxDrawData,
     ) -> Result<(), DrawError> {
         re_tracing::profile_function!();
 
-        let pipeline = pools.render_pipelines.get_resource(self.render_pipeline)?;
+        let pipeline = render_pipelines.get(self.render_pipeline)?;
 
         pass.set_pipeline(pipeline);
         pass.draw(0..3, 0..1);

diff --git a/crates/re_renderer/src/renderer/lines.rs b/crates/re_renderer/src/renderer/lines.rs
@@ -119,7 +119,8 @@ use crate::{
     view_builder::ViewBuilder,
     wgpu_resources::{
         BindGroupDesc, BindGroupEntry, BindGroupLayoutDesc, GpuBindGroup, GpuBindGroupLayoutHandle,
-        GpuRenderPipelineHandle, PipelineLayoutDesc, PoolError, RenderPipelineDesc, TextureDesc,
+        GpuRenderPipelineHandle, GpuRenderPipelinePoolAccessor, PipelineLayoutDesc, PoolError,
+        RenderPipelineDesc, TextureDesc,
     },
     Color32, DebugLabel, DepthOffset, LineStripSeriesBuilder, OutlineMaskPreference,
     PickingLayerObjectId, PickingLayerProcessor,
@@ -961,7 +962,7 @@ impl Renderer for LineRenderer {
 
     fn draw<'a>(
         &self,
-        pools: &'a WgpuResourcePools,
+        render_pipelines: &'a GpuRenderPipelinePoolAccessor<'a>,
         phase: DrawPhase,
         pass: &mut wgpu::RenderPass<'a>,
         draw_data: &'a Self::RendererDrawData,
@@ -982,7 +983,7 @@ impl Renderer for LineRenderer {
             return Ok(()); // No lines submitted.
         };
 
-        let pipeline = pools.render_pipelines.get_resource(pipeline_handle)?;
+        let pipeline = render_pipelines.get(pipeline_handle)?;
 
         pass.set_pipeline(pipeline);
         pass.set_bind_group(1, bind_group_all_lines, &[]);