-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Enable WASM_BIGINT by default #19156
Conversation
Looks like we'd also need to bump the minimum node from 10.18 to 15.0, which is a big jump... And the node-compat tests fail in this PR, but we could run then in a mode that adjust the min node version perhaps? |
Before we do that we would probably want to update the emsdk version of node which is currently on 14.18.2. Going from 14.18.2 to 15.X seems reasonable but we have had push back in the past when trying to update the emsdk version.
Yes, that seems reasonable. |
emsdk node update PR that has been up for a long time now: emscripten-core/emsdk#829 |
Ah right, thanks @sbc100 . Ok, this should wait on that node upgrade then to happen first. |
The emsdk update is now done |
src/settings.js
Outdated
@@ -1382,7 +1382,7 @@ var DYNCALLS = false; | |||
// i64 is used. If WASM_BIGINT is present, the default minimum supported browser | |||
// versions will be increased to the min version that supports BigInt. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This last sentence should probably just be removed I think
src/settings.js
Outdated
// MAX_INT (0x7FFFFFFF, or -1) specifies that target is not supported. | ||
// [link] | ||
var MIN_FIREFOX_VERSION = 65; | ||
var MIN_FIREFOX_VERSION = 68; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add this to the ChangeLog, along with a note about bigint itself being enabled by default.
self.emcc_args += ['-lembind'] | ||
self.node_args += shared.node_bigint_flags() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any remaining callers of node_bigint_flags
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is not needed, but there are still two uses after it, in emcc.py
and gen_struct_info.py
which I am not sure about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To remove it in them, we might need to add node version checks...
I've very excited to see this change happening! |
Thanks, feedback addressed. But note that there is a long road here given all the test failures, and I'll be splitting this up into smaller PRs once it works, so not sure it's worth reviewing eagerly. |
This is still a version from 3 years ago, so it is a very small change, but it does bring us to a version that has wasm-bigint enabled, which will help #19156
) This version of node supports wasm-bigint by default, so it will help #19156 Node 16 is over 2 years old, and the node website already provides newer versions both for latest and for LTS. This is possible after emscripten-core/emsdk#829 made the emsdk install a 15.x version by default, and then emscripten-core/emsdk#1232 switched to 16.x.
Could we at least make the default for WASM_BIGINT driven by the feature matrix, so that those folks who set the right min browser versions will get it by default? Or, for example, if you target only node and not the web? |
is all of that 3.2k polyfill necessary for our use of bigint64array? or can we get away with only part of it? |
@dschuff We might not need all of it (e.g. I don't think we use |
@sbc100 We could split that part out of this PR, to enable bigint automatically if the user happens to target new-enough things. I'm not sure if it's worth the complexity, though - the big benefit is when we can change the default entirely, I think. Unless you think it's common for people to target new things? |
Some tests fail here because this PR no longer does what it is advertised as doing: the default of WASM_BIGINT is set to the default based on the features, but as mentioned a few comments back, we need Safari 15 for full WASM_BIGINT support and we just depend on 14 for now. So this does not actually end up setting WASM_BIGINT by default in CI, causing certain test failures. I'm not sure it's worth going through those atm - might be more churn than is worth it (add/remove annotations on tests, that will be redundant once we can actually depend on Safari 15). I lean towards leaving it for when we can actually flip the setting's default. |
Can this be closed now that #22993 was landed? |
Yes, thanks @kleisauke ! |
This explores the option of enabling it by default. To do that, fix our feature matrix on Safari (it was too pessimistic) and bump Firefox a few versions (but just to an ESR from 2019, which is still a very long time ago), and integrate the feature matrix in
emcc.py
.