Remove SetupEmscripten C++ code by hoodmane · Pull Request #6658 · cloudflare/workerd

hoodmane · 2026-04-24T00:42:37Z

Instead import setupEmscripten.ts directly. As requested by @jasnell because SetupEmscripten does not work with new module registry.

Instead import `setupEmscripten.ts` directly. As requested by jasnell because SetupEmscripten does not work with new module registry.

ask-bonk

I'm Bonk, and I've done a quick review of your PR.

This PR removes the SetupEmscripten C++ code path and instead imports setupEmscripten.ts directly as an internal module, simplifying the Emscripten initialization pipeline.

[HIGH] Debug print() statement left in helpers.bzl — this will produce output on every Bazel load of this file, polluting build logs.
[LOW] python_stdlib.zip.d.ts is now a dead file — the declare module 'pyodide-internal:generated/python_stdlib.zip' was removed but the file remains with bare top-level declarations that nothing references. Consider deleting it entirely since modules.d.ts already declares the new pyodideRuntime-internal:python_stdlib.zip module.

This review was generated by an AI assistant and may contain inaccuracies.

ask-bonk · 2026-04-24T00:45:03Z

        deps = ["pyodide.asm.js@rule@" + version],
    )

+    print(_out_path("emscriptenSetup.js", version))


Debug print() left in — this will produce output on every Bazel load.

Suggested change

print(_out_path("emscriptenSetup.js", version))

ask-bonk · 2026-04-24T00:45:03Z

-  export default stdlib;
-}
+const stdlib: Uint8Array;
+export default stdlib;


This file no longer declares a module (the old declare module 'pyodide-internal:generated/python_stdlib.zip' was removed). The new import path pyodideRuntime-internal:python_stdlib.zip is already typed in modules.d.ts. This file is now dead — consider deleting it.

ask-bonk · 2026-04-24T00:45:16Z

Review posted on PR #6658 with two findings:

[HIGH] Debug print() left in src/pyodide/helpers.bzl:284 — this will produce output on every Bazel load, polluting build logs. Posted a suggestion to remove it.
[LOW] src/pyodide/types/python_stdlib.zip.d.ts is now a dead file — the module declaration was removed but the file remains with bare top-level declarations that nothing references. Suggested deleting it since modules.d.ts already declares the new pyodideRuntime-internal:python_stdlib.zip module.

The rest of the PR looks clean: all SetupEmscripten/EmscriptenRuntime references are fully removed, the C++ deletion is complete, the Bazel module type reclassifications (data -> internal/wasm) are correct, and the TypeScript signature change from sync to async loadPyodide is properly awaited at the call site.

github run

codspeed-hq · 2026-04-24T01:10:40Z

Merging this PR will not alter performance

✅ 72 untouched benchmarks
⏩ 129 skipped benchmarks¹

_{Comparing hoodmane/remove-emscripten-setup (1fd9e45) with main (73c75fa)}

129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

danlapid · 2026-04-24T02:05:19Z

Goodbye old friend - you will not be missed

ryanking13

I don't have the exact context of why it was existed in C++ originally, but the change looks good overall.

ryanking13 · 2026-04-24T02:13:05Z

 export async function getSentinelImport() {
-  const module: WebAssembly.Module = new WebAssembly.Module(sentinelWasm);
-  const instance = await WebAssembly.instantiate(module);
+  const { instance } = await wasmInstantiate(sentinelWasm, {});


Is the second paramater of wasmInstantiate ever used?

Well it's the imports parameter so in principle it's pretty important. This module just happens to have no imports.

Perhaps worth at least documenting what the parameter is.

Suggested change

const { instance } = await wasmInstantiate(sentinelWasm, {});

const { instance } = await wasmInstantiate(sentinelWasm, {} /* imports */);

ryanking13 · 2026-04-24T02:18:44Z

 import { getTrustedReadFunc } from 'pyodide-internal:readOnlyFS';
 import { PyodideVersion } from 'pyodide-internal:const';
+import { default as pythonStdlibZip } from 'pyodideRuntime-internal:python_stdlib.zip';
+import { default as pyodideAsmWasmReader } from 'pyodideRuntime-internal:pyodide.asm.wasm';


nitpick: why call it a "reader"?

Good point. Language model decision obviously.

ryanking13 · 2026-04-24T02:20:57Z

-    // Need to set these here so that the logs go to the right context. If we don't they will go
-    // to SetupEmscripten's context and end up being KJ_LOG'd, which we do not want.


Why is this comment removed?

It's not true anymore.

Then can we (and should we) remove this initializeStreams call?

ryanking13 · 2026-04-24T02:22:40Z

 }

 export function newWasmModule(buffer: Uint8Array): WebAssembly.Module {
  if (!UnsafeEval) {


As UnsafeEval is imported directly from top-level, I guess this barrier can be removed.

dom96

I wonder if there are other things we can remove now that SetupEmscripten is gone.

dom96 · 2026-04-24T14:43:56Z

-    // Need to set these here so that the logs go to the right context. If we don't they will go
-    // to SetupEmscripten's context and end up being KJ_LOG'd, which we do not want.


Then can we (and should we) remove this initializeStreams call?

dom96 · 2026-04-24T14:45:03Z

 export async function getSentinelImport() {
-  const module: WebAssembly.Module = new WebAssembly.Module(sentinelWasm);
-  const instance = await WebAssembly.instantiate(module);
+  const { instance } = await wasmInstantiate(sentinelWasm, {});


Perhaps worth at least documenting what the parameter is.

Suggested change

const { instance } = await wasmInstantiate(sentinelWasm, {});

const { instance } = await wasmInstantiate(sentinelWasm, {} /* imports */);

hoodmane requested review from a team as code owners April 24, 2026 00:42

Remove SetupEmscripten C++ code

1fd9e45

Instead import `setupEmscripten.ts` directly. As requested by jasnell because SetupEmscripten does not work with new module registry.

hoodmane force-pushed the hoodmane/remove-emscripten-setup branch from 650f0e3 to 1fd9e45 Compare April 24, 2026 00:43

ask-bonk Bot reviewed Apr 24, 2026

View reviewed changes

ryanking13 reviewed Apr 24, 2026

View reviewed changes

dom96 approved these changes Apr 24, 2026

View reviewed changes

	const { instance } = await wasmInstantiate(sentinelWasm, {});
	const { instance } = await wasmInstantiate(sentinelWasm, {} /* imports */);

		// Need to set these here so that the logs go to the right context. If we don't they will go
		// to SetupEmscripten's context and end up being KJ_LOG'd, which we do not want.

Conversation

hoodmane commented Apr 24, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Apr 24, 2026

Uh oh!

codspeed-hq Bot commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

danlapid commented Apr 24, 2026

Uh oh!

ryanking13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dom96 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codspeed-hq Bot commented Apr 24, 2026 •

edited

Loading