Reuse exact positional arguments as function frame locals - #701
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Merging this PR will improve performance by 19.65%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | fib__monty |
156.9 ms | 131.1 ms | +19.65% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing KRRT7:exact-positional-call-fast-path (c772a86) with main (2680c32)
Footnotes
-
16 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
samuelcolvin
left a comment
There was a problem hiding this comment.
the principle looks good, is it fully tested?
I tested it on a few scenarios with no issues. |
…KRRT7/monty into exact-positional-call-fast-path
Switch Function::exact_positional_call and Signature::bind_mode to lazily-derived OnceCell fields instead of serde-skip-plus-rebuild, so a value loaded from an older REPL dump can never be stale relative to the derivation logic in the loading binary. Also dedupes the fast path's namespace-padding and callable-identity checks, and tightens the exact-call test coverage (arg-count-guard regression test, and exception-message assertions on the new arity-mismatch tests).
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
While doing the final review, I found an adjacent, pre-existing argument-binding bug. I verified it directly on Monty supports functions with more than 64 named parameters, but the binder tracked bound parameters in a Commit |
Summary
Optimize direct synchronous Python function calls when the supplied positional arguments exactly match a simple function signature.
Before this change, the VM:
ArgValues;For eligible calls, the stack already contains:
Removing the immediate
DefFunctionshifts its arguments directly into their final parameter slots. The VM then extends that region for remaining locals and pushes the frame without argument wrappers, binding, scratch storage, or another stack copy.The fast path is limited to synchronous direct functions with:
*args, keyword-only arguments, or**kwargs;All other calls retain the existing binding path.
Performance
Local Criterion benchmark for recursive
fib(25):Criterion reported a statistically significant improvement:
Validation
cargo test -p montyfunction__*Monty/CPython test cases: 74 passedfunction__arity_defaultswithmemory-model-checksmemory-model-checkscargo clippy -p monty --lib -- -D warningsgit diff --checkSummary by cubic
Speeds up direct calls by reusing stack arguments as frame locals when positional args exactly match the signature, for direct
DefFunctioncalls in sync funcs and async coroutines. Local fib(25): 18.74 ms → 14.70 ms (−21.4%).New Features
OnceCellforSignaturebind mode andFunctioncall plans; computed on first use, not serialized.Bug Fixes
Undefinedslots instead of a 64-bit bitmap.Written for commit c772a86. Summary will update on new commits.