[turbopack] Promote exports and module to factory parameters for cjs (#82285)

## Optimze cjs codegen by promoting `module` and `exports` to factory parameters

*tl;dr; destructing is hard, lets go shopping!*

Currently we prefix factories for cjs modules with
`var {m:module, e:exports} = __turbopack_context__;`

This is problematic since swc is unable to eliminate these bindings when they are dead (too afraid of es5 getter properties and side effects 😭).  Furthermore we are playing a game of telephone with ourselves since our caller basically is doing

`factory({m:module, e: module.exports})` when they call us (this is skimming over a bunch of details, ofc).

Instead just pass them directly as function parameters.  One downside of this is that we still pass them to `esm` factories even though they are ignored (and simply not declared by the factories).  This is not too costly since the caller has trivial access already and browsers are pretty good at handling/ignoring extra parameters (see https://v8.dev/blog/adaptor-frame for the clever way v8 optimizes this).

### Alternatives

The main alternative considered was compile time replacing references to `module` and `exports` with `__turbopack_context__.(m|e)`. Unfortunately this breaks subtle edge cases of the cjs spec since it is allowed for users to rewrite `module` and `exports` with assignments  (e.g. `exports = 'bad idea'`).  This is generally a linter error but not unheard of and we cannot break it.

The other alternative considered was enhancing `swc` to inline and dead code eliminate destructuring patterns, but from playing with the playgound it looks like swc is pretty conservative about moving/inlining variables across side effectful statements, so we would still be paying the cost of the declaration in many cases.

## Turbopack Performance Comparison: Head vs Change

As expected this is a substantial improvement to commonjs evaluation. The ESM results are kind of confusing.  I generally expected 'neutral' since the cost of passing a dead parameters is so small in modern vms.

| Test Case | Metric | Head (ms) | Change (ms) | Speedup | Improvement |
|-----------|--------|-----------|-------------|---------|-------------|
| **Client CommonJS** | Load - Average | 31.61 | 24.38 | **1.30x** | **+29.6%** |
| | Load - Median | 32.05 | 24.30 | **1.32x** | **+31.9%** |
| | Load - P75 | 33.10 | 24.90 | **1.33x** | **+32.9%** |
| | Execute - Average | 25.96 | 19.80 | **1.31x** | **+31.1%** |
| | Execute - Median | 25.90 | 19.50 | **1.33x** | **+32.8%** |
| | Execute - P75 | 26.50 | 20.20 | **1.31x** | **+31.2%** |
| **Client ESM** | Load - Average | 22.28 | 21.94 | **1.02x** | **+1.6%** |
| | Load - Median | 21.80 | 21.60 | **1.01x** | **+0.9%** |
| | Load - P75 | 23.50 | 22.10 | **1.06x** | **+6.3%** |
| | Execute - Average | 50.77 | 48.12 | **1.06x** | **+5.5%** |
| | Execute - Median | 50.85 | 47.55 | **1.07x** | **+6.9%** |
| | Execute - P75 | 51.60 | 48.30 | **1.07x** | **+6.8%** |
| **Pages API CommonJS** | Load - Average | 24.18 | 23.33 | **1.04x** | **+3.7%** |
| | Load - Median | 23.27 | 23.57 | 0.99x | -1.3% |
| | Load - P75 | 25.29 | 23.96 | **1.06x** | **+5.5%** |
| | Execute - Average | 32.23 | 24.26 | **1.33x** | **+32.8%** |
| | Execute - Median | 31.16 | 24.34 | **1.28x** | **+28.0%** |
| | Execute - P75 | 33.42 | 24.52 | **1.36x** | **+36.3%** |
| **Pages API ESM** | Load - Average | 23.48 | 28.91 | 0.81x | -18.8% |
| | Load - Median | 23.13 | 28.91 | 0.80x | -20.0% |
| | Load - P75 | 25.55 | 29.57 | 0.86x | -13.6% |
| | Execute - Average | 63.01 | 55.94 | **1.13x** | **+12.6%** |
| | Execute - Median | 61.56 | 55.22 | **1.11x** | **+11.5%** |
| | Execute - P75 | 69.56 | 56.40 | **1.23x** | **+23.3%** |

**Legend:**
- **Bold values** indicate performance improvements (speedups)
- Regular values indicate performance regressions (slowdowns)
- Speedup = Head Time ÷ Change Time (>1.0 = faster, <1.0 = slower)
- P75 = 75th percentile

Author: lukesandberg

crates/next-core/src/hmr_entry.rs

@@ -192,7 +192,6 @@ impl EcmascriptChunkItem for HmrEntryChunkItem {
             inner_code: code.build(),
             options: EcmascriptChunkItemOptions {
                 strict: true,
-                module: true,
                 ..Default::default()
             },
             ..Default::default()

turbopack/crates/turbopack-ecmascript-runtime/js/src/browser/runtime/base/build-base.ts

@@ -59,13 +59,17 @@ function instantiateModule(
   }
 
   const module: Module = createModuleObject(id)
+  const exports = module.exports
 
   moduleCache[id] = module
 
   // NOTE(alexkirsz) This can fail when the module encounters a runtime error.
-  const context = new (Context as any as ContextConstructor<Module>)(module)
+  const context = new (Context as any as ContextConstructor<Module>)(
+    module,
+    exports
+  )
   try {
-    moduleFactory(context)
+    moduleFactory(context, module, exports)
   } catch (error) {
     module.error = error as any
     throw error

turbopack/crates/turbopack-ecmascript-runtime/js/src/browser/runtime/base/dev-base.ts

@@ -165,15 +165,20 @@ const getOrInstantiateModuleFromParent: GetOrInstantiateModuleFromParent<
 function DevContext(
   this: TurbopackDevContext,
   module: HotModule,
+  exports: Exports,
   refresh: RefreshContext
 ) {
-  Context.call(this, module)
+  Context.call(this, module, exports)
   this.k = refresh
 }
 DevContext.prototype = Context.prototype
 
 type DevContextConstructor = {
-  new (module: HotModule, refresh: RefreshContext): TurbopackDevContext
+  new (
+    module: HotModule,
+    exports: Exports,
+    refresh: RefreshContext
+  ): TurbopackDevContext
 }
 
 function instantiateModule(
@@ -217,6 +222,7 @@ function instantiateModule(
   }
 
   const module: HotModule = createModuleObject(id) as HotModule
+  const exports = module.exports
   module.parents = parents
   module.children = []
   module.hot = hot
@@ -229,9 +235,10 @@ function instantiateModule(
     runModuleExecutionHooks(module, (refresh) => {
       const context = new (DevContext as any as DevContextConstructor)(
         module,
+        exports,
         refresh
       )
-      moduleFactory(context)
+      moduleFactory(context, module, exports)
     })
   } catch (error) {
     module.error = error as any

turbopack/crates/turbopack-ecmascript-runtime/js/src/nodejs/runtime.ts

@@ -216,12 +216,16 @@ function instantiateModule(
   }
 
   const module: Module = createModuleObject(id)
+  const exports = module.exports
   moduleCache[id] = module
 
+  const context = new (Context as any as ContextConstructor<Module>)(
+    module,
+    exports
+  )
   // NOTE(alexkirsz) This can fail when the module encounters a runtime error.
   try {
-    const context = new (Context as any as ContextConstructor<Module>)(module)
-    moduleFactory(context)
+    moduleFactory(context, module, exports)
   } catch (error) {
     module.error = error as any
     throw error

turbopack/crates/turbopack-ecmascript-runtime/js/src/shared/runtime-types.d.ts

@@ -115,7 +115,7 @@ interface ModuleWithDirection extends Module {
 
 interface TurbopackBaseContext<M> {
   a: AsyncModule
-  e: Module['exports']
+  e: Exports
   r: CommonJsRequire
   t: RuntimeRequire
   f: ModuleContextFactory

turbopack/crates/turbopack-ecmascript-runtime/js/src/shared/runtime-utils.ts

@@ -22,9 +22,20 @@ const REEXPORTED_OBJECTS = Symbol('reexported objects')
 /**
  * Constructs the `__turbopack_context__` object for a module.
  */
-function Context(this: TurbopackBaseContext<Module>, module: Module) {
+function Context(
+  this: TurbopackBaseContext<Module>,
+  module: Module,
+  exports: Exports
+) {
   this.m = module
-  this.e = module.exports
+  // We need to store this here instead of accessing it from the module object to:
+  // 1. Make it available to factories directly, since we rewrite `this` to
+  //    `__turbopack_context__.e` in CJS modules.
+  // 2. Support async modules which rewrite `module.exports` to a promise, so we
+  //    can still access the original exports object from functions like
+  //    `esmExport`
+  // Ideally we could find a new approach for async modules and drop this property altogether.
+  this.e = exports
 }
 const contextPrototype = Context.prototype as TurbopackBaseContext<Module>
 
@@ -145,11 +156,16 @@ function esmExport(
 }
 contextPrototype.s = esmExport
 
-function ensureDynamicExports(module: Module, exports: Exports) {
-  let reexportedObjects = module[REEXPORTED_OBJECTS]
+type ReexportedObjects = Record<PropertyKey, unknown>[]
+function ensureDynamicExports(
+  module: Module,
+  exports: Exports
+): ReexportedObjects {
+  let reexportedObjects: ReexportedObjects | undefined =
+    module[REEXPORTED_OBJECTS]
 
   if (!reexportedObjects) {
-    reexportedObjects = module[REEXPORTED_OBJECTS] = []
+    module[REEXPORTED_OBJECTS] = reexportedObjects = []
     module.exports = module.namespaceObject = new Proxy(exports, {
       get(target, prop) {
         if (
@@ -176,6 +192,7 @@ function ensureDynamicExports(module: Module, exports: Exports) {
       },
     })
   }
+  return reexportedObjects
 }
 
 /**
@@ -186,16 +203,19 @@ function dynamicExport(
   object: Record<string, any>,
   id: ModuleId | undefined
 ) {
-  let module = this.m
-  let exports = this.e
+  let module: Module
+  let exports: Module['exports']
   if (id != null) {
     module = getOverwrittenModule(this.c, id)
     exports = module.exports
+  } else {
+    module = this.m
+    exports = this.e
   }
-  ensureDynamicExports(module, exports)
+  const reexportedObjects = ensureDynamicExports(module, exports)
 
   if (typeof object === 'object' && object !== null) {
-    module[REEXPORTED_OBJECTS]!.push(object)
+    reexportedObjects.push(object)
   }
 }
 contextPrototype.j = dynamicExport
@@ -205,9 +225,11 @@ function exportValue(
   value: any,
   id: ModuleId | undefined
 ) {
-  let module = this.m
+  let module: Module
   if (id != null) {
     module = getOverwrittenModule(this.c, id)
+  } else {
+    module = this.m
   }
   module.exports = value
 }
@@ -218,9 +240,11 @@ function exportNamespace(
   namespace: any,
   id: ModuleId | undefined
 ) {
-  let module = this.m
+  let module: Module
   if (id != null) {
     module = getOverwrittenModule(this.c, id)
+  } else {
+    module = this.m
   }
   module.exports = module.namespaceObject = namespace
 }
@@ -664,7 +688,7 @@ function requireStub(_moduleId: ModuleId): never {
 contextPrototype.z = requireStub
 
 type ContextConstructor<M> = {
-  new (module: Module): TurbopackBaseContext<M>
+  new (module: Module, exports: Exports): TurbopackBaseContext<M>
 }
 
 function applyModuleFactoryName(factory: Function) {

turbopack/crates/turbopack-ecmascript/src/chunk/item.rs

@@ -21,7 +21,7 @@ use crate::{
     EcmascriptModuleContent,
     references::async_module::{AsyncModuleOptions, OptionAsyncModuleOptions},
     runtime_functions::TURBOPACK_ASYNC_MODULE,
-    utils::{FormatIter, StringifyJs},
+    utils::StringifyJs,
 };
 
 #[turbo_tasks::value(shared)]
@@ -66,7 +66,6 @@ impl EcmascriptChunkItemContent {
                     strict: true,
                     externals,
                     async_module,
-                    stub_require: true,
                     ..Default::default()
                 }
             } else {
@@ -78,8 +77,7 @@ impl EcmascriptChunkItemContent {
                     strict,
                     externals,
                     // These things are not available in ESM
-                    module: true,
-                    exports: true,
+                    module_and_exports: true,
                     ..Default::default()
                 }
             },
@@ -90,37 +88,27 @@ impl EcmascriptChunkItemContent {
 
     #[turbo_tasks::function]
     pub async fn module_factory(&self) -> Result<Vc<Code>> {
-        let mut args = Vec::new();
-        if self.options.module {
-            args.push("m: module");
-        }
-        if self.options.exports {
-            args.push("e: exports");
-        }
-
         let mut code = CodeBuilder::default();
         for additional_id in self.additional_ids.iter().try_join().await? {
             writeln!(code, "{}, ", StringifyJs(&*additional_id))?;
         }
-        code += "((__turbopack_context__) => {\n";
+        if self.options.module_and_exports {
+            code += "((__turbopack_context__, module, exports) => {\n";
+        } else {
+            code += "((__turbopack_context__) => {\n";
+        }
         if self.options.strict {
             code += "\"use strict\";\n\n";
         } else {
             code += "\n";
         }
-        if !args.is_empty() {
-            let args = FormatIter(|| args.iter().copied().intersperse(", "));
-            writeln!(code, "var {{ {args} }} = __turbopack_context__;")?;
-        }
 
         if self.options.async_module.is_some() {
             writeln!(
                 code,
                 "return {TURBOPACK_ASYNC_MODULE}(async (__turbopack_handle_async_dependencies__, \
                  __turbopack_async_result__) => {{ try {{\n"
             )?;
-        } else if !args.is_empty() {
-            code += "{\n";
         }
 
         let source_map = if let Some(rewrite_source_path) = &self.rewrite_source_path {
@@ -138,8 +126,6 @@ impl EcmascriptChunkItemContent {
                  }}, {});",
                 opts.has_top_level_await
             )?;
-        } else if !args.is_empty() {
-            code += "}";
         }
 
         code += "})";
@@ -154,15 +140,9 @@ impl EcmascriptChunkItemContent {
 pub struct EcmascriptChunkItemOptions {
     /// Whether this chunk item should be in "use strict" mode.
     pub strict: bool,
-    /// Whether this chunk item's module factory should include a `module`
-    /// argument.
-    pub module: bool,
-    /// Whether this chunk item's module factory should include an `exports`
-    /// argument.
-    pub exports: bool,
-    /// Whether this chunk item's module factory should include an argument for a throwing require
-    /// stub (for ESM)
-    pub stub_require: bool,
+    /// Whether this chunk item's module factory should include a `module` and
+    /// `exports` argument.
+    pub module_and_exports: bool,
     /// Whether this chunk item's module factory should include a
     /// `__turbopack_external_require__` argument.
     pub externals: bool,

turbopack/crates/turbopack-ecmascript/src/tree_shake/chunk_item.rs

@@ -169,7 +169,6 @@ impl EcmascriptChunkItem for SideEffectsModuleChunkItem {
             rewrite_source_path: None,
             options: EcmascriptChunkItemOptions {
                 strict: true,
-                exports: true,
                 async_module: if has_top_level_await {
                     Some(AsyncModuleOptions {
                         has_top_level_await: true,

turbopack/crates/turbopack-tests/tests/execution/turbopack/code-gen/typeof-exports-module/input/exports-reassign.js

@@ -0,0 +1,3 @@
+exports = () => 'hello'
+if (typeof exports === 'object') throw 'oh no'
+module.exports = 1234

turbopack/crates/turbopack-tests/tests/execution/turbopack/code-gen/typeof-exports-module/input/index.js

@@ -1,10 +1,15 @@
 import moduleUniversal from './module-universal.js'
 import moduleReassign from './module-reassign.js'
+import exportsReassign from './exports-reassign.js'
 
 it('should use the CommonJS variant of universal module definitions', () => {
   expect(moduleUniversal()).toBe('other-dep 1234')
 })
 
 it('should not replace typeof exports for non-free variables', () => {
-  expect(moduleReassign).toBe(1234)
+  expect(exportsReassign).toBe(1234)
+})
+
+it('should not replace typeof module for non-free variables', () => {
+  expect(moduleReassign.foo).toBe(1234)
 })