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

Caches per recording #7513

Merged
merged 1 commit into from
Sep 26, 2024
Merged

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Sep 25, 2024

Moves Caches from AppState to the StoreHub, and make them per-recording instead of viewer-wide in the process.

This is:

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:

24-09-25_17.29.57.patched.mp4

After:

24-09-25_17.29.08.patched.mp4

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
  • 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.

@teh-cmc teh-cmc added 📉 performance Optimization, memory use, etc 🚜 refactor Change the code, not the functionality include in changelog labels Sep 25, 2024
@teh-cmc teh-cmc requested a review from Wumpf September 25, 2024 15:30
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Makes sense!

@Wumpf
Copy link
Member

Wumpf commented Sep 25, 2024

Should also fix

but should probably check first

@teh-cmc
Copy link
Member Author

teh-cmc commented Sep 26, 2024

Should also fix

* [GPU memory not freed when closing recording, but keeping viewer open #6755](https://github.com/rerun-io/rerun/issues/6755)

but should probably check first

I checked and it's definitely not the case: Counted GPU barely budges when closing the recording. 😞

@teh-cmc teh-cmc merged commit 0034e6d into main Sep 26, 2024
37 of 38 checks passed
@teh-cmc teh-cmc deleted the cmc/viewer_cache_cleanup_0_move_to_bundle branch September 26, 2024 07:01
@teh-cmc
Copy link
Member Author

teh-cmc commented Sep 26, 2024

(Although maybe the problem is not the GPU memory itself, but the reporting done by Counted GPU, e.g. stats don't get properly updated (or downdated, rather) ? I don't know how Counted GPU is fed.)

@Wumpf
Copy link
Member

Wumpf commented Sep 26, 2024

Counted GPU is fed by what re_renderer holds on to, so yeah something goes sideways there.
I think the caches are not properly cleaning up - the "mesh manager" on re_renderer is probably in a sorry state too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog 📉 performance Optimization, memory use, etc 🚜 refactor Change the code, not the functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants