size: per-module registry for callable-export attach decoration (stream/fs/cp/wasi/vm finally strip; hello −133 KB) - #6931
Conversation
…ndlers (NM_ATTACH_REGISTRY, installed by js_nm_install_<m>)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughNative callable attachment now uses a per-module registry. Module installers register attach handlers, and bound native callable exports look up and invoke handlers instead of using a centralized conditional ladder. ChangesNative callable attachment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant bound_native_callable_export_value
participant native_module_registry
participant nm_attach_handler
bound_native_callable_export_value->>native_module_registry: nm_attach_lookup(export_module_name)
native_module_registry-->>bound_native_callable_export_value: registered nm_attach_* handler
bound_native_callable_export_value->>nm_attach_handler: attach(property_name, value, closure_addr)
nm_attach_handler-->>bound_native_callable_export_value: updated value
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/perry-runtime/src/object/native_module/callable_exports.rs (1)
1632-1639: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the vestigial
if true &&.Tautology left over from the ladder extraction.
♻️ Proposed cleanup
- if true - && matches!( - property_name, - "Readable" | "Writable" | "Duplex" | "Transform" | "PassThrough" - ) - { + if matches!( + property_name, + "Readable" | "Writable" | "Duplex" | "Transform" | "PassThrough" + ) { attach_stream_constructor_prototype(value, property_name); }🤖 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/callable_exports.rs` around lines 1632 - 1639, Remove the vestigial `if true &&` from the conditional surrounding `attach_stream_constructor_prototype`, preserving the existing `matches!` check for the stream constructor property names.
🤖 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.
Inline comments:
In `@crates/perry-runtime/src/object/native_module/callable_exports.rs`:
- Around line 1587-1597: Rename the unused closure_addr parameter to
_closure_addr in nm_attach_tty, nm_attach_tls, nm_attach_wasi, nm_attach_stream,
nm_attach_sqlite, and nm_attach_assert. Keep each function’s signature
compatible with NmAttachFn and leave the existing value handling unchanged.
---
Nitpick comments:
In `@crates/perry-runtime/src/object/native_module/callable_exports.rs`:
- Around line 1632-1639: Remove the vestigial `if true &&` from the conditional
surrounding `attach_stream_constructor_prototype`, preserving the existing
`matches!` check for the stream constructor property names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c6d4058c-3451-41fb-ac81-fa7d13c716db
📒 Files selected for processing (4)
changelog.d/6931-callable-attach-registry.mdcrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/native_module/callable_exports.rscrates/perry-runtime/src/object/native_module_registry.rs
Second increment of the namespace devirt (follows #6928; the mapped plan from that PR's follow-up section).
bound_native_callable_export_value's hand-written per-module attach ladder — the last static reference from the callable-exports core into every module's prototype/statics machinery — is nowNM_ATTACH_REGISTRY(mirrorsNM_DISPATCH_REGISTRY/NM_CTOR_REGISTRY), with 12 per-module handlers (module/tty/tls/wasi/stream/sqlite/assert/crypto/perf_hooks/async_hooks/events/util) registered by the existingjs_nm_install_<module>()fns. Bodies moved verbatim; arming is sound because this path is only reachable through a module's namespace, which exists only after its install ran (plus the same#[cfg(test)]lazy-install fallback the ctor registry uses).Measured on a hello world (default flags): 4,709,904 → 4,576,952 bytes across this branch (−133 KB with #6928's foundation);
ld64 -why_liveconfirmsjs_node_stream_pipelineis now dead, and per-module attribution shows fs 161→27 KB, child_process → 0.9 KB, wasi → 0, node:vm → 6.5 KB.Verification: differential probes byte-identical to Node (stream
Readable.fromiteration,pipeline[promisify.custom],events.once+ EventEmitter statics incl.defaultMaxListeners,AsyncLocalStorage.bind); 12-test gap sweep byte-identical;cargo test -p perry-runtime --lib --test-threads=1: 1476/1476 (parallel runs show only the known #6926 / gc-teardown flakes).Remaining (next increment, same recipe): the twin
get_native_module_constantladder inconstants.rs— now the sole retainer of the node_submodules export tables (80 KB) and the node_stream residual (158 KB) — then Intl (219 KB, needs fallbacks) and lazyglobalThis.Summary by CodeRabbit