blockfrost / blockfrost-js

Node.js SDK for the Blockfrost.io API.
https://blockfrost.io
Apache License 2.0
103 stars 25 forks source link

Some memory leaks here and there due to cardano serialization lib #272

Closed AlexDochioiu closed 1 year ago

AlexDochioiu commented 1 year ago

It seems that with CSL, every single object is always copied / returned as a copy and should be freed. This means that you cannot chain calls without causing a leak instantaneously.

Looking at helpers file, one such example is:

const stakeKeyHash = addressSpecificStakeKey.to_raw_key().hash();

The result of to_raw_key() is never stored in a variable and freed, hence it will leak memory endlessly.

Those should be split into

const stakeRawKey = addressSpecificStakeKey.to_raw_key();
const stakeKeyHash = stakeRawKey.hash();
...
stakeRawKey.free();
stakeKeyHash.free();

to avoid memory leaks.

slowbackspace commented 1 year ago

@AlexDochioiu great catch! I've opened a PR with a fix. I'll run some tests to confirm that it indeed resolved the memory leak. Hopefully I didn't miss anything :)

https://github.com/blockfrost/blockfrost-js/pull/273

AlexDochioiu commented 1 year ago

That was quick 😂 . I'm pretty much looking at all our usage (directly/indirectly) of CSL because our server is leaking 1GB per day because of it and going up 😬. P.S. The issue is not actually from your lib cause I'm not even using those helpers but I thought it's worth mentioning.

vladimirvolek commented 1 year ago

@AlexDochioiu Maybe you can try this? https://github.com/Emurgo/cardano-serialization-lib/issues/542#issuecomment-1374453823

slowbackspace commented 1 year ago

That was quick 😂 . I'm pretty much looking at all our usage (directly/indirectly) of CSL because our server is leaking 1GB per day because of it and going up 😬. P.S. The issue is not actually from your lib cause I'm not even using those helpers but I thought it's worth mentioning.

We are also experiencing memory leaks in some of our projects which, I assume, is because of CSL (called from deriveAddress). But for us the leak is manageable so it doesn't bother us that much.

I ran deriveAddress approx. 200k times (for different address indexes) and it seems that process.memoryUsage() reports about 50MB increase in external where (I assume) should be all WASM bloat. This memory usage is basically same in master branch and PR's branch so I'm not sure how much did the additional frees() improved things, but it certainly does make sense.

What are you using to track down the memory leak?

AlexDochioiu commented 1 year ago

Took heap snapshots at fixed intervals and checked/compared them.

slowbackspace commented 1 year ago

Took heap snapshots at fixed intervals and checked/compared them.

Oh interesting! I didn't expect it would affect heap since in our projects we are mostly seeing leaks in external memory, but the fix does save about 20MB of heap mem (per 200k derivations). Thank you!

AlexDochioiu commented 1 year ago

@AlexDochioiu Maybe you can try this? Emurgo/cardano-serialization-lib#542 (comment)

Unfortunately my main expertise is not exactly rust/wasm/nodejs and I basically ran into issues trying to recompile CSL myself. Also decided it's not worth the effort to debug it any further as I'm almost fully rolling it out of our codebase in favor of Lucid (which uses CML built with that flag enabled ; can already confirm that there's no signs of any leaks coming from lucid's usage of CML).

vladimirvolek commented 1 year ago

@AlexDochioiu Maybe you can try this? Emurgo/cardano-serialization-lib#542 (comment)

Unfortunately my main expertise is not exactly rust/wasm/nodejs and I basically ran into issues trying to recompile CSL myself. Also decided it's not worth the effort to debug it any further as I'm almost fully rolling it out of our codebase in favor of Lucid (which uses CML built with that flag enabled ; can already confirm that there's no signs of any leaks coming from lucid's usage of CML).

Thank you for the info ❤️