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

Free unused memory in Data.from #232

Closed
wants to merge 54 commits into from
Closed

Free unused memory in Data.from #232

wants to merge 54 commits into from

Conversation

yHSJ
Copy link

@yHSJ yHSJ commented Nov 1, 2023

At JPG Store, we have been dealing with memory leaks in our code that uses either Lucid or the CML. After some research, we discovered that the CML does a lot of clones to avoid issues with freed memory across the WASM boundary ( dcSpark/cardano-multiplatform-lib#142 (comment)).

Any object that is returned from WASM to JavaScript seems to never get garbage collected. Thus, all objects returned from wasm to the js runtime must be freed, including intermediary values when chaining function calls together.

This PR specifically only fixes the memory usage in the Data.from function to resolve the following issue: #222

I will continue to fix the memory management over time, as I have free time. However, I would argue it is best to merge these smaller pieces whenever they are done, as they will reduce the memory footprint of several applications relying on Lucid.

Copy link

@joacohoyos joacohoyos left a comment

Choose a reason for hiding this comment

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

Small comment. We might also want to add a comment to the freeable class cause other people might not have the context we have due to our internal discussions

@yHSJ
Copy link
Author

yHSJ commented Nov 2, 2023

We might also want to add a comment to the freeable class cause other people might not have the context we have due to our internal discussions

Good point, I went ahead and added some comments to the freeables.ts file that should provide context.

@yHSJ
Copy link
Author

yHSJ commented Nov 4, 2023

Closing in favor of #234 which includes managing WASM memory everywhere it's used.

@yHSJ yHSJ closed this Nov 4, 2023
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