[WIP] wasm support #2113

xtuc · 2018-04-06T13:28:44Z

Closes #2099

This is a very early PR but i'd like to collect some feedback from you guys.

xtuc · 2018-04-06T13:30:10Z

src/Chunk.ts

@@ -478,6 +479,8 @@ export default class Chunk {
 				} else if (resolution instanceof ExternalModule) {
 					node.renderFinalResolution(code, `"${resolution.id}"`);
 					// AST Node -> source replacement
+				} else if (resolution instanceof WasmModule) {
+					node.renderFinalResolution(code, `"${resolution.id}"`);


Currently the generated code is

Promise.resolve(require("/foobar/sventest/addTwo/src/addTwo.wasm")).then(({addTwo}) => { console.log("addTwo(1, 2)", addTwo(1, 2)); }); function then(resolve) { // ... } var addTwo = /*#__PURE__*/Object.freeze({ then: then });

I believe I need to change the resolution to return the addTwo object.

The inlining decision for dynamic imports is made here - https://github.com/rollup/rollup/pull/2113/files#diff-8c6f11349cc1bb370bdd557ed2bcbe7dR443. Can that be extended to a WASM check?

Ok, this has been fixed.

xtuc · 2018-04-06T13:31:19Z

src/rollup/index.ts

+
+							if (wasmModule !== undefined) {
+								promises.push(writeFile('./dist/addTwo.wasm', wasmModule.code));
+							}


I have no idea how to emit the file correct since it's a single file build.

This is why I was wondering if it's worth considering each chunk (file) as having a WASM and JS component, where if the WASM or JS component is empty it is just a WASM or JS file and not both. So Chunk as consisting of both WASM and JS modules, where the render process for a chunk then returns both a magic string and a buffer, and if either is empty it's a WASM or JS file. But perhaps that is quite odd.

The alternative would be a more generic emission API, or to just do a specific check for a WASM output.

I would prefer a more generic emission API. I feel like it's more future proof and we should take advantage of the Graph (to handle spllited or nested wasm builds).

Note that I have to MagicString in the initial JS module and write the wasm module in a file.

xtuc · 2018-04-06T13:31:36Z

src/rollup/types.d.ts

@@ -3,6 +3,10 @@ import * as ESTree from 'estree';

 export const VERSION: string;

+export interface WebAssemblyJSAst {
+	Program: object;
+}


I'll generate TS typing to have a better integration here

Not to worry about this - I think it may be best to keep WASM off this outward user-facing API initially. ModuleJSON is a semi-private caching interface for watched rebuilds.

I didn't realized at first that this is intended to be used by users. If I generate the TS typings on my side the user could still use them from NPM.

xtuc · 2018-04-06T13:32:33Z

src/WasmModule.ts

+
+	scope: ModuleScope;
+
+	ast: WebAssemblyJSAst.Program;


We could provide the same AST manipulation logic to plugins as with JS.

Forgot to mention that we can enable AST diffing + wasm module manipulation using this API. Which could be useful for plugin (mutable global import polyfilling for ex 💯 ).

xtuc · 2018-04-06T13:37:16Z

src/WasmModule.ts

+		this.sources = [];
+		this.dependencies = [];
+
+		this.scope = new ModuleScope(this);


I would like to remove the scope but a lot of code depends on it.

As far as I can see the only outside usages of .scope are in Chunk.setIdentifierRenderResolutions() which is basically the deconflicting logic for the given chunk. Instead of adding the scope here, we could introduce a local variable const orderedJsModules = this.orderedModules.filter(module => module instanceOf Module) and use it in Chunk.setIdentifierRenderResolutions() to only deconflict JS modules. But I may be overlooking other usages here.

xtuc · 2018-04-06T13:37:51Z

src/WasmModule.ts

+		const URL = 'foo';
+		const content = buildLoader({ URL });
+
+		return { trim() {}, content };


I'm not sure how to constructor the MagicString object in that case.

WasmModule.render must return a Buffer surely? I think we should handle the distinction in the caller sites.

Following my comment #2113 (comment), since i'm writing the file myself I bypass this logic actually, but I use it to inject the loader needed on the JS module. Does it make sense to you?

guybedford · 2018-04-06T14:05:14Z

src/Graph.ts

@@ -523,6 +524,7 @@ export default class Graph {

 			for (let depModule of module.dependencies) {
 				if (depModule instanceof ExternalModule) continue;
+				if (depModule instanceof WasmModule) continue;


Perhaps we should not permit WasmModule dependencies at all initially, and throw before even populating dependencies with a WasmModule?

guybedford · 2018-04-06T14:05:43Z

src/Graph.ts

+					this.modules.push(module);
+
+					return module;
+				});


Don't worry about having this blatant hack here - let's leave this as a big TODO and come back to it later.

Thinking about this a little more - instead of trying to introduce some new type of plugin or resolve API to distinguish WASM, let's just use the binary header bytes detection method for this. Saves a lot of pain!

lukastaegert

Hi, sorry for not getting back to you earlier, I'm a little tied up in other things right now. This looks really nice and concise, I like it! I hope to find some time soon to have a deeper look at this.

For a longer term perspective, I would advocate the following changes (which we might postpone for now to avoid bigger merge conflicts):

rename Module -> JsModule
introduce a new Interface Module that is implemented by JsModule, WasmModule and ExternalModule and contains the minimal shared API of those
introduce another Interface InternalModule extends Module that is implemented only by JsModule and WasmModule
Use those interfaces wherever possible in Graph and Chunk

The interfaces should contain minimal APIs; this will help us structure our architecture better and greatly improve encapsulation. Also, this will make it much easier later to extract the Wasm logic.

Along with this, improving the way files are emitted is certainly worthwhile.

lukastaegert · 2018-04-09T06:51:22Z

src/WasmModule.ts

+		this.sources = [];
+		this.dependencies = [];
+
+		this.scope = new ModuleScope(this);


As far as I can see the only outside usages of .scope are in Chunk.setIdentifierRenderResolutions() which is basically the deconflicting logic for the given chunk. Instead of adding the scope here, we could introduce a local variable const orderedJsModules = this.orderedModules.filter(module => module instanceOf Module) and use it in Chunk.setIdentifierRenderResolutions() to only deconflict JS modules. But I may be overlooking other usages here.

xtuc · 2018-04-09T11:33:18Z

@lukastaegert no worries, we are all busy. Thanks for your review!

I totally agree with the Module interface. I would also separate TextModule from BinaryModule because they are a bit different and might be easier for emitting the files.

xtuc · 2018-04-10T10:21:45Z

I noticed that most of the typing issues left are from not having an abstraction between Modules. Do you think it would make sense to introduce it here?

Apart from tests we already have a MVP.

guybedford · 2018-04-13T12:05:37Z

Sorry for the delay here - just trying to work through some of the considerations here more thoroughly. Will aim to post back some suggestions / thoughts for feedback by next week.

xtuc · 2018-04-13T15:43:02Z

No worries @guybedford

We should probably define what the MVP should include?

Currently:

dynamic loading of one WASM binaries

Before we can merge I would like to add:

multiple binaries (conflict with the then function)
unit test
Proper file emit API (especially because of the destination being mapped on the URL)

guybedford

Thanks for your patience here @xtuc, I've been trying to think about this direction quite carefully, but it seems like close-nit graph handling via classes for both the input and output graphs is the ideal model we're after.

There will still be pain points moving in this direction, but those are the things we should be cleaning up as we go.

guybedford · 2018-04-13T15:39:11Z

src/Chunk.ts

@@ -11,6 +11,7 @@ import { normalize, resolve, extname, dirname, relative, basename } from './util
 import { RawSourceMap } from 'source-map';
 import Graph from './Graph';
 import ExternalModule from './ExternalModule';
+import WasmModule from './WasmModule';


I've been thinking perhaps we need to create WasmChunk.ts as well as Chunk is effectively designed to mimic the output graph, so having the two source types would be the best model to match.... this will likely have some complexities on the types like with Module, but I tend to think those are the problems we should be solving here.

Then as well, just like with Module, an abstract Chunk class would likely make sense to share between both implementations, ideally providing the same sort of interface for src/rollup.index.ts as far as possible, although for the actual render, the output value would be a different interface representing the binary which could be split handled at that point.

guybedford · 2018-04-13T15:40:30Z

src/Graph.ts

@@ -52,7 +53,7 @@ export default class Graph {
 		importedModule: string,
 		importerStart?: number
 	) => void;
-	moduleById: Map<string, Module | ExternalModule>;
+	moduleById: Map<string, Module | WasmModule | ExternalModule>;


A base-level Module abstract class may well make sense at this point I think.

guybedford · 2018-04-13T15:41:25Z

src/WasmModule.ts

+		const content = buildLoader({ URL, NAME });
+
+		return { trim() {}, content };
+	}


If we have WasmChunk calling render, then it will be ok for the render method to return a binary (plus binary source map in due course).

guybedford · 2018-04-13T15:42:45Z

src/WasmModule.ts

+	getDynamicImportExpressions(): (string | Node)[] {
+		// FIXME(sven): consider ModuleImport as dynamicImports?
+		return [];
+	}


It won't be possible for WASM to dynamically import another module, so these things all can then be removed.

I agree, it was more an implementation detail about how to declare the imports. The static imports are better.

guybedford · 2018-04-13T15:47:51Z

@xtuc good timing :) Yes I think we're agreed on this initial PR just covering the dynamic import case, although recursive loading of dependencies could be useful.

I guess I've answer your question for file emission based on having a new Chunk class? But let me know if you have any further questions around the emission at all.

multiple binaries (conflict with the then function)

Could you clarify this a little?

xtuc · 2018-04-13T15:53:57Z

src/WasmModule.ts

+};
+
+const buildLoader = ({ NAME, URL, IMPORT_OBJECT }: BuildLoadArgs) => `
+	// function then$${NAME}(resolve) {


multiple binaries (conflict with the then function)

I had a comment but it seems to be gone. I was referring to this. @guybedford

guybedford · 2018-04-13T15:59:17Z

src/WasmModule.ts

+			.then(resolve)
+			.catch(resolve);
+	}
+`;


Can this be treated as the "dynamic import mechanism" as is used in the JS Chunk.ts file that does the import to the associated wasmChunk.ts file?

(and let me know if that would handle the issue you raised now?)

I guess it would need to be a wasmImport or similar mechanism associated with the dynamic import specifically in the wasm case.

The point being that we are handling this in the src/ast/nodes/Import.ts render itself as the dynamic import rendering.

Sorry but i'm really not sure to understand that.

"dynamic import mechanism"

Yes, that's the code behind import.

(and let me know if that would handle the issue you raised now?)

You mean the binding conflict issue?

guybedford · 2018-04-13T16:03:23Z

Also, please let me know if what I'm suggesting isn't making sense here, and I can always try and run some commits against your branch myself to see if I can better explain the models.

xtuc · 2018-04-13T16:09:17Z

Also, please let me know if what I'm suggesting isn't making sense here, and I can always try and run some commits against your branch myself to see if I can better explain the models.

I have some difficulties for understanding the Rollup stuff, which is normal.

I would probably ask for some help for the WasmChunk and module split.

guybedford · 2018-04-13T16:12:50Z

So basically import('./x.wasm').then(...) could be replaced inline with:

Promise.resolve().then(() => {
  if (typeof WebAssembly.instantiateStreaming !== 'function')
    throw new Error('WebAssembly.instantiateStreaming is not supported');
  if (typeof window.fetch !== 'function')
    throw new Error('window.fetch is not supported');
  return WebAssembly.instantiateStreaming(fetch(...url...), {...importObject...})
  .then(res => res.instance.exports);
}).then(...)

With this rewriting itself being treated as the "dynamic import mechanism". This way we can change the mechanism for different module formats, or have it work in Node and browser or both etc.

guybedford · 2018-04-13T16:13:38Z

Sure, if you try do as much as you can, then I'll jump in when I have time maybe not this weekend but the next.

xtuc · 2018-04-13T16:21:56Z

I think I understand what you mean. I already thought about using a JS module to inject the loader but had some trouble creating a module from a string.

Would that make sense to you?

Sure, if you try do as much as you can, then I'll jump in when I have time maybe not this weekend but the next.

Thanks!

guybedford · 2018-04-13T16:48:51Z

I think I understand what you mean. I already thought about using a JS module to inject the loader but had some trouble creating a module from a string.

Yes completely, so what I'm suggesting is basically inlining the wasm loader into each of its callsites - the benefit being that when outputting for system modules or es modules when they support it, we can fall back to outputting a normal dynamic import to the wasm utilising the environment loader.

As for separating the loader into its own shared helper module / block, this is certainly an optimization that can be added, I guess I'm just trying to focus on the architecture first here.

xtuc · 2018-04-13T17:44:23Z

src/WasmModule.ts

+};
+
+// FIXME(sven): uncommented the loader you need for your current env
+// how to get the env?


That was an attempt for adding tests. How can I get access to the env i'm in? I guess I could use the finalizers for that?

Also sometime I see __dirname is not defined when running the tests.

shellscape · 2019-08-11T16:36:23Z

@xtuc hey there. any interest in picking this back up? Rollup has made a lot of progress since the last activity on this PR.

xtuc · 2019-08-15T10:53:42Z

@shellscape, I'm interested in moving this forward. However, I'm not able to spend the time I'd like.

Could you please tell me what changed in Rollup since this PR? I remember that I had to add blob/binary support to JavaScript Modules.

Also, I'm happy to jump on a video chat.

shellscape · 2019-12-23T16:23:15Z

I think we can safely close this one now. @rollup/plugin-wasm has come a ways since and works fairly well, and it's about to get a major boost thanks to contributors. Please ping if anyone disagrees with this choice.

feat: add WasmModule

9c67efe

xtuc commented Apr 6, 2018

View reviewed changes

xtuc mentioned this pull request Apr 6, 2018

Better WebAssembly integration #2099

Open

xtuc commented Apr 6, 2018

View reviewed changes

guybedford reviewed Apr 6, 2018

View reviewed changes

xtuc added 4 commits April 6, 2018 21:44

fix: resolution

5297d6c

feat: allow multiple modules

2f154ca

chore: [skip ci] remove unnecessary change

9791a12

fix: @webassemblyjs/ast typing

4602b1b

lukastaegert reviewed Apr 9, 2018

View reviewed changes

fix: few typing issues

f015343

guybedford reviewed Apr 13, 2018

View reviewed changes

xtuc commented Apr 13, 2018

View reviewed changes

guybedford reviewed Apr 13, 2018

View reviewed changes

xtuc mentioned this pull request Apr 13, 2018

Circular dependency Issue with v1.2.4 xtuc/webassemblyjs#254

Closed

xtuc added 2 commits April 13, 2018 19:41

chore: bump webassemblyjs

f34980b

feat: attempt at adding tests

5caf648

xtuc commented Apr 13, 2018

View reviewed changes

guybedford mentioned this pull request Apr 18, 2018

Binary files become corrupted by Rollup's default load function #1786

Closed

chore: bump webassemblyjs

0dcf890

shellscape added pr³ 🔧 feature x⁴ ⋅ hold labels Aug 11, 2019

jamen mentioned this pull request Dec 20, 2019

WebAssembly #1636

Closed

shellscape closed this Dec 23, 2019

[WIP] wasm support #2113

[WIP] wasm support #2113

Conversation

xtuc commented Apr 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtuc Apr 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtuc Apr 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukastaegert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtuc commented Apr 9, 2018

xtuc commented Apr 10, 2018

guybedford commented Apr 13, 2018

xtuc commented Apr 13, 2018

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtuc Apr 13, 2018 • edited

Choose a reason for hiding this comment

guybedford commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford commented Apr 13, 2018

xtuc commented Apr 13, 2018 • edited

guybedford commented Apr 13, 2018

guybedford commented Apr 13, 2018

xtuc commented Apr 13, 2018 • edited

guybedford commented Apr 13, 2018

Choose a reason for hiding this comment

shellscape commented Aug 11, 2019

xtuc commented Aug 15, 2019

shellscape commented Dec 23, 2019

xtuc Apr 6, 2018 •

edited

xtuc Apr 6, 2018 •

edited

xtuc Apr 13, 2018 •

edited

xtuc commented Apr 13, 2018 •

edited

xtuc commented Apr 13, 2018 •

edited