Skip to content

Commit 5c896d4

Browse files
timfennisclaude
andauthored
perf(vm): lazily clone parent upvalues on Closure 🪺 (#156)
## Context A code-review agent flagged `OpCode::Closure` at `ndc_vm/src/vm.rs:514` as wasteful: every closure creation pre-cloned the entire parent frame's `upvalues` Vec, even when the new closure didn't reference any of them. The Vec was only there to outlive the `frame` borrow so that `capture_upvalue` (which needs `&mut self`) could be called. ## Change Walk `CaptureSource` entries directly and resolve each on demand: - `Local(slot)` → `self.capture_upvalue(...)` as before - `Upvalue(slot)` → briefly re-borrow the frame and clone just that one parent upvalue `Rc` The 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 | Bench | Baseline | New | Speedup | |---|---|---|---| | Targeted (parent has 16 upvalues, 200k closures) | 215.0 ± 12.3 ms | 200.4 ± 8.8 ms | 1.07× | | `closures.ndc` (existing, parent has 0 upvalues) | 72.0 ± 3.4 ms | 70.2 ± 3.2 ms | 1.03× (noise) | | `fibonacci.ndc` (regression check, no closures) | 70.0 ± 2.5 ms | 70.5 ± 3.0 ms | no change | The existing `closures.ndc` bench doesn't exercise the case this fix targets — the captured closures sit directly under the script root, so the parent's `upvalues` was empty and the old code's pre-clone was `Vec::new()`. The targeted ad-hoc bench (16 parent upvalues × 200k creations) shows the real ~7% win. A `nested_closures.ndc` bench could be added to track this path going forward — happy to do that as a follow-up if useful. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
1 parent b8d1c05 commit 5c896d4

1 file changed

Lines changed: 17 additions & 15 deletions

File tree

‎ndc_vm/src/vm.rs‎

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -498,28 +498,30 @@ impl Vm {
498498
// and before `capture_upvalue` needs `&mut self`.
499499
let idx = *idx;
500500
let values = Rc::clone(values);
501-
let frame = self.frames.last().expect("no frame");
502-
let Value::Object(obj) = frame.closure.prototype.body.constant(idx) else {
503-
panic!("Closure opcode: constant at index {idx} is not an Object");
504-
};
505-
let Object::Function(Function::Compiled(compiled)) = &**obj else {
506-
panic!(
507-
"Closure opcode: constant at index {idx} is not a Compiled function"
508-
);
501+
let (compiled, frame_pointer) = {
502+
let frame = self.frames.last().expect("no frame");
503+
let Value::Object(obj) = frame.closure.prototype.body.constant(idx) else {
504+
panic!("Closure opcode: constant at index {idx} is not an Object");
505+
};
506+
let Object::Function(Function::Compiled(compiled)) = &**obj else {
507+
panic!(
508+
"Closure opcode: constant at index {idx} is not a Compiled function"
509+
);
510+
};
511+
(Rc::clone(compiled), frame.frame_pointer)
509512
};
510-
let compiled = Rc::clone(compiled);
511-
let frame_pointer = frame.frame_pointer;
512-
// Pre-clone parent upvalue Rcs so we can drop the frame borrow
513-
// before calling capture_upvalue (which needs &mut self).
514-
let parent_upvalues: Vec<_> =
515-
frame.closure.upvalues.iter().map(Rc::clone).collect();
513+
// Resolve captures lazily: only clone the specific parent upvalue
514+
// Rcs that this closure references, and capture locals on demand.
516515
let upvalues = values
517516
.iter()
518517
.map(|c| match c {
519518
CaptureSource::Local(slot) => {
520519
self.capture_upvalue(frame_pointer + slot)
521520
}
522-
CaptureSource::Upvalue(slot) => Rc::clone(&parent_upvalues[*slot]),
521+
CaptureSource::Upvalue(slot) => {
522+
let frame = self.frames.last().expect("no frame");
523+
Rc::clone(&frame.closure.upvalues[*slot])
524+
}
523525
})
524526
.collect();
525527
let closure = Value::function(Function::Closure(ClosureFunction {

0 commit comments

Comments
 (0)