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

wasm: expose methods to dispose or recreate wasm instances #522

Closed
kzhsw opened this issue Dec 29, 2022 · 9 comments · Fixed by #626
Closed

wasm: expose methods to dispose or recreate wasm instances #522

kzhsw opened this issue Dec 29, 2022 · 9 comments · Fixed by #626

Comments

@kzhsw
Copy link

kzhsw commented Dec 29, 2022

Motivation:
The WebAssembly.Memory can only grow, not shrink, the only way to reclaim memory is after the wasm instance being garbege collectet. After decompressing a large mesh using meshoptimizer, the wasm memory would never release since it is defined in the module closure.

Below is a screenshot from the memory view of chrome devtools:
图片

Risks:
Third-party libraries using meshoptimizer may have assumptions on MeshoptDecoder.ready.
Recreation of wasm might need to be async, so mechanism like double-buffering might be needed to keep backwards compatibility, or maybe create a seperate js module with manual instance managing in addition to existing ones.

Alternatives:

  1. Create a worker for meshoptimizer and terminate or recreate the worker, which may introduce extra overhead on cloning and transfering. Also note that the worker in decodeGltfBufferAsync is not terminated and can not be manually terminated on idle.
  2. Use the js-based meshopt_decoder_reference.js, which would be much slower than wasm.
  3. Use meshopt_decoder.js instead of module ones making it possible to detach the MeshoptDecoder from global context.
  4. Wait for wasm spec update in Wasm needs a better memory management story WebAssembly/design#1397
  5. Implement a streaming API for compressing or decompressing in small fixed-size chunks.
@zeux
Copy link
Owner

zeux commented Dec 29, 2022

One other alternative that should be available right now (similar to 3) is to use dynamic imports, eg

var MeshoptDecoder = (await import("path/to/meshopt_decoder.module.js")).MeshoptDecoder;

... after which presumably you can just make sure MeshoptDecoder gets GC'ed entirely.

For 1, I think it would be better if MeshoptDecoder.useWorkers(0) cleared the workers array - right now it's a no-op. This should offer a convenient way to solve this problem without changing the import flow, although it would necessitate at least MeshoptDecoder.useWorkers(1) before use.

@kzhsw
Copy link
Author

kzhsw commented Dec 30, 2022

One other alternative that should be available right now (similar to 3) is to use dynamic imports, eg

For webpack, dynamic imports did not seem to dispose loaded module when called again.

For 1, I think it would be better if MeshoptDecoder.useWorkers(0) cleared the workers array - right now it's a no-op. This should offer a convenient way to solve this problem without changing the import flow, although it would necessitate at least MeshoptDecoder.useWorkers(1) before use.

Maybe a secondary parameter for MeshoptDecoder.useWorkers to specify whether to terminate and recreate existing workers would help.

@zeux
Copy link
Owner

zeux commented Jan 4, 2023

Looks like in Chrome at least, you must call Worker.terminate(), as simply dropping the reference keeps it running. I'd like useWorkers(0) to have the semantics of "finish all pending requests first" instead of outright breaking requests in progress, so this needs a little more work than just resetting the worker array.

@kzhsw
Copy link
Author

kzhsw commented Jan 18, 2023

Or would it be possible, to create chunk-based api, decoding buffers by chunk, making it possibly to use fixed-size wasm memory.

@OrigamiDev-Pete
Copy link

I'm also interested in this. Working on a project that has strict memory requirements and would be great to be able to release memory being held by the MeshOptDecoder.

@zeux
Copy link
Owner

zeux commented Jun 8, 2023

Looks like there's some advances on memory.discard which would allow to solve this. I do think it would be valuable to have some sort of API in the decoder for this, which could fully recreate the Wasm instance for now, and use memory.discard in the future transparently.

@OrigamiDev-Pete in your application, are you using web workers (useWorkers mentioned above) or just decoding on the main thread? Trying to figure out what the best intermediate solution here might look like.

@OrigamiDev-Pete
Copy link

At this stage we're decoding on the main thread.

@zeux zeux mentioned this issue Oct 17, 2023
10 tasks
@zeux
Copy link
Owner

zeux commented Oct 29, 2023

I think for now it would make sense to focus on the WebWorkers usage here.

When a lot of geometry is processed, WebWorkers are probably beneficial either way, if only to avoid main thread stalls. useWorkers(1) can be used to get the same total memory consumption as main thread decoding at what seems like a slightly lower overall cost, and the extra potential latency (I've measured around 1ms delay on Windows between request dispatch and processing for small buffers) shouldn't be that impactful.

On the opposite side, outside of WebWorkers the solutions here are... not clean. There's issues with recreating instances, both inside WebWorkers and outside of them due to promise handling and what not, and while the future solution here is pretty clearly memory.discard, it's not there yet.

Streaming/chunking solutions can be explored in the future, but also have issues due to more complicated state handoff that is necessary. This almost necessitates a second version of the Wasm/JS library if we want to keep the code size small... and there's a lot of subtleties here.

On the flip side, we already should support a better way to handle worker management (calling useWorkers twice right now, for example, allocates new workers but doesn't terminate the old ones), and it provides a 80% solution to this issue when useWorkers(1) is used by the calling application, as you can call useWorkers(0).

To that end, I'll open a PR that supports useWorkers properly and will close this issue. I recognize that this isn't going to be a perfect solution to this, but realistically a perfect solution will either require WebAssembly spec evolution, or a lot of highly special work on this library, so I don't want the perfect to be the enemy of the good here.

@zeux
Copy link
Owner

zeux commented Oct 29, 2023

Oh I should say that it’s simple to add something like dispose: function() { instance = undefined; } to the interface.

For anyone running into this issue in a scenario where WebWorkers aren’t a reasonable solution this may be fine, and could be applied as a workaround by patching the module source. But it leads to issues if decoding is requested again after this call, and recreating and propagating the modules throughout existing workers is enough trouble that it might be better to wait for Wasm memory enhancements.

@zeux zeux closed this as completed in #626 Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants