Skip to content

fix: Blocks memory leak (#100) #110

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

Closed
wants to merge 1 commit into from
Closed

Conversation

darind
Copy link
Collaborator

@darind darind commented Apr 8, 2021

Related to #100

@darind darind requested a review from NathanWalker April 8, 2021 11:59
@darind darind self-assigned this Apr 8, 2021
@cla-bot cla-bot bot added the cla: yes label Apr 8, 2021
@darind darind requested review from NathanaelA and rigor789 April 8, 2021 12:10
@brysem
Copy link

brysem commented Apr 8, 2021

Hi @darind,

Thank you for your effort! I've tested this PR in one of the larger apps I've made. The issue from #100 still seems to be present.

I tested it by using the app as normal and monitoring memory usage.

I've also tested it with a button that executes the following piece of code. This piece of code dramatically increases memory usage in V8. In JSC it does not.

var dispatch = function() {
    setTimeout(function() {
    for(var i=0; i < 100000; i++) {
        var p = new Promise((resolve, reject) => { resolve('') })
    }
    dispatch()
    }, 50)
}
dispatch();

@darind
Copy link
Collaborator Author

darind commented Apr 8, 2021

@brysem, by creating 100k promises every 50ms you are putting too much pressure on the JS engine. Here's the profiler graph when I reduce it to 5 seconds:

image

Make sure you leave some CPU cycles for the GC to complete.

As an alternative, we can make the PromiseProxy script conditional. It might break some plugins that rely on it but it will drastically improve performance.

@NathanaelA

This comment was marked as abuse.

@darind
Copy link
Collaborator Author

darind commented May 5, 2021

@NathanaelA, no I haven’t tried this on a real device in a fresh NS app. Does this crash occur in the Simulator?

But looking at the error, this looks like some issue with passing C strings as arguments to functions: https://github.com/NativeScript/ns-v8ios-runtime/blob/master/NativeScript/runtime/Interop.mm#L150

Are you sure that the crash doesn’t occur with a build from the master branch? Also is the use of the NSGetSizeAndAlignment function something new to the NS 8 core modules? Has this function been used before?

@NathanaelA

This comment was marked as abuse.

@NathanaelA

This comment was marked as abuse.

@NathanaelA

This comment was marked as abuse.

@NathanWalker
Copy link
Contributor

NathanWalker commented Jul 3, 2021

This pr is rolled up in #111 and in rc for further validations, thank you @darind for all the efforts here ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants