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

Loader: support .wasm imports #3328

Merged
merged 6 commits into from Nov 14, 2019

Conversation

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Nov 13, 2019

import { myFunc } from "./my.wasm";
myFunc(10);

Imported WASM file can import from other JS files as follows

(module
  (import "./wasm-dep.js" "jsFn" (func $jsFn (result i32)))
  (import "./wasm-dep.js" "jsInitFn" (func $jsInitFn))
  (import "http://localhost:4545/cli/tests/051_wasm_import/remote.ts" "jsRemoteFn" (func $jsRemoteFn (result i32)))
  ;; ...
)

See cli/compilers/wasm.rs and cli/compilers/wasm_wrap.js for details.

Currently supporting only default exports since we currently have no access to Worker for running code from Compiler trait. (since also means currently we need --allow-read to do dynamic imports (doable under top-level await)). This is different from Node.js, which implements named exports, since Node tries to compile WASM module first (as it can run JS code before module creation), extracting wasmModule.imports and wasmModule.exports, and build a dynamic script with static imports and explicit named exports and compile the script as a separate ES module. See https://github.com/nodejs/node/blob/35ec01097b2a397ad0a22aac536fe07514876e21/lib/internal/modules/esm/translators.js#L190-L210

Named exports is implemented.

Usage of base64 encoding is debatable if we can accept failure of resolution given cases where Deno namespace is hidden. Braces in wasm_wrap.js are double braces to avoid breaking format!() macro. Using string replacements

@kevinkassimo kevinkassimo force-pushed the kevinkassimo:loader/wasm branch from 48b64fd to 22e4670 Nov 13, 2019
@kevinkassimo

This comment has been minimized.

Copy link
Contributor Author

kevinkassimo commented Nov 13, 2019

Looks like test lock_check_err2 is badly flaky.

@kevinkassimo kevinkassimo force-pushed the kevinkassimo:loader/wasm branch from ee3fdfb to a8cd4ab Nov 13, 2019
@kevinkassimo

This comment has been minimized.

Copy link
Contributor Author

kevinkassimo commented Nov 13, 2019

This actually also implies that ./target/debug/deno --allow-read my.wasm would also work.

@kevinkassimo kevinkassimo force-pushed the kevinkassimo:loader/wasm branch from bbad50e to e9a2763 Nov 13, 2019
Copy link
Contributor

kitsonk left a comment

Awseome 🎉 Just a couple of thoughts.

sourceFileJson.filename
);
// Create declaration file on the fly.
const _ = new SourceFile({

This comment has been minimized.

Copy link
@kitsonk

kitsonk Nov 13, 2019

Contributor

Why even assign this to a variable? It is never read.

But good way of tackling the problem. The interesting thing will be if we can actually get the shape of the named exports of the WASM file, and actually pass into the compiler a dynamically generated set of source code.

@@ -0,0 +1,24 @@
import wasmExports from "./051_wasm_import/simple.wasm";

This comment has been minimized.

Copy link
@kitsonk

kitsonk Nov 13, 2019

Contributor

Hmmm, is this the proposed semantics for WASM imports, for all of the exports to go into the default export, instead of import * as wasmExports from "..."?

This comment has been minimized.

Copy link
@kevinkassimo

kevinkassimo Nov 14, 2019

Author Contributor

The standards seems to imply named exports. However we have problem collecting WASM imports and exports names since our current compiler does not support running some JS code. I don’t really know if there is a way I can directly get these info from Rust side other than either delegate this task to JS or using something like wasmer_runtime (which is too heavy ad it is a full runtime). Will investigate if there is some V8 APIs I could use instead...

This comment has been minimized.

Copy link
@kevinkassimo

kevinkassimo Nov 14, 2019

Author Contributor

Actually I think I might be able to figure out export names with some extra tricks. Experimenting

This comment has been minimized.

This comment has been minimized.

Copy link
@kevinkassimo

kevinkassimo Nov 14, 2019

Author Contributor

@ry I'm actually planning to do something similar to TsCompiler to delegate discovery of imports/exports through a separate worker, and construct a script based on the message posted back. These V8 APIs seems lacking access to import/export details...

This comment has been minimized.

Copy link
@ry

ry Nov 14, 2019

Collaborator

I'm not sure what you need exactly but I'm sure it's there somewhere. Check out GetModuleNamespace.

This comment has been minimized.

Copy link
@kevinkassimo

kevinkassimo Nov 14, 2019

Author Contributor

@ry I believe that is from v8::internal that is not public?
Anyways I am now able to get named exports working now by having a JS worker compile and get imports/exports for me before I construct the static scripts.

This comment has been minimized.

@@ -0,0 +1,25 @@
function base64ToUint8Array(data) {{

This comment has been minimized.

Copy link
@ry

ry Nov 14, 2019

Collaborator

If this file is to have a .js filename extension, then it should be syntactically valid JS.

Rather than using format!, which induces these double braces, it would be better to use a template of some sort. Maybe you can just do a substring replacement on BASE64_ENCODED_WASM (or something)


const instance = new WebAssembly.Instance(compiled, importObject);

export default instance.exports;

This comment has been minimized.

Copy link
@ry

ry Nov 14, 2019

Collaborator

What's cool about this is I think it would even work with deno bundle

This comment has been minimized.

Copy link
@kitsonk

kitsonk Nov 14, 2019

Contributor

yeah, actually that would simplify what I was going to do, there are consequences which I will detail in the related issue though.

@kevinkassimo

This comment has been minimized.

Copy link
Contributor Author

kevinkassimo commented Nov 14, 2019

Now supports named exports. Done through booting up a JS worker to extract imports/exports for us, sending back the info, and construct a static script based on it.

Bonus point: now imports are all static, so --allow-read flag can be dropped

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented on cli/tests/051_wasm_import.ts in aab0857 Nov 14, 2019

👍

@ry
ry approved these changes Nov 14, 2019
Copy link
Collaborator

ry left a comment

LGTM - very nice - I'm sure we'll need to iterate on this but I'm happy to land now that you've got the exports working.

@ry ry merged commit 4189cc1 into denoland:master Nov 14, 2019
10 checks passed
10 checks passed
test macOS-latest
Details
test_std macOS-latest
Details
test windows-2019
Details
test_std windows-2019
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
test_std ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
axetroy added a commit to axetroy/deno that referenced this pull request Nov 15, 2019
fix lint

format code

fix

format rust code

update arg name

refactor: per-worker resource table (denoland#3306)

- removes global `RESOURCE_TABLE` - resource tables are now created per `Worker`
  in `State`
- renames `CliResource` to `StreamResource` and moves all logic related
  to it to `cli/ops/io.rs`
- removes `cli/resources.rs`
- adds `state` argument to `op_read` and `op_write` and consequently adds
  `stateful_minimal_op` to `State`
- IMPORTANT NOTE: workers don't have access to process stdio - this is
  caused by fact that dropping worker would close stdout for process
  (because it's constructed from raw handle, which closes underlying file
  descriptor on drop)

Revert "refactor: per-worker resource table (denoland#3306)"

This patch does not work with the recent bundler changes (denoland#3325).
Unfortunately I didn't merge master before landing this patch. It has
something to do with console.log not working inside the compiler worker.

This reverts commit fd62379.

Loader: support .wasm imports (denoland#3328)

* loader: support .wasm imports

* http_server: true

* Support named exports

* Clippy

add RUST_BACKTRACE to ci

refactor: per-worker resource table, take 2 (denoland#3342)

- removes global `RESOURCE_TABLE` - resource tables are now created per `Worker`
  in `State`
- renames `CliResource` to `StreamResource` and moves all logic related
  to it to `cli/ops/io.rs`
- removes `cli/resources.rs`
- adds `state` argument to `op_read` and `op_write` and consequently adds
  `stateful_minimal_op` to `State`
- IMPORTANT NOTE: workers don't have access to process stdio - this is
  caused by fact that dropping worker would close stdout for process
  (because it's constructed from raw handle, which closes underlying file
  descriptor on drop)

Turn on TS strict mode for deno_typescript (denoland#3330)

improve test

add test output message for debug

try fix

update

update

fix lint

disable test

update

update

debug

debug msg

trigger ci

trigger ci 1

update

update

format code
axetroy added a commit to axetroy/deno that referenced this pull request Nov 15, 2019
* loader: support .wasm imports

* http_server: true

* Support named exports

* Clippy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.