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

Stop Leaking Memory #234

Open
wants to merge 51 commits into
base: main
Choose a base branch
from
Open

Stop Leaking Memory #234

wants to merge 51 commits into from

Conversation

yHSJ
Copy link

@yHSJ yHSJ commented Nov 4, 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)).

It turns out we were not the only ones to notice it, as you can see from this issue: #222

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. We experimented with the FinalizationRegistry API and introducing destructors to clean up memory, but this experiments proved to be unsuccessful. Even if they were successful, it would entirely depend on the user's runtime engine if it were impactful for them. That's why we settled on explicitly freeing memory through the new APIs we introduced.

This PR introduces proper memory management internally without introducing breaking changes to the API. It also exposes new methods on classes that include fields from the CML to free that memory, and new APIs to handle freeing memory on the user side (if they are using CML stuff directly).

yHSJ and others added 30 commits November 1, 2023 11:48
@AlexDochioiu
Copy link

AlexDochioiu commented Dec 18, 2023

@yHSJ We use lucid extensively on vespr servers too and there's no memory leaks in it. I had a discussion with Ales a while ago when I thought I may have some leaks due to it (I was wrong).

Confirmed by Ales: Lucid is using a a version of CML built with weak refs support and does not need manual disposal for the objects (https://rustwasm.github.io/wasm-bindgen/reference/weak-references.html).

P.s. In my case, I was also using emurgo's CSL library on our servers, which was causing the memory leaks (in a few places where I missed the .free() call). I changed instead to not import the library published by emurgo and just use the CML built into lucid for my needs. Haven't had memory leaks since (for prob half a year now)

@yHSJ
Copy link
Author

yHSJ commented Dec 18, 2023

We use lucid extensively on vespr servers too and there's no memory leaks in it. I had a discussion with Ales a while ago when I thought I may have some leaks due to it (I was wrong).

Confirmed by Ales: Lucid is using a a version of CML built with weak refs support and does not need manual disposal for the objects (https://rustwasm.github.io/wasm-bindgen/reference/weak-references.html).

I think it is most likely that your servers are not running long enough to see a memory leak or you are not calling functions that are using the WASM bindings. Internal testing at JPG showed that Lucid was the cause of memory leaks and this PR directly fixed that issue. This has been corroborated by several other tests including by @dpbeaumont in #222 and several other developers privately.

@klntsky
Copy link

klntsky commented Mar 20, 2024

@yHSJ

We experimented with the FinalizationRegistry API and introducing destructors to clean up memory, but this experiments proved to be unsuccessful

What exactly didn't work?

We are using a vendored version[0] for quite a while, and it works well. Here[1] is our "GC" that uses FinalizationRegistry - and we have a test[2] that gives us some assurance.

[0] https://github.com/mlabs-haskell/cardano-serialization-lib-gc
[1] https://github.com/mlabs-haskell/csl-gc-wrapper
[2] https://github.com/Plutonomicon/cardano-transaction-lib/blob/develop/test/CslGc.js

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.

4 participants