perf(vm): lazily clone parent upvalues on Closure 🪺#156
Merged
Conversation
Walk the closure's CaptureSource list directly instead of pre-cloning the entire parent upvalues Vec. Closures only pay for the upvalues they actually reference; ones that only capture locals pay nothing extra. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
A code-review agent flagged
OpCode::Closureatndc_vm/src/vm.rs:514as wasteful: every closure creation pre-cloned the entire parent frame'supvaluesVec, even when the new closure didn't reference any of them. The Vec was only there to outlive theframeborrow so thatcapture_upvalue(which needs&mut self) could be called.Change
Walk
CaptureSourceentries directly and resolve each on demand:Local(slot)→self.capture_upvalue(...)as beforeUpvalue(slot)→ briefly re-borrow the frame and clone just that one parent upvalueRcThe two borrows never overlap in time, so the borrow checker is happy without the intermediate Vec. Closures that only capture locals now do zero parent-upvalue work.
Performance
closures.ndc(existing, parent has 0 upvalues)fibonacci.ndc(regression check, no closures)The existing
closures.ndcbench doesn't exercise the case this fix targets — the captured closures sit directly under the script root, so the parent'supvalueswas empty and the old code's pre-clone wasVec::new(). The targeted ad-hoc bench (16 parent upvalues × 200k creations) shows the real ~7% win.A
nested_closures.ndcbench could be added to track this path going forward — happy to do that as a follow-up if useful.🤖 Generated with Claude Code