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

Support wasm module loading #4235

Merged
merged 12 commits into from
Jun 21, 2022
Merged

Support wasm module loading #4235

merged 12 commits into from
Jun 21, 2022

Conversation

slimbuck
Copy link
Member

@slimbuck slimbuck commented May 10, 2022

This PR adds standard engine support for loading and initialising emscripten-style wasm modules.

The API supports lazy loading, which is useful for tools that don't require all modules loaded at startup.

The following API is added:

WasmModule.setConfig(moduleName, config)         // set wasm module url config
WasmModule.getInstance(moduleName, callback)     // request the wasm module instance

The two functions can be invoked in any order and will resolve correctly.

The Draco decompressor has been updated to use this interface.

Once this PR is merged, the other uses of wasm modules in the codebase (editor applications, examples, model-viewer) should all migrate to this interface.

@slimbuck slimbuck requested a review from a team May 10, 2022 10:37
@slimbuck slimbuck self-assigned this May 10, 2022
src/core/module.js Outdated Show resolved Hide resolved
@mvaligursky
Copy link
Contributor

I wonder what is the ideal behaviour with wasm loading for web application that create multiple instances of the engine - do we need just one instance of wasm module, or does it need to be per application.
Also, do we need to take care of their release?

@slimbuck
Copy link
Member Author

does it need to be per application.

I think it depends on the module. Draco probably can be used across-application whereas Ammo might require separate instances. If this became a requirement we could update the config accordingly.

Also, do we need to take care of their release?

Don't believe you can directly destroy wasm modules.

@slimbuck
Copy link
Member Author

I'm wondering about error reporting with this API.

Currently all users of getInstance receive a load error when it happens. However it's usually the invoker of setConfig that would want to manage load errors.

I'm thinking the setConfig object could include an optional errorHandler member which receives the error instead. Then getInstance callback will receive a 'null' instance in case of error (or the callback will never be raised).

@yaustar
Copy link
Contributor

yaustar commented May 10, 2022

Does this also support network retries if the module fails to load from the network?

@mvaligursky
Copy link
Contributor

I wonder if this should be utilized when supported. (Could be done in a separate PR)
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WebAssembly/instantiateStreaming

@slimbuck
Copy link
Member Author

I wonder if this should be utilized when supported. (Could be done in a separate PR) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WebAssembly/instantiateStreaming

The wasm binary is downloaded by the emscripten-generated module glue script, which does attempt instantiateStreaming before falling back to fetch.

@slimbuck
Copy link
Member Author

Does this also support network retries if the module fails to load from the network?

I don't think the glue code has retry logic for the wasm module, but if this was something we needed, we could manage the download & compile of the wasm module ourselves.

@yaustar
Copy link
Contributor

yaustar commented May 10, 2022

I don't think the glue code has retry logic for the wasm module, but if this was something we needed, we could manage the download & compile of the wasm module ourselves.

I'm just thinking of the issues that we have with Snapchat where network requests can fail on iOS. It's on a WASM module that doesn't have retry, it could stop loading of the game or prevent the game from working correctly.

Here's the PR for basis that we needed for this: #4148

@slimbuck
Copy link
Member Author

I don't think the glue code has retry logic for the wasm module, but if this was something we needed, we could manage the download & compile of the wasm module ourselves.

I'm just thinking of the issues that we have with Snapchat where network requests can fail on iOS. It's on a WASM module that doesn't have retry, it could stop loading of the game or prevent the game from working correctly.

Here's the PR for basis that we needed for this: #4148

They are two different scenarios.

The basis code downloadings the script source 'manually' using http interface since the code is destined for execution on webworkers (not the main thread).

In this case we're using an html script tag to download and execute the script in place (on the main thread). When we load scripts using script tags elsewhere in the engine there is no retry logic (see https://github.com/playcanvas/engine/blob/main/src/resources/script.js#L100).

I don't -think- we need script tag retry logic (and wouldn't know how to implement such a thing). If we wanted manual retry logic we'd have to download the script manually (using http interface) instead. I'm not sure that'd be better and suspect the browser's built-in script tag handling is sufficient.

@yaustar
Copy link
Contributor

yaustar commented May 10, 2022

I don't -think- we need script tag retry logic (and wouldn't know how to implement such a thing). If we wanted manual retry logic we'd have to download the script manually (using http interface) instead. I'm not sure that'd be better and suspect the browser's built-in script tag handling is sufficient.

It will be something we could test if/when it becomes an issue I guess. Even if it doesn't handle retrying, given this can be lazy loaded, we could potentially mitigate the issue by loading the WASM module first and than other assets that are needed?

@mvaligursky
Copy link
Contributor

  • just looking at wasm-module.js and wonder why you created those functions at the start of the file as standalone functions instead of making them static functions of the class?

  • should WasmModule have some jsdocs for the class itself? I think it might need to to have api docs generated for the class, not sure

  • I wonder if we should upgrade one of the engine examples to register the draco wasm and load draco glb? multi-view example would be good, as that uses vertex heavy chess-board. I can try that later as well.

@slimbuck
Copy link
Member Author

  • just looking at wasm-module.js and wonder why you created those functions at the start of the file as standalone functions instead of making them static functions of the class?
  • should WasmModule have some jsdocs for the class itself? I think it might need to to have api docs generated for the class, not sure
  • I wonder if we should upgrade one of the engine examples to register the draco wasm and load draco glb? multi-view example would be good, as that uses vertex heavy chess-board. I can try that later as well.

Nice! Thanks for the suggestions:

  • I moved the naked functions into an Impl class with static functions. This way code looks a little neater and doesn't expose the internal functions to the public interface
  • Added class jsdoc, pls check it
  • Upgraded all the examples to use the new wasm loading and removed loading from example code

cc @ellthompson - please check changes to examples.

}
});
} else {
callback('No supported wasm modules found.', null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • could this add the module name to the error message
  • and perhaps even log a debug warning - to help find the problem in case the user does not handle errors correctly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default error handler logs the details as you describe. Otherwise if the caller registers an error handler, it's up to them to do this.

Copy link
Contributor

@mvaligursky mvaligursky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Approving with one minor comment.

@slimbuck slimbuck merged commit 57186aa into playcanvas:main Jun 21, 2022
@slimbuck slimbuck deleted the module-support branch June 21, 2022 17:10
@slimbuck slimbuck mentioned this pull request Aug 4, 2022
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