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.

Sign up for GitHub

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

Jump to bottom

static=True image logging still stores all the data #7404

Closed
nisseknudsen opened this issue Sep 11, 2024 · 2 comments · Fixed by #7525
Closed

static=True image logging still stores all the data #7404

nisseknudsen opened this issue Sep 11, 2024 · 2 comments · Fixed by #7525
Assignees
@teh-cmc
Labels
🪳 bug Something isn't working 🚀 performance Optimization, memory use, etc ⛃ re_datastore affects the datastore itself user-request This is a pressing issue for one of our users
Milestone
0.19 - Dataframe ...

Comments

@nisseknudsen
Copy link

Describe the bug
I am using rr.log(..., static=True) with images and the expectation that the memory consumption should stay stable over time. I only want to see the latest logged image.
I also have the expectation that when opening the (web) viewer, I do not have to watch through all of the history to the current frame.
Both is not met for some reason.

To Reproduce
Script to repro (uses webcam 0 on macOS, but should work with any computer with we webcam. Maybe try /dev/video0 for ubuntu).
scratch_5.py.zip

Expected behavior
Sidebar data source size stays stable and whatever a single JPEG image takes, let's say 20mbyte to be generous.
Unfortunately, it doesn't

image

Desktop (please complete the following information):

  • OS: macOS Sonoma 14.6.1

Rerun version
Tested on 18.0 and 18.2 with same unexpected outcomes.

Additional context
Discord post: https://discord.com/channels/1062300748202921994/1075873257124810852/1283124563902992537
@nisseknudsen nisseknudsen added 👀 needs triage This issue needs to be triaged by the Rerun team 🪳 bug Something isn't working labels Sep 11, 2024
@Wumpf Wumpf added this to the 0.19 - Dataframe and web video milestone Sep 12, 2024
@Wumpf Wumpf added ⛃ re_datastore affects the datastore itself user-request This is a pressing issue for one of our users and removed 👀 needs triage This issue needs to be triaged by the Rerun team labels Sep 12, 2024
@emilk emilk added the 🚀 performance Optimization, memory use, etc label Sep 12, 2024
@teh-cmc teh-cmc self-assigned this Sep 23, 2024
@teh-cmc
Copy link
Member

teh-cmc commented Sep 23, 2024

There are two issues at play:

  1. Static chunks are not subject to the dangling detection/cleanup logic of the garbage collector, since static data as a whole is immune to garbage collection.
  2. The decoded image cache doesn't subscribe to store events, and therefore continues caching stuff that has long been overwritten.

The fix for 1) is to properly account for static overwrites on the write path: patch all dangling references, decreases stats, fire events.
That should be a small amount of (likely tricky) code.

The fix for 2) is to set up a store subscriber so that the image cache drops overwritten data immediately.
That can range from trivial to extremely painful depending on how things are setup at the moment; I need to have a closer look.

@teh-cmc teh-cmc mentioned this issue Sep 25, 2024
Merged
6 tasks
teh-cmc added a commit that referenced this issue Sep 26, 2024
@teh-cmc
Caches per recording (#7513)
0034e6d 
Moves `Caches` from `AppState` to the `StoreHub`, and make them
per-recording instead of viewer-wide in the process.

This is:
* a pre-requisite to fix the repeated static logging issues (#7404)
* a necessary step towards proper secondary caching
* a fix for a very old issue: cache memory is not reclaimed when closing
a recording

I'm not a fan of the implementation: it's brittle, confusing, and I
don't like it. Still, it's a net improvement over the status quo, and
fixes a very annoying "leak" for end users.

Before:


https://github.com/user-attachments/assets/a54c1923-a939-4700-a485-eeb2a2dd4966



After:


https://github.com/user-attachments/assets/dd5adb65-d328-474c-bc31-e65a59332f03



---

* Part of #7404
@teh-cmc teh-cmc mentioned this issue Sep 26, 2024
Merged
6 tasks
teh-cmc added a commit that referenced this issue Sep 26, 2024
@teh-cmc
Automatic removal of unreachable static chunks (#7518)
ca44717 
Unreachable static chunks are dead weight: there exists no query that
can access their data (at least when using Rerun as a Visualizer).

By automatically removing dangling chunks, we make it possible for user
to use the Rerun Viewer as a soft-realtime telemetry system (provided we
properly invalidate our caches too, which is the subject of an upcoming
PR).

This raises the question of what should happen when using Rerun as a
database: should this data be kept and made accessible?
If so, this behavior should probably be made configurable (e.g. when
instantiating a ChunkStore in the SDK).

* Part of #7404 

---

Test:
```python
from pathlib import Path

import rerun as rr

image_file_path = Path(__file__).parent / "ferris.png"

rr.init("rerun_example_encoded_image", spawn=True)

for _ in range(0, 10):
    rr.log("image", rr.EncodedImage(path=image_file_path), static=True)
```

Command:
```
RERUN_FLUSH_NUM_ROWS=0 python test.py
```

Before:

![image](https://github.com/user-attachments/assets/a9823055-5247-4d63-9295-fc310aa4923f)

After:

![image](https://github.com/user-attachments/assets/c095fda2-02ac-4456-bf3f-e251b24dc75d)
@teh-cmc teh-cmc mentioned this issue Sep 26, 2024
Merged
6 tasks
@nisseknudsen
Copy link
Author

Thank you for the fix! Can confirm it works now 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@teh-cmc teh-cmc

Labels
🪳 bug Something isn't working 🚀 performance Optimization, memory use, etc ⛃ re_datastore affects the datastore itself user-request This is a pressing issue for one of our users
Projects
None yet
Milestone
0.19 - Dataframe and video
Development

Successfully merging a pull request may close this issue.

Invalidate hub-wide caches on deletions and overwrites rerun-io/rerun
4 participants
@emilk @Wumpf @nisseknudsen @teh-cmc