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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

use WebAssembly.instantiateStreaming #6709

Closed
wants to merge 50 commits into from
Closed

Conversation

@xtuc
Copy link
Member

@xtuc xtuc commented Mar 8, 2018

This is probably going to be a long PR, that's just an early preview.

What kind of change does this PR introduce?

wasm-edit and WebAssembly.instantiateStreaming

Also includes #6531.

Closes #6433

For ref xtuc/webassemblyjs#196

Did you add tests for your changes?

馃憤 Not needed, existing tests.

If relevant, link to documentation update:

馃憤 nothing changes from the user point of view

Summary

Does this PR introduce a breaking change?

馃憤 no, it mostly unbreaks

Other information

A few open thoughts

  • Node 8 is not required anymore (because in #6531 it does)
  • Currently each edit/add/decode decodes the binary, I will find a way to batch them and avoid the unnecessary work

Unfortunately using a WebAssembly.Table for the interop with Webpack don't work because we are not able to set JavaScript functions.

The table has some good advantages tho:

  • avoids shifting every call indices (because it uses call_indirect instead)
  • reduces the integration points webpack<>wasm
  • will hold more type of values in the future which would fix the live-binding issue
  • when multiple table will be allowed that will be completely isolated

Another idea, we could inject a new function that will fill the globals that were imported.

For example:

(module
  (import "env" "global" (global i32))
  (func (export "t") (result i32)
    (get_global 0)
  )
)

Transformations:

  • remove the global import (because you can't import a mutable global)
  • adds the init function
(module
  (global (mut i32) (i32.const 0))
  (func (export "t") (result i32)
    (get_global 0)
  )
  (func (export "init") (param i32)
    (get_local 0)
    (set_global 0)
  )
)

We first call init with the value of the global, which will then call the start function if provided.

This solution doesn't require much transformation to the binary. According to v8's limits, a function can have 1k params (so 1k global imports, note that if that doesn't suffice we can add a second init function):
https://github.com/v8/v8/blob/db1e370decbd4d2acd4b5c2ebfee76b3c24c5cae/src/wasm/wasm-limits.h#L36

While we change the mutability of the global, that program has already been compiled and ensures that the user don't modify the global (I hope).

Globals and functions indices are preserved.

sokra and others added 12 commits Feb 20, 2018
Fixes the parsing of the start section in WASM
chore: bump webassembly-interpreter
refactor: use webassemblyjs tools
鈥r-tooling
import("./test.wasm").then(({getTwo, logFoo}) => {
console.log("getTwo", getTwo());
console.log(logFoo());
})

This comment has been minimized.

@xtuc

xtuc Mar 8, 2018
Author Member

Here's a small example.

"var instance = new WebAssembly.Instance(__webpack_require__.w[module.i], {" +
generateImports(),
"});",
"var instance = __webpack_require__.w[module.i]",

This comment has been minimized.

@xtuc

xtuc Mar 8, 2018
Author Member

I made the choose that __webpack_require__.w only contains the WebAssembly.instance (WebAssembly.Module is not preserve because not used)

xtuc added 2 commits Mar 9, 2018
xtuc added 4 commits Mar 12, 2018
Copy link
Member

@sokra sokra left a comment

Looks pretty good so far.

FetchCompileWasmMainTemplatePlugin is only used for browser target. The tests and node.js target uses ReadFileCompileWasmTemplatePlugin, this need to be changed too. It makes sense here to move the common code into a new file.

)})[${JSON.stringify(data.usedName)}](${params})`;
}

if (data.description.type === "GlobalType") {

This comment has been minimized.

@sokra

sokra Mar 14, 2018
Member

Is this used? I guess all globals are filtered by the first loop.

This comment has been minimized.

@xtuc

xtuc Mar 14, 2018
Author Member

Yes, I removed it

]),
"}"
// FIXME(sven): ensrue this still works / change it
"promises.push(response.arrayBuffer().then(function(bytes) { installedWasmModules[wasmModuleId] = WebAssembly.compile(bytes); }));"

This comment has been minimized.

@sokra

sokra Mar 14, 2018
Member

WebAssembly.compile -> WebAssembly.instantiate

It returns a promise and the result need to be stored in installedWasmModules

This comment has been minimized.

@xtuc

xtuc Mar 14, 2018
Author Member

Yeah I didn't change this branch, that's why I added the FIXME. I'll fix this at the end so I can also test it

xtuc added 2 commits Mar 14, 2018
@webpack-bot
Copy link
Contributor

@webpack-bot webpack-bot commented Mar 14, 2018

@xtuc Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

This reverts commit baea536.
@xtuc xtuc changed the title use WebAssembly.instantiateStreaming [WIP] use WebAssembly.instantiateStreaming Mar 29, 2018
Sven SAULEAU and others added 9 commits Mar 29, 2018
Sven SAULEAU
This reverts commit 0ef8c12.
@xtuc xtuc changed the title [WIP] use WebAssembly.instantiateStreaming use WebAssembly.instantiateStreaming Mar 29, 2018
@webpack-bot webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Mar 30, 2018
@webpack-bot
Copy link
Contributor

@webpack-bot webpack-bot commented Mar 30, 2018

Thank you for your pull request! The most important CI builds succeeded, we鈥檒l review the pull request soon.

@ashleygwilliams
Copy link

@ashleygwilliams ashleygwilliams commented Apr 19, 2018

hello! i am a member of the Rust-WASM WG and we would really love to see this PR merged. is there anything we can do to help get it over the finish line?

@sokra
Copy link
Member

@sokra sokra commented Apr 23, 2018

Hi @ashleygwilliams

I want to merge @xtuc code in a way keeping the old mode and allowing to choose between modes (i. e. when unsupported fetures are used, or when running in other environments like webworker or node.js).

I hope to be able to takle that this week.

@xtuc
Copy link
Member Author

@xtuc xtuc commented Apr 23, 2018

@sokra feel free to ping me, I can make some time to help you.

@sokra
Copy link
Member

@sokra sokra commented Apr 28, 2018

merged into next branch

@sokra sokra closed this Apr 28, 2018
@sokra
Copy link
Member

@sokra sokra commented Apr 28, 2018

see #7144

@xtuc xtuc deleted the xtuc:feat-rewrite-wasm branch Apr 28, 2018
@sokra sokra added the Web Assembly label May 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can鈥檛 perform that action at this time.