WASM: Tree Shaking, Export and import name mangling by sokra · Pull Request #7275 · webpack/webpack

sokra · 2018-05-11T07:12:31Z

What kind of change does this PR introduce?
feature + bugfix

Did you add tests for your changes?
yes

If relevant, link to documentation update:
n/a

Summary

remove unused exports from WASM
replace export names with shorter names when used in a analyseable way
replace import names with shorter names
replace import modules with a
generate shorter code for the importsObject
enforce direct connection when using non-js types

Does this PR introduce a breaking change?
no

Other information

sokra · 2018-05-11T07:14:49Z

+			if (usedName) {
+				path.node.name = usedName;
+				// TODO remove this when fixed in @webassemblyjs
+				path.node.descr.id = t.numberLiteral(+path.node.descr.id.raw);


@xtuc This seem to be a bug in edit. Identifiers are not accepted as id.

sokra · 2018-05-11T07:15:11Z

+				if (path.node.descr.id)
+					path.node.descr.id = t.numberLiteral(+path.node.descr.id.raw);
+				if (path.node.descr.name)
+					path.node.descr.name = t.numberLiteral(+path.node.descr.name.raw);


@xtuc Same here for id and name

xtuc · 2018-05-11T07:32:24Z

 	generate(module) {
 		const bin = module.originalSource().source();

+		const initFuncId = t.identifier(


Btw we need to avoid colliding with an user's export, like __webpack_init__ is a legal export.

I take the risk for now...

xtuc · 2018-05-11T07:33:28Z

 */
 const isTableImport = moduleImport => moduleImport.descr.type === "Table";

+const JS_COMPAT_TYPES = new Set(["i32", "u32", "f32"]);


u32 shouldn't be there, I believe it's an implementation details in webassemblyjs which showed up.

Also missing f64.

xtuc · 2018-05-11T07:41:37Z

 	return add(bin, [func, moduleExport, funcindex, functype]);
 };

+const getImportMangleMap = module => {


Could we also allow an noMangle option in the rule? Might be easier to debug.

Jessidhia

I guess there's no infrastructure to compile the .wasm in the test suite yet?

Jessidhia · 2018-05-11T09:50:45Z

+		return Template.asString([
+			`${JSON.stringify(module.id)}: function() {`,
+			Template.indent([
+				`return promiseResolve().then(function() { return ${promise}; }).then(function(${variable}) {`,


If you don't need to wait for a microtask you could just resolve the promise directly.

It's used to delay the evaluation of the promise code, because wasm modules may not be created in the "correct" order. A microtask later all variables are filled.

But you are right we don't need the microtask delay if we would use the correct order. I'll add a TODO to topo-sort them instead.

Ok I tried but it's impossible to sort them correctly, because of splitChunks.

webpack generates code like this to load multiple parts of a chunk group: Promise.all([__webpack_require__.e(1), __webpack_require__.e(2)]). For modules with a dependency chain of a -> b -> c and with a and c in chunk 1 and b in chunk 2, we can't solve this by sorting. So I'll stay with the microtask delay solution.

I added a comment into the code

Jessidhia · 2018-05-11T09:51:57Z

 			`${JSON.stringify(module.id)}: function() {`,
 			Template.indent([
-				`return Promise.resolve().then(function() { return Promise.all([${promises}]); }).then(function(array) {`,
+				`return promiseResolve().then(function() { return Promise.all([${promises}]); }).then(function(array) {`,


Jessidhia · 2018-05-11T09:52:06Z

 					"// object to store loaded and loading wasm modules",
 					"var installedWasmModules = {};",
 					"",
+					"function promiseResolve() { return Promise.resolve(); }",


Why no Promise.resolve.bind(Promise)?

I would have used var x = Promise.resolve() but the problem is that Promise may not be available on bootstrap, as the user could load the Promise polyfill in the entrypoint. This ensures that Promise is not used until import() is used.

We actually had issue reports about that...

Jessidhia · 2018-05-11T09:53:17Z

+			if (result === undefined) {
+				path.remove();
+			} else {
+				path.node.module = "a";


Why is this hardcoded? 🤔

xtuc · 2018-05-11T12:51:38Z

@Kovensky

I guess there's no infrastructure to compile the .wasm in the test suite yet?

We could compile wast (text format) to wasm (binary format) files.

mangle webpack init function store WebAssembly.Instance.exports directly connect wasm modules directly when already cached

xtuc · 2018-05-12T09:50:20Z

+				// TODO remove this when fixed in @webassemblyjs
+				path.node.descr.id = t.numberLiteral(+path.node.descr.id.raw);
+			} else {
+				path.remove();


This only removes a couple of bytes from the export section. We can implement proper func elimination in the future , I'ill work on that.

This is all work we need to do at this place. DCE will be the job of the WASM minimizer (to be defined/implemented), which will operate on asset level.

Ok, sounds good. I have a couple of wasm optimizations that we could put there as well.

xtuc · 2018-05-12T10:20:56Z

+			if (usedName) {
+				path.node.name = usedName;
+				// TODO remove this when fixed in @webassemblyjs
+				path.node.descr.id = t.numberLiteral(+path.node.descr.id.raw);


Sorry but I don't understand why you are renaming the internal name here?

How are the usage detected? I'm not able to trigger the DCE.

import {test} from "./test.c.wasm"; test(); // generator.js module.isUsed("test") // test module.isUsed("nonexisting") // nonexisting

You need to compile in production mode or enable optimization.usedExports.

I also don't understand why this line is needed, but it crashes without with some error about Identifiers is not allowed somewhere inside.

Ok, I couldn't reproduce it thanks. I'll clarify that.

Maybe it's already fixed, I'll check that.

Nope, still broken. To reproduce:

checkout this branch

yarn jest TestCasesProduction -t wasm --watch --no-verbose

tests are passing

remove this line

tests are failing

Error: C:\Users\tobia\Repos\webpack\test\cases\wasm\simple\wasm.wasm?1 Unsupported node Identifier 198 | */ 199 | const rewriteExportNames = ({ ast, module }) => bin => { > 200 | return editWithAST(ast, bin, { 201 | ModuleExport(path) { 202 | const usedName = module.isUsed(path.node.name); 203 | if (usedName) { at assertNotIdentifierNode (node_modules/@webassemblyjs/wasm-gen/lib/encoder/index.js:39:11) at Object.encodeModuleExport (node_modules/@webassemblyjs/wasm-gen/lib/encoder/index.js:205:3) at encodeNode (node_modules/@webassemblyjs/wasm-gen/lib/index.js:40:22) at applyUpdate (node_modules/@webassemblyjs/wasm-edit/lib/apply.js:42:54) at node_modules/@webassemblyjs/wasm-edit/lib/apply.js:224:17 at Array.forEach (<anonymous>) at applyOperations (node_modules/@webassemblyjs/wasm-edit/lib/apply.js:218:7) at editWithAST (node_modules/@webassemblyjs/wasm-edit/lib/index.js:83:44) at bin (lib/wasm/WebAssemblyGenerator.js:200:9) at value (lib/wasm/WebAssemblyGenerator.js:26:19) at value (lib/wasm/WebAssemblyGenerator.js:26:26) at value (lib/wasm/WebAssemblyGenerator.js:26:26) at value (lib/wasm/WebAssemblyGenerator.js:26:26) at value (lib/wasm/WebAssemblyGenerator.js:26:26) at WebAssemblyGenerator.generate (lib/wasm/WebAssemblyGenerator.js:369:18)

xtuc · 2018-05-12T10:39:29Z

+				if (path.node.descr.id)
+					path.node.descr.id = t.numberLiteral(+path.node.descr.id.raw);
+				if (path.node.descr.name)
+					path.node.descr.name = t.numberLiteral(+path.node.descr.name.raw);


i'm not sure we need to rename the path.node.descr, it's only the internal reference and uses an index anyway.

Same as above. Try to remove these lines and run the test suite

xtuc · 2018-05-14T10:33:23Z

Could we transform this warning into an error? That will fail for sure at runtime.

3:0-3 "export 'foo' was not found in './test.c.wasm'

webpack-bot · 2018-05-16T08:01:54Z

For maintainers only:

This need to be documented (issue in webpack/webpack.js.org will be filed when merged)

bump webassembly and few fixes

# Conflicts: # package.json # yarn.lock

xtuc · 2018-05-21T13:42:09Z

+			if (result === undefined) {
+				path.remove();
+			} else {
+				path.node.module = WebAssemblyUtils.MANGLED_MODULE;


This must be variable, because otherwise every import will be from the same module.

module = file
name = named export

or we need to update the ImportObject in the runtime but something isn't matching correctly

Few wasm fixes

# Conflicts: # yarn.lock

bump webassemblyjs 1.5.7

webpack-bot · 2018-05-25T14:34:36Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

xtuc · 2018-05-25T14:35:20Z

🎉

wasm: correct initializer type for rewritten globals

webpack-bot added the PR: unreviewed label May 11, 2018

sokra requested a review from xtuc May 11, 2018 07:12

sokra commented May 11, 2018

View reviewed changes

webpack-bot added the PR: CI-not-ok label May 11, 2018

sokra force-pushed the feature/wasm-mangling branch from 0871d77 to 4131026 Compare May 11, 2018 07:43

xtuc reviewed May 11, 2018

View reviewed changes

Jessidhia reviewed May 11, 2018

View reviewed changes

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels May 11, 2018

sokra added 7 commits May 11, 2018 20:57

mangle exports of wasm modules

f903cd3

mangle webpack init function store WebAssembly.Instance.exports directly connect wasm modules directly when already cached

Mangle import names and module requests

fb29d63

enforce direct import when types are incompatible with JS types

cb3a0ed

generate shorter runtime code for the imports map

ee0b5af

fix compat types list

08e72f5

move common code into separate module

42c0214

lint and type

22ec604

sokra force-pushed the feature/wasm-mangling branch from 062fe94 to 22ec604 Compare May 11, 2018 19:05

add comment about the microtask delay

cd7980e

xtuc reviewed May 12, 2018

View reviewed changes

sokra added the Web Assembly label May 13, 2018

xtuc mentioned this pull request May 14, 2018

ModuleExport by index in wasm xtuc/webassemblyjs#313

Merged

xtuc added 3 commits May 16, 2018 13:22

chore: bump to v1.5.0

426e625

refactor : use AST utils

d7d82b2

chore: bump webassemblyjs to v1.5.1

78cecc7

xtuc mentioned this pull request May 16, 2018

bump webassembly and few fixes #7319

Merged

sokra added 2 commits May 16, 2018 16:51

Merge pull request #7319 from xtuc/feat-bump-webassemblyjs

b2aece7

bump webassembly and few fixes

Merge branch 'master' into feature/wasm-mangling

8e05ae2

# Conflicts: # package.json # yarn.lock

webpack-bot added PR: CI-not-ok and removed PR: CI-ok labels May 17, 2018

xtuc added 4 commits May 21, 2018 13:27

chore: update webassemblyjs

98541fd

feat: add missing preprocessing

6d8bc91

feat(wasm): pass arg in test

ffeca24

chore: bump webassemblyjs

7b535b3

xtuc reviewed May 21, 2018

View reviewed changes

xtuc and others added 3 commits May 21, 2018 21:15

chore: correct version

586469a

Merge pull request #7364 from xtuc/chore-bump-webassemblyjs6

ec0369d

Few wasm fixes

Merge branch 'master' into feature/wasm-mangling

d46e35e

# Conflicts: # yarn.lock

xtuc mentioned this pull request May 25, 2018

correct ModuleImport funcidx xtuc/webassemblyjs#352

Merged

xtuc and others added 2 commits May 25, 2018 15:01

chore: bump webassemblyjs 1.5.7

d0ce47d

Merge pull request #7396 from xtuc/chore-bump-webassemblyjs7

cc57b12

bump webassemblyjs 1.5.7

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels May 25, 2018

xtuc and others added 2 commits May 28, 2018 14:55

fix(wasm): correct initializer type for rewritten globals

fd4e84b

Merge pull request #7416 from xtuc/fix-rewritten-global-type

3ac1d02

wasm: correct initializer type for rewritten globals

sokra merged commit 29cbf98 into master May 28, 2018

sokra deleted the feature/wasm-mangling branch June 7, 2018 12:07

Uh oh!

Conversation

sokra commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jessidhia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtuc commented May 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtuc May 12, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

xtuc May 12, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtuc commented May 14, 2018

Uh oh!

webpack-bot commented May 16, 2018 • edited by sokra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webpack-bot commented May 25, 2018

Uh oh!

xtuc commented May 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

sokra commented May 11, 2018 •

edited

Loading

xtuc May 12, 2018 •

edited

Loading

xtuc May 12, 2018 •

edited

Loading

webpack-bot commented May 16, 2018 •

edited by sokra

Loading