Skip to content

Commit

Permalink
Ensure all unused elements are removed during applyDCEGraphRemovals. …
Browse files Browse the repository at this point in the history 
…NFC (#21840)

There were bugs in `applyDCEGraphRemovals` that were preventing it from
finding certain unused exports in certain cases.

This change cleanup up the pass and adds assertions that all unused
imports and exports are actually located and removed, thus preventing
these types of bugs from sneaking in again.

The test we had for `applyDCEGraphRemovals` were worked, but they were
writing in a slightly different style to the current emscripten output.
In particular the export name and JS name were matching, even though
the actual compiler output produces JS names that contain a leading
underscore.  I also updates the tests to match this style for
consistency.

I believe the effects of this bug were not captured by our code size
tests because they all run closure compiler afterwards which was
removing the unused exports itself.
  • Loading branch information
@sbc100
sbc100 committed Apr 30, 2024
1 parent 286c206 commit 63c648f
Show file tree
Hide file tree
Showing 8 changed files with 164 additions and 126 deletions.
36 changes: 14 additions & 22 deletions test/optimizer/applyDCEGraphRemovals-output.js
View file
Expand Up @@ -5,38 +5,30 @@ var wasmImports = {
save2: 2
};

var expD1 = Module["expD1"] = wasmExports["expD1"];
var _expD1 = Module["_expD1"] = wasmExports["expD1"];

var expD2 = Module["expD2"] = wasmExports["expD2"];
var _expD2 = Module["_expD2"] = wasmExports["expD2"];

var expD3 = Module["expD3"] = wasmExports["expD3"];
var _expD3 = Module["_expD3"] = wasmExports["expD3"];

var expD4;
var _expD5 = wasmExports["expD5"];

var expD5 = wasmExports["expD5"];
var _expI1 = Module["_expI1"] = () => (expI1 = Module["_expI1"] = wasmExports["expI1"])();

var expD6;
var _expI2 = Module["_expI2"] = () => (expI2 = Module["_expI2"] = wasmExports["expI2"])();

var expI1 = Module["expI1"] = () => (expI1 = Module["expI1"] = wasmExports["expI1"])();
var _expI3 = Module["_expI3"] = () => (expI3 = Module["_expI3"] = wasmExports["expI3"])();

var expI2 = Module["expI2"] = () => (expI2 = Module["expI2"] = wasmExports["expI2"])();
var _expI5 = () => (_expI5 = wasmExports["expI5"])();

var expI3 = Module["expI3"] = () => (expI3 = Module["expI3"] = wasmExports["expI3"])();
_expD1;

var expI4;
Module["_expD2"];

var expI5 = () => (expI5 = wasmExports["expI5"])();
wasmExports["_expD3"];

var expI6;
_expI1;

expD1;
Module["_expI2"];

Module["expD2"];

wasmExports["expD3"];

expI1;

Module["expI2"];

wasmExports["expI3"];
wasmExports["_expI3"];
46 changes: 26 additions & 20 deletions test/optimizer/applyDCEGraphRemovals.js
View file
@@ -1,32 +1,38 @@
var name;
var wasmImports = { save1: 1, number: 33, name: name, func: function() {}, save2: 2 };
var wasmImports = {
save1: 1,
number: 33,
name: name,
func: function() {},
save2: 2
};

// exports gotten directly
var expD1 = Module['expD1'] = wasmExports['expD1'];
var expD2 = Module['expD2'] = wasmExports['expD2'];
var expD3 = Module['expD3'] = wasmExports['expD3'];
var expD4 = Module['expD4'] = wasmExports['expD4'];
var _expD1 = Module['_expD1'] = wasmExports['expD1'];
var _expD2 = Module['_expD2'] = wasmExports['expD2'];
var _expD3 = Module['_expD3'] = wasmExports['expD3'];
var _expD4 = Module['_expD4'] = wasmExports['expD4'];
// Like above, but not exported on the Module
var expD5 = wasmExports['expD5'];
var expD6 = wasmExports['expD6'];
var _expD5 = wasmExports['expD5'];
var _expD6 = wasmExports['expD6'];

// exports gotten indirectly (async compilation
var expI1 = Module['expI1'] = () => (expI1 = Module['expI1'] = wasmExports['expI1'])();
var expI2 = Module['expI2'] = () => (expI2 = Module['expI2'] = wasmExports['expI2'])();
var expI3 = Module['expI3'] = () => (expI3 = Module['expI3'] = wasmExports['expI3'])();
var expI4 = Module['expI4'] = () => (expI4 = Module['expI4'] = wasmExports['expI4'])();
var _expI1 = Module['_expI1'] = () => (expI1 = Module['_expI1'] = wasmExports['expI1'])();
var _expI2 = Module['_expI2'] = () => (expI2 = Module['_expI2'] = wasmExports['expI2'])();
var _expI3 = Module['_expI3'] = () => (expI3 = Module['_expI3'] = wasmExports['expI3'])();
var _expI4 = Module['_expI4'] = () => (expI4 = Module['_expI4'] = wasmExports['expI4'])();

// Like above, but not exported on the Module
var expI5 = () => (expI5 = wasmExports['expI5'])();
var expI6 = () => (expI6 = wasmExports['expI6'])();
var _expI5 = () => (_expI5 = wasmExports['expI5'])();
var _expI6 = () => (_expI6 = wasmExports['expI6'])();

// add uses for some of them, leave *4 as non-roots
expD1;
Module['expD2'];
wasmExports['expD3'];
_expD1;
Module['_expD2'];
wasmExports['_expD3'];

expI1;
Module['expI2'];
wasmExports['expI3'];
_expI1;
Module['_expI2'];
wasmExports['_expI3'];

// EXTRA_INFO: { "unused": ["emcc$import$number", "emcc$import$name", "emcc$import$func", "emcc$export$expD4", "emcc$export$expD6", "emcc$export$expI4", "emcc$export$expI6"] }
// EXTRA_INFO: { "unusedImports": ["number", "name", "func"], "unusedExports": ["expD4", "expD6", "expI4", "expI6"] }
12 changes: 6 additions & 6 deletions test/optimizer/minimal-runtime-applyDCEGraphRemovals-output.js
View file
Expand Up @@ -7,15 +7,15 @@ var wasmImports = {

WebAssembly.instantiate(Module["wasm"], imports).then(output => {
wasmExports = output.instance.exports;
expD1 = wasmExports["expD1"];
expD2 = wasmExports["expD2"];
expD3 = wasmExports["expD3"];
_expD1 = wasmExports["expD1"];
_expD2 = wasmExports["expD2"];
_expD3 = wasmExports["expD3"];
initRuntime(wasmExports);
ready();
});

expD1;
_expD1;

Module["expD2"];
Module["_expD2"];

wasmExports["expD3"];
wasmExports["_expD3"];
24 changes: 15 additions & 9 deletions test/optimizer/minimal-runtime-applyDCEGraphRemovals.js
View file
@@ -1,20 +1,26 @@
var name;
var wasmImports = { save1: 1, number: 33, name: name, func: function() {}, save2: 2 };
var wasmImports = {
save1: 1,
number: 33,
name: name,
func: function() {},
save2: 2
};

// exports gotten directly in the minimal runtime style
WebAssembly.instantiate(Module["wasm"], imports).then((output) => {
wasmExports = output.instance.exports;
expD1 = wasmExports['expD1'];
expD2 = wasmExports['expD2'];
expD3 = wasmExports['expD3'];
expD4 = wasmExports['expD4'];
_expD1 = wasmExports['expD1'];
_expD2 = wasmExports['expD2'];
_expD3 = wasmExports['expD3'];
_expD4 = wasmExports['expD4'];
initRuntime(wasmExports);
ready();
});

// add uses for some of them, leave *4 as non-roots
expD1;
Module['expD2'];
wasmExports['expD3'];
_expD1;
Module['_expD2'];
wasmExports['_expD3'];

// EXTRA_INFO: { "unused": ["emcc$import$number", "emcc$import$name", "emcc$import$func", "emcc$export$expD4", "emcc$export$expI4"] }
// EXTRA_INFO: { "unusedImports": ["number", "name", "func"], "unusedExports": ["expD4"] }
125 changes: 73 additions & 52 deletions tools/acorn-optimizer.mjs
View file
Expand Up @@ -89,14 +89,6 @@ function emptyOut(node) {
node.type = 'EmptyStatement';
}

// This converts the node into something that terser will ignore in a var
// declaration, that is, it is a way to get rid of initial values.
function convertToNothingInVarInit(node) {
node.type = 'Literal';
node.value = undefined;
node.raw = 'undefined';
}

function convertToNullStatement(node) {
node.type = 'ExpressionStatement';
node.expression = {
Expand Down Expand Up @@ -591,18 +583,25 @@ function isDynamicDynCall(node) {

//
// Matches the wasm export wrappers generated by emcc (see make_export_wrappers
// in emscripten.py). For example:
// in emscripten.py). For example, the right hand side of these assignments:
//
// var _foo = (a0, a1) => (_foo = wasmExports['foo'])(a0, a1):
//
// or
//
// var _foo = (a0, a1) => (_foo = Module['_foo'] = wasmExports['foo'])(a0, a1):
//
function isExportWrapperFunction(f) {
if (f.body.type != 'CallExpression') return null;
let callee = f.body.callee;
if (callee.type == 'ParenthesizedExpression') {
callee = callee.expression;
}
if (callee.type != 'AssignmentExpression' || callee.right.type != 'MemberExpression') return null;
if (callee.type != 'AssignmentExpression') return null;
var rhs = callee.right;
if (rhs.type == 'AssignmentExpression') {
rhs = rhs.right;
}
if (rhs.type != 'MemberExpression' || !isExportUse(rhs)) return null;
return getExportOrModuleUseName(rhs);
}
Expand Down Expand Up @@ -740,7 +739,9 @@ function emitDCEGraph(ast) {
emptyOut(node);
} else if (value && value.type === 'ArrowFunctionExpression') {
// this is
// var x = () => (x = wasmExports['x'])(..)
// () => (x = wasmExports['x'])(..)
// or
// () => (x = Module['_x'] = wasmExports['x'])(..)
let asmName = isExportWrapperFunction(value);
if (asmName) {
saveAsmExport(name, asmName);
Expand Down Expand Up @@ -978,67 +979,84 @@ function emitDCEGraph(ast) {
// way (and we leave further DCE on the JS and wasm sides to their respective
// optimizers, closure compiler and binaryen).
function applyDCEGraphRemovals(ast) {
const unused = new Set(extraInfo.unused);
const unusedExports = new Set(extraInfo.unusedExports);
const unusedImports = new Set(extraInfo.unusedImports);
const foundUnusedImports = new Set();
const foundUnusedExports = new Set();
trace('unusedExports:', unusedExports);
trace('unusedImports:', unusedImports);

fullWalk(ast, (node) => {
if (isWasmImportsAssign(node)) {
const assignedObject = getWasmImportsValue(node);
assignedObject.properties = assignedObject.properties.filter((item) => {
const name = item.key.name;
const value = item.value;
const full = 'emcc$import$' + name;
return !(unused.has(full) && !hasSideEffects(value));
});
} else if (node.type === 'AssignmentExpression') {
// when we assign to a thing we don't need, we can just remove the assign
// var x = Module['x'] = wasmExports['x'];
const target = node.left;
if (isExportUse(target) || isModuleUse(target)) {
const name = getExportOrModuleUseName(target);
const full = 'emcc$export$' + name;
const value = node.right;
if (unused.has(full) && (isExportUse(value) || !hasSideEffects(value))) {
// This will be in a var init, and we just remove that value.
convertToNothingInVarInit(node);
if (unusedImports.has(name)) {
foundUnusedImports.add(name);
return hasSideEffects(value);
}
}
return true;
});
} else if (node.type === 'VariableDeclaration') {
// Handle the case we declare a variable but don't assign to the module:
// var x = wasmExports['x'];
// and
// var x = () => (x = wasmExports['x']).apply(...);
// Handle the various ways in which we extract wasmExports:
// 1. var _x = wasmExports['x'];
// or
// 2. var _x = Module['_x'] = wasmExports['x'];
//
// Or for delayed instantiation:
// 3. var _x = () => (_x = wasmExports['x'])(...);
// or
// 4. var _x = Module['_x] = () => (_x = Module['_x'] = wasmExports['x'])(...);
const init = node.declarations[0].init;
if (init) {
if (isExportUse(init)) {
const name = getExportOrModuleUseName(init);
const full = 'emcc$export$' + name;
if (unused.has(full)) {
convertToNothingInVarInit(init);
}
} else if (init.type == 'ArrowFunctionExpression') {
const name = isExportWrapperFunction(init);
const full = 'emcc$export$' + name;
if (unused.has(full)) {
convertToNothingInVarInit(init);
}
if (!init) {
return;
}

// Look through the optional `Module['_x']`
let realInit = init;
if (init.type == 'AssignmentExpression' && isModuleUse(init.left)) {
realInit = init.right;
}

if (isExportUse(realInit)) {
const export_name = getExportOrModuleUseName(realInit);
if (unusedExports.has(export_name)) {
// Case (1) and (2)
trace('found unused export:', export_name);
emptyOut(node);
foundUnusedExports.add(export_name);
}
} else if (realInit.type == 'ArrowFunctionExpression') {
const export_name = isExportWrapperFunction(realInit);
if (unusedExports.has(export_name)) {
// Case (3) and (4)
trace('found unused export:', export_name);
emptyOut(node);
foundUnusedExports.add(export_name);
}
}
} else if (node.type === 'ExpressionStatement') {
const expr = node.expression;
// In the MINIMAL_RUNTIME code pattern we have just
// x = wasmExports['x']
// _x = wasmExports['x']
// and never in a var.
if (expr.operator === '=' && expr.left.type === 'Identifier' && isExportUse(expr.right)) {
const name = expr.left.name;
if (name === getExportOrModuleUseName(expr.right)) {
const full = 'emcc$export$' + name;
if (unused.has(full)) {
emptyOut(node);
}
const export_name = getExportOrModuleUseName(expr.right);
if (unusedExports.has(export_name)) {
emptyOut(node);
foundUnusedExports.add(export_name);
}
}
}
});

for (const i of unusedImports) {
assert(foundUnusedImports.has(i), 'unused import not found: ' + i);
}
for (const e of unusedExports) {
assert(foundUnusedExports.has(e), 'unused export not found: ' + e);
}
}

// Need a parser to pass to acorn.Node constructor.
Expand Down Expand Up @@ -2050,7 +2068,10 @@ const registry = {
minifyGlobals: minifyGlobals,
};

passes.forEach((pass) => registry[pass](ast));
passes.forEach((pass) => {
trace(`running AST pass: ${pass}`);
registry[pass](ast);
});

if (!noPrint) {
const terserAst = terser.AST_Node.from_mozilla_ast(ast);
Expand Down

0 comments on commit 63c648f

Please sign in to comment.