size: NmNamespaceOps armed dispatch for namespace-object behaviors + armed reactor pump slots (devirt foundation, −50 KB) - #6928
Conversation
… NmNamespaceOps (descriptors/field-set/reflect/own-keys)
…p slots (unpin from js_run_stdlib_pump)
…s before callable-exports arms (probe-caught)
📝 WalkthroughWalkthroughNative-module namespace operations now use a lazily armed ChangesNamespace hooks and reactor dispatch
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NamespaceObject as Native-module namespace object
participant ObjectRuntime as Object runtime
participant NamespaceOps as NmNamespaceOps
participant NativeModule as Native-module implementation
NamespaceObject->>ObjectRuntime: request descriptor, key, field, or method operation
ObjectRuntime->>NamespaceOps: query armed operation table
NamespaceOps->>NativeModule: invoke routed namespace helper
NativeModule-->>ObjectRuntime: return operation result
sequenceDiagram
participant ChildProcess as child_process reactor
participant PTY as node-pty reactor
participant RuntimePump as stdlib_pump
participant StdlibPump as js_run_stdlib_pump
ChildProcess->>RuntimePump: register slot 0 callback
PTY->>RuntimePump: register slot 1 callback
StdlibPump->>RuntimePump: run_runtime_pumps()
RuntimePump->>ChildProcess: invoke child-process pump
RuntimePump->>PTY: invoke PTY pump
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-runtime/src/object/native_module.rs (1)
336-337: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: drop the pass-through wrappers if signatures already match.
nm_own_keys_array_optandnm_bind_method_opsjust forward tovt_own_keys_arrayandjs_native_module_bind_methodwith identical signatures — if those functions areunsafe fnwith exactly the same argument/return types as theNmNamespaceOpsfields, you could assign them directly and drop the two wrapper functions.Also applies to: 357-365
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/object/native_module.rs` around lines 336 - 337, In the native module initialization around the NmNamespaceOps fields own_keys_array and bind_method, verify whether vt_own_keys_array and js_native_module_bind_method already have matching unsafe function signatures; if so, assign those functions directly and remove the redundant pass-through wrappers nm_own_keys_array_opt and nm_bind_method_ops.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/perry-runtime/src/object/native_module.rs`:
- Around line 336-337: In the native module initialization around the
NmNamespaceOps fields own_keys_array and bind_method, verify whether
vt_own_keys_array and js_native_module_bind_method already have matching unsafe
function signatures; if so, assign those functions directly and remove the
redundant pass-through wrappers nm_own_keys_array_opt and nm_bind_method_ops.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 04b21c87-24f3-48b2-85d7-1d36900e879e
📒 Files selected for processing (15)
changelog.d/6928-nm-namespace-hooks.mdcrates/perry-runtime/src/child_process/reactor.rscrates/perry-runtime/src/closure/dispatch/value_call.rscrates/perry-runtime/src/lib.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/class_registry/parent_static.rscrates/perry-runtime/src/object/class_registry/prototype_objects.rscrates/perry-runtime/src/object/descriptors.rscrates/perry-runtime/src/object/field_set_by_name.rscrates/perry-runtime/src/object/mod.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/nm_namespace_hooks.rscrates/perry-runtime/src/object/reflect_support.rscrates/perry-runtime/src/pty/reactor.rscrates/perry-runtime/src/value/dynamic_object.rs
Summary
Foundation for the native-module namespace devirt (−50 KB now on a hello world: 4,709,904 → 4,659,792 bytes; unlocks the per-module
callable_exports/constantsregistry split where the bulk lands).Changes
NmNamespaceOpsarmed function table (object/nm_namespace_hooks.rs): the always-linked generic object paths reach native-module namespace behavior through an armed table instead of static calls —Object.getOwnPropertyDescriptor's namespace branch (fs accessor special cases,process.permission) —descriptors.rsBuffer.poolSize) —field_set_by_name.rsReflectexisting-own-key probe (repeatedObject.defineProperty(console, …)) —reflect_support.rsdescriptors.rsconst fn = fs.lstatSync) —value/dynamic_object.rsclass_registry/prototype_objects.rssuper()EventEmitter subclass init —closure/dispatch/value_call.rsThe table is armed by
install_native_module_vtable(), which every path that mints aNATIVE_MODULE_CLASS_IDobject calls first (all five mint sites verified adjacent). Extracted implementations stay in their original files; the ONLY reference to each is the ops static on the native-module side, so the reference direction does the dead-stripping. Unarmed + matching class-id is unreachable, so behavior is bit-identical (the hook bodies provably never execute in a program without namespaces — traced).Armed runtime-pump slots (
lib.rs): the child_process and node-pty reactors self-register (at first live handle,black_box-guarded likeNM_INSTALL_ALL_HOOK) instead of being statically drained byjs_run_stdlib_pump— this cut thepump → cp emitter → cluster → namespace-creationretainer chain and is where the −50 KB comes from.One hook was attempted and reverted in-branch (Buffer-subclass static dispatch): a differential probe showed
class X extends Bufferregisters its parent before the lazy callable-exports closure would arm. The probe suite is the enforcement mechanism for every arming invariant here.Verification
console.errorredefinition,require('node:timers').setImmediatepatching,Object.keys(fs), dynamic-super()EventEmitter subclassing (bus.emit/setMaxListeners).cargo test -p perry-runtime --lib: serial run 1472/1472 green. Parallel runs show the known gc::teardown/prop_plan flakes plusglobal_this_webassembly::namespace_members_exist_with_expected_shapes— which fails identically on pristine main (6/6) and whose hook bodies provably never run in the suite; filed as flaky: global_this_webassembly::namespace_members_exist_with_expected_shapes fails under parallel cargo test on current main (6/6), passes serial/solo #6926.class X extends Bufferinherited statics).Follow-up (mapped, not in this PR)
-why_livenow terminates at two structures: the eagerpopulate_global_this_builtins(multi-rooted) and the universalbound_native_callable_export_value(module, prop) dispatcher. Splitting the latter into per-module registries + lazy globalThis installs is the ~300-400 KB unlock; this PR is deliberately limited to the reference-direction foundation.Summary by CodeRabbit