Skip to content

fix: memory leaks & WeakRefs #111

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 13 commits into from
Jul 3, 2021
Merged

fix: memory leaks & WeakRefs #111

merged 13 commits into from
Jul 3, 2021

Conversation

rigor789
Copy link
Member

@rigor789 rigor789 commented Apr 9, 2021

WIP

@cla-bot cla-bot bot added the cla: yes label Apr 9, 2021
@brysem
Copy link

brysem commented Apr 26, 2021

Would it be possible to get a RC release for testing?

@NathanaelA

This comment was marked as abuse.

@rigor789
Copy link
Member Author

rigor789 commented May 5, 2021

It's the strdup change, it's excluded from the alpha build, just haven't found a good solution for it's leak yet...

@NathanaelA

This comment was marked as abuse.

@NathanaelA

This comment was marked as abuse.

@rigor789
Copy link
Member Author

rigor789 commented May 6, 2021

So the alpha has all these changes except the strdup change, I assume you are talking about this strdup change?
0f96dc4#diff-bec6e3d6b4fa0eacfb7726d6e5915c9eecda4e319bc61f71a0527e740eb197ddR160

Yeah, that strdup needs to be present (or something that makes a copy of the data, as once it leaves the scope it looks like the value is freed).

That one is a tricky one - the data in some cases does persist fine without the strdup call, and is released once no longer needed, but sometimes that data is released too early. Having that strdup there does leak all the memory though, since it's never released afterwards. This is a big issue for apps that rely heavily on SQLite, since as far as I understand - all the queries are going through that exact branch - for most apps, it's probably not a huge deal if it only leaks a few times (still bad, but tolerable).

But yes, the latest alpha has that one change reverted before build - because it does cause the failure you are seeing in some cases (garbage passed instead of the right value, because it's freed too early).

@darind suggested we try making the arguments into Persistent values and then add them to a global cache where we can get notified when the argument is eligible for releasing, but I couldn't test that yet (won't be able to for the next 2 weeks).

The fastest way to debug/step through and reach that branch is to call

NSMethodSignature.signatureWithObjCTypes('v@:c');

if you want to experiment with that idea.


WeakRefs seem to take a LOT longer to be collected

v8's WeakRefs make no guarantees when they are actually released or GC'd - with this PR, we are switching to the built-in WeakRef implementation, so that change is expected...

@NathanaelA

This comment was marked as abuse.

@brysem
Copy link

brysem commented May 10, 2021

If you would like you can also try this version (a version I built for a different nStudio client)

Thanks. I will give it a try today.

As already mentioned by Igor the change on 0f96dc4#diff-bec6e3d6b4fa0eacfb7726d6e5915c9eecda4e319bc61f71a0527e740eb197ddR160 does indeed create a lot of weird issues for the nativescript-sqlite package. It also broke something in frame transitions from the core (if my memory serves me right).

@cla-bot
Copy link

cla-bot bot commented Jun 10, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign the CLA at https://www.nativescript.org/cla.
CLA has not been signed by users: @darind, @brysem, @Logikgate.
After signing the CLA, you can ask me to recheck this PR by posting @cla-bot check as a comment to the PR.

@cla-bot cla-bot bot removed the cla: yes label Jun 10, 2021
@NathanWalker NathanWalker changed the base branch from master to release/8-1 July 3, 2021 17:33
@NathanWalker NathanWalker marked this pull request as ready for review July 3, 2021 17:54
@NathanWalker NathanWalker merged commit 431e816 into release/8-1 Jul 3, 2021
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.

6 participants