perf(timer): cut setTimeout/unref/clearTimeout churn cost (#10522) - #11373
Conversation
The timer tables' linear scans named in #10522 were already gone (per-agent heap + id index, FIFO retired-id eviction); the remaining cost was constant factor, ~13.7k instructions per set+unref+clear round: - t.ref()/unref()/hasRef()/refresh() on a timer handle skip the dynamic dispatch tower when the call provably resolves to the family's native method (pristine receiver + prototype slot still holding the thunk). Every user override fails a guard and takes the tower as before. - The timer ref-state registry and async_hooks RESOURCES use aHash for their internal monotonic ids instead of SipHash. - async_hooks emit_init returns before allocating the type-name string when no hooks are enabled. ~7.8k instructions per round now; per-op cost is flat in ids created.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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 (7)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe runtime adds guarded direct dispatch for four timer methods. It also changes two map hashers to aHash and skips async-hook resource setup when no hooks are active. ChangesRuntime performance
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant js_native_call_method
participant try_timer_method_fast_dispatch
participant TimerHandle
participant GenericDispatch
js_native_call_method->>try_timer_method_fast_dispatch: Try named timer method
alt Eligibility checks pass
try_timer_method_fast_dispatch->>TimerHandle: Invoke timer operation
try_timer_method_fast_dispatch-->>js_native_call_method: Return direct result
else Eligibility checks fail
try_timer_method_fast_dispatch-->>js_native_call_method: Return None
js_native_call_method->>GenericDispatch: Continue normal dispatch
end
Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk was identified in the timer or async-hook changes. The reported wall-clock performance target remains unmet, but that is a stated limitation rather than an established regression. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new timer call path checks that a handle still uses its original native method before bypassing ordinary lookup. No new security boundary bypass was identified, but the available security coverage is incomplete. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Directly linked issue Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 6 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 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 |
f41281d to
ae91ecb
Compare
Summary
#10522 measured two linear scans in the timer tables: the
min()eviction inrecord_timer_handle_kind, andclearTimeout's find/retain overCALLBACK_TIMERS. Both are already gone onmain. Timers now live in a per-agent slab + heap with an id index (turnloop P3), and the ref-state registry evicts retired ids first-in-first-out (#10447). Re-measuring on currentmainconfirmed the id-count and live-count cliffs are gone, but the per-round cost was still ~13.7 k instructions persetTimeout+unref()+clearTimeout. The issue targets ≤10 k. This PR removes the three biggest constant costs; the round is now ~7.8 k instructions.No version bump, per request.
Changes
t.ref()/t.unref()/t.hasRef()/t.refresh()(timer/handle_object.rs::try_timer_method_fast_dispatch, hooked intojs_native_call_methodright after the perf(runtime): class dispatch and instanceof stop consulting locked hash maps — shapes.ts 0.28s → 0.23s #7769 class-vtable fast path).metarecord and a recorded prototype, so perf(runtime): class dispatch and instanceof stop consulting locked hash maps — shapes.ts 0.28s → 0.23s #7769's fast path refuses it. Each call walked the full tower (handle/primitive probes, a by-name prototype read that allocated the key string, a closure rebind and a call through the thunk), which cost ~6.3 k instructions per call.t.unref,Timeout.prototype.unref = f, a getter viadefineProperty,Object.setPrototypeOf(t, …)anddelete. Immediate handles have norefresh, so that call still reaches the tower.timer/ref_states.rs) andasync_hooks::RESOURCEShash their internal monotonic ids with aHash instead of SipHash. aHash is already aperry-runtimedependency, and neither map takes untrusted keys.async_hooks::emit_initreturns early when no hook is enabled. It used to allocate the"Timeout"type-name string once persetTimeoutand then drop it;with_hook_callbacksalready returned at once in that case, so behaviour is unchanged.changelog.d/10522-timer-churn-constant-factor.md.Related issue
Closes #10522. The issue's four targets:
Leaving it to the maintainer whether that last target should keep the issue open.
Test plan
Benchmark: the issue's
bench.ts,PERRY_NO_AUTO_OPTIMIZE=1, Linux x64. Instructions counted with callgrind; the benchmark was built withPERRY_TARGET_CPU=x86-64because valgrind can't decode the host-tuned code.Wall clock, median of 5, against Node 26.5.1 (from
.node-version):Checksums matched Node in every run.
Unit tests:
RUST_TEST_THREADS=1 cargo test --release -p perry-runtime --lib -- timer async_hooksgives 64 passed. That includes the newtimer::tests_inline::method_fast_path_tests, which asserts the fast path's answers, that thejs_native_call_methodhook is reached, and that each override above (own property, replaced prototype method, prototype getter,setPrototypeOf) falls back to the tower.Timer parity tests: every
test-files/test matching timer/timeout/immediate/unref/async_hook/refresh/using, compiled with the new compiler and compared byte-for-byte against Node 26.5.1. Alltest_gap_*andtest_parity_*timer tests pass. Two exceptions:test_issue_5540_sock_write_map_from_timerdiffers only in how a refused-connection error prints ([object Object]vsError: connect ECONNREFUSED …, no server in the sandbox). That's not in the timer path or the gap gate.Lint checks:
cargo fmt --check,scripts/check_file_size.sh,scripts/addr_class_inventory.pyandscripts/gc_runtime_root_holders.pyall clean.cargo build --releaseclean (-p perry -p perry-runtime-static -p perry-stdlib-static)full
cargo test --workspace(left to CI)Added
#[test]s in the affected crateChecklist
perf:prefix convention🤖 Generated with Claude Code
https://claude.ai/code/session_01EShQaaTSCSytaaXwsZzaYG
Generated by Claude Code
Summary by CodeRabbit
ref,unref,hasRef, andrefreshnow use a faster path when applicable, while preserving normal behavior when methods or prototypes have been modified.