From 71afae75d8907647b77b6f117e0a1fc135d07f2f Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Thu, 12 Dec 2024 09:39:47 +0100 Subject: [PATCH] Do query for default components only once per view (#8424) Moves query of default components to the "execute query" stage which we perform for every view at the start of the frame. Previously, we've done this as part of blueprint resolve on every Gives me about 2ms cpu frame time in the 2h airtraffic demo on my windows machine running with single thread (since it's easier to profile, `pixi run rerun-release --threads 1`) with the 3d view maximized and the timline minimized. Change is expected to have high impact for: * many entities * no or few transforms (because the transform cost shadows this win too much and adds a lot of noise) profile snapshots for said scenario: before ![image](https://github.com/user-attachments/assets/009c00f9-d48b-4754-8cab-2539be87a2c1) after: ![image](https://github.com/user-attachments/assets/3e5eb488-caec-49b4-937f-7bdea59a7b17) Related future work: * need short circuiting for overrides as well - we can just know when there's none (and that's naturally very common!!) * short circuit when there's no (fitting) default components? * reduce the amount of query objects being passed around, we have too many of those at this point! --- crates/store/re_query/src/latest_at.rs | 2 +- .../re_selection_panel/src/visualizer_ui.rs | 12 ++--- crates/viewer/re_view/src/query.rs | 36 ++++---------- crates/viewer/re_view/src/results_ext.rs | 24 +++++----- crates/viewer/re_viewer/src/app_state.rs | 9 +++- .../re_viewer_context/src/query_context.rs | 25 ++++++++-- .../re_viewer_context/src/test_context.rs | 2 +- .../src/view/view_context.rs | 2 +- .../viewer/re_viewport_blueprint/src/view.rs | 16 +++++-- .../src/view_contents.rs | 47 +++++++++++++++++-- 10 files changed, 110 insertions(+), 65 deletions(-) diff --git a/crates/store/re_query/src/latest_at.rs b/crates/store/re_query/src/latest_at.rs index 02a42649a4cb..a9b61bcaa627 100644 --- a/crates/store/re_query/src/latest_at.rs +++ b/crates/store/re_query/src/latest_at.rs @@ -208,7 +208,7 @@ impl QueryCache { /// /// Use [`LatestAtResults::get`] and/or [`LatestAtResults::get_required`] in order to access /// the results for each individual component. -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct LatestAtResults { /// The associated [`EntityPath`]. pub entity_path: EntityPath, diff --git a/crates/viewer/re_selection_panel/src/visualizer_ui.rs b/crates/viewer/re_selection_panel/src/visualizer_ui.rs index e6554a0cbc9d..d1645afff8df 100644 --- a/crates/viewer/re_selection_panel/src/visualizer_ui.rs +++ b/crates/viewer/re_selection_panel/src/visualizer_ui.rs @@ -246,19 +246,19 @@ fn visualizer_components( ValueSource::Override => ( ctx.viewer_ctx.blueprint_query, ctx.blueprint_db(), - override_path, + override_path.clone(), result_override.unwrap(), ), ValueSource::Store => ( &store_query, ctx.recording(), - &data_result.entity_path, + data_result.entity_path.clone(), result_store.unwrap(), ), ValueSource::Default => ( ctx.viewer_ctx.blueprint_query, ctx.blueprint_db(), - ctx.defaults_path, + ViewBlueprint::defaults_path(ctx.view_id), result_default.unwrap(), ), ValueSource::FallbackOrPlaceholder => { @@ -280,7 +280,7 @@ fn visualizer_components( }; re_data_ui::ComponentPathLatestAtResults { - component_path: ComponentPath::new(entity_path.clone(), component_name), + component_path: ComponentPath::new(entity_path, component_name), unit: latest_at_unit, } .data_ui(ctx.viewer_ctx, ui, UiLayout::List, query, db); @@ -340,7 +340,7 @@ fn visualizer_components( &query_ctx, ui, "Default", - ctx.defaults_path, + &ViewBlueprint::defaults_path(ctx.view_id), component_name, *row_id, raw_default.as_ref(), @@ -501,7 +501,7 @@ fn menu_more( if ui.button("Make default for current view").clicked() { ctx.save_blueprint_array( - ctx.defaults_path, + &ViewBlueprint::defaults_path(ctx.view_id), component_name, raw_current_value.to_boxed(), ); diff --git a/crates/viewer/re_view/src/query.rs b/crates/viewer/re_view/src/query.rs index 98f3bcdd2ab5..173a6e453831 100644 --- a/crates/viewer/re_view/src/query.rs +++ b/crates/viewer/re_view/src/query.rs @@ -25,13 +25,13 @@ use re_viewer_context::{ /// /// Data should be accessed via the [`crate::RangeResultsExt`] trait which is implemented for /// [`crate::HybridResults`]. -pub fn range_with_blueprint_resolved_data( - ctx: &ViewContext<'_>, +pub fn range_with_blueprint_resolved_data<'a>( + ctx: &ViewContext<'a>, _annotations: Option<&re_viewer_context::Annotations>, range_query: &RangeQuery, data_result: &re_viewer_context::DataResult, component_names: impl IntoIterator, -) -> HybridRangeResults { +) -> HybridRangeResults<'a> { re_tracing::profile_function!(data_result.entity_path.to_string()); let mut component_name_set = component_names.into_iter().collect::>(); @@ -47,20 +47,10 @@ pub fn range_with_blueprint_resolved_data( component_name_set.iter(), ); - // TODO(jleibs): This doesn't work when the component set contains empty results. - // This means we over-query for defaults that will never be used. - // component_set.retain(|component| !results.components.contains_key(component)); - - let defaults = ctx.viewer_ctx.blueprint_engine().cache().latest_at( - ctx.viewer_ctx.blueprint_query, - ctx.defaults_path, - component_name_set.iter().copied(), - ); - HybridRangeResults { overrides, results, - defaults, + defaults: &ctx.query_result.component_defaults, } } @@ -76,14 +66,14 @@ pub fn range_with_blueprint_resolved_data( /// Data should be accessed via the [`crate::RangeResultsExt`] trait which is implemented for /// [`crate::HybridResults`]. /// -/// If `query_shadowed_defaults` is true, all defaults will be queried, even if they are not used. +/// If `query_shadowed_components` is true, store components will be queried, even if they are not used. pub fn latest_at_with_blueprint_resolved_data<'a>( ctx: &'a ViewContext<'a>, _annotations: Option<&'a re_viewer_context::Annotations>, latest_at_query: &LatestAtQuery, data_result: &'a re_viewer_context::DataResult, component_names: impl IntoIterator, - query_shadowed_defaults: bool, + query_shadowed_components: bool, ) -> HybridLatestAtResults<'a> { // This is called very frequently, don't put a profile scope here. @@ -92,7 +82,7 @@ pub fn latest_at_with_blueprint_resolved_data<'a>( let overrides = query_overrides(ctx.viewer_ctx, data_result, component_set.iter()); // No need to query for components that have overrides unless opted in! - if !query_shadowed_defaults { + if !query_shadowed_components { component_set.retain(|component| !overrides.components.contains_key(component)); } @@ -102,20 +92,10 @@ pub fn latest_at_with_blueprint_resolved_data<'a>( component_set.iter().copied(), ); - // TODO(jleibs): This doesn't work when the component set contains empty results. - // This means we over-query for defaults that will never be used. - // component_set.retain(|component| !results.components.contains_key(component)); - - let defaults = ctx.viewer_ctx.blueprint_engine().cache().latest_at( - ctx.viewer_ctx.blueprint_query, - ctx.defaults_path, - component_set.iter().copied(), - ); - HybridLatestAtResults { overrides, results, - defaults, + defaults: &ctx.query_result.component_defaults, ctx, query: latest_at_query.clone(), data_result, diff --git a/crates/viewer/re_view/src/results_ext.rs b/crates/viewer/re_view/src/results_ext.rs index 8e9300a4d508..e69e764482f0 100644 --- a/crates/viewer/re_view/src/results_ext.rs +++ b/crates/viewer/re_view/src/results_ext.rs @@ -21,7 +21,7 @@ use crate::DataResultQuery as _; pub struct HybridLatestAtResults<'a> { pub overrides: LatestAtResults, pub results: LatestAtResults, - pub defaults: LatestAtResults, + pub defaults: &'a LatestAtResults, pub ctx: &'a ViewContext<'a>, pub query: LatestAtQuery, @@ -33,10 +33,10 @@ pub struct HybridLatestAtResults<'a> { /// Although overrides are never temporal, when accessed via the [`crate::RangeResultsExt`] trait /// they will be merged into the results appropriately. #[derive(Debug)] -pub struct HybridRangeResults { +pub struct HybridRangeResults<'a> { pub(crate) overrides: LatestAtResults, pub(crate) results: RangeResults, - pub(crate) defaults: LatestAtResults, + pub(crate) defaults: &'a LatestAtResults, } impl HybridLatestAtResults<'_> { @@ -130,7 +130,7 @@ pub enum HybridResults<'a> { LatestAt(LatestAtQuery, HybridLatestAtResults<'a>), // Boxed because of size difference between variants - Range(RangeQuery, Box), + Range(RangeQuery, Box>), } impl HybridResults<'_> { @@ -141,9 +141,8 @@ impl HybridResults<'_> { match self { Self::LatestAt(_, r) => { let mut indices = Vec::with_capacity( - r.defaults.components.len() - + r.overrides.components.len() - + r.results.components.len(), + // Don't add defaults component count because that's defaults for the entire view. + r.overrides.components.len() + r.results.components.len(), ); indices.extend( @@ -170,9 +169,8 @@ impl HybridResults<'_> { Self::Range(_, r) => { let mut indices = Vec::with_capacity( - r.defaults.components.len() - + r.overrides.components.len() - + r.results.components.len(), // Don't know how many results per component. + // Don't add defaults component count because that's defaults for the entire view. + r.overrides.components.len() + r.results.components.len(), ); indices.extend( @@ -213,9 +211,9 @@ impl<'a> From<(LatestAtQuery, HybridLatestAtResults<'a>)> for HybridResults<'a> } } -impl From<(RangeQuery, HybridRangeResults)> for HybridResults<'_> { +impl<'a> From<(RangeQuery, HybridRangeResults<'a>)> for HybridResults<'a> { #[inline] - fn from((query, results): (RangeQuery, HybridRangeResults)) -> Self { + fn from((query, results): (RangeQuery, HybridRangeResults<'a>)) -> Self { Self::Range(query, Box::new(results)) } } @@ -288,7 +286,7 @@ impl RangeResultsExt for RangeResults { } } -impl RangeResultsExt for HybridRangeResults { +impl<'a> RangeResultsExt for HybridRangeResults<'a> { #[inline] fn get_required_chunks(&self, component_name: &ComponentName) -> Option> { if let Some(unit) = self.overrides.get(component_name) { diff --git a/crates/viewer/re_viewer/src/app_state.rs b/crates/viewer/re_viewer/src/app_state.rs index 8e07b53d6c7c..1d0a1559b13f 100644 --- a/crates/viewer/re_viewer/src/app_state.rs +++ b/crates/viewer/re_viewer/src/app_state.rs @@ -245,8 +245,13 @@ impl AppState { ( view.id, - view.contents - .execute_query(store_context, &visualizable_entities), + view.contents.execute_query( + store_context, + view_class_registry, + &blueprint_query, + view.id, + &visualizable_entities, + ), ) }) .collect::<_>() diff --git a/crates/viewer/re_viewer_context/src/query_context.rs b/crates/viewer/re_viewer_context/src/query_context.rs index 77202194cf30..67cc7e76fe7f 100644 --- a/crates/viewer/re_viewer_context/src/query_context.rs +++ b/crates/viewer/re_viewer_context/src/query_context.rs @@ -5,7 +5,7 @@ use smallvec::SmallVec; use re_log_types::{EntityPath, EntityPathHash}; -use crate::{DataResult, ViewContext, ViewId, ViewState, ViewerContext}; +use crate::{blueprint_timeline, DataResult, ViewContext, ViewId, ViewState, ViewerContext}; slotmap::new_key_type! { /// Identifier for a [`DataResultNode`] @@ -48,8 +48,7 @@ impl QueryContext<'_> { } } -/// The result of executing a single data query -#[derive(Default)] +/// The result of executing a single data query for a specific view. pub struct DataQueryResult { /// The [`DataResultTree`] for the query pub tree: DataResultTree, @@ -62,6 +61,25 @@ pub struct DataQueryResult { /// This does *not* take into account the actual selection of visualizers /// which may be an explicit none for any given entity. pub num_visualized_entities: usize, + + /// Latest-at results for all component defaults in this view. + pub component_defaults: re_query::LatestAtResults, +} + +impl Default for DataQueryResult { + fn default() -> Self { + Self { + tree: Default::default(), + num_matching_entities: 0, + num_visualized_entities: 0, + component_defaults: re_query::LatestAtResults { + entity_path: "".into(), + query: re_chunk_store::LatestAtQuery::latest(blueprint_timeline()), + compound_index: (re_chunk::TimeInt::STATIC, re_chunk::RowId::ZERO), + components: Default::default(), + }, + } + } } impl DataQueryResult { @@ -85,6 +103,7 @@ impl Clone for DataQueryResult { tree: self.tree.clone(), num_matching_entities: self.num_matching_entities, num_visualized_entities: self.num_visualized_entities, + component_defaults: self.component_defaults.clone(), } } } diff --git a/crates/viewer/re_viewer_context/src/test_context.rs b/crates/viewer/re_viewer_context/src/test_context.rs index 0ce951bf7c85..70fae8e9238e 100644 --- a/crates/viewer/re_viewer_context/src/test_context.rs +++ b/crates/viewer/re_viewer_context/src/test_context.rs @@ -29,7 +29,7 @@ pub struct TestContext { pub selection_state: ApplicationSelectionState, pub recording_config: RecordingConfig, - blueprint_query: LatestAtQuery, + pub blueprint_query: LatestAtQuery, component_ui_registry: ComponentUiRegistry, command_sender: CommandSender, diff --git a/crates/viewer/re_viewer_context/src/view/view_context.rs b/crates/viewer/re_viewer_context/src/view/view_context.rs index 6667a893fc67..9f8104552328 100644 --- a/crates/viewer/re_viewer_context/src/view/view_context.rs +++ b/crates/viewer/re_viewer_context/src/view/view_context.rs @@ -19,8 +19,8 @@ pub struct ViewContext<'a> { pub viewer_ctx: &'a crate::ViewerContext<'a>, pub view_id: ViewId, pub view_state: &'a dyn crate::ViewState, - pub defaults_path: &'a EntityPath, pub visualizer_collection: Arc, + pub query_result: &'a DataQueryResult, } impl<'a> ViewContext<'a> { diff --git a/crates/viewer/re_viewport_blueprint/src/view.rs b/crates/viewer/re_viewport_blueprint/src/view.rs index be3648446e80..3177beae78da 100644 --- a/crates/viewer/re_viewport_blueprint/src/view.rs +++ b/crates/viewer/re_viewport_blueprint/src/view.rs @@ -400,8 +400,8 @@ impl ViewBlueprint { viewer_ctx: ctx, view_id: self.id, view_state, - defaults_path: &self.defaults_path, visualizer_collection: self.visualizer_collection(ctx), + query_result: ctx.lookup_query_result(self.id), } } @@ -414,8 +414,8 @@ impl ViewBlueprint { viewer_ctx: ctx, view_id: self.id, view_state, - defaults_path: &self.defaults_path, visualizer_collection: self.visualizer_collection(ctx), + query_result: ctx.lookup_query_result(self.id), } } @@ -715,7 +715,7 @@ mod tests { // Set up a store query and update the overrides. let query_result = - update_overrides(&test_ctx, &view.contents, &visualizable_entities, &resolver); + update_overrides(&test_ctx, &view, &visualizable_entities, &resolver); // Extract component overrides for testing. let mut visited: HashMap> = @@ -745,7 +745,7 @@ mod tests { fn update_overrides( test_ctx: &TestContext, - contents: &ViewContents, + view: &ViewBlueprint, visualizable_entities: &PerVisualizer, resolver: &DataQueryPropertyResolver<'_>, ) -> re_viewer_context::DataQueryResult { @@ -759,7 +759,13 @@ mod tests { hub: &re_viewer_context::StoreHub::test_hub(), }; - let mut query_result = contents.execute_query(&store_ctx, visualizable_entities); + let mut query_result = view.contents.execute_query( + &store_ctx, + &test_ctx.view_class_registry, + &test_ctx.blueprint_query, + view.id, + visualizable_entities, + ); let mut view_states = ViewStates::default(); test_ctx.run_in_egui_central_panel(|ctx, _ui| { diff --git a/crates/viewer/re_viewport_blueprint/src/view_contents.rs b/crates/viewer/re_viewport_blueprint/src/view_contents.rs index 06fd584df7b8..2351767c15da 100644 --- a/crates/viewer/re_viewport_blueprint/src/view_contents.rs +++ b/crates/viewer/re_viewport_blueprint/src/view_contents.rs @@ -1,4 +1,4 @@ -use nohash_hasher::IntMap; +use nohash_hasher::{IntMap, IntSet}; use slotmap::SlotMap; use smallvec::SmallVec; @@ -232,6 +232,9 @@ impl ViewContents { pub fn execute_query( &self, ctx: &re_viewer_context::StoreContext<'_>, + view_class_registry: &re_viewer_context::ViewClassRegistry, + blueprint_query: &LatestAtQuery, + view_id: ViewId, visualizable_entities_for_visualizer_systems: &PerVisualizer, ) -> DataQueryResult { re_tracing::profile_function!(); @@ -261,10 +264,37 @@ impl ViewContents { ) }; + // Query defaults for all the components that any visualizer in this view is interested in. + let component_defaults = { + re_tracing::profile_scope!("component_defaults"); + + let visualizer_collection = + view_class_registry.new_visualizer_collection(self.view_class_identifier); + + // Figure out which components are relevant. + let mut components_for_defaults = IntSet::default(); + for (visualizer, entities) in visualizable_entities_for_visualizer_systems.iter() { + if entities.is_empty() { + continue; + } + let Ok(visualizer) = visualizer_collection.get_by_identifier(*visualizer) else { + continue; + }; + components_for_defaults.extend(visualizer.visualizer_query_info().queried.iter()); + } + + ctx.blueprint.latest_at( + blueprint_query, + &ViewBlueprint::defaults_path(view_id), + components_for_defaults, + ) + }; + DataQueryResult { tree: DataResultTree::new(data_results, root_handle), num_matching_entities, num_visualized_entities, + component_defaults, } } } @@ -558,7 +588,7 @@ mod tests { use re_chunk::{Chunk, RowId}; use re_entity_db::EntityDb; use re_log_types::{example_components::MyPoint, StoreId, TimePoint, Timeline}; - use re_viewer_context::{StoreContext, StoreHub, VisualizableEntities}; + use re_viewer_context::{blueprint_timeline, StoreContext, StoreHub, VisualizableEntities}; use super::*; @@ -571,6 +601,7 @@ mod tests { let timeline_frame = Timeline::new_sequence("frame"); let timepoint = TimePoint::from_iter([(timeline_frame, 10)]); + let view_class_registry = ViewClassRegistry::default(); // Set up a store DB with some entities for entity_path in ["parent", "parent/skipped/child1", "parent/skipped/child2"] { @@ -696,14 +727,20 @@ mod tests { ]; for (i, Scenario { filter, outputs }) in scenarios.into_iter().enumerate() { + let view_id = ViewId::random(); let contents = ViewContents::new( - ViewId::random(), + view_id, "3D".into(), EntityPathFilter::parse_forgiving(filter, &space_env), ); - let query_result = - contents.execute_query(&ctx, &visualizable_entities_for_visualizer_systems); + let query_result = contents.execute_query( + &ctx, + &view_class_registry, + &LatestAtQuery::latest(blueprint_timeline()), + view_id, + &visualizable_entities_for_visualizer_systems, + ); let mut visited = vec![]; query_result.tree.visit(&mut |node| {