Skip to content

Commit

Permalink
Use less mut when using RenderContext (#2312)
Browse files Browse the repository at this point in the history
### What
The goal is to have `&RenderContext` instead of `&mut RenderContext` in
`ViewerContext`. This is a big missing piece for parallelizing scene
building. This PR makes some progress towards that.

`GpuRenderPipelinePool` is a bit of a tougher nut to crack though…

### 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)

<!-- This line will get updated when the PR build summary job finishes.
-->
PR Build Summary: https://build.rerun.io/pr/2312

<!-- pr-link-docs:start -->
Docs preview: https://rerun.io/preview/11595bb/docs
<!-- pr-link-docs:end -->
  • Loading branch information
emilk authored Jun 5, 2023
1 parent 6d96d37 commit 2777270
Show file tree
Hide file tree
Showing 10 changed files with 82 additions and 80 deletions.
2 changes: 1 addition & 1 deletion crates/re_renderer/examples/2d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ impl framework::Example for Render2D {
let rerun_logo_texture = re_ctx
.texture_manager_2d
.create(
&mut re_ctx.gpu_resources.textures,
&re_ctx.gpu_resources.textures,
&Texture2DCreationDesc {
label: "rerun logo".into(),
data: image_data.into(),
Expand Down
4 changes: 2 additions & 2 deletions crates/re_renderer/examples/depth_cloud.rs
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ impl DepthTexture {
.texture_manager_2d
.get_or_create(
hash(&label),
&mut re_ctx.gpu_resources.textures,
&re_ctx.gpu_resources.textures,
Texture2DCreationDesc {
label: label.into(),
data: bytemuck::cast_slice(&data).into(),
Expand Down Expand Up @@ -458,7 +458,7 @@ impl AlbedoTexture {
.texture_manager_2d
.get_or_create(
hash(&label),
&mut re_ctx.gpu_resources.textures,
&re_ctx.gpu_resources.textures,
Texture2DCreationDesc {
label: label.into(),
data: bytemuck::cast_slice(&rgba8).into(),
Expand Down
2 changes: 1 addition & 1 deletion crates/re_renderer/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ impl RenderContext {
&mut resolver,
)));
let texture_manager_2d =
TextureManager2D::new(device.clone(), queue.clone(), &mut gpu_resources.textures);
TextureManager2D::new(device.clone(), queue.clone(), &gpu_resources.textures);

let active_frame = ActiveFrameContext {
before_view_builder_encoder: Mutex::new(FrameGlobalCommandEncoder::new(&device)),
Expand Down
15 changes: 5 additions & 10 deletions crates/re_renderer/src/importer/gltf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub fn load_gltf_from_buffer(
mesh_name: &str,
buffer: &[u8],
lifetime: ResourceLifeTime,
ctx: &mut RenderContext,
ctx: &RenderContext,
) -> anyhow::Result<Vec<MeshInstance>> {
re_tracing::profile_function!();

Expand Down Expand Up @@ -77,7 +77,7 @@ pub fn load_gltf_from_buffer(
images_as_textures.push(
match ctx
.texture_manager_2d
.create(&mut ctx.gpu_resources.textures, &texture)
.create(&ctx.gpu_resources.textures, &texture)
{
Ok(texture) => texture,
Err(err) => {
Expand All @@ -92,13 +92,8 @@ pub fn load_gltf_from_buffer(
for ref mesh in doc.meshes() {
re_tracing::profile_scope!("mesh");

let re_mesh = import_mesh(
mesh,
&buffers,
&images_as_textures,
&mut ctx.texture_manager_2d,
)
.with_context(|| format!("mesh {} (name {:?})", mesh.index(), mesh.name()))?;
let re_mesh = import_mesh(mesh, &buffers, &images_as_textures, &ctx.texture_manager_2d)
.with_context(|| format!("mesh {} (name {:?})", mesh.index(), mesh.name()))?;
meshes.insert(
mesh.index(),
(
Expand Down Expand Up @@ -143,7 +138,7 @@ fn import_mesh(
mesh: &gltf::Mesh<'_>,
buffers: &[gltf::buffer::Data],
gpu_image_handles: &[GpuTexture2D],
texture_manager: &mut TextureManager2D, //imported_materials: HashMap<usize, Material>,
texture_manager: &TextureManager2D, //imported_materials: HashMap<usize, Material>,
) -> anyhow::Result<Mesh> {
re_tracing::profile_function!();

Expand Down
55 changes: 35 additions & 20 deletions crates/re_renderer/src/resource_managers/texture_manager.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::sync::Arc;

use ahash::{HashMap, HashSet};
use parking_lot::Mutex;

use crate::{
wgpu_resources::{GpuTexture, GpuTexturePool, TextureDesc},
Expand Down Expand Up @@ -128,6 +129,8 @@ impl From<TextureManager2DError<never::Never>> for TextureCreationError {
/// More complex textures types are typically handled within renderer which utilize the texture pool directly.
/// This manager in contrast, deals with user provided texture data!
/// We might revisit this later and make this texture manager more general purpose.
///
/// Has intertior mutability.
pub struct TextureManager2D {
// Long lived/short lived doesn't make sense for textures since we don't yet know a way to
// optimize for short lived textures as we do with buffer data.
Expand All @@ -142,6 +145,12 @@ pub struct TextureManager2D {
device: Arc<wgpu::Device>,
queue: Arc<wgpu::Queue>,

/// The mutable part of the manager.
inner: Mutex<Inner>,
}

#[derive(Default)]
struct Inner {
// Cache the texture using a user-provided u64 id. This is expected
// to be derived from the `TensorId` which isn't available here for
// dependency reasons.
Expand All @@ -153,11 +162,20 @@ pub struct TextureManager2D {
accessed_textures: HashSet<u64>,
}

impl Inner {
fn begin_frame(&mut self, _frame_index: u64) {
// Drop any textures that weren't accessed in the last frame
self.texture_cache
.retain(|k, _| self.accessed_textures.contains(k));
self.accessed_textures.clear();
}
}

impl TextureManager2D {
pub(crate) fn new(
device: Arc<wgpu::Device>,
queue: Arc<wgpu::Queue>,
texture_pool: &mut GpuTexturePool,
texture_pool: &GpuTexturePool,
) -> Self {
re_tracing::profile_function!();

Expand Down Expand Up @@ -192,16 +210,15 @@ impl TextureManager2D {
zeroed_texture_uint,
device,
queue,
texture_cache: Default::default(),
accessed_textures: Default::default(),
inner: Default::default(),
}
}

/// Creates a new 2D texture resource and schedules data upload to the GPU.
/// TODO(jleibs): All usages of this should be be replaced with `get_or_create`, which is strictly preferable
pub fn create(
&mut self,
texture_pool: &mut GpuTexturePool,
&self,
texture_pool: &GpuTexturePool,
creation_desc: &Texture2DCreationDesc<'_>,
) -> Result<GpuTexture2D, TextureCreationError> {
// TODO(andreas): Disabled the warning as we're moving towards using this texture manager for user-logged images.
Expand All @@ -226,9 +243,9 @@ impl TextureManager2D {
/// Creates a new 2D texture resource and schedules data upload to the GPU if a texture
/// wasn't already created using the same key.
pub fn get_or_create(
&mut self,
&self,
key: u64,
texture_pool: &mut GpuTexturePool,
texture_pool: &GpuTexturePool,
texture_desc: Texture2DCreationDesc<'_>,
) -> Result<GpuTexture2D, TextureCreationError> {
self.get_or_create_with(key, texture_pool, || texture_desc)
Expand All @@ -237,9 +254,9 @@ impl TextureManager2D {
/// Creates a new 2D texture resource and schedules data upload to the GPU if a texture
/// wasn't already created using the same key.
pub fn get_or_create_with<'a>(
&mut self,
&self,
key: u64,
texture_pool: &mut GpuTexturePool,
texture_pool: &GpuTexturePool,
create_texture_desc: impl FnOnce() -> Texture2DCreationDesc<'a>,
) -> Result<GpuTexture2D, TextureCreationError> {
self.get_or_try_create_with(key, texture_pool, || -> Result<_, never::Never> {
Expand All @@ -251,12 +268,13 @@ impl TextureManager2D {
/// Creates a new 2D texture resource and schedules data upload to the GPU if a texture
/// wasn't already created using the same key.
pub fn get_or_try_create_with<'a, Err: std::fmt::Display>(
&mut self,
&self,
key: u64,
texture_pool: &mut GpuTexturePool,
texture_pool: &GpuTexturePool,
try_create_texture_desc: impl FnOnce() -> Result<Texture2DCreationDesc<'a>, Err>,
) -> Result<GpuTexture2D, TextureManager2DError<Err>> {
let texture_handle = match self.texture_cache.entry(key) {
let mut inner = self.inner.lock();
let texture_handle = match inner.texture_cache.entry(key) {
std::collections::hash_map::Entry::Occupied(texture_handle) => {
texture_handle.get().clone() // already inserted
}
Expand All @@ -274,7 +292,7 @@ impl TextureManager2D {
}
};

self.accessed_textures.insert(key);
inner.accessed_textures.insert(key);
Ok(texture_handle)
}

Expand Down Expand Up @@ -311,7 +329,7 @@ impl TextureManager2D {
fn create_and_upload_texture(
device: &wgpu::Device,
queue: &wgpu::Queue,
texture_pool: &mut GpuTexturePool,
texture_pool: &GpuTexturePool,
creation_desc: &Texture2DCreationDesc<'_>,
) -> Result<GpuTexture2D, TextureCreationError> {
re_tracing::profile_function!();
Expand Down Expand Up @@ -376,16 +394,13 @@ impl TextureManager2D {
Ok(GpuTexture2D(texture))
}

pub(crate) fn begin_frame(&mut self, _frame_index: u64) {
// Drop any textures that weren't accessed in the last frame
self.texture_cache
.retain(|k, _| self.accessed_textures.contains(k));
self.accessed_textures.clear();
pub(crate) fn begin_frame(&self, _frame_index: u64) {
self.inner.lock().begin_frame(_frame_index);
}
}

fn create_zero_texture(
texture_pool: &mut GpuTexturePool,
texture_pool: &GpuTexturePool,
device: &Arc<wgpu::Device>,
format: wgpu::TextureFormat,
) -> GpuTexture2D {
Expand Down
2 changes: 1 addition & 1 deletion crates/re_space_view_spatial/src/mesh_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ impl MeshCache {
&mut self,
name: &str,
mesh: &Mesh3D,
render_ctx: &mut RenderContext,
render_ctx: &RenderContext,
) -> Option<Arc<LoadedMesh>> {
re_tracing::profile_function!();

Expand Down
12 changes: 4 additions & 8 deletions crates/re_space_view_spatial/src/mesh_loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,7 @@ pub struct LoadedMesh {
}

impl LoadedMesh {
pub fn load(
name: String,
mesh: &Mesh3D,
render_ctx: &mut RenderContext,
) -> anyhow::Result<Self> {
pub fn load(name: String, mesh: &Mesh3D, render_ctx: &RenderContext) -> anyhow::Result<Self> {
// TODO(emilk): load CpuMesh in background thread.
match mesh {
// Mesh from some file format. File passed in bytes.
Expand All @@ -32,7 +28,7 @@ impl LoadedMesh {
name: String,
format: MeshFormat,
bytes: &[u8],
render_ctx: &mut RenderContext,
render_ctx: &RenderContext,
) -> anyhow::Result<Self> {
re_tracing::profile_function!();

Expand Down Expand Up @@ -60,7 +56,7 @@ impl LoadedMesh {
fn load_encoded_mesh(
name: String,
encoded_mesh: &EncodedMesh3D,
render_ctx: &mut RenderContext,
render_ctx: &RenderContext,
) -> anyhow::Result<Self> {
re_tracing::profile_function!();
let EncodedMesh3D {
Expand Down Expand Up @@ -88,7 +84,7 @@ impl LoadedMesh {
fn load_raw_mesh(
name: String,
raw_mesh: &RawMesh3D,
render_ctx: &mut RenderContext,
render_ctx: &RenderContext,
) -> anyhow::Result<Self> {
re_tracing::profile_function!();

Expand Down
54 changes: 25 additions & 29 deletions crates/re_space_view_spatial/src/scene/parts/images.rs
Original file line number Diff line number Diff line change
Expand Up @@ -323,35 +323,31 @@ impl ImagesPart {
let mut data_f32 = Vec::new();
ctx.render_ctx
.texture_manager_2d
.get_or_try_create_with(
texture_key,
&mut ctx.render_ctx.gpu_resources.textures,
|| {
// TODO(andreas/cmc): Ideally we'd upload the u16 data as-is.
// However, R16Unorm is behind a feature flag and Depth16Unorm doesn't work on WebGL (and is awkward as this is a depth buffer format!).
let data = match &tensor.data {
TensorData::U16(data) => {
data_f32.extend(data.as_slice().iter().map(|d| *d as f32));
bytemuck::cast_slice(&data_f32).into()
}
TensorData::F32(data) => bytemuck::cast_slice(data).into(),
_ => {
return Err(format!(
"Tensor datatype {} is not supported for back-projection",
tensor.dtype()
));
}
};

Ok(Texture2DCreationDesc {
label: format!("Depth cloud for {ent_path:?}").into(),
data,
format: wgpu::TextureFormat::R32Float,
width: width as _,
height: height as _,
})
},
)
.get_or_try_create_with(texture_key, &ctx.render_ctx.gpu_resources.textures, || {
// TODO(andreas/cmc): Ideally we'd upload the u16 data as-is.
// However, R16Unorm is behind a feature flag and Depth16Unorm doesn't work on WebGL (and is awkward as this is a depth buffer format!).
let data = match &tensor.data {
TensorData::U16(data) => {
data_f32.extend(data.as_slice().iter().map(|d| *d as f32));
bytemuck::cast_slice(&data_f32).into()
}
TensorData::F32(data) => bytemuck::cast_slice(data).into(),
_ => {
return Err(format!(
"Tensor datatype {} is not supported for back-projection",
tensor.dtype()
));
}
};

Ok(Texture2DCreationDesc {
label: format!("Depth cloud for {ent_path:?}").into(),
data,
format: wgpu::TextureFormat::R32Float,
width: width as _,
height: height as _,
})
})
.map_err(|err| format!("Failed to create depth cloud texture: {err}"))?
};

Expand Down
8 changes: 4 additions & 4 deletions crates/re_viewer_context/src/gpu_bridge/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,25 +57,25 @@ pub fn viewport_resolution_in_pixels(clip_rect: egui::Rect, pixels_from_point: f
}

pub fn try_get_or_create_texture<'a, Err: std::fmt::Display>(
render_ctx: &mut RenderContext,
render_ctx: &RenderContext,
texture_key: u64,
try_create_texture_desc: impl FnOnce() -> Result<Texture2DCreationDesc<'a>, Err>,
) -> Result<GpuTexture2D, TextureManager2DError<Err>> {
render_ctx.texture_manager_2d.get_or_try_create_with(
texture_key,
&mut render_ctx.gpu_resources.textures,
&render_ctx.gpu_resources.textures,
try_create_texture_desc,
)
}

pub fn get_or_create_texture<'a>(
render_ctx: &mut RenderContext,
render_ctx: &RenderContext,
texture_key: u64,
create_texture_desc: impl FnOnce() -> Texture2DCreationDesc<'a>,
) -> Result<GpuTexture2D, TextureCreationError> {
render_ctx.texture_manager_2d.get_or_create_with(
texture_key,
&mut render_ctx.gpu_resources.textures,
&render_ctx.gpu_resources.textures,
create_texture_desc,
)
}
Expand Down
Loading

1 comment on commit 2777270

@github-actions
Copy link

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Rust Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.25.

Benchmark suite Current: 2777270 Previous: 6d96d37 Ratio
datastore/num_rows=1000/num_instances=1000/packed=false/insert/default 6050705 ns/iter (± 428926) 3404731 ns/iter (± 72164) 1.78

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.