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

Support ES6 module syntax in Acorn and Closure #23730

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

kleisauke
Copy link
Collaborator

Avoids the need to (temporarily) replace import.meta, await import and export default usages with placeholders during the preprocess phase.

Alternatively, removing the dependency on Closure (see #23358) and updating the JSDCE pass in Acorn to support export default could also be a solution.

Marked as draft due to dependence on PR #23261.

sbc100 added 2 commits January 3, 2025 13:52
I don't see why `-sMINIMAL_RUNTIME` users would not want the same
exporting of the module.  The EXPORT_ES6 behaviour was already matching.
This change is necessary are part of emscripten-core#23261 which feeds the modularized
code through closure and our acorn optimizations.  Without this closer
things the entire module factory is seen as unused and is deleted.
fafb1a877e851f84c71de6531fe37de2c7700c29
Move modularization code into js compiler

Prior to this change the mudularization was done as a post-processing
step in python.  This complicated things since acorn and closure passes
would only see the inner code and not the whole module.

One concrete benefit is that we can now use `await` in the body of the
factory function since closure no longer sees it as top level (which it
isn't).

Fixes: emscripten-core#23158, emscripten-core#23283
@@ -1,10 +1,10 @@
{
"a.html": 454,
"a.html.gz": 328,
"a.js": 3920,
"a.js.gz": 2089,
"a.js": 5629,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty big regression sadly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the consolidated and prettified diff between main and this PR for other.test_minimal_runtime_code_size_hello_webgl_wasm:

Details
--- a/a.js
+++ b/a.js
@@ -1,4 +1,4 @@
-var Module = async function (B = {}) {
+async function Module(B = {}) {
   var l,
     p,
     r,
@@ -7,6 +7,88 @@ var Module = async function (B = {}) {
     y,
     D,
     E,
+    h = {
+      xa: 0,
+      Ha: 0,
+      Ka: 0,
+      Qa: 0,
+      hb: 0,
+      eb: 0,
+      Ea: 0,
+      Ca: 0,
+      Sa: 0,
+      Ba: 0,
+      Ga: 0,
+      Ta: 0,
+      gb: 0,
+      cb: 0,
+      Oa(a, b, c) {
+        r.set(r.subarray(b, b + c), a);
+      },
+      Ya() {
+        for (; h.L.length; ) h.V(h.L.length - 1);
+        h.N = [];
+      },
+      X: 0,
+      N: [],
+      za(a, b, c) {
+        function d(k, p) {
+          if (k.length != p.length) return !1;
+          for (var q in k) if (k[q] != p[q]) return !1;
+          return !0;
+        }
+
+        for (var f of h.N) if (f.Y == a && d(f.$, c)) return;
+        h.N.push({Y: a, da: b, $: c});
+        h.N.sort((k, p) => k.da < p.da);
+      },
+      Za(a) {
+        h.N = h.N.filter((b) => b.Y != a);
+      },
+      ja() {
+        return navigator.userActivation ? navigator.userActivation.isActive : h.X && h.la.wa;
+      },
+      ea() {
+        if (h.ja()) {
+          var a = h.N;
+          h.N = [];
+          for (var b of a) b.Y(...b.$);
+        }
+      },
+      L: [],
+      sa: (a, b) => {
+        for (var c = 0; c < h.L.length; ++c) h.L[c].target != a || (b && b != h.L[c].T) || h.V(c--);
+      },
+      V(a) {
+        var b = h.L[a];
+        b.target.removeEventListener(b.T, b.ba, b.va);
+        h.L.splice(a, 1);
+      },
+      Xa(a) {
+        if (!a.target) return -4;
+        if (a.ya)
+          (a.ba = function (c) {
+            ++h.X;
+            h.la = a;
+            h.ea();
+            a.Ja(c);
+            h.ea();
+            --h.X;
+          }),
+            a.target.addEventListener(a.T, a.ba, a.va),
+            h.L.push(a);
+        else
+          for (var b = 0; b < h.L.length; ++b)
+            h.L[b].target == a.target && h.L[b].T == a.T && h.V(b--);
+        return 0;
+      },
+      Ia(a) {
+        return a ? (a == window ? '#window' : a == screen ? '#screen' : a?.nodeName || '') : '';
+      },
+      fullscreenEnabled() {
+        return document.fullscreenEnabled || document.webkitFullscreenEnabled;
+      },
+    },
     M = new TextDecoder(),
     w = (a, b) => {
       if (!a) return '';
@@ -283,4 +365,8 @@ var Module = async function (B = {}) {
     L();
   });
   return {};
-};
+}
+
+'object' === typeof exports && 'object' === typeof module
+  ? ((module.exports = Module), (module.exports.default = Module))
+  : 'function' === typeof define && define.amd && define([], () => Module);

One factor is that the CJS/AMD module exports are now present under MINIMAL_RUNTIME (see PR #23282). The other factor is likely(?) that JSDCE is unable to remove certain parts of libhtml5.js.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit 507b455 workarounds this, but I'm not sure why this is necessary. 🤷‍♂️

How could -sMINIMAL_RUNTIME=2 have removed that? It doesn't seem like it should.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... it looks like that should be moved to libhtml5_webgl.js instead, see commit f53a92d.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should try to land f53a92d as its own patch maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split to PR #23847.

FWIW, the *.gzsize code sizes also cause slight differences when I run tools/maint/rebaseline_tests.py on my Fedora 41 workstation, likely because zlib was replaced with zlib-ng.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #23850 resolves this size regression instead.

It seems that other.test_minimal_runtime_code_size_hello_webgl_wasm depends on:

  • emscripten_set_element_css_size()
  • emscripten_set_canvas_element_size()
  • emscripten_webgl_create_context()

All of these had an unused dependency on $JSEvents, which, for some reason, JSDCE fails to remove in this changeset.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure its JSDCE and not closure compile that is failing to remove these things?

That was my experience too in #23261.. I figured the optimizers were just less good at removing non-top-level code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closure might also be failing to remove this unused dependency - I haven't checked.

We could try replacing Closure with https://github.com/swc-project/swc, as it shows promising results in https://github.com/privatenumber/minification-benchmarks. However, I'm unsure how much effort that would require.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to replace closure with pretty much anything else :)

However, so far nothing as come along that can't do as good of a job at minifying. Maybe its worth looking into swc!

This is an automatic change generated by tools/maint/rebaseline_tests.py.

The following (29) test expectation files were updated by
running the tests with `--rebaseline`:

```
code_size/embind_hello_wasm.json: 17653 => 17653 [+0 bytes / +0.00%]
code_size/embind_val_wasm.json: 16699 => 16699 [+0 bytes / +0.00%]
code_size/hello_webgl2_wasm.json: 13122 => 13734 [+612 bytes / +4.66%]
code_size/hello_webgl2_wasm2js.json: 18420 => 19184 [+764 bytes / +4.15%]
code_size/hello_webgl_wasm.json: 12660 => 13241 [+581 bytes / +4.59%]
code_size/hello_webgl_wasm2js.json: 17947 => 18682 [+735 bytes / +4.10%]
code_size/random_printf_wasm.json: 12442 => 12442 [+0 bytes / +0.00%]
code_size/random_printf_wasm2js.json: 17205 => 17205 [+0 bytes / +0.00%]
other/codesize/test_codesize_cxx_ctors1.gzsize: 8262 => 8289 [+27 bytes / +0.33%]
other/codesize/test_codesize_cxx_ctors2.gzsize: 8252 => 8279 [+27 bytes / +0.33%]
other/codesize/test_codesize_cxx_except.gzsize: 9271 => 9297 [+26 bytes / +0.28%]
other/codesize/test_codesize_cxx_except_wasm.gzsize: 8206 => 8234 [+28 bytes / +0.34%]
other/codesize/test_codesize_cxx_except_wasm_legacy.gzsize: 8206 => 8234 [+28 bytes / +0.34%]
other/codesize/test_codesize_cxx_lto.gzsize: 8279 => 8303 [+24 bytes / +0.29%]
other/codesize/test_codesize_cxx_mangle.gzsize: 9275 => 9301 [+26 bytes / +0.28%]
other/codesize/test_codesize_cxx_noexcept.gzsize: 8262 => 8289 [+27 bytes / +0.33%]
other/codesize/test_codesize_cxx_wasmfs.gzsize: 3469 => 3472 [+3 bytes / +0.09%]
other/codesize/test_codesize_files_js_fs.gzsize: 7570 => 7590 [+20 bytes / +0.26%]
other/codesize/test_codesize_files_wasmfs.gzsize: 2710 => 2712 [+2 bytes / +0.07%]
other/codesize/test_codesize_hello_O0.gzsize: 7984 => 7999 [+15 bytes / +0.19%]
other/codesize/test_codesize_hello_dylink.gzsize: 5919 => 5926 [+7 bytes / +0.12%]
other/codesize/test_codesize_mem_O3.gzsize: 2241 => 2242 [+1 bytes / +0.04%]
other/codesize/test_codesize_minimal_O0.gzsize: 6330 => 6336 [+6 bytes / +0.09%]
other/codesize/test_codesize_minimal_esm.gzsize: 1437 => 1397 [-40 bytes / -2.78%]
other/codesize/test_codesize_minimal_esm.jssize: 2982 => 2851 [-131 bytes / -4.39%]
other/codesize/test_codesize_minimal_pthreads.gzsize: 4113 => 4119 [+6 bytes / +0.15%]
other/test_unoptimized_code_size.js.size: 52478 => 52447 [-31 bytes / -0.06%]
other/test_unoptimized_code_size_no_asserts.js.size: 27521 => 27490 [-31 bytes / -0.11%]
other/test_unoptimized_code_size_strict.js.size: 50766 => 50735 [-31 bytes / -0.06%]

Average change: +0.47% (-4.39% - +4.66%)
```
@kleisauke kleisauke force-pushed the es6-acorn-closure branch from c6f028c to d8831e2 Compare March 6, 2025 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants