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

wasm: remove i64 workaround, use BigInt instead #2375

Closed
wants to merge 1 commit into from
Closed

Conversation

aykevl
Copy link
Member

@aykevl aykevl commented Dec 10, 2021

Browsers previously didn't support the WebAssembly i64 type, so we had
to work around that limitation by converting the LLVM i64 type to
something else. Some people used a pair of i32 values, but we used a
pointer to a stack allocated i64.

Now however, all major browsers and Node.js do support WebAssembly
BigInt integration so that i64 values can be passed back and forth
between WebAssembly and JavaScript easily. Therefore, I think the time
has come to drop support for this workaround.

For more information: https://v8.dev/features/wasm-bigint (note that
TinyGo has used a slightly different way of passing i64 values between
JS and Wasm).

For information on browser support: https://webassembly.org/roadmap/

I have tested this PR with the following steps:

  1. (before this PR) Modify the syscall/js package to comment out all tests that don't pass yet, until tinygo test -target=wasm syscall/js works.
  2. Remove the i64 workaround.
  3. Fix wasm_exec.js until tinygo test -target=wasm syscall/js passes again.

Tests that currently fail (both before and after this PR) are TestIntConversion (partially), TestInterleavedFunctions, TestGarbageCollection, and TestGlobal.

Thoughts? Is there any reason why we should delay this change? I think the last browser to add support for wasm bigint was Safari 14.1.

Browsers previously didn't support the WebAssembly i64 type, so we had
to work around that limitation by converting the LLVM i64 type to
something else. Some people used a pair of i32 values, but we used a
pointer to a stack allocated i64.

Now however, all major browsers and Node.js do support WebAssembly
BigInt integration so that i64 values can be passed back and forth
between WebAssembly and JavaScript easily. Therefore, I think the time
has come to drop support for this workaround.

For more information: https://v8.dev/features/wasm-bigint (note that
TinyGo has used a slightly different way of passing i64 values between
JS and Wasm).

For information on browser support: https://webassembly.org/roadmap/
@aykevl
Copy link
Member Author

aykevl commented Dec 10, 2021

One consideration may be that Node.js only added support for i64 BigInt integration (without a flag) starting with version 16, which was released in April this year. And version 14 is still supported into 2023, a long time from now.

@dgryski
Copy link
Member

dgryski commented Dec 10, 2021

I think @matthewcp and @natemoo-re are using tinygo+wasm in the browser for https://github.com/withastro/compiler . They might have an opinion on this

@justinclift
Copy link
Member

Interesting concept, but ouch:

Node.js only added support for i64 BigInt integration (without a flag) starting with version 16... and version 14 is still supported into 2023.

Without reviewing the code (which I can't yet, am smashed for time already) my thinking is "Which is more important, BigInt support or Node 14 support?".

Do we have an idea of the existing TinyGo userbase, and if/whether this would be a major pita or not? Just from the perspective that locking out a significant qty of users isn't a good idea. However, if it's just 1 person out of a Community of 10k, then it's not the same sized problem.

With a similar thought, if BigInt support would open up a lot of other optimisations and fixes, it might be worth it anyway.

Either way, if this is merged, it'll need to be documented. eg "We only support Node 16 if you're outputting to wasm" or similar. 😄

@natemoo-re
Copy link

One consideration may be that Node.js only added support for i64 BigInt integration (without a flag) starting with version 16, which was released in April this year. And version 14 is still supported into 2023, a long time from now.

This would be a big blocker for us. We are supporting back to node@12 which is still in long-term support (LTS). It would keep Astro's compiler pinned to an old version of TinyGo.

@dgryski
Copy link
Member

dgryski commented Dec 10, 2021

I wonder if we could at least support this with a tinygo flag? It looked like a big PR though, so that might be a pain :(.

@natemoo-re
Copy link

Without reviewing the code (which I can't yet, am smashed for time already) my thinking is "Which is more important, BigInt support or Node 14 support?".

Do we have an idea of the existing TinyGo userbase, and if/whether this would be a major pita or not? Just from the perspective that locking out a significant qty of users isn't a good idea. However, if it's just 1 person out of a Community of 10k, then it's not the same sized problem.

Just wanted to weigh in that Astro uses TinyGo + WASM for the @astrojs/compiler, mainly in Node but also in the browser. Per the wide browser support for this feature, no objections.

But due to Node's LTS schedule, we're stuck supporting node@12 (currently in "maintenance" LTS) until April 30, 2022 while the ecosystem and cloud providers slowly phase it out in favor of node@14. But for security, Node packages don't have an ability to pass the --experimental-wasm-bigint flag automatically, meaning it would become a user burden until April 30, 2023 when node@14 reaches EOL.

To give you a sense of our community's size, Astro launched publicly in June 2021. We currently have 8.6k stars on GitHub and a Discord community with 3.6k members. Our monthly downloads on npm reached about 16.5k downloads last month.

@aykevl
Copy link
Member Author

aykevl commented Dec 11, 2021

Ok, sounds like we shouldn't do this right now.

Without reviewing the code (which I can't yet, am smashed for time already) my thinking is "Which is more important, BigInt support or Node 14 support?".

It's really just an optimization. Avoiding this pass makes binaries a bit smaller, hopefully (I didn't measure this but it should). Removing this pass also simplifies the compiler a bit so I would like to do it at some point.

One consideration may be that Node.js only added support for i64 BigInt integration (without a flag) starting with version 16, which was released in April this year. And version 14 is still supported into 2023, a long time from now.

This would be a big blocker for us. We are supporting back to node@12 which is still in long-term support (LTS). It would keep Astro's compiler pinned to an old version of TinyGo.

Okay, that's useful to know! I certainly don't want to break that use case.

I believe Node.js 14 also supports this, but behind a flag. Would that be acceptable once Node.js 12 is no longer supported? Otherwise we'd have to wait until 2024 until even Node.js 14 is deprecated.

I wonder if we could at least support this with a tinygo flag? It looked like a big PR though, so that might be a pain :(.

Not really. IMHO it's all or nothing. I'd rather want to avoid having to maintain two wasm_exec.js files.
(Sidenote: WASI doesn't use this i64 pass, because it doesn't need to interact with JavaScript).

@aykevl aykevl closed this Dec 11, 2021
@natemoo-re
Copy link

natemoo-re commented Dec 11, 2021

I believe Node.js 14 also supports this, but behind a flag. Would that be acceptable once Node.js 12 is no longer supported? Otherwise we'd have to wait until 2024 until even Node.js 14 is deprecated.

Unfortunately, probably not! It would still be a pain for users. Packages can't set the flags automatically, so it'd be a user education thing. The Node ecosystem moves pretty slowly. ☹️

@aykevl
Copy link
Member Author

aykevl commented Dec 11, 2021

Unfortunately, probably not! It would still be a pain for users. Packages can't set the flags automatically, so it's be a user education thing. The Node ecosystem moves pretty slowly. ☹️

Ok, thanks!

Slightly off-topic but I'm wondering about some similar things. There are some upcoming features in WebAssembly that we'd like to take advantage of, eventually.
What do you think of providing a -target=wasm that is supported by all the major browsers and embedders, and -target=wasm-legacy that provides support for older versions such as Node.js 12 and older browsers? Or would that be too complicated? If selecting the version at compile time is too complicated, Astro could of course default to the hypothetical -target=wasm-legacy.
For example, one major issue for us is stack switching. There is a group working on a proposal, but it's not ready yet. Once ready, it would be super useful to be able to use it especially once major browsers and embedders support it, while also providing support for older browsers/embedders. (In this case, the scheduler can be selected using -scheduler= but that isn't true for other features).

@aykevl
Copy link
Member Author

aykevl commented Dec 11, 2021

With a similar thought, if BigInt support would open up a lot of other optimisations and fixes, it might be worth it anyway.

Using BigInt doesn't give many extra optimizations, luckily. I was just hoping we could get rid of this pass, but sadly we can't.

@justinclift
Copy link
Member

@natemoo-re

and a Discord community with 3.6k members. Our monthly downloads on npm reached about 16.5k downloads last month.

Cool, those seem like really, really good Discord community engagement numbers. Pretty much 1 in 4 ratio.

That's bodes well for the future. 😄

@aykevl

and -target=wasm-legacy that provides support for older versions such as Node.js 12 and older browsers?

Sounds reasonable, though it'll add to the maintenance burden having to do things two ways. 🤷

@aykevl
Copy link
Member Author

aykevl commented Dec 11, 2021

Hmm, yeah, let's just keep things as-is and re-evaluate once a really compelling new feature becomes widely supported.

@justinclift
Copy link
Member

justinclift commented Dec 11, 2021

@natemoo-re Just saw this on the Astro intro page:

We’re inspired by the early success of projects like Tailwind, Rome,
Remix, Ionic, and others who are experimenting with long-term
financial sustainability on top of Open Source. Over the next year
we’ll be exploring how we can create a sustainable business to
support a 100% free, open source Astro for years to come.

That's super good. It's amazing how many OSS projects get launched based on
good technical ideas / founder desire, but fizzle out after a while because there
was no serious consideration of long term sustainability.

Hopefully Astro nails it, in good Community enhancing ways. 😄

@natemoo-re
Copy link

natemoo-re commented Dec 11, 2021

What do you think of providing a -target=wasm that is supported by all the major browsers and embedders, and -target=wasm-legacy that provides support for older versions such as Node.js 12 and older browsers? Or would that be too complicated? If selecting the version at compile time is too complicated, Astro could of course default to the hypothetical -target=wasm-legacy.

I'd very much be in favor of this! Especially if it unblocks some major improvements that couldn't be made otherwise. I think we might even be able to compile to both and ship based on the user's Node version, but TBD on that. Always have the fallback option of a slow migration away from wasm-legacy, too.

Hopefully Astro nails it, in good Community enhancing, ways. 😄

Thanks for the kind words! We're so excited about how Astro's been going so far and we have so many amazing ideas to explore, but building a really solid community foundation is definitely our top priority.

@deadprogram deadprogram deleted the wasm-bigint-2 branch April 28, 2022 08:46
@aykevl aykevl restored the wasm-bigint-2 branch April 30, 2023 15:41
@aykevl aykevl deleted the wasm-bigint-2 branch April 30, 2023 15:42
@aykevl
Copy link
Member Author

aykevl commented Apr 30, 2023

New PR, I think now is the time to finally do this: #3695
This will remove Node.js 14 support, which is EOL as of today.

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.

None yet

4 participants