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

Possible Memory Leak #216

Open
DSalman opened this issue Mar 16, 2023 · 15 comments · May be fixed by #318
Open

Possible Memory Leak #216

DSalman opened this issue Mar 16, 2023 · 15 comments · May be fixed by #318

Comments

@DSalman
Copy link

DSalman commented Mar 16, 2023

Hi. I'm using with Node.js and I need to loop through many svg frames and convert them to a png sequence. I'm seeing memory usage continually increase and stay very high even after the job is complete. The heap is not increasing, only the rss. I found that Node users experiencing similar issues usually trace it back to a leak in native code. I've reduced the script down to the simplest possible code where all I do is create a new Resvg and call render().asPng() on it. The demo script includes an http server to run the test, but the result is the same if you call it directly. The service set values from the console match my activity monitor for the node process and show a memory usage close to 3G at the end of the loop. This was also confirmed when running on the server where we have limited memory, about 1G. This will reliably trigger an out of memory termination.
leak-test.zip

@yisibl
Copy link
Owner

yisibl commented Mar 20, 2023

Thank you for your feedback, I've been very busy lately. I expect to have time to look at this issue in April.

Until then, you can try using the wasm version and see what happens? https://github.com/yisibl/resvg-js/tree/main/wasm

@DSalman
Copy link
Author

DSalman commented Mar 21, 2023

I did try the wasm version and although I have problems getting it to render images and text, it revealed something significant. It seems quite a bit slower and at fist it did the same thing; grew memory indefinitely. I noticed that RenderedImage has a "free()" method attached to it. When I used that, all was good, memory remained pretty much stable.

        const resvgJS = new ResvgWasm.Resvg(svg, opts)
        const pngData = resvgJS.render() 
        const png = pngData.asPng()

// do something with png, e.g. save to file...
pngData.free(); // without this line memory keeps growing.

I don't see free() referenced in any of the examples, so I'm not sure about its intended usage, but it seems important if it releases memory. I would like to continue using resvg-js for its speed, so I look forward to your findings in a couple weeks. Thanks!

@yisibl
Copy link
Owner

yisibl commented Apr 4, 2023

I can reproduce this and can observe a continuous memory growth(MacBook Pro M1). This does not happen in resvg-js@1.4.0.

image

// rendering frame: 0
{
Memory: rss 85.55 MB
Memory: heapTotal 11.83 MB
Memory: heapUsed 8.14 MB
Memory: external 1.39 MB
Memory: arrayBuffers 0.06 MB
}


// rendering frame: 250
Memory: rss 2316.67 MB
Memory: heapTotal 11.83 MB
Memory: heapUsed 8.33 MB
Memory: external 6.95 MB
Memory: arrayBuffers 0.06 MB

@zimond
Copy link
Collaborator

zimond commented Apr 4, 2023

@DSalman
Copy link
Author

DSalman commented Apr 6, 2023

I still see memory growth using 1.4.0. Its not as much but well over 1G by the end of the run. From zimond's comment it looks like this has something to do with how the rust library is integrated with the js code?

@zimond
Copy link
Collaborator

zimond commented Apr 13, 2023

@DSalman The simple answer is that rust code needs an integration framework which wasm-bindgen provides to work easier with JS. And any objects created in Rust need to be .free()-ed when you don't need them. Or you could use this weak-reference feature that wasm-bindgen provides to auto-free objects, but with a compatibility loss.

So is calling .free() acceptable? If not , maybe we can discuss about whether the weak-ref feature should be used or not.

@iKBAHT
Copy link

iKBAHT commented Dec 1, 2023

I had the same problem when processing a large number of files. Tried Node v16.16.0 and v20.5.1

@yisibl
Copy link
Owner

yisibl commented Dec 1, 2023

@iKBAHT
Thanks for the feedback, this is a known issue due to not aggressively doing memory reclamation, making it look like a memory leak. I'll take the time to fix it one weekend.

@ppjjzz
Copy link

ppjjzz commented Dec 3, 2023

我也遇到了内存不断增长的问题,看起来像内存泄漏

@mizdra
Copy link

mizdra commented Mar 13, 2024

I have also faced this problem when using @vercel/og.

The cause seems to be that @vercel/og forgot to call .free(). Rewriting the code as follows solved the memory leak. Hope this helps you!

diff --git a/node_modules/@vercel/og/dist/index.node.js b/node_modules/@vercel/og/dist/index.node.js
index c92983c..a516f4f 100644
--- a/node_modules/@vercel/og/dist/index.node.js
+++ b/node_modules/@vercel/og/dist/index.node.js
@@ -18786,7 +18786,11 @@ async function render(satori2, resvg, opts, defaultFonts, element) {
       value: options.width
     }
   });
-  return resvgJS.render().asPng();
+  const pngData = resvgJS.render();
+  const pngBuffer = pngData.asPng();
+  pngData.free();
+  resvgJS.free();
+  return pngBuffer;
 }
 
 // src/figma/index.tsx

@tyutjohn
Copy link

Is it feasible to provide api like .free() in Nodejs version? I need to render fonts, so I can‘t use the wasm version. If this is possible I can try to provide a PR

@ducan-ne
Copy link

Not sure it's related but I'm opened an issue due to memory usage #343

@ducan-ne
Copy link

Is it feasible to provide api like .free() in Nodejs version? I need to render fonts, so I can‘t use the wasm version. If this is possible I can try to provide a PR

@tyutjohn You can use the resvg-wasm as wasm is supported by nodejs

@tyutjohn
Copy link

I spent some time researching the memory leak problem in node. I now have reason to suspect that the memory is not released due to the integration of rapi-rs with rust. I created the following test code

#[cfg(not(target_arch = "wasm32"))]
#[napi]
impl Resvg {
    #[napi(constructor)]
    pub fn new(svg: Either<String, Buffer>, options: Option<String>) {
        Resvg::new_inner(&svg, options)
    }

    fn new_inner(
        svg: &Either<String, Buffer>,
        options: Option<String>,
    ) {
        let js_options: JsOptions = options
            .and_then(|o| serde_json::from_str(o.as_str()).ok())
            .unwrap_or_default();
    }
}

It returns nothing, I created 10000 instances, then I manually triggered the drop and called node gc, rss no changes

@martinklepsch
Copy link

martinklepsch commented Jun 25, 2024

Wanted to add/confirm my understanding here

With that in mind is it advisable to use the WASM "runtime" if memory is of concern? @vercel/og seems to use WASM over node already.

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 a pull request may close this issue.

9 participants