Skip to content

Fixes memory leak in ViewStateCache. #718

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

Merged
merged 1 commit into from
Apr 12, 2022
Merged

Conversation

rjrjr
Copy link
Contributor

@rjrjr rjrjr commented Apr 8, 2022

ViewStateCache was Parcelable, in an attempt to simplify View.onSaveInstanceState() chores for client container views. That became a source of memory leaks once we added a WorkflowSavedStateRegistryAggregator to it, so we've dialed back the convenience.

ViewStateCache now offers save(): Saved and restore(saved: Saved), where Saved is a simple Parcelable. Client views are responsible for managing their own subclasses of View.BaseSavedState, which is sad but not terribly unusual.

`ViewStateCache` was `Parcelable`, in an attempt to simplify `View.onSaveInstanceState()` chores for client container views. That became a source of memory leaks once we added a `WorkflowSavedStateRegistryAggregator` to it, so we've dialed back the convenience.

`ViewStateCache` now offers `save(): Saved` and `restore(saved: Saved)`, where `Saved` is a simple `Parcelable`. Client views are responsible for managing their own subclasses of `View.BaseSavedState`, which is sad but not terribly unusual.
@rjrjr rjrjr force-pushed the ray/view-state-cache-leak-oops branch from 782d604 to c515971 Compare April 8, 2022 22:11
@rjrjr
Copy link
Contributor Author

rjrjr commented Apr 12, 2022

Before putting this PR up for review, I'm publishing it as 1.7.0-SNAPSHOT so that I can verify the leak fix.

@rjrjr
Copy link
Contributor Author

rjrjr commented Apr 12, 2022

Before putting this PR up for review, I'm publishing it as 1.7.0-SNAPSHOT so that I can verify the leak fix.

That was dumb, it got stomped immediately. Making 1.7.0-rjrjr-SNAPSHOT this time.

@rjrjr rjrjr marked this pull request as ready for review April 12, 2022 21:48
@rjrjr rjrjr requested review from a team and zach-klippenstein as code owners April 12, 2022 21:49
@rjrjr rjrjr merged commit cd7bd1b into main Apr 12, 2022
@rjrjr rjrjr deleted the ray/view-state-cache-leak-oops branch April 12, 2022 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants