Skip to content

Commit 87e2e1b

Browse files
timfennisclaude
andauthored
perf(vm): defer GetIterator type stringification to error path 🦥 (#147)
## Summary `OpCode::GetIterator` was eagerly building the *"X is not iterable"* error message *before* checking whether the value was iterable. For nested containers (`List<Tuple<Int, List<Tuple<Int,Int>>>>`-shaped values), `val.static_type()` recursively walks every element of every container and allocates a fresh `StaticType` tree — which was then thrown away in the common case because the value *was* iterable. Moving the `static_type()` call into the actual error branch turns a per-`for`-loop O(n) deep walk into a check that only runs when iteration genuinely fails. ## How I found it A user-reported script (AoB 2025 day 9 part 2) ran *slower* on the VM than on the pre-VM tree-walk interpreter, while every other script in that project was 3-5× faster on the VM. `perf` showed ~50% of CPU time in `StaticType` allocation / cloning / dropping / equality. I instrumented `Object::static_type` for `List`/`Tuple`/`Map`/`Deque` with atomic counters and added a backtrace on calls against any `List` with >100 elements. A reduced repro of the script's hot loop logged **25.9 million** Tuple `static_type` calls — all traced back to `vm.rs:391`, the `format!("{}", val.static_type())` inside `OpCode::GetIterator`. `matches_param` fallback (the other obvious suspect) fired 0 times. The eager error-message construction was the entire problem. ## Benchmark suite Hyperfine on `benches/programs/*.ndc` plus two scripts that exercise the regression (`part2_aob` is the originally-reported case; `nested` is a reduced repro): | Benchmark | Before (ms) | After (ms) | Speedup | |---|---:|---:|---:| | `ackermann` | 132.1 ± 4.0 | 132.9 ± 4.2 | 0.99× | | `bigint` | 6.9 ± 1.9 | 7.5 ± 1.7 | 0.92× | | `closures` | 72.6 ± 3.4 | 73.8 ± 3.3 | 0.98× | | `enumerate_find` | 155.4 ± 3.5 | 156.7 ± 1.8 | 0.99× | | `enumerate_for_loop` | 108.0 ± 3.7 | 107.0 ± 4.5 | 1.01× | | `enumerate_take_small` | 30.2 ± 3.7 | 30.5 ± 3.2 | 0.99× | | `enumerate_to_list` | 65.9 ± 2.8 | 64.9 ± 2.8 | 1.01× | | `fibonacci` | 68.2 ± 2.8 | 68.7 ± 4.1 | 0.99× | | `fibonacci_typed` | 58.3 ± 3.9 | 59.5 ± 3.8 | 0.98× | | `hof_pipeline` | 34.2 ± 3.4 | 35.6 ± 2.7 | 0.96× | | `map_ops` | 25.9 ± 3.1 | 24.7 ± 2.8 | 1.05× | | `matrix_mul` | 57.0 ± 4.0 | 54.3 ± 3.7 | 1.05× | | `nested` (repro) | 1552.4 ± 9.6 | **122.5 ± 2.4** | **12.68×** | | `part1_aob` | 83.3 ± 3.4 | 75.5 ± 3.3 | 1.10× | | `part2_aob` | 5638.2 ± 81.8 | **2042.2 ± 11.9** | **2.76×** | | `perlin` | 64.9 ± 4.1 | 65.2 ± 4.6 | 1.00× | | `pi_approx` | 31.9 ± 3.9 | 31.5 ± 3.4 | 1.01× | | `print_heavy` | 7.2 ± 1.5 | 7.1 ± 1.1 | 1.02× | | `quicksort` | 72.7 ± 3.3 | 71.6 ± 3.9 | 1.02× | | `sieve` | 107.9 ± 2.9 | 107.7 ± 3.3 | 1.00× | | `string_concat` | 14.6 ± 1.2 | 14.8 ± 3.5 | 0.98× | | `vec_hot_loop` | 43.7 ± 3.8 | 41.6 ± 3.1 | 1.05× | Most of the curated suite shows no measurable change — expected, since the bug only fires when `for ... in <expr>` evaluates over deeply-nested containers. The two benches that hit the bug improve by ~3× and ~13×. `hyperfine --warmup 2 --min-runs 5 --max-runs 15` for the curated suite; longer-running scripts used fewer runs. Stats are mean ± stddev of wall time. ## Test plan - [x] `cargo test` — all 380+ tests across crates pass - [x] `cargo fmt --check` — clean - [x] `cargo clippy` — no new warnings (pre-existing ones remain) - [x] Output of part2 still matches `1529011204` - [x] Benchmarked against `master` binary with `hyperfine` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
1 parent ae1e616 commit 87e2e1b

1 file changed

Lines changed: 6 additions & 12 deletions

File tree

‎ndc_vm/src/vm.rs‎

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -387,22 +387,16 @@ impl Vm {
387387
continue;
388388
}
389389

390-
// Get type string before moving val
391-
let type_str = format!("{}", val.static_type());
392-
393-
// Try to create an iterator
394-
let iter_val = match val {
390+
let iter_val = match &val {
395391
Value::Object(rc) => match rc.as_ref() {
396-
Object::List(_) | Object::Tuple(_) | Object::Deque(_) => {
397-
Some(Value::iterator(Rc::new(RefCell::new(SeqIter::new(
398-
Rc::clone(&rc),
399-
)))))
400-
}
392+
Object::List(_) | Object::Tuple(_) | Object::Deque(_) => Some(
393+
Value::iterator(Rc::new(RefCell::new(SeqIter::new(Rc::clone(rc))))),
394+
),
401395
Object::String(s) => Some(Value::iterator(Rc::new(RefCell::new(
402396
StringIter::new(Rc::clone(s)),
403397
)))),
404398
Object::Map { .. } => Some(Value::iterator(Rc::new(RefCell::new(
405-
MapIter::new(Rc::clone(&rc)),
399+
MapIter::new(Rc::clone(rc)),
406400
)))),
407401
Object::MinHeap(h) => {
408402
Some(Value::iterator(Rc::new(RefCell::new(HeapIter::new_min(h)))))
@@ -419,7 +413,7 @@ impl Vm {
419413
Some(iter) => self.stack.push(iter),
420414
None => {
421415
return Err(VmError::new(
422-
format!("{} is not iterable", type_str),
416+
format!("{} is not iterable", val.static_type()),
423417
span,
424418
));
425419
}

0 commit comments

Comments
 (0)