feat: add experimental QuickJS backend - #36194
Conversation
8e6c7bc to
92e5d3d
Compare
0ddd7f0 to
26e891c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
26e891c to
c0e5946
Compare
|
I've got a runtime error with QuickJS build. Step to reproduce% cat main.ts
import { App } from "jsr:@fresh/core@2.3.0";
export const app = new App().fsRoutes();
% cat vite.config.ts
import { fresh } from "jsr:@fresh/plugin-vite@1.0.8";
import { defineConfig } from "npm:vite@7.1.11";
export default defineConfig({
plugins: [fresh()],
});
% cat index.html
<!doctype html>
<html>
<body></body>
</html>Expected behaviour% deno --version
deno 2.8.0 (stable, release, aarch64-apple-darwin)
v8 14.9.207.2-rusty
typescript 6.0.3
% deno -A npm:vite@7.1.11 build
vite v7.1.11 building for production...
✓ 21 modules transformed.
_fresh/client/.vite/manifest.json 0.16 kB │ gzip: 0.12 kB
_fresh/client/assets/client-entry-DXOs4TEk.js 44.85 kB │ gzip: 17.07 kB
✓ built in 1.44s
vite v7.1.11 building SSR bundle for production...
✓ 168 modules transformed.
_fresh/server/.vite/manifest.json 0.15 kB
_fresh/server/server-entry.mjs 207.31 kB
✓ built in 1.38sActual behaviour% git log --oneline
c1b2628c5f (HEAD -> quickjs-36194) build: release QuickJS denort + libdenort for deno compile/desktop [ci]
2de172fb84 feat(compile): add --engine flag to select the QuickJS denort [ci]
c0e5946ce6 build: keep the QuickJS backend building and check it in CI [ci]
23c95db610 build: consume v8x backend from crates.io [ci]
269de85af7 build: preserve Deno defaults across JS backends
34e840d895 build: expose JavaScript backend features for denort
7d353964e1 build: select JavaScript backend through Deno features
7f2c945360 refactor(core): route JavaScript engine through deno_v8
f4580b1060 test(core): preserve snapshot module object identity
b8be11115b fix(core): wake event loop for uv timer deadlines
7ac8c35f25 fix(node): allow heap snapshot GC reentrancy
8da37e96f1 fix(node): allow queryObjects GC reentrancy
% cargo build --release --bin deno --no-default-features --features quickjs% ./deno --version
deno 2.9.3 (stable, release, aarch64-apple-darwin)
v8 14.9.207.2-rusty
typescript 6.0.3
% ./deno -A npm:vite@7.1.11 build
error: Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'stack')
createLogger(options.logLevel).error(import_picocolors.default.red(`error during build:\n${e.stack}`), { error: e });
^
at CAC.<anonymous> (file:///path/to/node_modules/.deno/vite@7.1.11/node_modules/vite/dist/node/cli.js:641:94)NitIs it correct to show the build info as % ./deno --version
deno 2.9.3 (stable, release, aarch64-apple-darwin)
v8 14.9.207.2-rusty
typescript 6.0.3 |
|
% cat intl-datetime-format.ts
const date2YYMMDDSegmenter = new Intl.DateTimeFormat("ja-JP", {
year: "numeric",
month: "2-digit",
day: "2-digit",
});
const date = new Date(2022, 1, 1);
const actual = date2YYMMDDSegmenter.format(date);
console.log({
input: date.toString(),
expected: "2022/02/01",
actual,
locale: date2YYMMDDSegmenter.resolvedOptions().locale,
timeZone: date2YYMMDDSegmenter.resolvedOptions().timeZone,
options: date2YYMMDDSegmenter.resolvedOptions(),
});Expected% deno --version
deno 2.8.0 (stable, release, aarch64-apple-darwin)
v8 14.9.207.2-rusty
typescript 6.0.3
% deno run intl-datetime-format.ts
{
input: "Tue Feb 01 2022 00:00:00 GMT+0900 (Japan Standard Time)",
expected: "2022/02/01",
actual: "2022/02/01",
locale: "ja-JP",
timeZone: "Asia/Tokyo",
options: {
locale: "ja-JP",
calendar: "gregory",
numberingSystem: "latn",
timeZone: "Asia/Tokyo",
year: "numeric",
month: "2-digit",
day: "2-digit"
}
}Actual% ./deno --version
deno 2.9.3 (stable, release, aarch64-apple-darwin)
v8 14.9.207.2-rusty
typescript 6.0.3
% ./deno run intl-datetime-format.ts
{
input: "Tue Feb 01 2022 00:00:00 GMT+0900 (JST)",
expected: "2022/02/01",
actual: "1/31/2022 AD, 15:00:00",
locale: "ja-JP",
timeZone: "UTC",
options: {
locale: "ja-JP",
calendar: "iso8601",
numberingSystem: "latn",
timeZone: "UTC",
hourCycle: "h23",
hour12: false
}
} |
|
Quickjs doesn't implement Intl due to size constraints, this will have to be documented as a limitation https://github.com/quickjs-ng/quickjs/blob/master/docs/docs/es_features.md |
|
@nathanwhit
|
bartlomieju
left a comment
There was a problem hiding this comment.
Reviewed the QuickJS backend work. The facade design is the right shape and the CI/lint plumbing is well commented — most of my notes are inline. Summarizing the ones that don't anchor to a line:
No end-to-end coverage for --engine. Every test change in the PR is field churn (engine: None). Nothing exercises the -quickjs zip name construction, --engine quickjs parsing on compile/desktop, or the failure mode when the artifact doesn't exist for a given target/channel. At minimum a unit test over the binary-name logic plus a spec test for the flag. Related: get_dev_binary_path() / get_dev_desktop_binary_path() short-circuit before engine resolution, so with DENORT_BIN set --engine quickjs silently produces a V8 binary — worth at least a log::warn!.
Unrelated changes bundled in. CLAUDE.md asks for minimal PRs with no drive-by changes, and this one carries: the #[op2(reentrant)] change on two heap ops, the uv timer-deadline fix + test, the snapshot object-identity test, TZ=UTC in node_test_mock_timers, and the tests/util/server zlib-ng-compat / preserve_order feature additions. Several are good fixes on their own, but reentrant in particular has real semantics (JS re-entering during a heap snapshot) and deserves its own review. If the test_server feature additions are fallout from the lint.js feature-enumeration change, please say so in the description — as-is they read as arbitrary.
Release artifacts. The QuickJS zips reach dl.deno.land through the --include "*.zip" S3 syncs, so deno compile --engine quickjs will resolve them. They are not in the explicit GitHub release asset list (ci.ts ~1140-1181). Probably intentional for an experimental artifact — just confirming it's a decision rather than an oversight.
Security note for the record. A second engine means a second attack surface on a separate patch cadence, and v8x is currently an RC of an older V8 line. The experimental framing in --help is appropriate; before this is user-visible, a docs note that QuickJS binaries don't carry the same security-update guarantees as V8 would be worth adding.
| quickjs = ["v8/quickjs"] | ||
| v8_use_custom_libcxx = ["v8/use_custom_libcxx"] | ||
| v8_enable_pointer_compression = ["v8/v8_enable_pointer_compression"] | ||
| v8_enable_v8_checks = ["v8/v8_enable_v8_checks"] | ||
| unsafe_runtime_options = [] | ||
| unsafe_use_unprotected_platform = [] | ||
| snapshot_flags_eager_parse = [] | ||
| uv_compat_export = [] | ||
| v8 = ["v8/v8"] |
There was a problem hiding this comment.
deno_core's default still is ["v8_use_custom_libcxx", "reactor-tokio"], and v8_use_custom_libcxx maps to the facade's optional-dep passthrough (rusty_v8?/...), which enables no engine. So a downstream cargo add deno_core now hits compile_error!("either feature v8orquickjs must be enabled") out of the box. Same for the published serde_v8 / serde_v8_utilities, which depend on deno_v8 without selecting a backend.
I think this is also what forces most of the in-tree workarounds in this PR (libs/ops/compile_test_runner, examples/snapshot/tests/output.rs, x.ts, the miri job, --example op2).
Suggest adding "v8" to deno_core's default, and having QuickJS builds go through --no-default-features --features quickjs,... like the CLI already does. That keeps published crates buildable and drops most of the workarounds.
| let engine_suffix = if compile_flags.engine.as_deref() == Some("quickjs") { | ||
| "-quickjs" | ||
| } else { | ||
| "" | ||
| }; | ||
| let binary_name = format!("denort{engine_suffix}-{target}.zip"); |
There was a problem hiding this comment.
This block is duplicated verbatim in get_desktop_base_binary below. A typed enum on CompileFlags would remove both copies and the stringly-typed comparison:
impl Engine {
fn artifact_suffix(&self) -> &'static str {
match self { Engine::V8 => "", Engine::QuickJs => "-quickjs" }
}
}I realize --backend set the Option<String> precedent, but here the string is load-bearing for a download URL, so a typo silently degrades to the V8 artifact rather than failing.
Also worth noting: this code is unreachable when get_dev_binary_path() returns Some (a few lines up), so DENORT_BIN builds ignore --engine entirely.
| let elapsed_ms = | ||
| Instant::now().duration_since(self.time_origin).as_millis() as u64; | ||
| let delay = Duration::from_millis(deadline_ms.saturating_sub(elapsed_ms)); |
There was a problem hiding this comment.
This recomputes elapsed time from time_origin with a fresh Instant::now(), while the timers in the set were scheduled against cached_time_ms / update_time(). Using the same clock source the timer phase uses would avoid sub-millisecond skew between the deadline that was stored and the delay computed here.
| let sleep = timer_sleep.as_mut().unwrap(); | ||
| if sleep.sleep.as_mut().poll(cx).is_ready() { | ||
| *timer_sleep = None; | ||
| cx.waker().wake_by_ref(); | ||
| } |
There was a problem hiding this comment.
Possible busy-loop: when the sleep resolves this clears timer_sleep and immediately wake_by_ref()s. If the earliest deadline has passed but that timer isn't removed by the following timer phase — a repeating timer re-armed at 0, or a stopped-but-not-yet-closed handle — the next poll rebuilds a zero-duration sleep, which is instantly Ready, which wakes again. That spins the event loop hot.
Could you keep a flag that a wake was already issued for deadline_ms and not re-arm until the timer set actually changes?
| if let Some(uv_inner_ptr) = context_state.uv_loop_inner.get() { | ||
| unsafe { (*uv_inner_ptr).poll_timer_deadline(cx) }; |
There was a problem hiding this comment.
Missing a // SAFETY: comment on this raw-pointer deref — every other one in uv_compat.rs documents its invariant, and clippy's undocumented_unsafe_blocks is the house style here.
| # -L SDK usr/lib: the v8x QuickJS backend's build script links bindgen->libc | ||
| # (-liconv); see the aarch64 block below. | ||
| "link-args=-L /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/lib -Wl,-fixup_chains -weak_framework Metal -weak_framework MetalPerformanceShaders -weak_framework QuartzCore -weak_framework CoreGraphics", |
There was a problem hiding this comment.
This hardcodes the Xcode.app SDK path for all macOS builds, including pure-V8 ones that don't need it. It breaks for anyone on a CommandLineTools-only install or with Xcode somewhere other than /Applications, which is a real papercut for contributors who never touch QuickJS.
Deriving it from xcrun --show-sdk-path (build script or an env var set by the tooling) would avoid regressing the default macOS dev setup.
| `(cd ../.. && cargo build --release --locked -p denort -p denort_desktop --no-default-features --features quickjs)`, | ||
| "strip --strip-debug ./denort", | ||
| `zip -r denort-quickjs-${buildItem.arch}-unknown-linux-gnu.zip denort`, | ||
| `shasum -a 256 denort-quickjs-${buildItem.arch}-unknown-linux-gnu.zip > denort-quickjs-${buildItem.arch}-unknown-linux-gnu.zip.sha256sum`, | ||
| "strip ./libdenort.so", | ||
| `zip -r libdenort-quickjs-${buildItem.arch}-unknown-linux-gnu.zip libdenort.so`, | ||
| `shasum -a 256 libdenort-quickjs-${buildItem.arch}-unknown-linux-gnu.zip > libdenort-quickjs-${buildItem.arch}-unknown-linux-gnu.zip.sha256sum`, | ||
| `unzip -o denort-${buildItem.arch}-unknown-linux-gnu.zip`, | ||
| `unzip -o libdenort-${buildItem.arch}-unknown-linux-gnu.zip`, |
There was a problem hiding this comment.
Two concerns with reusing the target dir and restoring via unzip:
- It thrashes the cache for
denort/denort_desktopon every tag build — those crates rebuild for QuickJS, then rebuild again for V8 in any later step that needs them — costing two extra link cycles per platform. - If the QuickJS build fails partway,
target/release/denortis left in an indeterminate state for the steps that follow (./deno typeshere, andcreate_symcacheagainsttarget/release/deno.exeon Windows), since the restoringunzipnever runs.
mv denort denort.v8 before and mv back after is cheaper than the zip round-trip and doesn't depend on the archive being intact. A separate --target-dir would be cleanest if the disk budget allows it.
| name: "Check QuickJS backend (deno + denort)", | ||
| if: isLinux.and(buildItem.arch.equals("x86_64")).and(isDebug), | ||
| run: | ||
| `cargo check --locked -p deno -p denort --no-default-features --features quickjs`, |
There was a problem hiding this comment.
This gate only runs on linux-x86_64 debug. Given that this PR adds a macOS-specific linker workaround for the v8x build script, a macOS QuickJS check seems worth the extra minutes — otherwise the exact platform with the fragile setup is the one that's unverified.
| } | ||
|
|
||
| #[op2] | ||
| #[op2(reentrant)] |
There was a problem hiding this comment.
Marking these ops reentrant isn't related to the QuickJS backend, and it's a real semantic change (JS can now re-enter during a heap snapshot / object count). Could this move to its own PR so it gets reviewed on its own merits? Same for the uv timer-deadline fix, the snapshot identity test, and the test_server feature changes.
| // Regression test for https://github.com/denoland/deno/issues/32987 | ||
| // mock.timers should be a real implementation, not notImplemented stubs. | ||
| "args": "test test.js", | ||
| "envs": { |
There was a problem hiding this comment.
Unrelated to the QuickJS backend — belongs in a separate PR (or at least call it out in the description as a determinism fix found while running the suites).
Repoint the deno_v8 facade's QuickJS backend from the git pin to the published crate: v8x = "=149.4.0-rc.1" from crates.io. The published RC is newer than the previous git rev (adds Linux/Windows binding support and a quickjs promise-hook UAF fix).
The deno_v8 facade requires exactly one of `v8`/`quickjs`. deno_core's default therefore can't enable a backend (that leaks v8 into the `--no-default-features --features quickjs` build and trips the mutually- exclusive guard). But isolated/nested builds that don't inherit a backend feature then fail to compile: the snapshot example's check_output spawns a fresh `cargo run`, and deno_ops trybuild compiles its op2/webidl test cases as their own crates. Select v8 explicitly at those two sites. Nothing in CI builds the QuickJS backend, so such breakage is invisible (reported in the PR). Add a Linux CI step that runs `cargo check` on `deno` + `denort` with `--no-default-features --features quickjs`, plus the macOS SDK lib path v8x's build script needs, so the QuickJS deno-compile / desktop path can't silently regress.
`deno compile --engine quickjs` (and `deno desktop --engine quickjs`) resolves the QuickJS runtime artifacts `denort-quickjs-<target>.zip` / `libdenort-quickjs-<target>.zip` instead of the V8 ones, producing a smaller experimental standalone binary. Default/`v8` is unchanged. `--engine` (not `--backend`, which `deno desktop` already uses for the render backend) threads through CompileFlags/DesktopFlags into the standalone binary resolver. The matching release artifacts are added separately.
Build denort and denort_desktop with `--no-default-features --features quickjs` on each release platform and publish `denort-quickjs-<target>.zip` and `libdenort-quickjs-<target>.zip` alongside the v8 artifacts (the s3 sync globs `*.zip`, so no upload change is needed). `deno compile/desktop --engine quickjs` downloads these. The QuickJS build reuses the same target dir (a second one would exceed the runner disk) so only backend-specific crates rebuild, then the v8 denort/libdenort are restored via unzip/Expand-Archive so the inter-job artifacts stay v8. Add the macOS SDK lib path to the x86_64 target too (v8x's build script needs -liconv there, like aarch64). Note: exercised only by a real tagged release; PR CI does not run the release jobs.
c1b2628 to
9d3ed6c
Compare
Summary
deno_v8facade crate that re-exports either upstreamrusty_v8orv8xv8andquickjsCargo features throughdeno_core,deno,denort, anddenort_desktopThe v8x dependency is pinned to a git revision for now. It can move to crates.io once the package is published.
Building
The existing V8 build remains unchanged:
Build Deno with QuickJS:
Build the standalone runtime used by compile output with QuickJS:
Validation
cargo check --locked -p deno_v8 --no-default-features --features v8,simdutfcargo check --locked -p deno_v8 --no-default-features --features quickjs,simdutfcargo build --locked --bin denocargo build --locked --bin deno --no-default-features --features quickjs./tools/lint.js --js