perf(runtime): memoize call/apply rest dispatch - #10127
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe runtime now resolves closure rest metadata through the existing dispatch memo. Tests verify cache reuse, late-registration invalidation, call/apply/spread semantics, receiver handling, argument padding, and imported class references. ChangesClosure dispatch cache
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Refactor · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The dispatch memoization retains late-registration invalidation and is covered across call, apply, spread, receiver, arity, and imported-class scenarios. No merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 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 |
`gc_runtime_root_holders` failed on the train: the call/apply rest-dispatch
cache adds a new identity-ratcheted thread-local that nothing classifies.
crates/perry-runtime/src/closure/registry.rs:312:
BODY_RECORD_LOOKUPS: std::cell::Cell<u32> [rule T]
It is a counter inside a `#[cfg(test)]` `std::thread_local!` block — absent from
production builds, holding a `u32` count and never an address — so it takes a
researched `test_only` verdict.
Its sibling in the same block, RESOLVE_STRATEGY_SLOW_CALLS, is pinned on the
frontier as debt. A pinned entry is explicitly not a GC-safety verdict, so the
new one is classified rather than pinned; that is the stronger record and it
keeps the frontier list from growing for a value that is trivially provable.
|
Landed on Your commits are on One maintainer follow-up: Closing as landed — GitHub cannot auto-close through a train branch. |
Summary
lookup_closure_rest_fullthrough the existing four-entry dispatch-strategy memo instead of probingCLOSURE_BODY_REGISTRYon every call-array dispatch.call, and.apply, plus ordinary/arrow/bound receivers and the imported-class 0x7FFE carve-outCloses #10085.
Performance
Measured from clean release builds of current
main(50e08e91dd6a54d9d9210c43a5d36c86d880d144) and this patch on the same AMD Ryzen 7 7700X host. Both compilers and matching static runtime/stdlib archives were rebuilt withcargo build --release --locked; reproducers used--no-auto-optimize. Node was exact v26.5.1 (SHA-256fb48e77df2f8e92fedfec39afa60a5f41563441f6b61316ada5fb295a431c2c6). Runs were serialized on one pinned CPU, with baseline/candidate order alternated. Values below are medians across paired complete sweeps; ratio is Perry/Node.function-call(5 paired sweeps)function-apply(7 paired sweeps)All Node/Perry checksums matched. Every paired median ratio improved, but the sub-1% movements—especially apply at n=100—are noise-sensitive. The deterministic counter test is the evidence for the targeted result: after the first resolution, repeated dispatch performs no closure-body hash lookup. The remaining gap is dominated by the wider method-call/apply path, consistent with the issue's note that its source attribution was not exclusive.
Validation
cargo test -p perry-runtime dispatch_recent_tests --libcargo test -p perry-runtime closure:: --lib -- --test-threads=1cargo test -p perry-runtime body_record_tests --libcargo fmt --all --checkcargo clippy -p perry-runtime --profile perry-devpython3 scripts/check_test_registration.pyokfortest_gap_10085_call_apply_dispatch.tsThe full parallel runtime suite reported three state-sensitive failures; each passed immediately in isolation.
pre-tag-check --quickpasses all relevant checks but reports the pre-existing public benchmark-evidence freshness failure on unmodified benchmark/Cargo inputs.Coordination
The sibling #10084 / PR #10119 branch does not touch
registry.rsor this memo, and this patch does not touch bind metadata.Summary by CodeRabbit
Performance
call,apply, and spread syntax by reducing dispatch overhead.Bug Fixes
Tests