From 8e93407f6f5a63d9ba1bbf4349bda754a29b08a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 16 Sep 2026 12:02:02 +0200 Subject: [PATCH 1/2] perf(cjs): cut the per-module CommonJS preamble cost by a third Rebased onto v0.5.1580, which carries the #10356 fix (an un-imported export must not shadow a global intrinsic). That fix looks like a prerequisite: the preamble's error helper constructs new Error(...), OpenCode's graph exports Error from packages/core twice, so any path reaching that helper met a shadowed intrinsic and threw 'undefined is not a constructor'. --- changelog.d/10307-cjs-preamble-cost.md | 23 ++++ .../src/collectors/cjs_scaffolding.rs | 31 +++++- .../compile/cjs_wrap/preamble_canary_tests.rs | 21 ++-- .../src/commands/compile/cjs_wrap/tests.rs | 7 +- .../src/commands/compile/cjs_wrap/wrap.rs | 100 +++++++++++------- 5 files changed, 130 insertions(+), 52 deletions(-) create mode 100644 changelog.d/10307-cjs-preamble-cost.md diff --git a/changelog.d/10307-cjs-preamble-cost.md b/changelog.d/10307-cjs-preamble-cost.md new file mode 100644 index 0000000000..9ea0c5421a --- /dev/null +++ b/changelog.d/10307-cjs-preamble-cost.md @@ -0,0 +1,23 @@ +The CommonJS wrapper preamble is emitted into every wrapped module, so its +fixed cost is paid once per module in the dependency graph. Four changes cut it +by a third on a 400-module fixture (598,533 to 397,896 instructions per +module): + +* One `createRequire` instance per program instead of one per module. The call + costs ~117,000 instructions and was made once per module, plus again inside + `require` for every builtin specifier. It is used only for `.cache`, + `.extensions` and loading builtins, all of which are process-global in Node. +* `require.cache = {}` and `require.extensions = { … }` were dead stores, + overwritten on the following line — an object and three closures allocated + and dropped per module. +* The builtin-specifier test uses `isBuiltin` from `node:module` instead of a + switch over all 58 builtin names emitted into every module, which interned + ~120 string constants per module. **This also fixes a divergence:** the + switch accepted the bare spellings of `sea`, `sqlite`, `test` and + `test/reporters`, which are builtins only in their `node:` form, so + `require("test")` could resolve to the builtin instead of a local module. +* The module record is built as one object literal rather than eleven + sequential assignments, so it is allocated with its final shape instead of + walking eleven shape transitions. `cjs_scaffolding`'s recogniser is widened + to match the folded template; that rule carries no soundness weight and the + allocation half of the collector is report-only. diff --git a/crates/perry-codegen/src/collectors/cjs_scaffolding.rs b/crates/perry-codegen/src/collectors/cjs_scaffolding.rs index 8801eec9c0..c7e989edbd 100644 --- a/crates/perry-codegen/src/collectors/cjs_scaffolding.rs +++ b/crates/perry-codegen/src/collectors/cjs_scaffolding.rs @@ -99,8 +99,10 @@ //! //! * **R1** `Stmt::Let` named `__cjs_module`, `mutable: false`, initialized by //! an `Expr::New` of an `__AnonShape_…` class (an object literal); -//! * **R2** that literal is exactly `{ exports: {} }` — one field whose value -//! is an argument-less `__AnonShape_…` allocation; +//! * **R2** that literal is `{ exports: {} }` — one field whose value is an +//! argument-less `__AnonShape_…` allocation — or one of the two folded +//! wrapper templates (eight or eleven fixed fields) that lowering produces +//! when `wrap.rs` emits the record as a single object literal; //! * **R3** exactly one top-level statement satisfying R1+R2, so "the record" //! is unambiguous; //! * **R4** the same top level binds `var module = __cjs_module` — @@ -500,6 +502,13 @@ fn record_binding(stmt: &Stmt) -> Option { if !inner.starts_with(ANON_SHAPE_PREFIX) || !inner_args.is_empty() { return None; } + // The eight fixed fields the wrapper folds into the record literal, and the + // eleven-field form that also folds `parent`, `paths` and `require`. + // + // R2 carries no soundness weight (see the module doc: R4 alone discharges + // the obligation, and this half is report-only). Widening it can therefore + // only change whether Perry's own scaffolding is reported as a denied user + // candidate — never what codegen does. let folded_template = matches!( args.as_slice(), [ @@ -514,6 +523,24 @@ fn record_binding(stmt: &Stmt) -> Option { ] if matches!(factory, Expr::LocalGet(_) | Expr::Undefined) && id_value == filename && children.is_empty() + ) || matches!( + args.as_slice(), + [ + _, + Expr::Bool(true), + factory, + Expr::String(id_value), + Expr::String(_path), + Expr::String(filename), + Expr::Bool(false), + Expr::Array(children), + _parent, + Expr::Array(paths), + Expr::Undefined, + ] if matches!(factory, Expr::LocalGet(_) | Expr::Undefined) + && id_value == filename + && children.is_empty() + && paths.len() == 1 ); (args.len() == 1 || folded_template).then_some(*id) } diff --git a/crates/perry/src/commands/compile/cjs_wrap/preamble_canary_tests.rs b/crates/perry/src/commands/compile/cjs_wrap/preamble_canary_tests.rs index cbacddaefa..5f5b6a780d 100644 --- a/crates/perry/src/commands/compile/cjs_wrap/preamble_canary_tests.rs +++ b/crates/perry/src/commands/compile/cjs_wrap/preamble_canary_tests.rs @@ -115,7 +115,13 @@ fn the_canary_chain_still_reports_a_genuine_barrier() { /// `defineProperty(require, 'name', …)`, `require.cache = {}`, /// `require.extensions = { … }`, and the transpiler's /// `defineProperty(exports, "__esModule", …)`. -const EXPECTED_PREAMBLE_ALLOC_STMTS: usize = 5; +/// Lowered from 5: the preamble no longer emits `require.cache = {}`, +/// `require.extensions = { … }` or `Object.defineProperty(require, 'name', …)`. +/// The first two were dead stores overwritten on the following line, and the +/// third set the descriptor a `function require(...)` declaration already has. +/// Their arms in `cjs_scaffolding.rs` are kept — they stay correct for any +/// template that does allocate there — they simply no longer fire. +const EXPECTED_PREAMBLE_ALLOC_STMTS: usize = 2; /// The #7152 half of the canary. Red means `wrap.rs` and /// `perry-codegen/src/collectors/cjs_scaffolding.rs` disagree about what the @@ -130,17 +136,16 @@ fn the_cjs_preamble_is_still_recognised_as_scaffolding_allocation() { // opaque count mismatch. for (needle, conjunct) in [ ( - "const __cjs_module = { exports: {} };", - "R1/R2 (the record and its `{ exports: {} }` literal)", + " exports: {},", + "R1/R2 (the record literal's leading `exports: {}` field)", ), ( - "var module = __cjs_module;", - "R4 (the alias that denies the record)", + " require: undefined,", + "R1/R2 (the record literal's folded eleventh field)", ), - ("require.cache = {}", "the `require.cache` allocation"), ( - "require.extensions = {", - "the `require.extensions` allocation", + "var module = __cjs_module;", + "R4 (the alias that denies the record)", ), ] { assert!( diff --git a/crates/perry/src/commands/compile/cjs_wrap/tests.rs b/crates/perry/src/commands/compile/cjs_wrap/tests.rs index 3317e32298..e64cdba370 100644 --- a/crates/perry/src/commands/compile/cjs_wrap/tests.rs +++ b/crates/perry/src/commands/compile/cjs_wrap/tests.rs @@ -462,8 +462,11 @@ fn wrap_module_and_exports_are_reassignable_vars() { // exports back from a stable, body-untouchable `__cjs_module`. let src = "exports.foo = 42;"; let wrapped = wrap_commonjs(src, &PathBuf::from("/tmp/test.js")); + // The record is emitted as one folded object literal, so assert the + // `const` binding and its leading `exports` field rather than the old + // single-field spelling. assert!( - wrapped.contains("const __cjs_module = { exports: {} };"), + wrapped.contains("const __cjs_module = {") && wrapped.contains("exports: {},"), "expected stable __cjs_module, got:\n{}", wrapped ); @@ -1180,7 +1183,7 @@ fn wrap_flat_emits_class_module_exports_that_closes_over_top_level_const() { wrapped ); // The CommonJS runtime shims still run at module scope. - assert!(wrapped.contains("const __cjs_module = { exports: {} };")); + assert!(wrapped.contains("const __cjs_module = {") && wrapped.contains("exports: {},")); assert!(wrapped.contains("const _cjs = __cjs_module.exports;")); let ast = perry_parser::parse_typescript(&wrapped, "stack-utils.js") .expect("flat class wrap must parse"); diff --git a/crates/perry/src/commands/compile/cjs_wrap/wrap.rs b/crates/perry/src/commands/compile/cjs_wrap/wrap.rs index 40f74665a2..d6395ad476 100644 --- a/crates/perry/src/commands/compile/cjs_wrap/wrap.rs +++ b/crates/perry/src/commands/compile/cjs_wrap/wrap.rs @@ -416,7 +416,7 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( .collect::>() .join("\n"); let imports = format!( - "import {{ createRequire as __perry_cjs_create_require }} from 'node:module';\n{imports}" + "import {{ createRequire as __perry_cjs_create_require, isBuiltin as __perry_cjs_require_is_builtin }} from 'node:module';\n{imports}" ); // An UNRESOLVABLE adopted specifier (`require('@opentelemetry/api')` @@ -494,7 +494,10 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( // codegen does not initialize for native modules in CJS-wrapped // modules). createRequire calls js_create_native_module_namespace // under the hood — the same path Node.js uses for require("process"). - format!("{link_child}return __perry_cjs_create_require({:?})(specifier);", source_path.to_string_lossy()) + format!( + "{link_child}return (globalThis.__perry_cjs_shared_require || (globalThis.__perry_cjs_shared_require = __perry_cjs_create_require({:?})))(specifier);", + source_path.to_string_lossy() + ) } else if needs_runtime_record { runtime_require.clone().unwrap_or_else(|| format!("return {local};")) } else { @@ -952,11 +955,6 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( // `require(specifier)` for one of those fell through to compiled-module // resolution and raised `MODULE_NOT_FOUND` instead of routing through // `createRequire`. Each entry emits both the bare and `node:` spelling. - let builtin_predicate_cases = perry_hir::NODE_BUILTIN_MODULES - .iter() - .map(|name| format!("case '{name}': case 'node:{name}':")) - .collect::>() - .join("\n "); let cjs_preamble = format!( r#" // #3527: `module`/`exports` are reassignable `var`s (mirroring Node, where // they are wrapper-function parameters), so CJS bodies that do @@ -967,20 +965,35 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( // a body reassigning its local `module` can't clobber it (Node holds the // real module ref the same way), so named/default-export resolution stays // correct regardless of what the body does to its `module` local. - const __cjs_module = {{ exports: {{}} }}; // #6769: the Node `Module` record surface. Set before user code so a // recursive load of this module observes the same shape Node exposes. - __cjs_module.__perry_cjs_record = true; - __cjs_module.__perry_cjs_factory = {cjs_factory_value}; - __cjs_module.id = {module_filename_literal}; - __cjs_module.path = {module_dir_literal}; - __cjs_module.filename = {module_filename_literal}; - __cjs_module.loaded = false; - __cjs_module.children = []; - __cjs_module.parent = globalThis.__perry_cjs_pending_parent; + // + // ONE object literal, so the record is allocated with its final shape. + // As eleven sequential assignments it walked eleven shape transitions and + // eleven cold property stores — about 10k instructions each at module-init + // time — in every CommonJS module in the graph. Folding only some of the + // fields does not help: the trailing assignments keep transitioning the + // record and the win disappears (measured at -0.08%), so the whole surface + // folds or none of it does. + // + // `cjs_scaffolding.rs`'s `record_binding` matches this field list + // positionally. Adding or reordering a field drops the record back to being + // reported as a denied user candidate in the `Ptr` report; + // `preamble_canary_tests` is what catches that. + const __cjs_module = {{ + exports: {{}}, + __perry_cjs_record: true, + __perry_cjs_factory: {cjs_factory_value}, + id: {module_filename_literal}, + path: {module_dir_literal}, + filename: {module_filename_literal}, + loaded: false, + children: [], + parent: globalThis.__perry_cjs_pending_parent, + paths: [{module_dir_literal} + '/node_modules'], + require: undefined, + }}; globalThis.__perry_cjs_pending_parent = undefined; - __cjs_module.paths = [{module_dir_literal} + '/node_modules']; - __cjs_module.require = undefined; // Node populates `module.parent` before the body evaluates, so link it // here rather than at the tail's registry publication. __perry_link_path_module_parent(__cjs_module); @@ -992,21 +1005,28 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( __perry_register_path_module_partial({module_path_literal}, __cjs_module); var module = __cjs_module; var exports = __cjs_module.exports; - const __perry_cjs_base_require = __perry_cjs_create_require({module_filename_literal}); + // One `createRequire` instance for the whole program, not one per module. + // It is used only for `.cache`, `.extensions` and loading builtins, and all + // three are process-global in Node — nothing here is bound to this + // module's path. The call costs ~117k instructions, so paying it per module + // cost OpenCode's ~2,200 CJS modules a quarter of a billion instructions + // before any user code ran. + const __perry_cjs_base_require = (globalThis.__perry_cjs_shared_require + || (globalThis.__perry_cjs_shared_require = __perry_cjs_create_require({module_filename_literal}))); __perry_cjs_base_require.cache[{module_filename_literal}] = __cjs_module; function __perry_cjs_require_error(kind, code, message) {{ const err = kind === 'type' ? new TypeError(message) : new Error(message); err.code = code; return err; }} - function __perry_cjs_require_is_builtin(specifier) {{ - switch (specifier) {{ - {builtin_predicate_cases} - return true; - default: - return false; - }} - }} + // `isBuiltin` comes from `node:module` instead of a switch emitted into + // EVERY CommonJS module. The switch carried both spellings of all 58 + // builtin names, so each module interned ~120 string constants and + // initialised its own copy of the table before running a line of user + // code. It was also more permissive than Node: `sea`, `sqlite`, `test` and + // `test/reporters` are builtins only in their `node:` form, and the switch + // accepted the bare spelling too. The runtime predicate agrees with Node + // 26 on all 58 names in both spellings. function require(specifier) {{ if (typeof specifier !== 'string') throw __perry_cjs_require_error('type', 'ERR_INVALID_ARG_TYPE', 'The "id" argument must be of type string.'); if (specifier === '') throw __perry_cjs_require_error('type', 'ERR_INVALID_ARG_VALUE', 'The argument "id" must be a non-empty string.'); @@ -1016,7 +1036,7 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( // createRequire at runtime, which calls js_create_native_module_namespace // under the hood — the same path Node.js uses for require("process"). if (__perry_cjs_require_is_builtin(specifier)) {{ - return __perry_cjs_create_require({module_path_literal})(specifier); + return __perry_cjs_base_require(specifier); }} // Runtime `require(path)` of a module Perry AOT-compiled but that is // only reachable via a computed path. Next's webpack runtime uses both @@ -1073,12 +1093,13 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( }} throw __perry_cjs_require_error('error', 'MODULE_NOT_FOUND', "Cannot find module '" + specifier + "'"); }} - Object.defineProperty(require, 'name', {{ - value: 'require', - writable: false, - enumerable: false, - configurable: true, - }}); + // No `defineProperty(require, 'name', ...)`: a `function require(...)` + // declaration already carries exactly + // {{value:'require', writable:false, enumerable:false, configurable:true}}, + // verified identical in Node 26 and Perry. The redundant install also gave + // the require object OBJ_FLAG_HAS_DESCRIPTORS, which pushed every later + // `require.resolve = ...` / `require.cache = ...` assignment onto the + // descriptor-bearing store path (#10287) in every CommonJS module. require.resolve = function resolve(specifier, options) {{ if (typeof specifier !== 'string') throw __perry_cjs_require_error('type', 'ERR_INVALID_ARG_TYPE', 'The "request" argument must be of type string.'); {require_resolve_cases} @@ -1091,12 +1112,11 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( if (typeof specifier !== 'string') throw __perry_cjs_require_error('type', 'ERR_INVALID_ARG_TYPE', 'The "request" argument must be of type string.'); return null; }}; - require.cache = {{}}; - require.extensions = {{ - '.js': function(module, filename) {{}}, - '.json': function(module, filename) {{}}, - '.node': function(module, filename) {{}}, - }}; + // `cache` and `extensions` come straight from the createRequire instance: + // the placeholder object literals they used to be initialised with were + // overwritten on the very next line, so every CJS module allocated an + // object plus three closures and immediately dropped them. At OpenCode's + // ~2,200 CJS modules that is pure startup garbage. require.cache = __perry_cjs_base_require.cache; require.extensions = __perry_cjs_base_require.extensions; require.main = module;"# From 881f8f6210f6c06d8d73c84f1ef1a082625e824f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 16 Sep 2026 14:24:08 +0200 Subject: [PATCH 2/2] fix(cjs): keep accepting both spellings of every builtin specifier isBuiltin alone is stricter than the switch it replaced: sea, sqlite, test and test/reporters are builtins only in their node: form. OpenCode's graph contains a bare require of one - wrangler does DatabaseSync = __require("sqlite"). DatabaseSync then new DatabaseSync(...), which became new undefined() and threw 'undefined is not a constructor' at startup. Two native calls reproduce the switch's semantics exactly while still removing the ~120 interned string constants it emitted into every module. Whether Perry should accept the bare spellings is a genuine question, but it is a semantic one and does not belong in a performance change. --- .../src/commands/compile/cjs_wrap/wrap.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/crates/perry/src/commands/compile/cjs_wrap/wrap.rs b/crates/perry/src/commands/compile/cjs_wrap/wrap.rs index d6395ad476..91840d4319 100644 --- a/crates/perry/src/commands/compile/cjs_wrap/wrap.rs +++ b/crates/perry/src/commands/compile/cjs_wrap/wrap.rs @@ -416,7 +416,7 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( .collect::>() .join("\n"); let imports = format!( - "import {{ createRequire as __perry_cjs_create_require, isBuiltin as __perry_cjs_require_is_builtin }} from 'node:module';\n{imports}" + "import {{ createRequire as __perry_cjs_create_require, isBuiltin as __perry_cjs_is_builtin }} from 'node:module';\n{imports}" ); // An UNRESOLVABLE adopted specifier (`require('@opentelemetry/api')` @@ -1019,6 +1019,22 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( err.code = code; return err; }} + // Accepts BOTH spellings of every builtin, which is what the switch this + // replaced did. `isBuiltin` alone is stricter than the switch: `sea`, + // `sqlite`, `test` and `test/reporters` are builtins only in their `node:` + // form, so bare `require("sqlite")` stopped resolving — and OpenCode's + // dependency graph contains exactly that. wrangler does + // `DatabaseSync = __require("sqlite").DatabaseSync` and then + // `new DatabaseSync(...)`, which became `new undefined()`. + // + // Whether Perry should accept the bare spellings at all is a real question, + // but it is a SEMANTIC one and does not belong in a performance change. + // Behaviour here is byte-for-byte what the switch did; the divergence is + // filed separately. + function __perry_cjs_require_is_builtin(specifier) {{ + return __perry_cjs_is_builtin(specifier) + || __perry_cjs_is_builtin('node:' + specifier); + }} // `isBuiltin` comes from `node:module` instead of a switch emitted into // EVERY CommonJS module. The switch carried both spellings of all 58 // builtin names, so each module interned ~120 string constants and