Skip to content

Commit 01127bc

Browse files
committed
Fix cache invalidation during playback
Remove blueprint_generation from VisualizableEntitiesCacheKey to prevent cache invalidation on every frame during playback. Visualizable entities only depend on recording data and view configuration, not the blueprint time cursor which updates continuously during playback.
1 parent 129d04e commit 01127bc

File tree

2 files changed

+133
-4
lines changed

2 files changed

+133
-4
lines changed

BLUEPRINT_GEN.md

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
# Blueprint Generation Cache Invalidation Issue
2+
3+
## Investigation Summary
4+
5+
This document describes the blueprint generation issue identified in PR #11743 review by @Wumpf.
6+
7+
## Root Cause
8+
9+
In `crates/viewer/re_viewer_context/src/time_control.rs:48-54`, during playback the time cursor is continuously written to the blueprint:
10+
11+
```rust
12+
fn set_time(&self, time: impl Into<TimeInt>) {
13+
self.save_static_blueprint_component(
14+
time_panel_blueprint_entity_path(),
15+
&TimePanelBlueprint::descriptor_time(),
16+
&re_types::blueprint::components::TimeInt(time.as_i64().into()),
17+
);
18+
}
19+
```
20+
21+
Every time this writes to the blueprint store, it increments the `blueprint_generation`. Since both cache keys in `app_state.rs` include `blueprint_generation`:
22+
23+
```rust
24+
// Line 185-189 in app_state.rs
25+
struct VisualizableEntitiesCacheKey {
26+
recording_generation: re_chunk_store::ChunkStoreGeneration,
27+
blueprint_generation: re_chunk_store::ChunkStoreGeneration,
28+
space_origin: EntityPath,
29+
}
30+
31+
// Line 159-163 in app_state.rs
32+
struct ViewQueryCacheKey {
33+
recording_generation: re_chunk_store::ChunkStoreGeneration,
34+
blueprint_generation: re_chunk_store::ChunkStoreGeneration,
35+
blueprint_query: LatestAtQuery,
36+
}
37+
```
38+
39+
**The cache is invalidated every frame during playback**, making it only effective when the viewer is paused.
40+
41+
## Impact
42+
43+
As @Wumpf noted in the review:
44+
> "The blueprint generation now changes continuously when playing due to the time cursor being stored there, making this cache a lot less effective and really only useful for a standstill viewer."
45+
46+
This significantly reduces the performance benefits of the caching optimization.
47+
48+
## Proposed Solutions
49+
50+
### Option 1: Remove blueprint_generation from VisualizableEntitiesCacheKey ⭐ RECOMMENDED
51+
52+
**Complexity:** Simple
53+
54+
**Rationale:**
55+
- Visualizable entities only depend on:
56+
- Recording data (which entities exist and what components they have)
57+
- View configuration (space_origin, view class)
58+
- **NOT** on the blueprint time cursor
59+
- We can safely remove `blueprint_generation` from `VisualizableEntitiesCacheKey`
60+
- Keep `recording_generation` since new entities being added should invalidate the cache
61+
- This would make the visualizable entities cache effective during playback
62+
63+
**Changes required:**
64+
```rust
65+
// app_state.rs:185-189
66+
struct VisualizableEntitiesCacheKey {
67+
recording_generation: re_chunk_store::ChunkStoreGeneration,
68+
// Remove: blueprint_generation: re_chunk_store::ChunkStoreGeneration,
69+
space_origin: EntityPath,
70+
}
71+
```
72+
73+
### Option 2: Make blueprint_query comparison sufficient for ViewQueryCacheKey
74+
75+
**Complexity:** Moderate
76+
77+
**Rationale:**
78+
- The `ViewQueryCacheKey` already includes `blueprint_query` which captures the actual time
79+
- We could remove `blueprint_generation` from `ViewQueryCacheKey` and rely solely on the query comparison
80+
- However, this might miss other blueprint changes (view contents, property overrides)
81+
82+
**Concerns:**
83+
- Query results may depend on blueprint property overrides that could change
84+
- Need to verify that `blueprint_query` comparison catches all relevant changes
85+
86+
### Option 3: Split blueprint generation into "structural" vs "state" changes
87+
88+
**Complexity:** High
89+
90+
**Rationale:**
91+
- Add a separate generation counter for structural blueprint changes (views, containers, space origins)
92+
- Keep time cursor changes separate from structural changes
93+
- This would require changes to the chunk store architecture
94+
95+
**Changes required:**
96+
- Modify `re_chunk_store` to support multiple generation counters
97+
- Categorize blueprint writes into "structural" vs "state"
98+
- Update all cache keys to use the appropriate generation counter
99+
100+
### Option 4: Track specific blueprint entities that affect each cache
101+
102+
**Complexity:** Very High
103+
104+
**Rationale:**
105+
- Instead of using global generation, track which specific blueprint entity paths affect each cache
106+
- More precise invalidation
107+
- Significantly more complex to implement and maintain
108+
109+
**Changes required:**
110+
- Design a dependency tracking system
111+
- Update all caches to register their dependencies
112+
- Monitor blueprint changes and invalidate only affected caches
113+
114+
## Recommendation
115+
116+
**Start with Option 1** for `VisualizableEntitiesCacheKey`. This is the safest and simplest fix:
117+
- ✅ Visualizable entities truly don't depend on blueprint generation
118+
- ✅ The recording generation is sufficient to invalidate when new entities arrive
119+
- ✅ This alone would restore cache effectiveness during playback for the most expensive operation
120+
- ✅ Low risk of introducing bugs
121+
- ✅ Easy to implement and test
122+
123+
For `ViewQueryCacheKey`, we need to be more careful since query results might depend on blueprint property overrides that could change. We could:
124+
- Keep it as-is for now, OR
125+
- Make it smarter by only including the blueprint_generation when we detect structural changes
126+
127+
## Next Steps
128+
129+
1. Implement Option 1 to fix `VisualizableEntitiesCacheKey`
130+
2. Test with playback to verify cache effectiveness
131+
3. Profile to measure performance improvement
132+
4. Evaluate whether `ViewQueryCacheKey` needs similar treatment based on profiling results
133+
5. Consider Option 3 as a longer-term architectural improvement if needed

crates/viewer/re_viewer/src/app_state.rs

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -187,19 +187,16 @@ struct VisualizableEntitiesCacheEntry {
187187
#[derive(Clone, PartialEq, Eq, Hash)]
188188
struct VisualizableEntitiesCacheKey {
189189
recording_generation: re_chunk_store::ChunkStoreGeneration,
190-
blueprint_generation: re_chunk_store::ChunkStoreGeneration,
191190
space_origin: EntityPath,
192191
}
193192

194193
impl VisualizableEntitiesCacheKey {
195194
fn new(
196195
recording_generation: re_chunk_store::ChunkStoreGeneration,
197-
blueprint_generation: re_chunk_store::ChunkStoreGeneration,
198196
space_origin: &EntityPath,
199197
) -> Self {
200198
Self {
201199
recording_generation,
202-
blueprint_generation,
203200
space_origin: space_origin.clone(),
204201
}
205202
}
@@ -372,7 +369,6 @@ impl AppState {
372369
.map(|view| {
373370
let cache_key = VisualizableEntitiesCacheKey::new(
374371
recording_generation.clone(),
375-
blueprint_generation.clone(),
376372
&view.space_origin,
377373
);
378374

0 commit comments

Comments
 (0)