Switching Vireo to Wasm from asm.js #512
Conversation
Now that the wasm is in a seperate file, it makes debugging easier to avoid minifying the vireo.js module. An end user can minify the rollup output if desired.
Found that WASM aggressive traps on numeric conversions caused exceptions (caused by overflow, double to int conversion, etc). Changed trap behavior to clamp numerics instead.
Based on feedback that the previous requestInstance could be confused with requesting from a singleton so changing to createInstance.
And mark the workarounds used for legacy browsers as optional and removable if needed.
Some changes have happened in emsdk that actually update the PATH correctly so running a seperate command and environment is not needed
and no longer accept callbacks. This is an improvement because it gives a better way to report fatal errors that have occurred during exxecution of the Vireo runtime.
These manual tests verify that vireo loads into memory in different scenarios such as global in browser, AMD in browser, ES6 module in browser, CommonJS in node, Web Workers, etc.
Some of the test runs execute significantly slower and increase build times by a lot (total 12min -> 35min). It does not seem worth it to execute every configuration on the CI. Only enabling the test-min run since that reflects the build executed by users.
With the JS math mode enabled the coerced values behave as expected. If the mode is ever changed these tests will fail and can be investigated further.
rajsite
requested review from
cglzaguilar,
sanmut and
spathiwa
as code owners
There was a problem hiding this comment.
Great work @rajsite! Please address minor feedback and questions.
|
This looks great! |
Previously findValueRef and findSubValueRef threw an Error for nonexistant paths. In general throwing an error from the Vireo API functions means that Vireo is in an unrecoverable state. However, there were tests that would cause errors to be thrown for nonexistent paths but continue to use the Vireo instance for additional tests. This now resulted in failures in the asm.js buid after a compiler upgrade. Instead of modifying the tests to use new Vireo instances between runs, the behavior of findValueRef and findSubValueRef was modified to safely return undefined if a path is not found. This has been a desired behavior on the ASW side as well.
|
@spathiwa Initially had tried to support both asm.js and wasm with the old UMD module system but that was very complex. After switching ES6 modules it simplified a lot of pieces and it wasn't too bad to have both wasm and asm.js builds supported so both are now enabled. The asm.js build also revealed a problem that existed in the tests and that mainline relied on and is now cleaned up so it was a nice bit that was uncovered. I'm resetting @cglzaguilar and @gleonoliva to buddy that api behavior change. Edit: Okay, I can't actually reset people, but if @cglzaguilar and @gleonoliva could take a gander at those parts that would be great |
|
Again, great job! |
This is awesome. Thanks for building both. |
|
Looks great. Go WASM and Go ES6 !!! |
|
I had a second look at it. It looks great! I think it makes more sense now that |
|
Also I just noticed that this is the 2^9 pull request to VireoSDK 🙌 🎉 |
|
Ran the tests suggested by @sanmut just waiting on ASW PR |
Major changes:
new Vireo()) to asynchronous (vireoHelpers.createInstance())executeSlicesUntilClumpsFinishedfrom being callback-based to being Promise-basedNote: It looks like a lot of files changes buts that mostly because two lines had to change in every JS test.
It was unavoidable due WebAssembly requiring async construction.
The actual changes are pretty well localized to the build scripts and the loader js files.