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

Use babel for transpiling rather than closure compiler #20879

Merged
merged 1 commit into from
Dec 9, 2023

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Dec 8, 2023

We recently switch from using closure-compiler with WHITESPACE_ONLY to closure-compiler with SIMPLE_OPTIMIZATIONS. However this had the negative side effect that all input need to be free of closure compiler warnings, and this turned out not to be practical for all users. See #20810 for more on this

When selecting a transpilation tool use I also evaluated swx (written in rust) and esbuild (written in go). esbuild was rejected because the simply don't support ES5
(evanw/esbuild#297). swx was rejected because it almost doubled the side of our node_modules directory by adding two 50mb binary files.

Fixes: #20810

@sbc100 sbc100 force-pushed the transpile_babel branch 5 times, most recently from d402f66 to 7b1da18 Compare December 8, 2023 18:09
@sbc100 sbc100 requested review from RReverser and kripken December 8, 2023 18:09
@sbc100 sbc100 changed the title Use babel for traspiling rather than closure compiler Use babel for transpiling rather than closure compiler Dec 8, 2023
@sbc100 sbc100 requested a review from tomayac December 8, 2023 18:10
@kripken
Copy link
Member

kripken commented Dec 8, 2023

Agreed on the logic of selecting babel. This is not a code path that needs to be super-fast since normally we don't need to transpile for older browsers, so picking something simpler and smaller seems best.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Any changes to code size or compile times?

@@ -13130,7 +13130,6 @@ def check_for_es6(filename, expect):
self.assertContained('foo(arg="hello")', js)
self.assertContained(['() => 2', '()=>2'], js)
self.assertContained('const ', js)
self.assertContained('let ', js)
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

babel doesn't strip comments by default when transpiling.. and we have comments that contains the string ".. let ..." making this hacky check fail. I think its find to drop it rather than try to get clever here.

Copy link
Member

Choose a reason for hiding this comment

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

I see. sgtm

Copy link
Collaborator

@tomayac tomayac left a comment

Choose a reason for hiding this comment

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

LGTM, with a suggestion for slightly improving the version checking function.

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 8, 2023

In terms of performance it does looks like babel is slow on large inputs. I measured using multitime -n2 ./emcc -sINCLUDE_FULL_LIBRARY -sLEGACY_VM_SUPPORT -O1 test/hello_world.c -v and got the following results:

babel: time 7.778, size 666625
closure compiler: 6.249s size 499042

I think the size difference here is because we are doing SIMPLE_OPTIMIZATIONS in the closure case so there is DCE happening.

If I run the same thing with -O2 and --closure=1 then I get:

babel: time 8.706s, size 61776
closure compiler: 7.855s size 47129

So I guess the babel polyfills could be more expensive or less DCE friendly.

@RReverser
Copy link
Collaborator

In terms of performance it does looks like babel is slow on large inputs.

Yeah this is pretty much the problem that swc is meant to fix.

@RReverser
Copy link
Collaborator

swx was rejected because it almost doubled the side of our node_modules directory by adding two 50mb binary files.

I wonder, could you install it lazily on first emcc invocation that needs transpiling? There is an existing precedent in fetching libraries via -s USE_* and that way all users who don't need transpilation, don't get the size increase, while others get faster transpilation.

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 8, 2023

In terms of performance it does looks like babel is slow on large inputs.

Yeah this is pretty much the problem that swc is meant to fix.

We don't care that much since most folks won't be using this path, and using closure was already really slow.

Comment on lines +36 to +37
canvas size in pixels, while `glfwGetWindowSize` returns the canvas size is
screen size. By default, this feature is disabled. You can enable it before
Copy link
Collaborator

Choose a reason for hiding this comment

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

The last part seems broken: "canvas size is screen size".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just a formatting change to the existing changelog entry. I don't want to mess with it as part of this PR really.

@kripken
Copy link
Member

kripken commented Dec 8, 2023

Slower speed is not great, obviously, but it seems fine to me here, since it's not a big regression compared to closure, and it avoids a large regression in the total sdk size.

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 8, 2023

swx was rejected because it almost doubled the side of our node_modules directory by adding two 50mb binary files.

I wonder, could you install it lazily on first emcc invocation that needs transpiling? There is an existing precedent in fetching libraries via -s USE_* and that way all users who don't need transpilation, don't get the size increase, while others get faster transpilation.

I don't think it worth the extra engineering effort to build that out. The cost of adding babel to node_modules is pretty marginal (~20Mb uncompressed).

I'd also like to move away from the linker doing magical extra work like downloading and building stuff.

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 8, 2023

Hopefully as a followup we can now remove some of our own pollyfills that we have accumulated over the years.

@sbc100 sbc100 force-pushed the transpile_babel branch 2 times, most recently from 7f2ddf3 to 2486bda Compare December 8, 2023 22:48
We recently switch from using closure-compiler with `WHITESPACE_ONLY` to
closure-compiler with `SIMPLE_OPTIMIZATIONS`.  However this had the
negative side effect that all input need to be free of closure compiler
warnings, and this turned out not to be practical for all users.
See emscripten-core#20810 for more on this

When selecting a transpilation tool use I also evaluated `swx` (written
in rust) and `esbuild` (written in go).  `esbuild` was rejected
because the simply don't support ES5
(evanw/esbuild#297).  `swx` was rejected
because it almost doubled the side of our `node_modules` directory by
adding two 50mb binary files.
@sbc100 sbc100 merged commit 7bed0e2 into emscripten-core:main Dec 9, 2023
@sbc100 sbc100 deleted the transpile_babel branch December 9, 2023 11:15
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.

-s WASM=0 -s LEGACY_VM_SUPPORT=1 results in JSC_CONSTANT_REASSIGNED_VALUE_ERROR warning during linking
4 participants