perf: stop rebuilding the selection filter list on every drawn point

`view-detail` on an edit frame:

    total=105.6ms  chrome=105.4   [every other section 0.0]

and the edit latency measurement puts ~102 ms of a 192 ms drawn point in
the gap between the handler returning and the scene primitive being built
— which is that same `chrome`.

The status bar asks for `entity_type_names_in_layout` on every frame to
populate the selection-filter menu, and the cache behind it was keyed on
`geometry_epoch`. Every drawn point bumps that epoch, so every drawn point
walked all 444 937 handles a model space owns to answer a question the
line did not change: adding a line to a drawing that already has lines
leaves the set of type names exactly as it was.

The cache now carries the set alongside the list, and folds edits in from
the delta journal — the same incremental shape `entity_index` already
uses. Additions can only grow the set, so they are applied directly and
the existing `Arc` is handed back untouched when nothing new appeared. A
removal can retire a type and cannot be resolved from a handle that is
already gone, so those still fall back to the full walk.

`entity_type_name` returns a literal for every arm but one — `Extended`
borrows its class name from the entity — so the set holds owned strings,
and the hot path tests `contains` before allocating.

Tested: a second line of an existing type must hand back the same `Arc`,
a new type must still appear, and the folded answer must equal what a
rebuild from scratch produces.

Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
PanosandClaude Opus 5 committed 2026-09-08 16:42:21 +03:00
1 parent a754fac6d4
commit 5e5a810ee5
2 files changed
+123 -11

No files matched your search

+14 -1
View File
@@ -1851,7 +1851,20 @@ pub struct Scene {
/// (geometry_epoch, layout block). The status bar asks for this on every
/// frame to populate the selection-filter menu, but the answer only
/// changes when entities are added or removed.
layout_type_names_cache: RefCell<Option<(u64, Handle, std::sync::Arc<Vec<String>>)>>,
/// `(epoch, block, present type names, the list handed to the UI)`.
///
/// The set is kept alongside the list so an edit can be folded in without
/// rebuilding either: drawing a line into a drawing that already has lines
/// does not change the answer, and the walk that produces it visits every
/// entity the layout owns.
layout_type_names_cache: RefCell<
Option<(
u64,
Handle,
std::collections::BTreeSet<String>,
std::sync::Arc<Vec<String>>,
)>,
>,
/// Reverse dependencies from layer/style/block definitions to the top-level
/// entities whose resident wire runs actually change. Kept independent from
/// `geometry_epoch`: a layer colour toggle can reuse the index, invalidate
+109 -10
View File
@@ -361,18 +361,71 @@ impl Scene {
}
/// Sorted entity types in the current layout, cached until geometry or layout changes.
/// The entity type names present in the current layout, for the selection
/// filter menu.
///
/// The status bar asks for this on **every** frame, and the cache used to
/// be keyed on `geometry_epoch` alone — so every drawn point invalidated it
/// and the walk below visited all 444 937 handles a model space owns. That
/// was ~100 ms per edit, the largest single cost of drawing a line, and
/// almost always to arrive at the same answer: adding a line to a drawing
/// that already has lines changes nothing here.
///
/// An edit is now folded in from the delta journal instead. Additions only
/// ever grow the set, so they are cheap to apply; a removal can retire a
/// type and cannot be resolved from a handle that is already gone, so those
/// fall back to the full walk.
pub fn entity_type_names_in_layout(&self) -> std::sync::Arc<Vec<String>> {
use crate::entities::traits::entity_type_name;
let block = self.current_layout_block_handle();
let mut cached_epoch = None;
{
let cache = self.layout_type_names_cache.borrow();
if let Some((epoch, cached_block, names)) = cache.as_ref() {
if *epoch == self.geometry_epoch && *cached_block == block {
return std::sync::Arc::clone(names);
if let Some((epoch, cached_block, _, names)) = cache.as_ref() {
if *cached_block == block {
if *epoch == self.geometry_epoch {
return std::sync::Arc::clone(names);
}
cached_epoch = Some(*epoch);
}
}
}
let mut names: std::collections::BTreeSet<&str> =
// Incremental: fold the changes since the cached epoch into the set.
if let Some(since) = cached_epoch {
if let Some(deltas) = self.replay_since(since) {
if !deltas.iter().any(|(_, kind)| *kind == ChangeKind::Removed) {
let mut cache = self.layout_type_names_cache.borrow_mut();
if let Some((epoch, _, present, names)) = cache.as_mut() {
let mut added = false;
for (handle, _) in &deltas {
if let Some(entity) = self.document.get_entity(*handle) {
if entity.common().owner_handle == block {
// `contains` first: the common case is a
// type already present, and that path must
// not allocate.
let name = entity_type_name(entity);
if !present.contains(name) {
present.insert(name.to_string());
added = true;
}
}
}
}
// Only rebuild the list when the set actually moved;
// otherwise the existing `Arc` is still the answer.
if added {
*names =
std::sync::Arc::new(present.iter().cloned().collect());
}
*epoch = self.geometry_epoch;
return std::sync::Arc::clone(names);
}
}
}
}
let mut present: std::collections::BTreeSet<String> =
std::collections::BTreeSet::new();
if let Some(record) = self
.document
@@ -382,15 +435,21 @@ impl Scene {
{
for &handle in &record.entity_handles {
if let Some(entity) = self.document.get_entity(handle) {
names.insert(entity_type_name(entity));
let name = entity_type_name(entity);
if !present.contains(name) {
present.insert(name.to_string());
}
}
}
}
let names: std::sync::Arc<Vec<String>> = std::sync::Arc::new(
names.into_iter().map(str::to_string).collect(),
);
*self.layout_type_names_cache.borrow_mut() =
Some((self.geometry_epoch, block, std::sync::Arc::clone(&names)));
let names: std::sync::Arc<Vec<String>> =
std::sync::Arc::new(present.iter().cloned().collect());
*self.layout_type_names_cache.borrow_mut() = Some((
self.geometry_epoch,
block,
present,
std::sync::Arc::clone(&names),
));
names
}
@@ -954,6 +1013,46 @@ impl Scene {
mod tests {
use super::*;
/// The reason the cache carries a set as well as a list: drawing into a
/// drawing that already has that type must not rebuild either, because the
/// rebuild walks every entity the layout owns — 444 937 of them on the
/// reproducer, ~100 ms, on every single drawn point.
#[test]
fn adding_a_type_already_present_reuses_the_list() {
use acadrust::entities::{Circle, EntityType, Line};
use acadrust::types::Vector3;
use std::sync::Arc;
let line = || {
EntityType::Line(Line::from_points(
Vector3::new(0.0, 0.0, 0.0),
Vector3::new(1.0, 0.0, 0.0),
))
};
let mut scene = Scene::new();
scene.add_entity(line());
let first = scene.entity_type_names_in_layout();
scene.add_entity(line());
let second = scene.entity_type_names_in_layout();
assert!(
Arc::ptr_eq(&first, &second),
"a second line changes no type name, so the list must be reused",
);
scene.add_entity(EntityType::Circle(Circle::new()));
let third = scene.entity_type_names_in_layout();
assert_eq!(third.as_slice(), ["Circle", "Line"], "a new type must appear");
// The incremental answer has to be the answer a full walk gives.
scene.layout_type_names_cache.borrow_mut().take();
assert_eq!(
scene.entity_type_names_in_layout().as_slice(),
third.as_slice(),
"folding edits in must match rebuilding from scratch",
);
}
#[test]
fn layout_type_cache_reuses_and_invalidates_on_edits_undo_and_layout() {
use acadrust::entities::{Circle, EntityType, Line};