Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved automatic view creation heuristic, major speedup for scenes with many entities #4874

Merged
merged 29 commits into from
Jan 28, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Jan 19, 2024

What

What space views get spawned is no longer determined by a centralized heuristic, but instead by one distributed to each space view class. Each of those heuristics can make much better use of existing information provided by store subscribers.

Significant speedup for scenes with many entities (100+):

opf --no-frames
before: App::update 39.0ms, of which default_created_space_views 10.4ms
after: App::update 27.4ms, of which default_created_space_views 0.17ms

python/tests/many_entities (1000 individual points)
before: App:::update 151ms, of which default_created_space_views 124ms
after: App::update 22.6ms, of which default_created_space_views 0.06ms
(still pretty bad, but less so and for different reasons now!)

(numbers from my macbook on a release build. Actual numbers might differ by now, these are from last week. )

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@Wumpf Wumpf added enhancement New feature or request 📉 performance Optimization, memory use, etc 🚜 refactor Change the code, not the functionality include in changelog labels Jan 19, 2024
@Wumpf Wumpf force-pushed the andreas/distributed-space-view-heuristics branch from df90c70 to 7af18e4 Compare January 19, 2024 14:04
Copy link

Size changes

Name main 4874/merge Change
plots.rrd 194.56 kiB 204.80 kiB +5.26%

@Wumpf Wumpf marked this pull request as ready for review January 24, 2024 18:05
@Wumpf Wumpf requested a review from jleibs January 24, 2024 18:12
@Wumpf Wumpf changed the title Improved automatic view creation heuristic Improved automatic view creation heuristic, major speedup for scenes with many entities Jan 24, 2024
Copy link
Member

@jleibs jleibs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! I find this so much easier to reason about than the old implementation.

Most of my complaints aren't with this change, so much as the fact that it's now easier to see and discuss what I think are flawed heuristics in and of themselves. For merge-sanity I'm totally happy to defer any of those other changes to future-work now that we have this much cleaner framework for discussing the behavior in a per-space-view context.

crates/re_space_view/src/heuristics.rs Show resolved Hide resolved
crates/re_viewer_context/src/viewer_context.rs Outdated Show resolved Hide resolved
return Default::default();
};

let visualizer = TVisualizer::default();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The need to instantiate this feels weird to me but I'm guessing the self in the signature stems from some context where we need to dynamically dispatch across a set of visualizers instead of being templated on <TVisualizer>?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah all these self-passings are about object safety really. We'd like this to be static but Rust makes this messy

crates/re_space_view_spatial/src/space_view_2d.rs Outdated Show resolved Hide resolved
crates/re_space_view_spatial/src/space_view_2d.rs Outdated Show resolved Hide resolved
crates/re_space_view_spatial/src/space_view_3d.rs Outdated Show resolved Hide resolved
crates/re_space_view_time_series/src/space_view_class.rs Outdated Show resolved Hide resolved
@Wumpf Wumpf merged commit 15cd60d into main Jan 28, 2024
40 of 41 checks passed
@Wumpf Wumpf deleted the andreas/distributed-space-view-heuristics branch January 28, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request include in changelog 📉 performance Optimization, memory use, etc 🚜 refactor Change the code, not the functionality
Projects
None yet
2 participants