Compile Binaryen to WebAssembly by dcodeIO · Pull Request #2503 · WebAssembly/binaryen

dcodeIO · 2019-12-05T19:02:41Z

This PR enables compiling Binaryen to WebAssembly when building binaryen.js. Since WebAssembly is best compiled and instantiated asynchronously in browsers, it also adds a new mechanism to tell if respectively when the module is ready by means of one of the following:

// Using a promise
const binaryen = require("binaryen");
binaryen.ready.then(() => {
  ... use normally ...
});

// Using await
const binaryen = require("binaryen");
(async () => {
  await binaryen.ready;
  ... use normally ...
})();

// Where top-level await is available
const binaryen = await require("binaryen").ready;
... use normally ...

One can also tell if Binaryen is already ready (for example when assuming it in follow-up code) by:

if (/* we already know that */ binaryen.isReady) {
  ... use normally ...
} else {
  throw Error("Binaryen is supposed to be ready here but isn't");
}

The JS test cases have been updated accordingly by wrapping everything in a test function and invoking it once ready. Documentation will have to be updated as well to cover this of course. New file size is about 2.5mb, even though the Wasm becomes inlined into the JS file which makes distribution across different environments a lot easier.

Thoughts? :)

Supersedes and thus fixes #1381
With .ready it also fixes #2452

sbc100

Seems reasonable to me. I actually didn't know we had so much JS-specific stuff in binaryen. I wonder if keep this upstream is the best place for it? Maybe a separate binaryen-js repo would make sense at some point.

sbc100 · 2019-12-05T20:24:40Z

+  Module['WidenHighSVecI16x8ToVecI32x4'] = Module['_BinaryenWidenHighSVecI16x8ToVecI32x4']();
+  Module['WidenLowUVecI16x8ToVecI32x4'] = Module['_BinaryenWidenLowUVecI16x8ToVecI32x4']();
+  Module['WidenHighUVecI16x8ToVecI32x4'] = Module['_BinaryenWidenHighUVecI16x8ToVecI32x4']();
+  Module['SwizzleVec8x16'] = Module['_BinaryenSwizzleVec8x16']();


Is this all maintained by hand? It seems pretty fragile? Can this not be done by looking at all the keys in Module that start with _Binaryen?

Yeah, that's all maintained by hand. I believe one reason why it's hard to automate is that not every exported function of the form _Binaryen... can be called like that, for example some of these return the respective constants while others like _BinaryenModuleCreate create a module. But might be possible to simplify by keeping a list of the good ones, doing something like

["WidenHighSVecI16x8ToVecI32x4", ...].forEach(name => Module[name] = Module['_Binaryen' + name]());

or renaming the good ones to a common scheme, like _BinaryenOp... so JS can decide based on that?

sbc100 · 2019-12-05T20:30:29Z

-wasm.addFunctionType("vI", Binaryen.i32, []);
-// It will cause a fatal error to try to add the same name a second time
-wasm.addFunctionType("vI", Binaryen.i32, []);
+function test(Binaryen) {


Is the Binaryen argument here different to the Binaryen in the global scope? If not then maybe we don't need it? Or at least maybe call it something else so its more obvious what it doesn't shadow the global one?

It's the same as the global one, yeah. Mostly a case of "why not" if we'd ever want to test multiple variants, like a fastcomp and an upstream one, but can as well remove the arguments since we don't do that.

dcodeIO · 2019-12-06T01:17:50Z

Last commit makes it so that the constants are created from arrays, making this a bit less error prone and slightly less code. Tests don't shadow Binaryen anymore as well and I added a line to .gitattributes to make sure that binaryen.js fixtures always use LF line endings (on Windows) so the diff doesn't fail there.

dcodeIO · 2019-12-06T03:46:20Z

Appears this already found something that was wrong before

Module['GeVecF32x4'] = Module['_BinaryenGeVecF32x4']();
Module['EqVecF64x2'] = Module['_BinaryenGeVecF32x4'](); // here

dcodeIO · 2019-12-06T04:11:58Z

I wonder if keep this upstream is the best place for it?

One aspect is that running the JS tests also tests the wrapped C-API, so there are benefits in keeping the JS tests upstream. Afaik some tests even exist in JS only. Not ideal ofc, but still better than not having them.

kripken

Nice, thanks!

I don't remember what the general best practices are here, though - does wabt use the .ready.then approach, and other projects?

Does this make us lose support for building to JS and not to wasm? It would be good to still support both.

dcodeIO · 2019-12-09T21:48:17Z

I don't remember what the general best practices are here, though - does wabt use the .ready.then approach, and other projects?

Hmm, I thought Wabt has a .ready promise, but from a quick check it seems it doesn't (anymore) and uses Emscripten's MODULARIZE=1 now without any extra code.

const wabt = require("wabt");
wabt().then(function(wabt) { ... });

This has a major problem though in that

(async () => {
  let w = await wabt();
})();

goes into an endless loop since the awaited object is promise-like again (here: the same object with a .then method), so the engine tries to resolve the result over and over, never returning.

I am not aware of general best practices here as something like this is uncommon in JS-space, and if there aren't any I suggest leading by example with a .ready promise. @MaxGraey, do you know if there are any JS libraries that depend on resolving a promise before becoming functional?

Does this make us lose support for building to JS and not to wasm? It would be good to still support both.

Yeah, this drops "compiling to just JS". Can try to make two build targets with binaryen_js building to JS and binaryen_wasm building to Wasm.

kripken · 2019-12-10T21:15:54Z

I'm not necessarily opposed to dropping the JS target. But one thing that's worried me is performance, specifically, binaryen uses exceptions, and with a JS build the VM can inline the invokes, but in wasm we end up with uninlineable wasm->js->wasm calls. Worth doing some benchmarking here to see if the wasm build is actually faster on a large input.

dcodeIO · 2019-12-11T08:34:44Z

With the changes there are two targets binaryen_wasm and binaryen_js now that both output bin/binaryen.js, and the Travis emcc test now checks both. Feels a bit strange that both targets emit the same file but, considering that we'll pick and ship a default one anyway with the other being relevant for testing or specific use cases only, I think it's fine?

kripken · 2019-12-11T22:09:44Z

Makes sense to me. Overall this looks good! Last thing, please update the Changelog, as this is a large breaking change.

MaxGraey · 2019-12-11T22:16:08Z

I guess Binaryen.ready approach will not very useful with esm wasm modules

kripken · 2019-12-19T18:27:55Z

+- Binaryen.js can now be compiled to Wasm using the `binaryen_wasm` target.
+  Unlike the JS variant, the Wasm variant requires asynchronously awaiting the
+  Wasm blob's instantiation and initialization before being usable, using the
+  `binaryen.ready` promise, e.g. `binaryen.ready.then(() => ...)`.


I think we export as Binaryen (with a capital B)?

Ah, yeah, the default is Binaryen when loading binaryen.js in the browser, that is just including the script. For the npm package I'm using const binaryen = require("binaryen"); simply because that's the package name and somewhat standard to do. Perhaps it makes sense to unify both to lower case to match the package name?

Oh, I see. Yeah, let's maybe standardize it on the lowercase name then. But maybe in a separate PR (with a changelog entry)?

So I think this is ready to land, except for the merge conflict.

Sounds good! Conflict should be resolved if CI doesn't state otherwise.

This PR enables compiling Binaryen to WebAssembly when building binaryen.js. Since WebAssembly is best compiled and instantiated asynchronously in browsers, it also adds a new mechanism to tell if respectively when the module is ready by means of one of the following: // Using a promise const binaryen = require("binaryen"); binaryen.ready.then(() => { ... use normally ... }); // Using await const binaryen = require("binaryen"); (async () => { await binaryen.ready; ... use normally ... })(); // Where top-level await is available const binaryen = await require("binaryen").ready; ... use normally ... One can also tell if Binaryen is already ready (for example when assuming it in follow-up code) by: if (/* we already know that */ binaryen.isReady) { ... use normally ... } else { throw Error("Binaryen is supposed to be ready here but isn't"); } The JS test cases have been updated accordingly by wrapping everything in a test function and invoking it once ready. Documentation will have to be updated as well to cover this of course. New file size is about 2.5mb, even though the Wasm becomes inlined into the JS file which makes distribution across different environments a lot easier. Also makes building binaryen (to either js or wasm) emit binaryen.js, and not binaryen_js.js etc. Supersedes and thus fixes #1381 With .ready it also fixes #2452

Compile Binaryen to WebAssembly

407fa60

sbc100 reviewed Dec 5, 2019

View reviewed changes

address comments

b058249

whoops [ci skip]

fe22b64

update kitchen-sink.js test

0fa5904

kripken reviewed Dec 9, 2019

View reviewed changes

dcodeIO added 3 commits December 11, 2019 08:39

make separate targets for wasm and js [ci skip]

42bf9e8

Merge branch 'master' into binaryenwasm

9cc20c0

emit as binaryen.js and test both

5bdec4b

dcodeIO added 2 commits December 17, 2019 10:19

Merge branch 'master' into binaryenwasm

ee75ab1

update changelog [ci skip]

2fc01fc

dcodeIO mentioned this pull request Dec 17, 2019

WIP: Also compile Binaryen to WebAssembly #1381

Closed

trigger ci

2f46dca

kripken reviewed Dec 19, 2019

View reviewed changes

kripken approved these changes Dec 19, 2019

View reviewed changes

Merge branch 'master' into binaryenwasm

c58f4b8

dcodeIO mentioned this pull request Dec 19, 2019

Update definitions AssemblyScript/binaryen.js#19

Merged

2 tasks done

kripken merged commit 02e6ba2 into WebAssembly:master Dec 19, 2019

aheejin mentioned this pull request Dec 20, 2019

Ninja does not work for bianryen.js build #2546

Closed

Conversation

dcodeIO commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

sbc100 Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

dcodeIO Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

dcodeIO Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

dcodeIO commented Dec 6, 2019

Uh oh!

dcodeIO commented Dec 6, 2019

Uh oh!

dcodeIO commented Dec 6, 2019

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

dcodeIO commented Dec 9, 2019

Uh oh!

kripken commented Dec 10, 2019

Uh oh!

dcodeIO commented Dec 11, 2019

Uh oh!

kripken commented Dec 11, 2019

Uh oh!

MaxGraey commented Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

dcodeIO Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

dcodeIO Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dcodeIO commented Dec 5, 2019 •

edited

Loading

dcodeIO Dec 5, 2019 •

edited

Loading

MaxGraey commented Dec 11, 2019 •

edited

Loading

dcodeIO Dec 19, 2019 •

edited

Loading