Skip to content

assemblyscript loader needs adapation (chrome) #295

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

Closed
t57ser opened this issue Oct 4, 2018 · 6 comments
Closed

assemblyscript loader needs adapation (chrome) #295

t57ser opened this issue Oct 4, 2018 · 6 comments

Comments

@t57ser
Copy link

t57ser commented Oct 4, 2018

Chrome limits the use of WebAssembly.Instance to files <= 4KB
I had to copy and adapt the assemblyscript loader to be able to use it correctly.

This issue has also been mentioned here: #240

From the current docs:

// From a module provided as a buffer, i.e. as returned by fs.readFileSync
const myModule = loader.instatiateBuffer(fs.readFileSync("myModule.wasm"), myImports);

// From a response object, i.e. as returned by window.fetch
const myModule = await loader.instantiateStreaming(fetch("myModule.wasm"), myImports);

Note that although instantiateStreaming is a promise, the underlying implementation still calls the "instantiate" function which calls WebAssembly.Instance

My proposal would be to change all exposed loader instantiate functions to async and replace the underlying instantiate function to use:
var instance = await WebAssembly.instantiate(module, imports);

Note:

This change would break the current docs and loader.

@dcodeIO
Copy link
Member

dcodeIO commented Oct 4, 2018

Yeah, I think we'll need to fix this. It's just the way it is because it was easier to share code between the synchronous and asynchronous APIs, but there might be other ways to achieve the same.

dcodeIO added a commit that referenced this issue Oct 4, 2018
@dcodeIO
Copy link
Member

dcodeIO commented Oct 4, 2018

Please let me know if this fixes it for you :)

@t57ser
Copy link
Author

t57ser commented Oct 4, 2018

This is what I get when using loader.instantiateStreaming

TypeError: Cannot read property 'memory' of undefined
    at postInstantiate (index.js:43)
    at Object.instantiateStreaming (index.js:189)

I tried to adapt the code to see where the issue might be.
First step:

async function instantiateStreaming(response, imports) {
	const a = await WebAssembly.instantiateStreaming(response, imports);
	console.log(a);
  return postInstantiate(
    preInstantiate(imports || (imports = {})),
    a
  );
}

I moved the await outside of the return to see what the result was.
I got a new error before I could continue:

LinkError: WebAssembly Instantiation: Import #0 module="env" function="abort" error: function import requires a callable

This got solved by adding:

env: {
	memory: new WebAssembly.Memory({ initial: 1 }),
	abort: () => null,
},

Apparently the abort function is required?

The result is the following:
{instance: Instance, module: Module}
Meaning that the instance passed to postInstantiate is actually supposed to be a.instance.

Chrome version: 69.0.3497.100 (Official Build) (64-bit)
I hope that helps

[EDIT] Forgot to mention:
After passing a.instance to postInstantiate it did work.
Obviously "a" is a bad name :)

@t57ser
Copy link
Author

t57ser commented Oct 4, 2018

The way I solved it for my project was:

async function instantiateBuffer(buffer, imports) {
  var module = new WebAssembly.Module(buffer);
  return await instantiate(module, imports);
}
exports.instantiateBuffer = instantiateBuffer;

async function instantiateStreaming(response, imports) {
  var module = await WebAssembly.compileStreaming(response);
  return await instantiate(module, imports);
}
exports.instantiateStreaming = instantiateStreaming;

I made the instantiate and co async and only changed the instance:

async function instantiate(module, imports) {
...
// Instantiate the module and obtain its (flat) exports
  var instance = await WebAssembly.instantiate(module, imports);

@dcodeIO
Copy link
Member

dcodeIO commented Oct 4, 2018

I see, yeah, I forgot about a.instance there. Does it work now? :)

@t57ser
Copy link
Author

t57ser commented Oct 5, 2018

Yes, it appears to work now. Thanks.

@t57ser t57ser closed this as completed Oct 5, 2018
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

No branches or pull requests

2 participants