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

chore: build query-engine-wasm with stable rust #5167

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aqrln
Copy link
Member

@aqrln aqrln commented Feb 14, 2025

We previously used the nightly toolchain to build query-engine-wasm to make use of some nightly-only rustc and cargo flags in an attempt to reduce the size as much as possible, as our goal was to fit into the 1 MB gzip bundle limit in the Cloudflare Workers free plan, while still leaving enough space for the client and the user's application.

Specifically, we used it to:

  • Rebuild the standard library with size optimizations and without the panic infrastructure, making all panics immediate aborts.
  • Remove all panic location details. In the case of a crash, we (or the user) would only know that a problem happened but wouldn't know where and why (or possibly not even know it happened at all — see below).

Other the the last point above, which is obviously a problem, there are more:

  • While abort in WASM immediately abort the Node.js process too, its behaviour on Cloudflare Workers is weird, and apparently somehow the JavaScript code may continue running and calling into QE again, leading to memory leaks and undefined behaviour.

  • Keeping the nightly toolchain up to date is a hassle (as it's pinned) and we usually don't even bother doing it until it's too old. When we do, we either pick latest (which might have bugs) or need to spend time figuring out the last nightly for a specific version, which would correspond to it promoted to beta.

All in all, it is a maintenance burden, a source of technical debt, and the reason I can't consider query-engine-wasm production ready. Given that:

  • we now have the query compiler project which reduces the size way further without these hacks
  • removing these questionable size optimizations and switching to the stable toolchain increases the gzipped size by around 80 KB
  • and Cloudflare has increased the limit on the free plan from 1 MB to 3 MB

I propose we accept this size increase and switch to the stable toolchain.

@aqrln aqrln added this to the 6.4.0 milestone Feb 14, 2025
Copy link

codspeed-hq bot commented Feb 14, 2025

CodSpeed Performance Report

Merging #5167 will not alter performance

Comparing push-qkrsypxvlupy (180cfa8) with main (11f45a2)

Summary

✅ 11 untouched benchmarks

Copy link
Contributor

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.332MiB 2.111MiB 226.193KiB
Postgres (gzip) 929.568KiB 846.679KiB 82.889KiB
Mysql 2.291MiB 2.073MiB 223.063KiB
Mysql (gzip) 914.825KiB 832.296KiB 82.529KiB
Sqlite 2.197MiB 1.983MiB 219.390KiB
Sqlite (gzip) 876.532KiB 795.695KiB 80.837KiB

@aqrln aqrln marked this pull request as ready for review February 14, 2025 17:21
@aqrln aqrln requested a review from a team as a code owner February 14, 2025 17:21
@aqrln aqrln requested review from jacek-prisma and removed request for a team February 14, 2025 17:21
Copy link
Contributor

@FGoessler FGoessler left a comment

Choose a reason for hiding this comment

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

Sounds like fair tradeoffs to me - let's do this 👍

@aqrln aqrln removed this from the 6.4.0 milestone Feb 19, 2025
@aqrln
Copy link
Member Author

aqrln commented Feb 21, 2025

@jkomyno pointed out that while it's not a concern anymore with Cloudflare Workers, it would still be a problem with Vercel Edge Functions on Hobby plan. What shall we do?

aqrln added a commit that referenced this pull request Feb 21, 2025
It's pretty old at this point and we need a newer one to migrate to Rust
2024 after the stable toolchain is updated to 1.85.0.

An alternative is to use the stable toolchain for the wasm build
(#5167).
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.

3 participants