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

fix: Remove static view caching to improve memory management #548

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

robmaceachern
Copy link
Member

@robmaceachern robmaceachern commented Dec 12, 2024

  • Remove static caches from ListProperties
  • Remove unused static cache vars

The static ReusableViewCaches used in ListProperties would cause views to be retained beyond the lifetime of the owning list and if the views themselves referenced other objects (directly or through closures) then large parts of application state could be inadvertently "leaked" for the lifetime of the application.

This change instantiates new caches every time PresentationState is instantiated. There is a measuring performance impact since each reuse identifier will need to be recreated once per layout. On a long homogenous list that probably isn't a big deal, but for a heterogenous list it could be more of an issue (in the extreme case of a list composed of unique cells for every row this caching would provide no benefit). I haven't done any formal profiling on this.

Ideally we'd be able to associate the caches to the list views themselves so that their lifetimes were bound together. That would help avoid many of the memory issues we've seen while improving the cache effectiveness.

Checklist

Please do the following before merging:

  • Ensure any public-facing changes are reflected in the changelog. Include them in the Main section.

@robmaceachern robmaceachern marked this pull request as ready for review December 12, 2024 17:30
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.

1 participant