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 Flipper Ribtree Plugin memory leak #630

Merged

Conversation

mamykin-andrey
Copy link
Contributor

Description:
Fix for #499, the problem was in storing router objects in RibEventPayload, even though they weren't required for correct plugin work. Added a new class RouterInfo for storing router-related meta-information.

Related issue(s):
#499

@tyvsmith
Copy link
Member

Thanks for the fix!

However, it is mostly unrelated to the issue you mentioned #499. The intention of that task was just to add leak canary integration to the sample app mentioned. Would you mind creating a separate issue and linking that in the description so we have the right paper trail? Thanks!

For this PR, would you mind adding a bit more color on the leak itself, how you verified it was gone, and that the plugin is still working as expected?

@mamykin-andrey
Copy link
Contributor Author

Thanks for the fix!

However, it is mostly unrelated to the issue you mentioned #499. The intention of that task was just to add leak canary integration to the sample app mentioned. Would you mind creating a separate issue and linking that in the description so we have the right paper trail? Thanks!

Hey @tyvsmith! I thought the Compose sample-app is just an example of how to reproduce the memory leak.
If in that issue we were supposed to just add LeakCanary - I'd indeed create another issue for the memory leak and would change this issue title from "[Compose Sample] Flipper Ribtree Plugin produces an Interactor Memory Leak" to "[Compose Sample] Integrate LeakCanary", what do you think?

For this PR, would you mind adding a bit more color on the leak itself, how you verified it was gone, and that the plugin is still working as expected?

Sure! The problem was in storing Router instance in the events: *Subject inside of the Plugin, that's the reason why Compose sample app triggers the memory leak warning. Honestly I don't understand why it wasn't spotted before.
We're also storing Router objects in other places as well, but in a WeakHashMap AFAIR, so it shouldn't cause the same problem.
For the testing approach I just ran the Compose sample-app before and after my fixes and checked its RIB structure in the Flipper Plugin, it works the same way.

After splitting the issues I'll update my PR and also will describe the points you mentioned (details of the memory leak and testing steps), thanks for the feedback!

@tyvsmith
Copy link
Member

Hey @tyvsmith! I thought the Compose sample-app is just an example of how to reproduce the memory leak.
If in that issue we were supposed to just add LeakCanary - I'd indeed create another issue for the memory leak and would change this issue title from "[Compose Sample] Flipper Ribtree Plugin produces an Interactor Memory Leak" to "[Compose Sample] Integrate LeakCanary", what do you think?

Nevermind on my suggestion on a new issue. I went back and looked at my internal notes, and you are correct. I had built a version and found the leak and had misremembered the intent of that issue from apr last year. In reviewing further, I don't think we actually want to commit a version with leak canary in the compose sample app as generally the idea with sample apps is to keep them as targeted as possible for folks to understand specific use-cases without extra noise, and we have a separate leak canary example already. So will approve after you update the PR with leak details/verification you mentioned in your comment.

@tyvsmith tyvsmith merged commit 4cec42f into uber:main Mar 20, 2024
2 checks passed
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