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

fix: memory leak issue #1056

Merged
merged 3 commits into from
Jul 21, 2023
Merged

fix: memory leak issue #1056

merged 3 commits into from
Jul 21, 2023

Conversation

TheRogue76
Copy link
Collaborator

@TheRogue76 TheRogue76 commented Jul 21, 2023 •
edited

As per our discussion, This PR does a bunch of refactoring to both:

  1. Simplify the logic of setSourceURL with a guard
  2. Break the reference cycle (only needed for the internal Dispatch back to main because the first one doesn't capture self anyway)
  3. Break the reference cycle for the callback

One area we can improve is with moving the call to decode Json in setSourceJson to a background thread, but it isn't a memory leak so much as "Kind of a slow op" so not in scope for this PR

I also made the completion callback a computed property so it doesn't recreate a new callback every time, just one, and while there was a small memory improvement, it was minuscule, so i am more than willing to roll it back

Picture of instruments showing a release:
Screenshot 2023-07-21 at 11 59 26

@TheRogue76
fix: refactor setSourceURL to not have strong reference cycles, refac…
bcf416b 
…tored a bit, and also broke reference cycle in callback
@matinzd matinzd changed the base branch from master to fix/ios_memory_leak July 21, 2023 09:56
@TheRogue76 TheRogue76 marked this pull request as ready for review July 21, 2023 10:25
matinzd
matinzd approved these changes Jul 21, 2023
View reviewed changes
Copy link
Collaborator

@matinzd matinzd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@matinzd matinzd merged commit 5d46594 into lottie-react-native:fix/ios_memory_leak Jul 21, 2023
emilioicai pushed a commit that referenced this pull request Jul 21, 2023
@matinzd @TheRogue76
fix: memory leak on iOS on deallocation (#1055)
076de43 
* fix: memory leak on iOS on deallocation

Fixes: #1010

* fix: add weak reference in setSourceURL

* fix: memory leak issue (#1056)

---------

Co-authored-by: Parsa Nasirimehr <40071952+TheRogue76@users.noreply.github.com>
@TheRogue76 TheRogue76 deleted the feature/fixMemoryLeak branch December 19, 2023 20:50
@TheRogue76 TheRogue76 mentioned this pull request Jan 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matinzd matinzd matinzd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@TheRogue76 @matinzd