Time panel chunkification by jprochazk · Pull Request #6934 · rerun-io/rerun

jprochazk · 2024-07-18T09:21:58Z

What

This PR removes all usage of the recursive time histograms tracked in EntityTree. The entity tree now only represents the... tree of entities, and no other information about them. All required information is queried from the database on demand.

Closes Optimize TimeHistogram #6065

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):
- Using examples from latest main build: rerun.io/viewer
- Using full set of examples from nightly build: rerun.io/viewer
The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
If applicable, add a new check to the release checklist!
If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide

To run all checks from main, comment on the PR with @rerun-bot full-check.

crates/store/re_entity_db/src/time_histogram_per_timeline.rs

This reverts commit 0534aa0.

jprochazk · 2024-07-19T11:50:56Z

entity and subtree have been erased from existence, and everything seems to still be working. I just want to confirm that the memory usage has indeed gone down.

jprochazk · 2024-07-19T11:58:09Z

It... has not? It seems to be the exact same as main, which is weird

jprochazk · 2024-07-19T14:01:40Z

Okay, according to others, the memory usage is not supposed to change all that much. What should improve is ingestion speed.

teh-cmc · 2024-07-19T14:15:32Z

Okay, according to others, the memory usage is not supposed to change all that much. What should improve is ingestion speed.

There are many many things that slow down ingestion right now on main, so to be more specific: what should be faster is this part of the frame (adding chunks to the store and running subscribers).

teh-cmc · 2024-07-19T14:20:12Z

I've just tried it and I can already tell you right now that your branch is a good order of magnitude faster :D Nice!

jprochazk · 2024-07-22T09:44:24Z

In terms of testing, as I was working on this I would manually add logs to the places with code changes just to make sure that those code paths are actually being hit and are still doing the right thing. The logs would print both the old and new values, so I could compare them.

I think there should be no observable changes between this branch and main, maybe only in the byte size of the entity tree in the time panel, so it's hard to say exactly what should be tested, but a good start would be opening up various examples and checking them against the latest web viewer build from main: https://rerun.io/viewer/commit/ddf60c4

teh-cmc

Works great.

Now we have to make our semantics consistent across the board.

crates/store/re_chunk_store/src/query.rs

teh-cmc · 2024-07-22T13:29:42Z

crates/store/re_entity_db/src/entity_db.rs

-        query_caches.on_events(store_events);
-
-        tree.on_store_deletions(store_events);
+        // Tree deletions depend on data store, so data store must have been notified of deletions already.


I'm confused by this comment -- the data store is the thing that emitted the changelog in the first place, how could it not know about what's in it?

Yes, the problem here is that the store events are not actually being used by the tree anymore, so there's no direct dependency between them, which is inconsistent with the rest of the function, where everything else uses the store events. Just wanted to make that as clear as possible

I guess I can just add the param back and then not use it

teh-cmc · 2024-07-22T13:34:03Z

crates/store/re_entity_db/src/entity_tree.rs

+    /// Returns `false` if this entity has no children and no data.
+    pub fn is_empty(&self, chunk_store: &ChunkStore) -> bool {
+        self.children.is_empty()
+            && !chunk_store.entity_has_any_component_on_any_timeline(&self.path)


Okay I'm starting to see where the ambiguities stem from.

What do you really want to know here: whether the entity has some component associated with it (which, once true, will always be true -- no matter of what the GC ends up removing), or whether the entity has some events available in the store (which can be true at some point but then false at a later time because GC)?

This method should make it clear which one of these is at play here.

We need to make that clear all over the place in all these store APIs and their docs, in fact.

If I look at it from the perspective of the time panel, it should not display any entities which do not actually have any visible data (either static or temporal). The way I understand it, that's how it worked previously:

https://github.com/rerun-io/rerun/pull/6934/files#diff-34bc602a4376f90828813d30cf2e1ae5f940f9db3d3b51196c383b2af2cdca14L335

The component was removed from that entity in the entity tree if the histogram was empty, which means the time panel would no longer show that component, and if it had no components and no children left, it would be removed completely.

I'll properly name all the APIs with data or component depending on what they're looking for, and make the distinction clear in their docs.

crates/store/re_entity_db/src/entity_tree.rs

teh-cmc · 2024-07-22T13:39:13Z

crates/viewer/re_data_ui/src/component.rs

Ha-ha! Now I see why you need those.

Yeah, it's just one specific place where we give the user some more information about what they're doing wrong

teh-cmc

Fantastic

crates/store/re_chunk_store/src/query.rs

Co-authored-by: Clement Rey <cr.rey.clement@gmail.com>

Regression introduced in #6934 and caught by `check_static_components_ui` Before: ![image](https://github.com/user-attachments/assets/b06bea44-ae51-4989-a3a2-eb4f4a7e1bc0) After: ![image](https://github.com/user-attachments/assets/1250995a-6ae8-42ad-90c9-aee42234ff4f)

remove old data density graph ui

8404179

jprochazk added 📺 re_viewer affects re_viewer itself include in changelog labels Jul 18, 2024

jprochazk changed the title ~~Finish time panel chunkification~~ Time panel chunkification Jul 18, 2024

jprochazk and others added 2 commits July 19, 2024 01:09

wip

0534aa0

Merge branch 'main' into jan/time-panel-chunkification

bb4e116

emilk reviewed Jul 19, 2024

View reviewed changes

crates/store/re_entity_db/src/time_histogram_per_timeline.rs Show resolved Hide resolved

jprochazk added 17 commits July 19, 2024 10:28

Revert "wip"

e3a252b

This reverts commit 0534aa0.

add new APIs

e823464

update data UI to not use EntityInfo

4bd129c

update blueprint to not use EntityInfo

81f33eb

use entity subtree size in entity_tree_stats_ui

dae84a8

update time UI to not use SubtreeInfo

fda44be

remove time histogram usage from component data ui

71bad34

remove EntityInfo usage from selection panel

339c66f

remove EntityInfo usage from 2d/3d space views

bcc92d4

actually chunkify time panel

28a130d

rm dead code

a186a00

remove time histogram usage from EntityLatestAtResults data ui

9293509

remove EntityInfo from entity tree visitors

8e8560e

remove some dead code

a19cd6b

add query method

c27b0b2

remove EntityTree.entity

5753834

remove SubtreeInfo

b5a140e

Merge branch 'main' into jan/time-panel-chunkification

6322bba

btreemap is sorted by keys

b153073

teh-cmc suggested changes Jul 22, 2024

View reviewed changes

jprochazk and others added 15 commits July 22, 2024 16:05

Merge branch 'main' into jan/time-panel-chunkification

292989f

rename all_components* queries

b96a5e0

more renamings

2aebfd0

invert

e702215

refactor component/data query APIs

7e6b5cc

use u64 everywhere

e7e57ea

more static/temporal semantics mess

0d88df1

refactor into combinators

1bf8ade

clarify temporal/static in num_events APIs

5ffb090

rationale

0291f80

specify units

e3083b6

clarify size_of_entity_on_timeline

4082b9c

typo

5d21b1c

use u64 for entity size

5ef30e9

add back unused param

b2001e5

jprochazk requested a review from teh-cmc July 22, 2024 16:39

teh-cmc approved these changes Jul 23, 2024

View reviewed changes

jprochazk and others added 6 commits July 23, 2024 09:54

Update crates/store/re_chunk_store/src/query.rs

eb98e81

Co-authored-by: Clement Rey <cr.rey.clement@gmail.com>

Update crates/store/re_chunk_store/src/query.rs

9eea627

Co-authored-by: Clement Rey <cr.rey.clement@gmail.com>

Update crates/store/re_chunk_store/src/query.rs

12dc339

Co-authored-by: Clement Rey <cr.rey.clement@gmail.com>

Update crates/store/re_chunk_store/src/query.rs

c95c182

Co-authored-by: Clement Rey <cr.rey.clement@gmail.com>

docs

82127ba

Merge branch 'main' into jan/time-panel-chunkification

63c806b

jprochazk merged commit f7d9e1c into main Jul 23, 2024

jprochazk deleted the jan/time-panel-chunkification branch July 23, 2024 08:13

abey79 mentioned this pull request Dec 17, 2024

Fix the event count in the static-over-temporal error message #8513

Merged

Conversation

jprochazk commented Jul 18, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Checklist

Uh oh!

Uh oh!

jprochazk commented Jul 19, 2024

Uh oh!

jprochazk commented Jul 19, 2024

Uh oh!

jprochazk commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teh-cmc commented Jul 19, 2024

Uh oh!

teh-cmc commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jprochazk commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teh-cmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

teh-cmc Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

jprochazk Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

jprochazk Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

teh-cmc Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

jprochazk Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

teh-cmc Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

jprochazk Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

teh-cmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jprochazk commented Jul 18, 2024 •

edited by github-actions bot

Loading

jprochazk commented Jul 19, 2024 •

edited

Loading

teh-cmc commented Jul 19, 2024 •

edited

Loading

jprochazk commented Jul 22, 2024 •

edited

Loading

jprochazk Jul 22, 2024 •

edited

Loading