perf(runtime): skip the arg-combine Vec for no-partial-args bound callbacks (−4.6% CPU) - #10818
proggeramlug wants to merge 2 commits into
Conversation
…s bound functions dispatch_bound_function unconditionally built a Vec<f64>, copied the call-time args into it, and freed it before every Function.prototype.bind call -- even for the overwhelmingly common .bind(thisArg) method-reference shape with no partial-applied arguments, where js_function_bind already leaves the bound-args capture null. That shape is exactly what direct.rs's per-loop dispatch hoisting excludes (BoundFunction is deliberately not resolved by resolve_direct_func_ptr), so a bound method used as an arr.forEach callback paid this allocation on every element.
|
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 (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe bound-function dispatcher now forwards caller arguments directly when no partial arguments are bound. Partial application still combines bound and caller arguments. New tests cover callback dispatch, binding, argument handling, exceptions, recursion, and closure capture. ChangesBound callback dispatch
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant CallbackCaller
participant dispatch_bound_function
participant js_native_call_value
CallbackCaller->>dispatch_bound_function: pass caller args
dispatch_bound_function->>js_native_call_value: forward caller args when no partial args are bound
dispatch_bound_function->>js_native_call_value: pass combined bound and caller args for partial application
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 29.41% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 2 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 |
|
Landed via merge train 242 (#10830) as v0.5.1621 — Eight PRs travelled together because their file sets are disjoint — 30 files, +1,514/−101, zero overlap. Validated as one tree: ten cheap gates, Two of the eight needed a fix before they could land, both made in the train rather than bounced back. #10816 bound #10817 added 15 dispatch entries without regenerating the docs, so the API-docs-drift check failed. Regenerated from a built binary: 2855 → 2870, exactly your 15, with For future PRs in this area: One more thing, aimed at whoever cuts the next PR here: |
1 participant
dispatch_bound_function— the entry everyjs_closure_call<N>routes a.bind()-created callback through — unconditionally allocated, populated and freed aVec<f64>on every call, including for.bind(thisArg)with no partial-applied args at all.That no-partial-args shape is the plain method reference:
arr.forEach(obj.method.bind(obj)). And it is precisely the shapedirect.rscannot hoist out of a loop —resolve_direct_func_ptrreturnsNoneforBOUND_FUNCTION_FUNC_PTR(direct.rs:70), deliberately — so it re-pays the allocate-copy-free on every element.js_function_bindalready leaves the bound-args capture slot null wheneverbound_arg_count == 0, so the case is free to detect. The fix skips theVecand handsjs_native_call_valuethe caller's own args slice; only actual partial application (.bind(obj, extra)) still builds a combined buffer.This lever's specific hazard, checked rather than assumed
A previous attempt in this area won on instruction count and cost 54% CPU. Removing an allocation is exactly that shape, so instructions alone were never going to be sufficient evidence here.
Measured both, under a measurement mutex, after waiting for host load to fall below 8:
CPU agrees with instructions rather than contradicting them, and the non-bound control is live and flat — which is what makes the comparison readable rather than merely green. An earlier wall-clock attempt was taken under load averages of 60–117 on 10 cores and resolved nothing; that run is not the evidence here.
GC safety: the old copy was waste, not protection
The obvious objection is that copying into a
Vecput the values in memory the collector cannot move, and passing the caller's pointer through removes that. Traced all 9 call sites: every one handsdispatch_bound_functioneither a Rust-stack[f64; N]array or a fresh RustVeccopy — the direct callers pass&[arg0, …]literals at each arity, and the one non-literal path (dispatch_registered_call) has eight call sites that each buildlet args = [arg0, arg1, …]. The onlyfrom_raw_partsuses underclosure/are capture arrays and UTF-8 method names, never the args slice. No path hands it GC-managed memory.The narrower claim, stated rather than glossed: a stack array of NaN-boxed values is not a GC root either, and neither was the
Vec. If a collection moves an object insidejs_native_call_value, neither buffer is rewritten, and the conservative native-stack scan is diagnostic-only by default. This change neither introduces nor fixes that — rooting semantics are unchanged.Empty-args behaviour is preserved exactly: empty maps to a null pointer, as an empty
combineddid, rather than the non-null-but-dangling pointer an empty slice'sas_ptr()would give.What was investigated and found already optimal
The rest of the callback path is not a gap. A direct arrow inlined into
forEach, a loop-called closure local, and a loop-called callback parameter all reach fast, largely branch-predictable paths — viajs_closure_resolve_arrow_direct_call, entry-resolved once per function, plusdirect.rs's per-loop hoisting for array-iteration builtins and codegen'searly_branches.rsfast tiers. The bound-function entry is the one shape excluded from that hoisting, which is why it was the one carrying avoidable per-call work.Validation
thisbinding across arrow/function/bound,arguments, extra and missing arguments,length, a throwing callback's stack, recursion, and a callback closing over a loop variable.cargo test -p perry-runtime --lib: 4,115 passed, 0 failed.retired_set=#9), no faults, output byte-identical.run_lint_gates.sh84/85 — the one failure is the known-red public-baseline step.cargo fmt --all -- --checkclean.Summary by CodeRabbit
Performance
Bug Fixes
Tests
thisbinding, argument handling, throwing callbacks, and closure behavior.