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 memory leaks #6889

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

YannisJustine
Copy link
Contributor

@YannisJustine YannisJustine commented Nov 28, 2024

Fixes #6715

Changes

  • Implemented event callback cleanup on component unmounting
  • Enhanced memory management through lifecycle hooks
  • Maintained existing callbacks until full cleanup to preserve potential intentional behavior

Testing Methodology

  1. Navigated through all component pages sequentially
  2. Monitored memory usage patterns
  3. Performed manual garbage collection after tests to ensure complete cleanup

Performance Impact

  • Reduced memory footprint:
    Before:
    image
    After:
    image

Implementation Note

Chose to implement cleanup on unmount as a conservative approach since the requirements weren't explicit about element persistence. I kept the existing callbacks until they are all removed, as I wasn't sure if their accumulation was intentional. This ensures cleaner memory management while maintaining component integrity.


I hope this helps resolve the bug. I'm open to revising the PR based on your feedback and requirements.

Copy link

vercel bot commented Nov 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Updated (UTC)
primevue ⬜️ Ignored (Inspect) Visit Preview Dec 11, 2024 7:40am
primevue-v3 ⬜️ Ignored (Inspect) Visit Preview Dec 11, 2024 7:40am

@YannisJustine YannisJustine changed the title Fix/issue 6715 Fix memory leaks Nov 28, 2024
@tugcekucukoglu
Copy link
Member

Are _themesCallback and _configCallback sets necessary here?

@YannisJustine
Copy link
Contributor Author

Are _themesCallback and _configCallback sets necessary here?

No, they're not necessary. I didn't know if the accumulation of callbacks was intentional or not.
I removed them in the last commit.

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.

All components: Memory leak
2 participants