From 98f073d5a18e28170b0023c8bd2216ba1cbd0fcd Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Tue, 6 Jun 2023 11:19:36 +0200 Subject: [PATCH 1/2] Fix deadlock when misusing the Caches I introduced this bug in my "refactor" PR https://build.rerun.io/pr/2303 (oops!), specifically commit 16a9aedcb04f9dbf547bb0dfbe7cd6ee37e8cd8b This is a quick-fix, with a better coming in a few minutes. --- .../src/scene/parts/images.rs | 24 +++++++++---------- crates/re_space_view_spatial/src/ui.rs | 8 ++++--- crates/re_viewport/src/view_tensor/ui.rs | 8 ++----- 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/crates/re_space_view_spatial/src/scene/parts/images.rs b/crates/re_space_view_spatial/src/scene/parts/images.rs index a3dac3e2cb03..487d77ad66fa 100644 --- a/crates/re_space_view_spatial/src/scene/parts/images.rs +++ b/crates/re_space_view_spatial/src/scene/parts/images.rs @@ -374,18 +374,18 @@ impl ImagesPart { let radius_scale = *properties.backproject_radius_scale.get(); let point_radius_from_world_depth = radius_scale * pixel_width_from_depth; - let max_data_value = - if let Some((_min, max)) = ctx.cache.entry::().entry(tensor).range { - max as f32 - } else { - // This could only happen for Jpegs, and we should never get here. - // TODO(emilk): refactor the code so that we can always calculate a range for the tensor - re_log::warn_once!("Couldn't calculate range for a depth tensor!?"); - match tensor.data { - TensorData::U16(_) => u16::MAX as f32, - _ => 10.0, - } - }; + let tensor_stats = *ctx.cache.entry::().entry(tensor); + let max_data_value = if let Some((_min, max)) = tensor_stats.range { + max as f32 + } else { + // This could only happen for Jpegs, and we should never get here. + // TODO(emilk): refactor the code so that we can always calculate a range for the tensor + re_log::warn_once!("Couldn't calculate range for a depth tensor!?"); + match tensor.data { + TensorData::U16(_) => u16::MAX as f32, + _ => 10.0, + } + }; Ok(DepthCloud { world_from_obj: world_from_obj.into(), diff --git a/crates/re_space_view_spatial/src/ui.rs b/crates/re_space_view_spatial/src/ui.rs index 2a1b19dc3f7b..060a938e3469 100644 --- a/crates/re_space_view_spatial/src/ui.rs +++ b/crates/re_space_view_spatial/src/ui.rs @@ -819,17 +819,19 @@ pub fn picking( match ctx.cache .entry::() .entry(tensor) { - Ok(decoded_tensor) => + Ok(decoded_tensor) => { + let tensor_stats = *ctx.cache.entry::().entry(&decoded_tensor); show_zoomed_image_region( ctx.render_ctx, ui, &decoded_tensor, - ctx.cache.entry::().entry(&decoded_tensor), + &tensor_stats, &scene.annotation_map.find(&instance_path.entity_path), decoded_tensor.meter, &tensor_name, [coords[0] as _, coords[1] as _], - ), + ); + } Err(err) => re_log::warn_once!( "Encountered problem decoding tensor at path {tensor_name}: {err}" ), diff --git a/crates/re_viewport/src/view_tensor/ui.rs b/crates/re_viewport/src/view_tensor/ui.rs index e93078851d45..dfd5047cdcd6 100644 --- a/crates/re_viewport/src/view_tensor/ui.rs +++ b/crates/re_viewport/src/view_tensor/ui.rs @@ -67,15 +67,11 @@ impl ViewTensorState { return; }; + let tensor_stats = *ctx.cache.entry::().entry(tensor); ctx.re_ui .selection_grid(ui, "tensor_selection_ui") .show(ui, |ui| { - tensor_summary_ui_grid_contents( - ctx.re_ui, - ui, - tensor, - ctx.cache.entry::().entry(tensor), - ); + tensor_summary_ui_grid_contents(ctx.re_ui, ui, tensor, &tensor_stats); self.texture_settings.ui(ctx.re_ui, ui); self.color_mapping.ui(ctx.render_ctx, ctx.re_ui, ui); }); From 637c88f42b6e6b94384c8ab8932e801841619b6b Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Tue, 6 Jun 2023 11:36:53 +0200 Subject: [PATCH 2/2] Rewrite the cache in a way that deadlocks can't happen as easily --- crates/re_data_ui/src/image.rs | 6 ++++-- .../src/scene/parts/images.rs | 6 +++--- .../src/scene/parts/meshes.rs | 9 ++++---- crates/re_space_view_spatial/src/ui.rs | 7 +++---- crates/re_viewer_context/src/caches.rs | 21 +++++++++---------- .../src/tensor/tensor_stats_cache.rs | 5 +++-- crates/re_viewport/src/view_tensor/scene.rs | 2 +- crates/re_viewport/src/view_tensor/ui.rs | 4 ++-- 8 files changed, 30 insertions(+), 30 deletions(-) diff --git a/crates/re_data_ui/src/image.rs b/crates/re_data_ui/src/image.rs index e093b4963e9a..f0e2a3d5f6d1 100644 --- a/crates/re_data_ui/src/image.rs +++ b/crates/re_data_ui/src/image.rs @@ -26,7 +26,9 @@ impl EntityDataUi for Tensor { ) { re_tracing::profile_function!(); - let decoded = ctx.cache.entry::().entry(self.clone()); + let decoded = ctx + .cache + .entry(|c: &mut TensorDecodeCache| c.entry(self.clone())); match decoded { Ok(decoded) => { let annotations = crate::annotations(ctx, query, entity_path); @@ -58,7 +60,7 @@ fn tensor_ui( ) { // See if we can convert the tensor to a GPU texture. // Even if not, we will show info about the tensor. - let tensor_stats = *ctx.cache.entry::().entry(tensor); + let tensor_stats = ctx.cache.entry(|c: &mut TensorStatsCache| c.entry(tensor)); let debug_name = entity_path.to_string(); let texture_result = gpu_bridge::tensor_to_gpu( ctx.render_ctx, diff --git a/crates/re_space_view_spatial/src/scene/parts/images.rs b/crates/re_space_view_spatial/src/scene/parts/images.rs index 487d77ad66fa..539115fc294d 100644 --- a/crates/re_space_view_spatial/src/scene/parts/images.rs +++ b/crates/re_space_view_spatial/src/scene/parts/images.rs @@ -61,7 +61,7 @@ fn to_textured_rect( let Some([height, width, _]) = tensor.image_height_width_channels() else { return None; }; let debug_name = ent_path.to_string(); - let tensor_stats = *ctx.cache.entry::().entry(tensor); + let tensor_stats = ctx.cache.entry(|c: &mut TensorStatsCache| c.entry(tensor)); match gpu_bridge::tensor_to_gpu( ctx.render_ctx, @@ -206,7 +206,7 @@ impl ImagesPart { return Ok(()); } - let tensor = match ctx.cache.entry::().entry(tensor) { + let tensor = match ctx.cache.entry(|c: &mut TensorDecodeCache| c.entry(tensor)) { Ok(tensor) => tensor, Err(err) => { re_log::warn_once!( @@ -374,7 +374,7 @@ impl ImagesPart { let radius_scale = *properties.backproject_radius_scale.get(); let point_radius_from_world_depth = radius_scale * pixel_width_from_depth; - let tensor_stats = *ctx.cache.entry::().entry(tensor); + let tensor_stats = ctx.cache.entry(|c: &mut TensorStatsCache| c.entry(tensor)); let max_data_value = if let Some((_min, max)) = tensor_stats.range { max as f32 } else { diff --git a/crates/re_space_view_spatial/src/scene/parts/meshes.rs b/crates/re_space_view_spatial/src/scene/parts/meshes.rs index ca39860307e3..251aa1e9fc7b 100644 --- a/crates/re_space_view_spatial/src/scene/parts/meshes.rs +++ b/crates/re_space_view_spatial/src/scene/parts/meshes.rs @@ -42,11 +42,10 @@ impl MeshPart { let outline_mask_ids = ent_context.highlight.index_outline_mask(instance_key); - if let Some(mesh) = - ctx.cache - .entry::() - .entry(&ent_path.to_string(), &mesh, ctx.render_ctx) - { + let mesh = ctx + .cache + .entry(|c: &mut MeshCache| c.entry(&ent_path.to_string(), &mesh, ctx.render_ctx)); + if let Some(mesh) = mesh { instances.extend(mesh.mesh_instances.iter().map(move |mesh_instance| { MeshInstance { gpu_mesh: mesh_instance.gpu_mesh.clone(), diff --git a/crates/re_space_view_spatial/src/ui.rs b/crates/re_space_view_spatial/src/ui.rs index 060a938e3469..21069f73dc91 100644 --- a/crates/re_space_view_spatial/src/ui.rs +++ b/crates/re_space_view_spatial/src/ui.rs @@ -816,11 +816,10 @@ pub fn picking( let tensor_name = instance_path.to_string(); - match ctx.cache - .entry::() - .entry(tensor) { + let decoded_tensor = ctx.cache.entry(|c: &mut TensorDecodeCache| c.entry(tensor)); + match decoded_tensor { Ok(decoded_tensor) => { - let tensor_stats = *ctx.cache.entry::().entry(&decoded_tensor); + let tensor_stats = ctx.cache.entry(|c: &mut TensorStatsCache| c.entry(&decoded_tensor)); show_zoomed_image_region( ctx.render_ctx, ui, diff --git a/crates/re_viewer_context/src/caches.rs b/crates/re_viewer_context/src/caches.rs index 4979730b3862..28cdfb210263 100644 --- a/crates/re_viewer_context/src/caches.rs +++ b/crates/re_viewer_context/src/caches.rs @@ -24,19 +24,18 @@ impl Caches { } } - /// Retrieves a cache for reading and writing. + /// Accesses a cache for reading and writing. /// /// Adds the cache lazily if it wasn't already there. - pub fn entry(&self) -> parking_lot::MappedMutexGuard<'_, C> { - parking_lot::MutexGuard::map(self.0.lock(), |map| { - map.entry(TypeId::of::()) - .or_insert(Box::::default()) - .as_any_mut() - .downcast_mut::() - .expect( - "Downcast failed, this indicates a bug in how `Caches` adds new cache types.", - ) - }) + pub fn entry(&self, f: impl FnOnce(&mut C) -> R) -> R { + f(self + .0 + .lock() + .entry(TypeId::of::()) + .or_insert(Box::::default()) + .as_any_mut() + .downcast_mut::() + .expect("Downcast failed, this indicates a bug in how `Caches` adds new cache types.")) } } diff --git a/crates/re_viewer_context/src/tensor/tensor_stats_cache.rs b/crates/re_viewer_context/src/tensor/tensor_stats_cache.rs index a4118a7d53a2..885db479ffca 100644 --- a/crates/re_viewer_context/src/tensor/tensor_stats_cache.rs +++ b/crates/re_viewer_context/src/tensor/tensor_stats_cache.rs @@ -7,8 +7,9 @@ use crate::Cache; pub struct TensorStatsCache(nohash_hasher::IntMap); impl TensorStatsCache { - pub fn entry(&mut self, tensor: &Tensor) -> &TensorStats { - self.0 + pub fn entry(&mut self, tensor: &Tensor) -> TensorStats { + *self + .0 .entry(tensor.tensor_id) .or_insert_with(|| TensorStats::new(tensor)) } diff --git a/crates/re_viewport/src/view_tensor/scene.rs b/crates/re_viewport/src/view_tensor/scene.rs index 0a1f7b61ce97..44f0aefe3ad9 100644 --- a/crates/re_viewport/src/view_tensor/scene.rs +++ b/crates/re_viewport/src/view_tensor/scene.rs @@ -33,7 +33,7 @@ impl SceneTensor { tensor: Tensor, ) { if !tensor.is_shaped_like_an_image() { - match ctx.cache.entry::().entry(tensor) { + match ctx.cache.entry(|c: &mut TensorDecodeCache| c.entry(tensor)) { Ok(tensor) => { let instance_path = InstancePath::instance(ent_path.clone(), InstanceKey(0)); self.tensors.insert(instance_path, tensor); diff --git a/crates/re_viewport/src/view_tensor/ui.rs b/crates/re_viewport/src/view_tensor/ui.rs index dfd5047cdcd6..070d15d1f845 100644 --- a/crates/re_viewport/src/view_tensor/ui.rs +++ b/crates/re_viewport/src/view_tensor/ui.rs @@ -67,7 +67,7 @@ impl ViewTensorState { return; }; - let tensor_stats = *ctx.cache.entry::().entry(tensor); + let tensor_stats = ctx.cache.entry(|c: &mut TensorStatsCache| c.entry(tensor)); ctx.re_ui .selection_grid(ui, "tensor_selection_ui") .show(ui, |ui| { @@ -174,7 +174,7 @@ fn paint_tensor_slice( ) -> anyhow::Result<(egui::Response, egui::Painter, egui::Rect)> { re_tracing::profile_function!(); - let tensor_stats = *ctx.cache.entry::().entry(tensor); + let tensor_stats = ctx.cache.entry(|c: &mut TensorStatsCache| c.entry(tensor)); let colormapped_texture = super::tensor_slice_to_gpu::colormapped_texture( ctx.render_ctx, tensor,