rive-app / rive-wasm

Wasm/JS runtime for Rive
MIT License
703 stars 51 forks source link

fix: unload Mat2D object reference before transliterating to JS for canvas2d renderer #281

Closed zplata closed 1 year ago

zplata commented 1 year ago

Problem: We're noticing a memory leak that's slow-burning on the memory footprint of an app that uses @rive-app/canvas. Because we allow memory to grow when building WASM, it's not immediately evident unless you run an app for potentially several hours.

tl;dr: Unload Mat2D properties, which are just floats, before calling into JS for a few functions.

After turning off '-s ALLOW_MEMORY_GROWTH=1' in our premake config on debug mode and running against the WASM parcel_example app with about 20 canvases going, it'll crash the app in a few seconds. Interestingly, WASM doesn't report a memory leak using our memory leak test function. So it's not that our JS heap is growing in size, but rather something is driving memory usage up on the browser that emscripten doesn't seem to be catching.

After a bit of a chase to find the leak, it seems that any CPP binding that calls into JS (renderer.js) to do some calculation with an object reference in particular as a parameter seems to be an offending case here. This is used in a few spots with respect to .transform() and .addRenderPath() in CPP-land, which call into JS with a const rive::Mat2D& transform object. Pointers and primitive types don't seem to be problematic in this conversion.

Some folks have tried to raise this - but nothing super clear here. Also, the docs don't give examples or guidance on this case so hard to tell the main issue beyond Emscripten trying to make a copy of the object before passing to JS from what I think I'm gathering.

Solution (for now): What seemed to have fixed the main issue for rendering right now is instead of passing the Mat2D object reference to JS, just unpacking the values of it and sending it to JS instead, since they're just float primitives. This is a purely internal call API change, so nothing consumer-facing and shouldn't be breaking.

Next steps I think are to try and re-create this underlying reference issue in a standalone WASM example and try to raise with the Emscripten team if we can

csmartdalton commented 1 year ago

Thanks for all your work tackling this @zplata ! Great findings!

luigi-rosso commented 1 year ago

:rubber stamp: (maybe luigi will have more input)

is there a performance difference?

I actually think performance might be slightly better like this, especially if it's newing up less objects for each call (and less calls back across the bridge to get each property).