Primary cache: don't denormalize defaulted component values (#4891)

The cache will now keep track of missing optional components, and store an empty slice instead of a bunch of `None` values. When queried, an empty shows up as a `None` option to the end-user, who can act appropriately. SFM before: ![image](https://github.com/rerun-io/rerun/assets/2910679/34256f8b-3b4b-4d1a-b1ea-5f9e1fd7860b) SFM after: ![image](https://github.com/rerun-io/rerun/assets/2910679/b676052a-b1fc-4840-bddf-67e5f490add2) --- - Fixes #4779 - DNR: requires #4856 ### What ### 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): * Using newly built examples: [app.rerun.io](https://app.rerun.io/pr/4891/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/4891/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [app.rerun.io](https://app.rerun.io/pr/4891/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json) * [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/4891) - [Docs preview](https://rerun.io/preview/bf89c307dac5dc8fd8016dd985f8af30a6ee73a7/docs)  - [Examples preview](https://rerun.io/preview/bf89c307dac5dc8fd8016dd985f8af30a6ee73a7/examples)  - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
rerun-io · Jan 25, 2024 · ce3ab5f · ce3ab5f
1 parent 7fcdb1d
commit ce3ab5f
Show file tree

Hide file tree

Showing 15 changed files with 173 additions and 100 deletions.
diff --git a/crates/re_data_store/benches/data_store.rs b/crates/re_data_store/benches/data_store.rs
@@ -1,7 +1,7 @@
 #[global_allocator]
 static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc;
 
-use arrow2::array::{Array as _, StructArray, UnionArray};
+use arrow2::array::{Array as _, StructArray};
 use criterion::{criterion_group, criterion_main, Criterion};
 
 use re_data_store::{
@@ -312,7 +312,7 @@ fn range(c: &mut Criterion) {
                             .unwrap()
                             .as_arrow_ref()
                             .as_any()
-                            .downcast_ref::<UnionArray>()
+                            .downcast_ref::<StructArray>()
                             .unwrap();
                         assert_eq!(NUM_INSTANCES as usize, large_structs.len());
                     }

diff --git a/crates/re_query_cache/benches/latest_at.rs b/crates/re_query_cache/benches/latest_at.rs
@@ -292,12 +292,14 @@ fn query_and_visit_points(
                 &query.clone().into(),
                 path,
                 |(_, _, positions, colors)| {
-                    itertools::izip!(positions.iter(), colors.iter()).for_each(|(pos, color)| {
-                        points.push(SavePoint {
-                            _pos: *pos,
-                            _color: *color,
-                        });
-                    });
+                    itertools::izip!(positions.iter(), colors.unwrap().iter()).for_each(
+                        |(pos, color)| {
+                            points.push(SavePoint {
+                                _pos: *pos,
+                                _color: *color,
+                            });
+                        },
+                    );
                 },
             )
             .unwrap();
@@ -328,7 +330,7 @@ fn query_and_visit_strings(
                 &query.clone().into(),
                 path,
                 |(_, _, _, labels)| {
-                    for label in labels.iter() {
+                    for label in labels.unwrap().iter() {
                         strings.push(SaveString {
                             _label: label.clone(),
                         });

diff --git a/crates/re_query_cache/src/cache.rs b/crates/re_query_cache/src/cache.rs
@@ -770,6 +770,7 @@ impl CacheBucket {
         Ok(added_size_bytes)
     }
 
+    /// This will insert an empty slice for a missing component (instead of N `None` values).
     #[inline]
     fn insert_component_opt<A: Archetype, C: Component + Send + Sync + 'static>(
         &mut self,
@@ -783,12 +784,20 @@ impl CacheBucket {
             .entry(C::name())
             .or_insert_with(|| Box::new(FlatVecDeque::<Option<C>>::new()));
 
-        // The `FlatVecDeque` will have to collect the data one way or another: do it ourselves
-        // instead, that way we can efficiently computes its size while we're at it.
-        let added: FlatVecDeque<Option<C>> = arch_view
-            .iter_optional_component::<C>()?
-            .collect::<VecDeque<Option<C>>>()
-            .into();
+        let added: FlatVecDeque<Option<C>> = if arch_view.has_component::<C>() {
+            // The `FlatVecDeque` will have to collect the data one way or another: do it ourselves
+            // instead, that way we can efficiently computes its size while we're at it.
+            arch_view
+                .iter_optional_component::<C>()?
+                .collect::<VecDeque<Option<C>>>()
+                .into()
+        } else {
+            // If an optional component is missing entirely, we just store an empty slice in its
+            // stead, rather than a bunch of `None` values.
+            let mut added = FlatVecDeque::<Option<C>>::new();
+            added.push_back(std::iter::empty());
+            added
+        };
         let added_size_bytes = added.total_size_bytes();
 
         // NOTE: downcast cannot fail, we create it just above.

diff --git a/crates/re_query_cache/src/latest_at.rs b/crates/re_query_cache/src/latest_at.rs
@@ -125,7 +125,7 @@ macro_rules! impl_query_archetype_latest_at {
                     (Option<TimeInt>, RowId),
                     MaybeCachedComponentData<'_, InstanceKey>,
                     $(MaybeCachedComponentData<'_, $pov>,)+
-                    $(MaybeCachedComponentData<'_, Option<$comp>>,)*
+                    $(Option<MaybeCachedComponentData<'_, Option<$comp>>>,)*
                 ),
             ),
         {
@@ -144,7 +144,7 @@ macro_rules! impl_query_archetype_latest_at {
                         ((!timeless).then_some(*time), *row_id),
                         MaybeCachedComponentData::Cached(instance_keys),
                         $(MaybeCachedComponentData::Cached($pov),)+
-                        $(MaybeCachedComponentData::Cached($comp),)*
+                        $((!$comp.is_empty()).then_some(MaybeCachedComponentData::Cached($comp)),)*
                     )
                 });
 

diff --git a/crates/re_query_cache/src/query.rs b/crates/re_query_cache/src/query.rs
@@ -22,6 +22,20 @@ pub enum MaybeCachedComponentData<'a, C> {
     Raw(Vec<C>),
 }
 
+impl<'a, C> MaybeCachedComponentData<'a, Option<C>> {
+    /// Iterates over the data of an optional component, or repeat `None` values if it's missing.
+    #[inline]
+    pub fn iter_or_repeat_opt(
+        this: &Option<Self>,
+        len: usize,
+    ) -> impl Iterator<Item = &Option<C>> + '_ {
+        this.as_ref().map_or(
+            itertools::Either::Left(std::iter::repeat(&None).take(len)),
+            |data| itertools::Either::Right(data.iter()),
+        )
+    }
+}
+
 impl<'a, C> std::ops::Deref for MaybeCachedComponentData<'a, C> {
     type Target = [C];
 
@@ -99,7 +113,7 @@ macro_rules! impl_query_archetype {
                     (Option<TimeInt>, RowId),
                     MaybeCachedComponentData<'_, InstanceKey>,
                     $(MaybeCachedComponentData<'_, $pov>,)+
-                    $(MaybeCachedComponentData<'_, Option<$comp>>,)*
+                    $(Option<MaybeCachedComponentData<'_, Option<$comp>>>,)*
                 ),
             ),
         {
@@ -119,7 +133,7 @@ macro_rules! impl_query_archetype {
                         (arch_view.data_time(), arch_view.primary_row_id()),
                         MaybeCachedComponentData::Raw(arch_view.iter_instance_keys().collect()),
                         $(MaybeCachedComponentData::Raw(arch_view.iter_required_component::<$pov>()?.collect()),)+
-                        $(MaybeCachedComponentData::Raw(arch_view.iter_optional_component::<$comp>()?.collect()),)*
+                        $(Some(MaybeCachedComponentData::Raw(arch_view.iter_optional_component::<$comp>()?.collect())),)*
                     );
 
                     f(data);
@@ -150,7 +164,7 @@ macro_rules! impl_query_archetype {
                             (arch_view.data_time(), arch_view.primary_row_id()),
                             MaybeCachedComponentData::Raw(arch_view.iter_instance_keys().collect()),
                             $(MaybeCachedComponentData::Raw(arch_view.iter_required_component::<$pov>()?.collect()),)+
-                            $(MaybeCachedComponentData::Raw(arch_view.iter_optional_component::<$comp>()?.collect()),)*
+                            $(Some(MaybeCachedComponentData::Raw(arch_view.iter_optional_component::<$comp>()?.collect())),)*
                         );
 
                         f(data);
@@ -259,7 +273,7 @@ macro_rules! impl_query_archetype_with_history {
                     (Option<TimeInt>, RowId),
                     MaybeCachedComponentData<'_, InstanceKey>,
                     $(MaybeCachedComponentData<'_, $pov>,)+
-                    $(MaybeCachedComponentData<'_, Option<$comp>>,)*
+                    $(Option<MaybeCachedComponentData<'_, Option<$comp>>>,)*
                 ),
             ),
         {

diff --git a/crates/re_query_cache/src/range.rs b/crates/re_query_cache/src/range.rs
@@ -136,7 +136,7 @@ macro_rules! impl_query_archetype_range {
                     (Option<TimeInt>, RowId),
                     MaybeCachedComponentData<'_, InstanceKey>,
                     $(MaybeCachedComponentData<'_, $pov>,)+
-                    $(MaybeCachedComponentData<'_, Option<$comp>>,)*
+                    $(Option<MaybeCachedComponentData<'_, Option<$comp>>>,)*
                 ),
             ),
         {
@@ -158,7 +158,7 @@ macro_rules! impl_query_archetype_range {
                         ((!timeless).then_some(*time), *row_id),
                         MaybeCachedComponentData::Cached(instance_keys),
                         $(MaybeCachedComponentData::Cached($pov),)+
-                        $(MaybeCachedComponentData::Cached($comp),)*
+                        $((!$comp.is_empty()).then_some(MaybeCachedComponentData::Cached($comp)),)*
                     )
                 });
 

diff --git a/crates/re_query_cache/tests/latest_at.rs b/crates/re_query_cache/tests/latest_at.rs
@@ -10,7 +10,7 @@ use re_log_types::{
     example_components::{MyColor, MyPoint, MyPoints},
     DataRow, EntityPath, RowId, TimePoint,
 };
-use re_query_cache::Caches;
+use re_query_cache::{Caches, MaybeCachedComponentData};
 use re_types_core::{components::InstanceKey, Loggable as _};
 
 // ---
@@ -463,7 +463,10 @@ fn query_and_compare(
                     uncached_data_time = data_time;
                     uncached_instance_keys.extend(instance_keys.iter().copied());
                     uncached_positions.extend(positions.iter().copied());
-                    uncached_colors.extend(colors.iter().copied());
+                    uncached_colors.extend(MaybeCachedComponentData::iter_or_repeat_opt(
+                        &colors,
+                        positions.len(),
+                    ));
                 },
             )
             .unwrap();
@@ -482,7 +485,10 @@ fn query_and_compare(
                     cached_data_time = data_time;
                     cached_instance_keys.extend(instance_keys.iter().copied());
                     cached_positions.extend(positions.iter().copied());
-                    cached_colors.extend(colors.iter().copied());
+                    cached_colors.extend(MaybeCachedComponentData::iter_or_repeat_opt(
+                        &colors,
+                        positions.len(),
+                    ));
                 },
             )
             .unwrap();

diff --git a/crates/re_query_cache/tests/range.rs b/crates/re_query_cache/tests/range.rs
@@ -10,7 +10,7 @@ use re_log_types::{
     example_components::{MyColor, MyLabel, MyPoint, MyPoints},
     DataRow, EntityPath, RowId, TimeInt, TimePoint, TimeRange,
 };
-use re_query_cache::Caches;
+use re_query_cache::{Caches, MaybeCachedComponentData};
 use re_types::components::InstanceKey;
 use re_types_core::Loggable as _;
 
@@ -597,7 +597,11 @@ fn query_and_compare(
                     uncached_data_times.push(data_time);
                     uncached_instance_keys.push(instance_keys.to_vec());
                     uncached_positions.push(positions.to_vec());
-                    uncached_colors.push(colors.to_vec());
+                    uncached_colors.push(
+                        MaybeCachedComponentData::iter_or_repeat_opt(&colors, positions.len())
+                            .copied()
+                            .collect_vec(),
+                    );
                 },
             )
             .unwrap();
@@ -616,7 +620,11 @@ fn query_and_compare(
                     cached_data_times.push(data_time);
                     cached_instance_keys.push(instance_keys.to_vec());
                     cached_positions.push(positions.to_vec());
-                    cached_colors.push(colors.to_vec());
+                    cached_colors.push(
+                        MaybeCachedComponentData::iter_or_repeat_opt(&colors, positions.len())
+                            .copied()
+                            .collect_vec(),
+                    );
                 },
             )
             .unwrap();

diff --git a/crates/re_space_view_spatial/benches/bench_points.rs b/crates/re_space_view_spatial/benches/bench_points.rs
@@ -116,17 +116,11 @@ fn bench_points(c: &mut criterion::Criterion) {
                 let data = Points3DComponentData {
                     instance_keys: instance_keys.as_slice(),
                     positions: positions.as_slice(),
-                    colors: colors.as_slice(),
-                    radii: radii.as_slice(),
-                    labels: labels.as_slice(),
-                    keypoint_ids: keypoint_ids
-                        .iter()
-                        .any(Option::is_some)
-                        .then_some(keypoint_ids.as_slice()),
-                    class_ids: class_ids
-                        .iter()
-                        .any(Option::is_some)
-                        .then_some(class_ids.as_slice()),
+                    colors: colors.as_deref(),
+                    radii: radii.as_deref(),
+                    labels: labels.as_deref(),
+                    keypoint_ids: keypoint_ids.as_deref(),
+                    class_ids: class_ids.as_deref(),
                 };
                 assert_eq!(data.instance_keys.len(), NUM_POINTS);
 

diff --git a/crates/re_space_view_spatial/src/visualizers/entity_iterator.rs b/crates/re_space_view_spatial/src/visualizers/entity_iterator.rs
@@ -139,7 +139,7 @@ macro_rules! impl_process_archetype {
                 (Option<TimeInt>, RowId),
                 &[InstanceKey],
                 $(&[$pov],)*
-                $(&[Option<$comp>],)*
+                $(Option<&[Option<$comp>]>,)*
             ) -> ::re_query::Result<()>,
         {
             // NOTE: not `profile_function!` because we want them merged together.
@@ -204,7 +204,7 @@ macro_rules! impl_process_archetype {
                             t,
                             keys.as_slice(),
                             $($pov.as_slice(),)+
-                            $($comp.as_slice(),)*
+                            $($comp.as_deref(),)*
                         ) {
                             re_log::error_once!(
                                 "Unexpected error querying {:?}: {err}",

diff --git a/crates/re_space_view_spatial/src/visualizers/mod.rs b/crates/re_space_view_spatial/src/visualizers/mod.rs
@@ -126,13 +126,19 @@ pub fn process_colors<'a, A: Archetype>(
 
 /// Process [`Color`] components using annotations and default colors.
 pub fn process_color_slice<'a>(
-    colors: &'a [Option<Color>],
+    colors: Option<&'a [Option<Color>]>,
+    default_len: usize,
     ent_path: &'a EntityPath,
     annotation_infos: &'a ResolvedAnnotationInfos,
 ) -> impl Iterator<Item = egui::Color32> + 'a {
     re_tracing::profile_function!();
     let default_color = DefaultColor::EntityPath(ent_path);
 
+    let colors = colors.as_ref().map_or(
+        itertools::Either::Left(std::iter::repeat(&None).take(default_len)),
+        |data| itertools::Either::Right(data.iter()),
+    );
+
     itertools::izip!(annotation_infos.iter(), colors).map(move |(annotation_info, color)| {
         annotation_info.color(color.map(|c| c.to_array()), default_color)
     })
@@ -169,14 +175,19 @@ pub fn process_radii<'a, A: Archetype>(
 /// Process [`re_types::components::Radius`] components to [`re_renderer::Size`] using auto size
 /// where no radius is specified.
 pub fn process_radius_slice<'a>(
-    radii: &'a [Option<re_types::components::Radius>],
+    radii: Option<&'a [Option<re_types::components::Radius>]>,
+    default_len: usize,
     ent_path: &EntityPath,
 ) -> impl Iterator<Item = re_renderer::Size> + 'a {
     re_tracing::profile_function!();
     let ent_path = ent_path.clone();
-    radii
-        .iter()
-        .map(move |radius| process_radius(&ent_path, radius))
+
+    let radii = radii.as_ref().map_or(
+        itertools::Either::Left(std::iter::repeat(&None).take(default_len)),
+        |data| itertools::Either::Right(data.iter()),
+    );
+
+    radii.map(move |radius| process_radius(&ent_path, radius))
 }
 
 fn process_radius(