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

Move heuristic for viable space view "candidates" to space view class #3079

Closed
Wumpf opened this issue Aug 22, 2023 · 0 comments · Fixed by #4874
Closed

Move heuristic for viable space view "candidates" to space view class #3079

Wumpf opened this issue Aug 22, 2023 · 0 comments · Fixed by #4874
Labels
enhancement New feature or request 📉 performance Optimization, memory use, etc 📺 re_viewer affects re_viewer itself

Comments

@Wumpf
Copy link
Member

Wumpf commented Aug 22, 2023

We figured out in the process that as long as auto-adding space views is enabled we take a significant amount of time determining them.

@jleibs and me discussed how to improve the way we do space view heuristics in the future and came up with a new schema where each space view class would decide on its own heuristic (this has the added benefit of removing remaining knowledge of specific views from re_viewport. The heuristic method per class would look something like this:

    fn auto_spawn_heuristic(
        &self,
        ctx: &ViewerContext<'_>,
        ent_paths: &PerSystemEntities,
        max_num_candidates: usize,
    ) -> Vec<(AutoSpawnHeuristic, EntityPath)>;

Meaning that candidate creation can be done in a much more sensible way. Some challenges there are dealing with disconnected-components.

Why does this help performance?
Right now we have an unrealistic large number of space view root candidates for which we determine which entities should be added for each possible space view class.
Instead, each class actually has a relatively limited set of candidates in the first place. Furthermore, only some of these candidates need to determine which entities they will contain by default in order to determine a heuristic score!

@Wumpf Wumpf added enhancement New feature or request 📺 re_viewer affects re_viewer itself 📉 performance Optimization, memory use, etc labels Aug 22, 2023
jleibs added a commit that referenced this issue Aug 29, 2023
### What

* Fixes #2285
(.. at least fixes it well enough for the moment I reckon)

Improves both the per space view rendering and the space view adding
heuristic. In particular on the later there's still significant gains to
be made.
Most importantly we should be scaling now much better with number of
added systems.

Went fairly early with the decision to have the primary datastructure to
pass around be a `Map<System, Vec<Entity>>` (conceptual) instead of the
other way round since this is what we need in the systems most of the
time.
This is another step towards a stronger contract of systems specifying
what components they will query ahead of time!

The way it is implemented on thr (rename from `BlueprintTree` 💥)
`SpaceViewContents` also paves the way for ui selection of systems for a
given entity path which complements the ongoing work on our new data
ingestion interfaces.

-----

Testcase: 
`examples/python/open_photogrammetry_format/main.py --no-frames`,
**fully reset viewer** and then hiding **NOT REMOVING** the points (the
world/pcl entity) and deselecting them.
(The bold marked pieces are very important as they have an influence on
what heuristics run - see #3077)

Before:

![image](https://github.com/rerun-io/rerun/assets/1220815/cda9d555-8839-4d78-8507-cf4ed34bad70)

After:

![image](https://github.com/rerun-io/rerun/assets/1220815/d34a00f8-05af-4160-85bf-b709a0d36ecb)


(release runs, averaged over a bunch of frames on my M1 Max)

Highlights:
* `AppState::show`: 24.0ms ➡️ 14.6ms 
   * `SpaceViewBlueprint::scene_ui`: 4.9ms ➡️ 2.6ms
      * this was the original primary optimization target!
   * `Viewport::on_frame_start`: 15.2ms ➡️ 7.4ms
       * `default_created_space_view`: 12.7ms ➡️ 5.1ms
* quite some time spent on this, but we want to do a paradigm shift
there: #3079
           
(slight number discrepancies from the screenshot are due to doing a
different run)

----
Draft todo: Code sanity check. Suspecting some heuristics might be
slightly broken, need to go through examples


### 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 [demo.rerun.io](https://demo.rerun.io/pr/3078) (if
applicable)

- [PR Build Summary](https://build.rerun.io/pr/3078)
- [Docs
preview](https://rerun.io/preview/fc5bb0b8908e2e8c666a6d5daa1dee105fe2aae8/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/fc5bb0b8908e2e8c666a6d5daa1dee105fe2aae8/examples)
<!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW--><!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)

---------

Co-authored-by: Jeremy Leibs <[email protected]>
@Wumpf Wumpf closed this as completed in 15cd60d Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 📉 performance Optimization, memory use, etc 📺 re_viewer affects re_viewer itself
Projects
None yet
1 participant