From 4fab5fe3715e1656f96829e11eadaed7cfbcb698 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Sat, 7 Oct 2023 17:02:59 +0200 Subject: [PATCH 01/16] Optimize gathering colors for point clouds: inline common case --- crates/re_viewer_context/src/annotations.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/crates/re_viewer_context/src/annotations.rs b/crates/re_viewer_context/src/annotations.rs index f42416f8011d..fafa2a733845 100644 --- a/crates/re_viewer_context/src/annotations.rs +++ b/crates/re_viewer_context/src/annotations.rs @@ -143,13 +143,19 @@ pub struct ResolvedAnnotationInfo { impl ResolvedAnnotationInfo { /// `rgba` should be unmultiplied + #[inline] pub fn color( &self, rgba: Option<&[u8; 4]>, default_color: DefaultColor<'_>, ) -> re_renderer::Color32 { - if let Some([r, g, b, a]) = rgba { - re_renderer::Color32::from_rgba_unmultiplied(*r, *g, *b, *a) + if let Some([r, g, b, a]) = rgba.copied() { + if a == 255 { + // Common-case optimization + re_renderer::Color32::from_rgb(r, g, b) + } else { + re_renderer::Color32::from_rgba_unmultiplied(r, g, b, a) + } } else if let Some(color) = self.annotation_info.as_ref().and_then(|info| { info.color .map(|c| c.into()) @@ -168,6 +174,7 @@ impl ResolvedAnnotationInfo { } } + #[inline] pub fn label(&self, label: Option<&str>) -> Option { if let Some(label) = label { Some(label.to_owned()) From 39cc5a7fdb8f4c34d254e06416bd2dd0656c72b6 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Sat, 7 Oct 2023 17:14:38 +0200 Subject: [PATCH 02/16] Remove unnecessary reference --- crates/re_space_view_spatial/src/parts/images.rs | 15 +++------------ crates/re_space_view_spatial/src/parts/mod.rs | 6 ++---- .../src/view_part_system.rs | 3 +-- crates/re_viewer_context/src/annotations.rs | 4 ++-- 4 files changed, 8 insertions(+), 20 deletions(-) diff --git a/crates/re_space_view_spatial/src/parts/images.rs b/crates/re_space_view_spatial/src/parts/images.rs index 1b0e0bb60fef..e4d3cfa521db 100644 --- a/crates/re_space_view_spatial/src/parts/images.rs +++ b/crates/re_space_view_spatial/src/parts/images.rs @@ -259,10 +259,7 @@ impl ImagesPart { .annotations .resolved_class_description(None) .annotation_info() - .color( - color.map(|c| c.to_array()).as_ref(), - DefaultColor::OpaqueWhite, - ); + .color(color.map(|c| c.to_array()), DefaultColor::OpaqueWhite); if let Some(textured_rect) = to_textured_rect( ctx, @@ -379,10 +376,7 @@ impl ImagesPart { .annotations .resolved_class_description(None) .annotation_info() - .color( - color.map(|c| c.to_array()).as_ref(), - DefaultColor::OpaqueWhite, - ); + .color(color.map(|c| c.to_array()), DefaultColor::OpaqueWhite); if let Some(textured_rect) = to_textured_rect( ctx, @@ -467,10 +461,7 @@ impl ImagesPart { .annotations .resolved_class_description(None) .annotation_info() - .color( - color.map(|c| c.to_array()).as_ref(), - DefaultColor::OpaqueWhite, - ); + .color(color.map(|c| c.to_array()), DefaultColor::OpaqueWhite); if let Some(textured_rect) = to_textured_rect( ctx, diff --git a/crates/re_space_view_spatial/src/parts/mod.rs b/crates/re_space_view_spatial/src/parts/mod.rs index 4060291583d7..2f859642ccd5 100644 --- a/crates/re_space_view_spatial/src/parts/mod.rs +++ b/crates/re_space_view_spatial/src/parts/mod.rs @@ -118,7 +118,7 @@ pub fn process_colors<'a, A: Archetype>( arch_view.iter_optional_component::()?, ) .map(move |(annotation_info, color)| { - annotation_info.color(color.map(move |c| c.to_array()).as_ref(), default_color) + annotation_info.color(color.map(|c| c.to_array()), default_color) })) } @@ -134,9 +134,7 @@ pub fn process_labels<'a, A: Archetype>( annotation_infos.iter(), arch_view.iter_optional_component::()?, ) - .map(move |(annotation_info, text)| { - annotation_info.label(text.as_ref().map(move |t| t.as_str())) - })) + .map(move |(annotation_info, text)| annotation_info.label(text.as_ref().map(|t| t.as_str())))) } /// Process [`re_types::components::Radius`] components to [`re_renderer::Size`] using auto size diff --git a/crates/re_space_view_time_series/src/view_part_system.rs b/crates/re_space_view_time_series/src/view_part_system.rs index 0237d2a714af..46d8976034c4 100644 --- a/crates/re_space_view_time_series/src/view_part_system.rs +++ b/crates/re_space_view_time_series/src/view_part_system.rs @@ -147,8 +147,7 @@ impl TimeSeriesSystem { arch_view.iter_optional_component::()?, arch_view.iter_optional_component::()?, ) { - let color = - annotation_info.color(color.map(|c| c.to_array()).as_ref(), default_color); + let color = annotation_info.color(color.map(|c| c.to_array()), default_color); let label = annotation_info.label(label.as_ref().map(|l| l.as_str())); const DEFAULT_RADIUS: f32 = 0.75; diff --git a/crates/re_viewer_context/src/annotations.rs b/crates/re_viewer_context/src/annotations.rs index fafa2a733845..f9e1aafd9336 100644 --- a/crates/re_viewer_context/src/annotations.rs +++ b/crates/re_viewer_context/src/annotations.rs @@ -146,10 +146,10 @@ impl ResolvedAnnotationInfo { #[inline] pub fn color( &self, - rgba: Option<&[u8; 4]>, + rgba: Option<[u8; 4]>, default_color: DefaultColor<'_>, ) -> re_renderer::Color32 { - if let Some([r, g, b, a]) = rgba.copied() { + if let Some([r, g, b, a]) = rgba { if a == 255 { // Common-case optimization re_renderer::Color32::from_rgb(r, g, b) From d0e47913e8b1a31b0cd5042db09cfc9e1d8dd69b Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 9 Oct 2023 13:33:58 +0200 Subject: [PATCH 03/16] Clean up the points2d/3d code paths and make them more similar to eachother --- .../src/parts/points2d.rs | 66 ++++++++--------- .../src/parts/points3d.rs | 70 ++++++++++--------- 2 files changed, 69 insertions(+), 67 deletions(-) diff --git a/crates/re_space_view_spatial/src/parts/points2d.rs b/crates/re_space_view_spatial/src/parts/points2d.rs index 9c3726999d57..9d2b6e6506af 100644 --- a/crates/re_space_view_spatial/src/parts/points2d.rs +++ b/crates/re_space_view_spatial/src/parts/points2d.rs @@ -84,9 +84,6 @@ impl Points2DPart { |p| (*p).into(), )?; - let colors = process_colors(arch_view, ent_path, &annotation_infos)?; - let radii = process_radii(arch_view, ent_path)?; - if arch_view.num_instances() <= self.max_labels { // Max labels is small enough that we can afford iterating on the colors again. let colors = @@ -108,7 +105,37 @@ impl Points2DPart { )?); } + let colors = process_colors(arch_view, ent_path, &annotation_infos)?; + let radii = process_radii(arch_view, ent_path)?; + + let positions = arch_view + .iter_required_component::()? + .map(|pt| pt.into()); + + let picking_instance_ids = arch_view + .iter_instance_keys() + .map(picking_id_from_instance_key); + + let positions: Vec = { + re_tracing::profile_scope!("collect_positions"); + positions.collect() + }; + let radii: Vec<_> = { + re_tracing::profile_scope!("collect_radii"); + radii.collect() + }; + let colors: Vec<_> = { + re_tracing::profile_scope!("collect_colors"); + colors.collect() + }; + let picking_instance_ids: Vec<_> = { + re_tracing::profile_scope!("collect_picking_instance_ids"); + picking_instance_ids.collect() + }; + { + re_tracing::profile_scope!("to_gpu"); + let mut point_builder = ent_context.shared_render_builders.points(); let point_batch = point_builder .batch("2d points") @@ -121,31 +148,6 @@ impl Points2DPart { .outline_mask_ids(ent_context.highlight.overall) .picking_object_id(re_renderer::PickingLayerObjectId(ent_path.hash64())); - let positions = arch_view - .iter_required_component::()? - .map(|pt| pt.into()); - - let picking_instance_ids = arch_view - .iter_instance_keys() - .map(picking_id_from_instance_key); - - let positions: Vec = { - re_tracing::profile_scope!("collect_positions"); - positions.collect() - }; - let radii: Vec<_> = { - re_tracing::profile_scope!("collect_radii"); - radii.collect() - }; - let colors: Vec<_> = { - re_tracing::profile_scope!("collect_colors"); - colors.collect() - }; - let picking_instance_ids: Vec<_> = { - re_tracing::profile_scope!("collect_picking_instance_ids"); - picking_instance_ids.collect() - }; - let mut point_range_builder = point_batch.add_points_2d(&positions, &radii, &colors, &picking_instance_ids); @@ -168,15 +170,13 @@ impl Points2DPart { } }; - load_keypoint_connections(ent_context, ent_path, &keypoints); - self.data.extend_bounding_box_with_points( - arch_view - .iter_required_component::()? - .map(|pt| pt.into()), + positions.iter().copied(), ent_context.world_from_entity, ); + load_keypoint_connections(ent_context, ent_path, &keypoints); + Ok(()) } } diff --git a/crates/re_space_view_spatial/src/parts/points3d.rs b/crates/re_space_view_spatial/src/parts/points3d.rs index 5f0b786ca17b..e0f38bc4b0b6 100644 --- a/crates/re_space_view_spatial/src/parts/points3d.rs +++ b/crates/re_space_view_spatial/src/parts/points3d.rs @@ -90,9 +90,6 @@ impl Points3DPart { |p| (*p).into(), )?; - let colors = process_colors(arch_view, ent_path, &annotation_infos)?; - let radii = process_radii(arch_view, ent_path)?; - if arch_view.num_instances() <= self.max_labels { re_tracing::profile_scope!("labels"); @@ -117,7 +114,38 @@ impl Points3DPart { )?); } + let colors = process_colors(arch_view, ent_path, &annotation_infos)?; + let radii = process_radii(arch_view, ent_path)?; + + let (positions, radii, colors, picking_instance_ids) = join4( + || { + re_tracing::profile_scope!("positions"); + arch_view + .iter_required_component::() + .map(|p| p.map(glam::Vec3::from).collect_vec()) + }, + || { + re_tracing::profile_scope!("radii"); + radii.collect_vec() + }, + || { + re_tracing::profile_scope!("colors"); + colors.collect_vec() + }, + || { + re_tracing::profile_scope!("picking_ids"); + arch_view + .iter_instance_keys() + .map(picking_id_from_instance_key) + .collect_vec() + }, + ); + + let positions = positions?; + { + re_tracing::profile_scope!("to_gpu"); + let mut point_builder = ent_context.shared_render_builders.points(); let point_batch = point_builder .batch("3d points") @@ -125,32 +153,6 @@ impl Points3DPart { .outline_mask_ids(ent_context.highlight.overall) .picking_object_id(re_renderer::PickingLayerObjectId(ent_path.hash64())); - let (positions, radii, colors, picking_instance_ids) = join4( - || { - re_tracing::profile_scope!("positions"); - arch_view - .iter_required_component::() - .map(|p| p.map(glam::Vec3::from).collect_vec()) - }, - || { - re_tracing::profile_scope!("radii"); - radii.collect_vec() - }, - || { - re_tracing::profile_scope!("colors"); - colors.collect_vec() - }, - || { - re_tracing::profile_scope!("picking_ids"); - arch_view - .iter_instance_keys() - .map(picking_id_from_instance_key) - .collect_vec() - }, - ); - - let positions = positions?; - let mut point_range_builder = point_batch.add_points(&positions, &radii, &colors, &picking_instance_ids); @@ -171,13 +173,13 @@ impl Points3DPart { } } } - - self.data.extend_bounding_box_with_points( - positions.iter().copied(), - ent_context.world_from_entity, - ); } + self.data.extend_bounding_box_with_points( + positions.iter().copied(), + ent_context.world_from_entity, + ); + load_keypoint_connections(ent_context, ent_path, &keypoints); Ok(()) From d0dc7fbafe5de87442d2f43f1e2967c0ffeea715 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 9 Oct 2023 14:43:52 +0200 Subject: [PATCH 04/16] Move labels last --- .../src/parts/points2d.rs | 42 ++++++++-------- .../src/parts/points3d.rs | 48 +++++++++---------- 2 files changed, 45 insertions(+), 45 deletions(-) diff --git a/crates/re_space_view_spatial/src/parts/points2d.rs b/crates/re_space_view_spatial/src/parts/points2d.rs index 9d2b6e6506af..4e96a498acac 100644 --- a/crates/re_space_view_spatial/src/parts/points2d.rs +++ b/crates/re_space_view_spatial/src/parts/points2d.rs @@ -84,27 +84,6 @@ impl Points2DPart { |p| (*p).into(), )?; - if arch_view.num_instances() <= self.max_labels { - // Max labels is small enough that we can afford iterating on the colors again. - let colors = - process_colors(arch_view, ent_path, &annotation_infos)?.collect::>(); - - let instance_path_hashes_for_picking = { - re_tracing::profile_scope!("instance_hashes"); - arch_view - .iter_instance_keys() - .map(|instance_key| InstancePathHash::instance(ent_path, instance_key)) - .collect::>() - }; - - self.data.ui_labels.extend(Self::process_labels( - arch_view, - &instance_path_hashes_for_picking, - &colors, - &annotation_infos, - )?); - } - let colors = process_colors(arch_view, ent_path, &annotation_infos)?; let radii = process_radii(arch_view, ent_path)?; @@ -177,6 +156,27 @@ impl Points2DPart { load_keypoint_connections(ent_context, ent_path, &keypoints); + if arch_view.num_instances() <= self.max_labels { + // Max labels is small enough that we can afford iterating on the colors again. + let colors = + process_colors(arch_view, ent_path, &annotation_infos)?.collect::>(); + + let instance_path_hashes_for_picking = { + re_tracing::profile_scope!("instance_hashes"); + arch_view + .iter_instance_keys() + .map(|instance_key| InstancePathHash::instance(ent_path, instance_key)) + .collect::>() + }; + + self.data.ui_labels.extend(Self::process_labels( + arch_view, + &instance_path_hashes_for_picking, + &colors, + &annotation_infos, + )?); + } + Ok(()) } } diff --git a/crates/re_space_view_spatial/src/parts/points3d.rs b/crates/re_space_view_spatial/src/parts/points3d.rs index e0f38bc4b0b6..e7509f0f4b30 100644 --- a/crates/re_space_view_spatial/src/parts/points3d.rs +++ b/crates/re_space_view_spatial/src/parts/points3d.rs @@ -90,30 +90,6 @@ impl Points3DPart { |p| (*p).into(), )?; - if arch_view.num_instances() <= self.max_labels { - re_tracing::profile_scope!("labels"); - - // Max labels is small enough that we can afford iterating on the colors again. - let colors = - process_colors(arch_view, ent_path, &annotation_infos)?.collect::>(); - - let instance_path_hashes_for_picking = { - re_tracing::profile_scope!("instance_hashes"); - arch_view - .iter_instance_keys() - .map(|instance_key| InstancePathHash::instance(ent_path, instance_key)) - .collect::>() - }; - - self.data.ui_labels.extend(Self::process_labels( - arch_view, - &instance_path_hashes_for_picking, - &colors, - &annotation_infos, - ent_context.world_from_entity, - )?); - } - let colors = process_colors(arch_view, ent_path, &annotation_infos)?; let radii = process_radii(arch_view, ent_path)?; @@ -182,6 +158,30 @@ impl Points3DPart { load_keypoint_connections(ent_context, ent_path, &keypoints); + if arch_view.num_instances() <= self.max_labels { + re_tracing::profile_scope!("labels"); + + // Max labels is small enough that we can afford iterating on the colors again. + let colors = + process_colors(arch_view, ent_path, &annotation_infos)?.collect::>(); + + let instance_path_hashes_for_picking = { + re_tracing::profile_scope!("instance_hashes"); + arch_view + .iter_instance_keys() + .map(|instance_key| InstancePathHash::instance(ent_path, instance_key)) + .collect::>() + }; + + self.data.ui_labels.extend(Self::process_labels( + arch_view, + &instance_path_hashes_for_picking, + &colors, + &annotation_infos, + ent_context.world_from_entity, + )?); + } + Ok(()) } } From fd71873d47f4d47ab66f337d08ba23645d30ee23 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 9 Oct 2023 14:49:24 +0200 Subject: [PATCH 05/16] Pass in less powerful objects as parameters --- crates/re_space_view_spatial/src/parts/mod.rs | 6 +++--- crates/re_space_view_spatial/src/parts/points2d.rs | 2 +- crates/re_space_view_spatial/src/parts/points3d.rs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/re_space_view_spatial/src/parts/mod.rs b/crates/re_space_view_spatial/src/parts/mod.rs index 2f859642ccd5..fb42dea672e9 100644 --- a/crates/re_space_view_spatial/src/parts/mod.rs +++ b/crates/re_space_view_spatial/src/parts/mod.rs @@ -174,7 +174,7 @@ fn process_annotations( where Primary: re_types::Component + Clone, { - process_annotations_and_keypoints(query, arch_view, annotations, |_: &Primary| { + process_annotations_and_keypoints(query.latest_at, arch_view, annotations, |_: &Primary| { glam::Vec3::ZERO }) .map(|(a, _)| a) @@ -182,7 +182,7 @@ where /// Resolves all annotations and keypoints for the given entity view. fn process_annotations_and_keypoints( - query: &ViewQuery<'_>, + latest_at: re_log_types::TimeInt, arch_view: &re_query::ArchetypeView, annotations: &Arc, mut primary_into_position: impl FnMut(&Primary) -> glam::Vec3, @@ -218,7 +218,7 @@ where if let (Some(keypoint_id), Some(class_id), primary) = (keypoint_id, class_id, primary) { keypoints - .entry((class_id, query.latest_at.as_i64())) + .entry((class_id, latest_at.as_i64())) .or_default() .insert(keypoint_id.0, primary_into_position(&primary)); class_description.annotation_info_with_keypoint(keypoint_id.0) diff --git a/crates/re_space_view_spatial/src/parts/points2d.rs b/crates/re_space_view_spatial/src/parts/points2d.rs index 4e96a498acac..bf8ae019b497 100644 --- a/crates/re_space_view_spatial/src/parts/points2d.rs +++ b/crates/re_space_view_spatial/src/parts/points2d.rs @@ -78,7 +78,7 @@ impl Points2DPart { let (annotation_infos, keypoints) = process_annotations_and_keypoints::( - query, + query.latest_at, arch_view, &ent_context.annotations, |p| (*p).into(), diff --git a/crates/re_space_view_spatial/src/parts/points3d.rs b/crates/re_space_view_spatial/src/parts/points3d.rs index e7509f0f4b30..8490f0cbc1f2 100644 --- a/crates/re_space_view_spatial/src/parts/points3d.rs +++ b/crates/re_space_view_spatial/src/parts/points3d.rs @@ -84,7 +84,7 @@ impl Points3DPart { let (annotation_infos, keypoints) = process_annotations_and_keypoints::( - query, + query.latest_at, arch_view, &ent_context.annotations, |p| (*p).into(), From ce410d085efabd006b0cc805a1694bd4724e7ab1 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 9 Oct 2023 14:57:02 +0200 Subject: [PATCH 06/16] Break out the CPU-specific code so we can benchmark it --- crates/re_space_view_spatial/src/parts/mod.rs | 5 +- .../src/parts/points3d.rs | 123 ++++++++++++------ 2 files changed, 86 insertions(+), 42 deletions(-) diff --git a/crates/re_space_view_spatial/src/parts/mod.rs b/crates/re_space_view_spatial/src/parts/mod.rs index fb42dea672e9..c6a47ae58906 100644 --- a/crates/re_space_view_spatial/src/parts/mod.rs +++ b/crates/re_space_view_spatial/src/parts/mod.rs @@ -23,7 +23,6 @@ pub use spatial_view_part::SpatialViewPartData; pub use transform3d_arrows::{add_axis_arrows, Transform3DArrowsPart}; use ahash::HashMap; -use std::sync::Arc; use re_data_store::{EntityPath, InstancePathHash}; use re_types::components::{Color, InstanceKey}; @@ -169,7 +168,7 @@ pub fn process_radii<'a, A: Archetype>( fn process_annotations( query: &ViewQuery<'_>, arch_view: &re_query::ArchetypeView, - annotations: &Arc, + annotations: &Annotations, ) -> Result where Primary: re_types::Component + Clone, @@ -184,7 +183,7 @@ where fn process_annotations_and_keypoints( latest_at: re_log_types::TimeInt, arch_view: &re_query::ArchetypeView, - annotations: &Arc, + annotations: &Annotations, mut primary_into_position: impl FnMut(&Primary) -> glam::Vec3, ) -> Result<(ResolvedAnnotationInfos, Keypoints), re_query::QueryError> where diff --git a/crates/re_space_view_spatial/src/parts/points3d.rs b/crates/re_space_view_spatial/src/parts/points3d.rs index 8490f0cbc1f2..292b69080936 100644 --- a/crates/re_space_view_spatial/src/parts/points3d.rs +++ b/crates/re_space_view_spatial/src/parts/points3d.rs @@ -1,13 +1,15 @@ use re_data_store::{EntityPath, InstancePathHash}; +use re_log_types::TimeInt; use re_query::{ArchetypeView, QueryError}; +use re_renderer::PickingLayerInstanceId; use re_types::{ archetypes::Points3D, components::{Position3D, Text}, Archetype as _, ComponentNameSet, }; use re_viewer_context::{ - NamedViewSystem, ResolvedAnnotationInfos, SpaceViewSystemExecutionError, ViewContextCollection, - ViewPartSystem, ViewQuery, ViewerContext, + Annotations, NamedViewSystem, ResolvedAnnotationInfos, SpaceViewSystemExecutionError, + ViewContextCollection, ViewPartSystem, ViewQuery, ViewerContext, }; use itertools::Itertools as _; @@ -21,7 +23,7 @@ use crate::{ view_kind::SpatialSpaceViewKind, }; -use super::{picking_id_from_instance_key, SpatialViewPartData}; +use super::{picking_id_from_instance_key, Keypoints, SpatialViewPartData}; pub struct Points3DPart { /// If the number of points in the batch is > max_labels, don't render point labels. @@ -82,42 +84,19 @@ impl Points3DPart { ) -> Result<(), QueryError> { re_tracing::profile_function!(); - let (annotation_infos, keypoints) = - process_annotations_and_keypoints::( - query.latest_at, - arch_view, - &ent_context.annotations, - |p| (*p).into(), - )?; - - let colors = process_colors(arch_view, ent_path, &annotation_infos)?; - let radii = process_radii(arch_view, ent_path)?; - - let (positions, radii, colors, picking_instance_ids) = join4( - || { - re_tracing::profile_scope!("positions"); - arch_view - .iter_required_component::() - .map(|p| p.map(glam::Vec3::from).collect_vec()) - }, - || { - re_tracing::profile_scope!("radii"); - radii.collect_vec() - }, - || { - re_tracing::profile_scope!("colors"); - colors.collect_vec() - }, - || { - re_tracing::profile_scope!("picking_ids"); - arch_view - .iter_instance_keys() - .map(picking_id_from_instance_key) - .collect_vec() - }, - ); - - let positions = positions?; + let LoadedPoints { + annotation_infos, + keypoints, + positions, + radii, + colors, + picking_instance_ids, + } = LoadedPoints::load( + arch_view, + ent_path, + query.latest_at, + &ent_context.annotations, + )?; { re_tracing::profile_scope!("to_gpu"); @@ -232,6 +211,72 @@ impl ViewPartSystem for Points3DPart { } } +// Public so we can write a benchmark for it! +pub struct LoadedPoints { + annotation_infos: ResolvedAnnotationInfos, + keypoints: Keypoints, + positions: Vec, + radii: Vec, + colors: Vec, + picking_instance_ids: Vec, +} + +impl LoadedPoints { + pub fn load( + arch_view: &ArchetypeView, + ent_path: &EntityPath, + latest_at: TimeInt, + annotations: &Annotations, + ) -> Result { + re_tracing::profile_function!(); + + let (annotation_infos, keypoints) = process_annotations_and_keypoints::< + Position3D, + Points3D, + >(latest_at, arch_view, annotations, |p| { + (*p).into() + })?; + + let colors = process_colors(arch_view, ent_path, &annotation_infos)?; + let radii = process_radii(arch_view, ent_path)?; + + let (positions, radii, colors, picking_instance_ids) = join4( + || { + re_tracing::profile_scope!("positions"); + arch_view + .iter_required_component::() + .map(|p| p.map(glam::Vec3::from).collect_vec()) + }, + || { + re_tracing::profile_scope!("radii"); + radii.collect_vec() + }, + || { + re_tracing::profile_scope!("colors"); + colors.collect_vec() + }, + || { + re_tracing::profile_scope!("picking_ids"); + arch_view + .iter_instance_keys() + .map(picking_id_from_instance_key) + .collect_vec() + }, + ); + + let positions = positions?; + + Ok(Self { + annotation_infos, + keypoints, + positions, + radii, + colors, + picking_instance_ids, + }) + } +} + /// Run 4 things in parallel fn join4( a: impl FnOnce() -> A + Send, From 16825ee6f687bf8be573dd01e97430f12a4902cc Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 9 Oct 2023 15:57:25 +0200 Subject: [PATCH 07/16] Make it easy to construct DataRow and DataCell from archetypes --- crates/re_log_types/src/data_cell.rs | 10 +++++++++- crates/re_log_types/src/data_row.rs | 26 +++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/crates/re_log_types/src/data_cell.rs b/crates/re_log_types/src/data_cell.rs index 9724c4f3c4d2..dc4b979463e1 100644 --- a/crates/re_log_types/src/data_cell.rs +++ b/crates/re_log_types/src/data_cell.rs @@ -1,7 +1,7 @@ use std::sync::Arc; use arrow2::datatypes::DataType; -use re_types::{Component, ComponentName, DeserializationError}; +use re_types::{Component, ComponentBatch, ComponentName, DeserializationError}; use crate::SizeBytes; @@ -164,6 +164,14 @@ pub struct DataCellInner { // TODO(#1696): Check that the array is indeed a leaf / component type when building a cell from an // arrow payload. impl DataCell { + /// Builds a new `DataCell` from a component batch. + #[inline] + pub fn from_component_batch(batch: &dyn ComponentBatch) -> re_types::SerializationResult { + batch + .to_arrow() + .map(|arrow| DataCell::from_arrow(batch.name(), arrow)) + } + /// Builds a new `DataCell` from a uniform iterable of native component values. /// /// Fails if the given iterable cannot be serialized to arrow, which should never happen when diff --git a/crates/re_log_types/src/data_row.rs b/crates/re_log_types/src/data_row.rs index 04e88c1765a4..04309d29cca0 100644 --- a/crates/re_log_types/src/data_row.rs +++ b/crates/re_log_types/src/data_row.rs @@ -1,6 +1,6 @@ use ahash::HashSetExt; use nohash_hasher::IntSet; -use re_types::ComponentName; +use re_types::{AsComponents, ComponentName}; use smallvec::SmallVec; use crate::{DataCell, DataCellError, DataTable, EntityPath, SizeBytes, TableId, TimePoint}; @@ -266,6 +266,30 @@ pub struct DataRow { } impl DataRow { + /// Builds a new `DataRow` from anything implementing [`AsComponents`]. + pub fn from_component_batches( + row_id: RowId, + timepoint: TimePoint, + entity_path: EntityPath, + as_components: &dyn AsComponents, + ) -> anyhow::Result { + re_tracing::profile_function!(); + + let data_cells = as_components + .as_component_batches() + .into_iter() + .map(|batch| DataCell::from_component_batch(batch.as_ref())) + .collect::, _>>()?; + + Ok(DataRow::from_cells( + row_id, + timepoint, + entity_path, + as_components.num_instances() as _, + data_cells, + )?) + } + /// Builds a new `DataRow` from an iterable of [`DataCell`]s. /// /// Fails if: From 144387b7687e3d8c69bf49dfeb15060e333f90b6 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 9 Oct 2023 16:17:21 +0200 Subject: [PATCH 08/16] Make it easier to benchmark individual parts --- crates/re_space_view_spatial/src/lib.rs | 3 + crates/re_space_view_spatial/src/parts/mod.rs | 3 + .../src/parts/points3d.rs | 88 +++++++++++-------- crates/re_viewer_context/src/annotations.rs | 14 +-- 4 files changed, 63 insertions(+), 45 deletions(-) diff --git a/crates/re_space_view_spatial/src/lib.rs b/crates/re_space_view_spatial/src/lib.rs index f9e480c25f64..af98c79c1e0e 100644 --- a/crates/re_space_view_spatial/src/lib.rs +++ b/crates/re_space_view_spatial/src/lib.rs @@ -20,6 +20,9 @@ mod ui_3d; pub use space_view_2d::SpatialSpaceView2D; pub use space_view_3d::SpatialSpaceView3D; +#[doc(hidden)] // Public for benchmarks +pub use parts::LoadedPoints; + // --- mod view_kind { diff --git a/crates/re_space_view_spatial/src/parts/mod.rs b/crates/re_space_view_spatial/src/parts/mod.rs index c6a47ae58906..3040c18c7ace 100644 --- a/crates/re_space_view_spatial/src/parts/mod.rs +++ b/crates/re_space_view_spatial/src/parts/mod.rs @@ -22,6 +22,9 @@ use re_types::components::Text; pub use spatial_view_part::SpatialViewPartData; pub use transform3d_arrows::{add_axis_arrows, Transform3DArrowsPart}; +#[doc(hidden)] // Public for benchmarks +pub use points3d::LoadedPoints; + use ahash::HashMap; use re_data_store::{EntityPath, InstancePathHash}; diff --git a/crates/re_space_view_spatial/src/parts/points3d.rs b/crates/re_space_view_spatial/src/parts/points3d.rs index 292b69080936..716d23c34009 100644 --- a/crates/re_space_view_spatial/src/parts/points3d.rs +++ b/crates/re_space_view_spatial/src/parts/points3d.rs @@ -5,15 +5,13 @@ use re_renderer::PickingLayerInstanceId; use re_types::{ archetypes::Points3D, components::{Position3D, Text}, - Archetype as _, ComponentNameSet, + Archetype as _, ComponentNameSet, DeserializationResult, }; use re_viewer_context::{ Annotations, NamedViewSystem, ResolvedAnnotationInfos, SpaceViewSystemExecutionError, ViewContextCollection, ViewPartSystem, ViewQuery, ViewerContext, }; -use itertools::Itertools as _; - use crate::{ contexts::{EntityDepthOffsets, SpatialSceneEntityContext}, parts::{ @@ -211,14 +209,14 @@ impl ViewPartSystem for Points3DPart { } } -// Public so we can write a benchmark for it! +#[doc(hidden)] // Public for benchmarks pub struct LoadedPoints { - annotation_infos: ResolvedAnnotationInfos, - keypoints: Keypoints, - positions: Vec, - radii: Vec, - colors: Vec, - picking_instance_ids: Vec, + pub annotation_infos: ResolvedAnnotationInfos, + pub keypoints: Keypoints, + pub positions: Vec, + pub radii: Vec, + pub colors: Vec, + pub picking_instance_ids: Vec, } impl LoadedPoints { @@ -237,44 +235,56 @@ impl LoadedPoints { (*p).into() })?; - let colors = process_colors(arch_view, ent_path, &annotation_infos)?; - let radii = process_radii(arch_view, ent_path)?; - let (positions, radii, colors, picking_instance_ids) = join4( - || { - re_tracing::profile_scope!("positions"); - arch_view - .iter_required_component::() - .map(|p| p.map(glam::Vec3::from).collect_vec()) - }, - || { - re_tracing::profile_scope!("radii"); - radii.collect_vec() - }, - || { - re_tracing::profile_scope!("colors"); - colors.collect_vec() - }, - || { - re_tracing::profile_scope!("picking_ids"); - arch_view - .iter_instance_keys() - .map(picking_id_from_instance_key) - .collect_vec() - }, + || Self::load_positions(arch_view), + || Self::load_radii(arch_view, ent_path), + || Self::load_colors(arch_view, ent_path, &annotation_infos), + || Self::load_picking_ids(arch_view), ); - let positions = positions?; - Ok(Self { annotation_infos, keypoints, - positions, - radii, - colors, + positions: positions?, + radii: radii?, + colors: colors?, picking_instance_ids, }) } + + pub fn load_positions( + arch_view: &ArchetypeView, + ) -> DeserializationResult> { + re_tracing::profile_function!(); + arch_view + .iter_required_component::() + .map(|p| p.map(glam::Vec3::from).collect()) + } + + pub fn load_radii( + arch_view: &ArchetypeView, + ent_path: &EntityPath, + ) -> Result, QueryError> { + re_tracing::profile_function!(); + process_radii(arch_view, ent_path).map(|radii| radii.collect()) + } + + pub fn load_colors( + arch_view: &ArchetypeView, + ent_path: &EntityPath, + annotation_infos: &ResolvedAnnotationInfos, + ) -> Result, QueryError> { + re_tracing::profile_function!(); + process_colors(arch_view, ent_path, annotation_infos).map(|colors| colors.collect()) + } + + pub fn load_picking_ids(arch_view: &ArchetypeView) -> Vec { + re_tracing::profile_function!(); + arch_view + .iter_instance_keys() + .map(picking_id_from_instance_key) + .collect() + } } /// Run 4 things in parallel diff --git a/crates/re_viewer_context/src/annotations.rs b/crates/re_viewer_context/src/annotations.rs index f9e1aafd9336..4a34da55d073 100644 --- a/crates/re_viewer_context/src/annotations.rs +++ b/crates/re_viewer_context/src/annotations.rs @@ -21,6 +21,13 @@ pub struct Annotations { } impl Annotations { + pub fn missing() -> Self { + Self { + row_id: MISSING_ROW_ID, + class_map: Default::default(), + } + } + pub fn try_from_view(view: &ArchetypeView) -> Option { re_tracing::profile_function!(); @@ -286,10 +293,5 @@ impl AnnotationMap { const MISSING_ROW_ID: RowId = RowId::ZERO; lazy_static! { - pub static ref MISSING_ANNOTATIONS: Arc = { - Arc::new(Annotations { - row_id: MISSING_ROW_ID, - class_map: Default::default(), - }) - }; + pub static ref MISSING_ANNOTATIONS: Arc = Arc::new(Annotations::missing()); } From 00417f1c41cf080bff7b053c7cb9874ebc7cb7bc Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 9 Oct 2023 16:19:11 +0200 Subject: [PATCH 09/16] Add a high-level benchmark of Points3D rendering --- Cargo.lock | 2 + crates/re_arrow_store/benches/arrow2.rs | 4 +- crates/re_space_view_spatial/Cargo.toml | 12 ++ .../benches/bench_points.rs | 135 ++++++++++++++++++ 4 files changed, 151 insertions(+), 2 deletions(-) create mode 100644 crates/re_space_view_spatial/benches/bench_points.rs diff --git a/Cargo.lock b/Cargo.lock index 64b31f9ab6bf..02eb6f28fcd6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4458,11 +4458,13 @@ dependencies = [ "ahash 0.8.3", "anyhow", "bytemuck", + "criterion", "eframe", "egui", "glam", "itertools 0.11.0", "macaw", + "mimalloc", "nohash-hasher", "parking_lot 0.12.1", "rayon", diff --git a/crates/re_arrow_store/benches/arrow2.rs b/crates/re_arrow_store/benches/arrow2.rs index 0bc81fa5afbe..7a0c0dc4e0b1 100644 --- a/crates/re_arrow_store/benches/arrow2.rs +++ b/crates/re_arrow_store/benches/arrow2.rs @@ -6,7 +6,7 @@ static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; use std::sync::Arc; use arrow2::array::{Array, PrimitiveArray, StructArray, UnionArray}; -use criterion::{criterion_group, Criterion}; +use criterion::Criterion; use itertools::Itertools; use re_log_types::{DataCell, SizeBytes as _}; @@ -19,7 +19,7 @@ use re_types::{ // --- -criterion_group!(benches, erased_clone, estimated_size_bytes); +criterion::criterion_group!(benches, erased_clone, estimated_size_bytes); #[cfg(not(feature = "core_benchmarks_only"))] criterion::criterion_main!(benches); diff --git a/crates/re_space_view_spatial/Cargo.toml b/crates/re_space_view_spatial/Cargo.toml index 6132a11ca419..d33d075aed41 100644 --- a/crates/re_space_view_spatial/Cargo.toml +++ b/crates/re_space_view_spatial/Cargo.toml @@ -44,3 +44,15 @@ parking_lot.workspace = true rayon.workspace = true serde = "1" smallvec = { workspace = true, features = ["serde"] } + + +[dev-dependencies] +criterion.workspace = true +mimalloc.workspace = true + +[lib] +bench = false + +[[bench]] +name = "bench_points" +harness = false diff --git a/crates/re_space_view_spatial/benches/bench_points.rs b/crates/re_space_view_spatial/benches/bench_points.rs new file mode 100644 index 000000000000..390fb3bc8a9d --- /dev/null +++ b/crates/re_space_view_spatial/benches/bench_points.rs @@ -0,0 +1,135 @@ +//! Keeping track of performance issues/regressions in `arrow2` that directly affect us. + +use re_arrow_store::{DataStore, LatestAtQuery}; +use re_log_types::{DataRow, EntityPath, RowId, TimeInt, TimePoint, Timeline}; +use re_space_view_spatial::LoadedPoints; +use re_types::{ + archetypes::Points3D, + components::{Color, InstanceKey, Position3D}, + Loggable as _, +}; +use re_viewer_context::Annotations; + +#[global_allocator] +static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; + +criterion::criterion_main!(benches); +criterion::criterion_group!(benches, bench_points); + +// --- + +#[cfg(not(debug_assertions))] +const NUM_POINTS: usize = 1_000_000; + +// `cargo test` also runs the benchmark setup code, so make sure they run quickly: +#[cfg(debug_assertions)] +const NUM_POINTS: usize = 10; + +// --- + +/// Mimics `examples/python/open_photogrammetry_format/main.py` +fn bench_points(c: &mut criterion::Criterion) { + let timeline = Timeline::log_time(); + let ent_path = EntityPath::from("points"); + + let store = { + let mut store = DataStore::new(InstanceKey::name(), Default::default()); + + let positions = vec![Position3D::new(0.1, 0.2, 0.3); NUM_POINTS]; + let colors = vec![Color::from(0xffffffff); NUM_POINTS]; + let points = Points3D::new(positions).with_colors(colors); + let mut timepoint = TimePoint::default(); + timepoint.insert(timeline, TimeInt::from_seconds(0)); + let data_row = + DataRow::from_component_batches(RowId::random(), timepoint, ent_path.clone(), &points) + .unwrap(); + store.insert_row(&data_row).unwrap(); + store + }; + + let latest_at = LatestAtQuery::latest(timeline); + let annotations = Annotations::missing(); + + { + let mut group = c.benchmark_group("Points3D"); + group.bench_function("query_archetype", |b| { + b.iter(|| { + let arch_view = + re_query::query_archetype::(&store, &latest_at, &ent_path).unwrap(); + assert_eq!(arch_view.num_instances(), NUM_POINTS); + arch_view + }); + }); + } + + let arch_view = re_query::query_archetype::(&store, &latest_at, &ent_path).unwrap(); + assert_eq!(arch_view.num_instances(), NUM_POINTS); + + { + let mut group = c.benchmark_group("Points3D"); + group.throughput(criterion::Throughput::Elements(NUM_POINTS as _)); + group.bench_function("load_all", |b| { + b.iter(|| { + let points = + LoadedPoints::load(&arch_view, &ent_path, latest_at.at, &annotations).unwrap(); + assert_eq!(points.positions.len(), NUM_POINTS); + assert_eq!(points.colors.len(), NUM_POINTS); + assert_eq!(points.radii.len(), NUM_POINTS); // NOTE: we don't log radii, but we should get a list of defaults! + points + }); + }); + } + + { + let mut group = c.benchmark_group("Points3D"); + group.throughput(criterion::Throughput::Elements(NUM_POINTS as _)); + group.bench_function("load_positions", |b| { + b.iter(|| { + let positions = LoadedPoints::load_positions(&arch_view).unwrap(); + assert_eq!(positions.len(), NUM_POINTS); + positions + }); + }); + } + + { + let points = LoadedPoints::load(&arch_view, &ent_path, latest_at.at, &annotations).unwrap(); + + let mut group = c.benchmark_group("Points3D"); + group.throughput(criterion::Throughput::Elements(NUM_POINTS as _)); + group.bench_function("load_colors", |b| { + b.iter(|| { + let colors = + LoadedPoints::load_colors(&arch_view, &ent_path, &points.annotation_infos) + .unwrap(); + assert_eq!(colors.len(), NUM_POINTS); + colors + }); + }); + } + + // NOTE: we don't log radii! + { + let mut group = c.benchmark_group("Points3D"); + group.throughput(criterion::Throughput::Elements(NUM_POINTS as _)); + group.bench_function("load_radii", |b| { + b.iter(|| { + let radii = LoadedPoints::load_radii(&arch_view, &ent_path).unwrap(); + assert_eq!(radii.len(), NUM_POINTS); + radii + }); + }); + } + + { + let mut group = c.benchmark_group("Points3D"); + group.throughput(criterion::Throughput::Elements(NUM_POINTS as _)); + group.bench_function("load_picking_ids", |b| { + b.iter(|| { + let picking_ids = LoadedPoints::load_picking_ids(&arch_view); + assert_eq!(picking_ids.len(), NUM_POINTS); + picking_ids + }); + }); + } +} From 7d6c14e7757a8b8c3c1ef6131bb0c81a25ba4dc4 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Tue, 10 Oct 2023 08:12:49 +0200 Subject: [PATCH 10/16] Compute the size of the row --- crates/re_log_types/src/data_row.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/re_log_types/src/data_row.rs b/crates/re_log_types/src/data_row.rs index 04309d29cca0..219283e5299b 100644 --- a/crates/re_log_types/src/data_row.rs +++ b/crates/re_log_types/src/data_row.rs @@ -281,13 +281,15 @@ impl DataRow { .map(|batch| DataCell::from_component_batch(batch.as_ref())) .collect::, _>>()?; - Ok(DataRow::from_cells( + let mut row = DataRow::from_cells( row_id, timepoint, entity_path, as_components.num_instances() as _, data_cells, - )?) + )?; + row.compute_all_size_bytes(); + Ok(row) } /// Builds a new `DataRow` from an iterable of [`DataCell`]s. From f727a72ef2671dfefa29be82437fe438be2b35ef Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Tue, 10 Oct 2023 08:26:13 +0200 Subject: [PATCH 11/16] Add profile-scopes around the `collect` calls --- .../src/parts/points3d.rs | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/crates/re_space_view_spatial/src/parts/points3d.rs b/crates/re_space_view_spatial/src/parts/points3d.rs index 716d23c34009..ffd4d07fa208 100644 --- a/crates/re_space_view_spatial/src/parts/points3d.rs +++ b/crates/re_space_view_spatial/src/parts/points3d.rs @@ -256,9 +256,10 @@ impl LoadedPoints { arch_view: &ArchetypeView, ) -> DeserializationResult> { re_tracing::profile_function!(); - arch_view - .iter_required_component::() - .map(|p| p.map(glam::Vec3::from).collect()) + arch_view.iter_required_component::().map(|p| { + re_tracing::profile_scope!("collect"); + p.map(glam::Vec3::from).collect() + }) } pub fn load_radii( @@ -266,7 +267,10 @@ impl LoadedPoints { ent_path: &EntityPath, ) -> Result, QueryError> { re_tracing::profile_function!(); - process_radii(arch_view, ent_path).map(|radii| radii.collect()) + process_radii(arch_view, ent_path).map(|radii| { + re_tracing::profile_scope!("collect"); + radii.collect() + }) } pub fn load_colors( @@ -275,15 +279,20 @@ impl LoadedPoints { annotation_infos: &ResolvedAnnotationInfos, ) -> Result, QueryError> { re_tracing::profile_function!(); - process_colors(arch_view, ent_path, annotation_infos).map(|colors| colors.collect()) + process_colors(arch_view, ent_path, annotation_infos).map(|colors| { + re_tracing::profile_scope!("collect"); + colors.collect() + }) } pub fn load_picking_ids(arch_view: &ArchetypeView) -> Vec { re_tracing::profile_function!(); - arch_view + let iterator = arch_view .iter_instance_keys() - .map(picking_id_from_instance_key) - .collect() + .map(picking_id_from_instance_key); + + re_tracing::profile_scope!("collect"); + iterator.collect() } } From 42a3b2e0a846719f341afc36c636ab602a29bfa9 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Tue, 10 Oct 2023 13:03:31 +0200 Subject: [PATCH 12/16] Fix docstring --- crates/re_space_view_spatial/benches/bench_points.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/re_space_view_spatial/benches/bench_points.rs b/crates/re_space_view_spatial/benches/bench_points.rs index 390fb3bc8a9d..3b8fb9c9322c 100644 --- a/crates/re_space_view_spatial/benches/bench_points.rs +++ b/crates/re_space_view_spatial/benches/bench_points.rs @@ -1,4 +1,4 @@ -//! Keeping track of performance issues/regressions in `arrow2` that directly affect us. +//! High-level benchmark of the CPU-side of our Points3D rendering. use re_arrow_store::{DataStore, LatestAtQuery}; use re_log_types::{DataRow, EntityPath, RowId, TimeInt, TimePoint, Timeline}; From 8a82d46d671ddd6fe703614df79d21c2b518d235 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Tue, 10 Oct 2023 13:04:20 +0200 Subject: [PATCH 13/16] `#[inline]` --- crates/re_space_view_spatial/src/parts/points3d.rs | 5 +++++ crates/re_viewer_context/src/annotations.rs | 1 + 2 files changed, 6 insertions(+) diff --git a/crates/re_space_view_spatial/src/parts/points3d.rs b/crates/re_space_view_spatial/src/parts/points3d.rs index ffd4d07fa208..c38b9f0260da 100644 --- a/crates/re_space_view_spatial/src/parts/points3d.rs +++ b/crates/re_space_view_spatial/src/parts/points3d.rs @@ -220,6 +220,7 @@ pub struct LoadedPoints { } impl LoadedPoints { + #[inline] pub fn load( arch_view: &ArchetypeView, ent_path: &EntityPath, @@ -252,6 +253,7 @@ impl LoadedPoints { }) } + #[inline] pub fn load_positions( arch_view: &ArchetypeView, ) -> DeserializationResult> { @@ -262,6 +264,7 @@ impl LoadedPoints { }) } + #[inline] pub fn load_radii( arch_view: &ArchetypeView, ent_path: &EntityPath, @@ -273,6 +276,7 @@ impl LoadedPoints { }) } + #[inline] pub fn load_colors( arch_view: &ArchetypeView, ent_path: &EntityPath, @@ -285,6 +289,7 @@ impl LoadedPoints { }) } + #[inline] pub fn load_picking_ids(arch_view: &ArchetypeView) -> Vec { re_tracing::profile_function!(); let iterator = arch_view diff --git a/crates/re_viewer_context/src/annotations.rs b/crates/re_viewer_context/src/annotations.rs index 4a34da55d073..ba4e28cedace 100644 --- a/crates/re_viewer_context/src/annotations.rs +++ b/crates/re_viewer_context/src/annotations.rs @@ -21,6 +21,7 @@ pub struct Annotations { } impl Annotations { + #[inline] pub fn missing() -> Self { Self { row_id: MISSING_ROW_ID, From f25d0b838e976ce9595e0442d998acc101471905 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Tue, 10 Oct 2023 13:51:17 +0200 Subject: [PATCH 14/16] backticks --- crates/re_space_view_spatial/benches/bench_points.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/re_space_view_spatial/benches/bench_points.rs b/crates/re_space_view_spatial/benches/bench_points.rs index 3b8fb9c9322c..11f4d6adedc1 100644 --- a/crates/re_space_view_spatial/benches/bench_points.rs +++ b/crates/re_space_view_spatial/benches/bench_points.rs @@ -1,4 +1,4 @@ -//! High-level benchmark of the CPU-side of our Points3D rendering. +//! High-level benchmark of the CPU-side of our `Points3D` rendering. use re_arrow_store::{DataStore, LatestAtQuery}; use re_log_types::{DataRow, EntityPath, RowId, TimeInt, TimePoint, Timeline}; From 4c7298ef1a80a97fc81a8edbe2acba0f3b4c05e1 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Tue, 10 Oct 2023 15:56:19 +0200 Subject: [PATCH 15/16] Make `from_component_batches` more complicated and add `from_archetype` --- crates/re_log_types/src/data_row.rs | 38 +++++++++++++------ .../benches/bench_points.rs | 5 +-- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/crates/re_log_types/src/data_row.rs b/crates/re_log_types/src/data_row.rs index 219283e5299b..939e55dae58b 100644 --- a/crates/re_log_types/src/data_row.rs +++ b/crates/re_log_types/src/data_row.rs @@ -267,7 +267,7 @@ pub struct DataRow { impl DataRow { /// Builds a new `DataRow` from anything implementing [`AsComponents`]. - pub fn from_component_batches( + pub fn from_archetype( row_id: RowId, timepoint: TimePoint, entity_path: EntityPath, @@ -275,19 +275,35 @@ impl DataRow { ) -> anyhow::Result { re_tracing::profile_function!(); - let data_cells = as_components - .as_component_batches() - .into_iter() - .map(|batch| DataCell::from_component_batch(batch.as_ref())) - .collect::, _>>()?; - - let mut row = DataRow::from_cells( + let batches = as_components.as_component_batches(); + Self::from_component_batches( row_id, timepoint, entity_path, - as_components.num_instances() as _, - data_cells, - )?; + batches.iter().map(|batch| batch.as_ref()), + ) + } + + /// Builds a new `DataRow` from anything implementing [`AsComponents`]. + pub fn from_component_batches<'a>( + row_id: RowId, + timepoint: TimePoint, + entity_path: EntityPath, + comp_batches: impl IntoIterator, + ) -> anyhow::Result { + re_tracing::profile_function!(); + + let data_cells = comp_batches + .into_iter() + .map(|batch| DataCell::from_component_batch(batch)) + .collect::, _>>()?; + + // TODO(emilk): should `DataRow::from_cells` calculate `num_instances` instead? + let num_instances = data_cells.iter().map(|cell| cell.num_instances()).max(); + let num_instances = num_instances.unwrap_or(0); + + let mut row = + DataRow::from_cells(row_id, timepoint, entity_path, num_instances, data_cells)?; row.compute_all_size_bytes(); Ok(row) } diff --git a/crates/re_space_view_spatial/benches/bench_points.rs b/crates/re_space_view_spatial/benches/bench_points.rs index 11f4d6adedc1..ab437877e434 100644 --- a/crates/re_space_view_spatial/benches/bench_points.rs +++ b/crates/re_space_view_spatial/benches/bench_points.rs @@ -6,7 +6,7 @@ use re_space_view_spatial::LoadedPoints; use re_types::{ archetypes::Points3D, components::{Color, InstanceKey, Position3D}, - Loggable as _, + AsComponents, Loggable as _, }; use re_viewer_context::Annotations; @@ -41,8 +41,7 @@ fn bench_points(c: &mut criterion::Criterion) { let mut timepoint = TimePoint::default(); timepoint.insert(timeline, TimeInt::from_seconds(0)); let data_row = - DataRow::from_component_batches(RowId::random(), timepoint, ent_path.clone(), &points) - .unwrap(); + DataRow::from_archetype(RowId::random(), timepoint, ent_path.clone(), &points).unwrap(); store.insert_row(&data_row).unwrap(); store }; From a55c07912e4d24103187b05e34ffa4dc4a606da2 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Tue, 10 Oct 2023 16:02:16 +0200 Subject: [PATCH 16/16] Remove unused import --- crates/re_space_view_spatial/benches/bench_points.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/re_space_view_spatial/benches/bench_points.rs b/crates/re_space_view_spatial/benches/bench_points.rs index ab437877e434..df939c3d9eaf 100644 --- a/crates/re_space_view_spatial/benches/bench_points.rs +++ b/crates/re_space_view_spatial/benches/bench_points.rs @@ -6,7 +6,7 @@ use re_space_view_spatial::LoadedPoints; use re_types::{ archetypes::Points3D, components::{Color, InstanceKey, Position3D}, - AsComponents, Loggable as _, + Loggable as _, }; use re_viewer_context::Annotations;