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

Removed space views are not GC'd from the EntityTree #6521

Closed
jleibs opened this issue Jun 7, 2024 · 4 comments · Fixed by #6534
Closed

Removed space views are not GC'd from the EntityTree #6521

jleibs opened this issue Jun 7, 2024 · 4 comments · Fixed by #6534
Assignees
@jleibs
Labels
🟦 blueprint The data that defines our UI 🦟 regression A thing that used to work in an earlier release
Milestone
0.17

Comments

@jleibs
Copy link
Member

jleibs commented Jun 7, 2024 •
edited
Loading

Removing a space view causes the viewer to start spewing spam:

[2024-06-07T22:22:25Z ERROR re_viewport_blueprint::space_view] View blueprint at SpaceViewId(8403bae3-7776-4b3c-8052-099161ac6a8c) is lacking the required `SpaceViewClass` component.
[2024-06-07T22:22:25Z ERROR re_viewport_blueprint::space_view] View blueprint at SpaceViewId(8403bae3-7776-4b3c-8052-099161ac6a8c) is lacking the required `SpaceViewClass` component.
[2024-06-07T22:22:25Z ERROR re_viewport_blueprint::space_view] View blueprint at SpaceViewId(8403bae3-7776-4b3c-8052-099161ac6a8c) is lacking the required `SpaceViewClass` component.

It appears we regressed on some aspect of fully purging the blueprint from the store when we clear it.

After GC runs, the blueprint inspector shows all the data is gone, but the entity tree entries persist.
@jleibs jleibs added 🦟 regression A thing that used to work in an earlier release 🟦 blueprint The data that defines our UI labels Jun 7, 2024
@jleibs jleibs added this to the 0.17 milestone Jun 7, 2024
@jleibs
Copy link
Member Author

jleibs commented Jun 7, 2024

Looks like this is a combination of two issues:

  • Remove to archetype & archetype queries #6501 has introduced an error when we query a space-view that's been cleared before it's been garbage collected
  • An unknown PR seems to have broken the GC behavior that would have cleaned this up after being cleared

@jleibs jleibs changed the title Cannot remove space views Removed space views are not GC'd from the EntityTree Jun 7, 2024
@jleibs
Copy link
Member Author

jleibs commented Jun 8, 2024

It looks like as part of:

@jleibs jleibs mentioned this issue Jun 8, 2024
Merged
5 tasks
@jleibs
Copy link
Member Author

jleibs commented Jun 8, 2024 •
edited
Loading

Even after adding and fixing the test: #6522 we still see the same behavior. Maybe there's static data leaking into the blueprint?

jleibs added a commit that referenced this issue Jun 10, 2024
@jleibs
Restore the clear test for the GC behavior blueprint depends on (#6522)
e2cb396 
### What
- Motivated by: #6521

This test was inadvertently removed here:
https://github.com/rerun-io/rerun/pull/5535/files#r1631789722

The test was previously (incorrectly) testing for timeless clears even
though the blueprint had been updated to use non-timeless / static data.
This updates it to use a blueprint-representative timepoint as well.

### 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 the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6522?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6522?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/6522)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
@jleibs jleibs mentioned this issue Jun 10, 2024
Merged
1 task
@jleibs
Copy link
Member Author

jleibs commented Jun 10, 2024

Turns out this is because the python-logged blueprint components include log_time and log_tick timelines, but the clear-event that we generate doesn't clear on those timelines, which means the data is still implicitly protected.

@jleibs jleibs mentioned this issue Jun 10, 2024
Merged
5 tasks
@jleibs jleibs self-assigned this Jun 11, 2024
jleibs added a commit that referenced this issue Jun 11, 2024
@jleibs
Opt-out of GC protection for log_seq and log_time when GCing blue…
63952fe 
…prints (#6534)

### What
- Resolves: #6521

We only care about the blueprint timeline when working with blueprints.
Since we only log clears to the blueprint timeline, we need to avoid
letting the default timelines lead to data-protection we don't otherwise
want.

### 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 the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6534?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/6534?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/6534)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jleibs jleibs

Labels
🟦 blueprint The data that defines our UI 🦟 regression A thing that used to work in an earlier release
Projects
None yet
Milestone
0.17
Development

Successfully merging a pull request may close this issue.

Opt-out of GC protection for log_seq and log_time when GCing blueprints rerun-io/rerun
1 participant
@jleibs