From 5a218f1db0ab2ab2ae1589f2d25a013441009571 Mon Sep 17 00:00:00 2001 From: jasisz Date: Fri, 25 Sep 2026 20:10:11 +0200 Subject: [PATCH] Set a Vector field in place when the match on Vector.set hands the record back `match Vector.set(s.cells, i, x)` with `Some(updated) -> State.update(s, cells = updated, ...)` and `None -> s` copied the whole Vector on every set, on the VM and in generated Rust, even with `s` held by nothing else. The field-take work for Map did not reach it: the Vector is read in the match subject, not inside the update, and the None arm reads `s` whole, so every analysis saw a later read of the field. The None arm runs only when the index is out of range, and then the set changes nothing. `field_moves::vector_set_match` names this shape, and `movable_projections` places the target read in the Some arm, so it moves when nothing in the Some arm or after the match needs the field. - Generated Rust evaluates the index and the value, checks the index against the length, and moves the field into `set_unchecked` in the Some arm. - The VM compiles the subject as the new `VECTOR_SET_FIELD` opcode. It answers None for an index out of range without touching the record; otherwise it takes the Vector out of the record only when nothing off the stack holds the record and exactly the expected stack cells do, counted while the index and value are still on the stack, and writes in place only under the same fence as every other in-place vector write. Anything else copies, as `VECTOR_SET` does. - `perf-shared-update` reads the same facts, so it no longer reports this shape. The arena counts the vector elements a `Vector.set` copies, so the tests can assert that a set copied nothing. The new fixture's 2000 and 20,000 sets of a 100,000-cell Vector take 0.04 s instead of 0.47 s and 5.0 s on the VM, and 0.03 s instead of 0.20 s and 1.8 s in generated Rust, in release builds. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 1 + aver-memory/src/arena.rs | 17 + aver-memory/src/lib.rs | 4 + src/checker/shared_update.rs | 5 +- src/codegen/rust/from_mir.rs | 68 +++- src/ir/mir/field_moves.rs | 249 +++++++++++++- src/ir/mir/optimize/own_param.rs | 3 +- src/types/vector.rs | 1 + src/vm/compiler/classify.rs | 8 +- src/vm/compiler/mir.rs | 53 ++- src/vm/compiler/mod.rs | 5 + src/vm/execute/dispatch.rs | 139 ++++---- src/vm/execute/slots.rs | 87 +++++ src/vm/opcode.rs | 16 +- tests/fixtures/vector_field_set/main.av | 49 +++ .../fixtures/vector_field_set_shapes/main.av | 179 ++++++++++ tests/rust_work_spec.rs | 72 ++++ tests/shared_update_spec.rs | 51 +++ tests/vm_record_field_take.rs | 307 ++++++++++++++++++ 19 files changed, 1227 insertions(+), 87 deletions(-) create mode 100644 tests/fixtures/vector_field_set/main.av create mode 100644 tests/fixtures/vector_field_set_shapes/main.av diff --git a/CHANGELOG.md b/CHANGELOG.md index ac7813757..9f529eee7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,7 @@ The generated loop is now written from the program's source alone, and the manif ### Fixed +- **The VM and generated Rust set a Vector in a field of a record in place when the match on `Vector.set` hands the record back whole.** In `match Vector.set(s.cells, i, x)` with `Option.Some(updated) -> State.update(s, cells = updated, …)` and `Option.None -> s`, the `None` arm still needs `s.cells`, so every set copied the whole Vector, even with `s` held by nothing else. The `None` arm runs only when the index is out of range, and then the set changes nothing, so the index is now checked first and the Vector leaves the record only in the `Some` arm. Generated Rust moves the field there. The VM takes it out of the record only when the record is held by exactly the holders the compiler accounted for, and writes it in place only when nothing else holds it. `aver check` no longer reports this shape as `perf-shared-update`. In a release build, 2000 sets of a 100,000-cell Vector held by a record run in 0.04 s instead of 0.47 s on the VM and 0.03 s instead of 0.20 s in generated Rust, and 20,000 sets in 0.04 s instead of 5.0 s and 0.03 s instead of 1.8 s. A Vector two records down (`o.inner.cells`) is still copied on both, and wasm-gc still copies it. - **Generated Rust hands a record to functions that tail-call each other without copying it.** The public function of such a group borrowed a record or collection argument and cloned it into the loop that runs the group, so while the caller still held its copy, the first `Map.set` on a Map inside it copied the whole Map, once per call. It now takes the argument by value, and a caller at its last use moves it in; a caller that keeps it clones at the call, as the function did before. An argument every function of the group passes on unchanged is still borrowed. A loop handing a pool with a million-entry Map to such a pair 100 times ran in 0.64 s and now runs in 0.07 s. - **`perf-shared-update` reports the copies generated code makes, and only those.** The check now reads the same field moves the Rust backend makes, from the module lowered as it compiles. `done = toppedUp(s.pool, s.book, s.nextKey)?` followed by `S.update(s, pool = done.pool, book = done.book, nextKey = done.nextKey)` moves the Book out of `s` and no longer warns. `match (s.window.created, s.height)` with `(created, _) -> f(s, absorbed(created, k))` copies the Map, since `s` still holds it, and now warns: "`absorbed` updates `created`, read from `s.window.created`, a Map that is still held by `s`". A field of a record a loop hands on unchanged is reported too. - **Generated Rust moves the rest of a nested record into its update.** `Setting.update(setting, window = Window.update(setting.window, created = Map.set(setting.window.created, k, v)), height = setting.height + 1)` used to clone `setting.window` and `setting.window.created`, so every `Map.set` copied the Map. When nothing reads that part of `setting` again, the Map now moves into `Map.set` and the other fields of `setting.window` move into the new `Window`. diff --git a/aver-memory/src/arena.rs b/aver-memory/src/arena.rs index baff1002b..35747460a 100644 --- a/aver-memory/src/arena.rs +++ b/aver-memory/src/arena.rs @@ -20,6 +20,7 @@ impl Arena { list_elements_scanned: 0, list_elements_flattened: SharedCount::default(), map_entries_copied: 0, + vector_elements_copied: 0, map_entries_scanned: 0, vector_elements_scanned: 0, out_of_region_entries_read: 0, @@ -75,6 +76,7 @@ impl Arena { list_elements_scanned: 0, list_elements_flattened: SharedCount::default(), map_entries_copied: 0, + vector_elements_copied: 0, map_entries_scanned: 0, vector_elements_scanned: 0, out_of_region_entries_read: 0, @@ -742,6 +744,20 @@ impl Arena { self.map_entries_copied += entries as u64; } + /// Vector elements `Vector.set` duplicated to preserve a target it was not + /// allowed to write in place. Per-arena, as [`Arena::map_entries_copied`]. + #[inline] + pub fn vector_elements_copied(&self) -> u64 { + self.vector_elements_copied + } + + /// Record that `elements` vector elements were duplicated to preserve a + /// target the caller was not allowed to write in place. + #[inline] + pub fn note_vector_elements_copied(&mut self, elements: usize) { + self.vector_elements_copied += elements as u64; + } + /// Add `child`'s copy / scan totals to this arena's. /// /// A child arena counts from zero ([`Arena::clone_static`]), so work an @@ -756,6 +772,7 @@ impl Arena { self.list_elements_flattened .add(child.list_elements_flattened.get()); self.map_entries_copied += child.map_entries_copied; + self.vector_elements_copied += child.vector_elements_copied; self.map_entries_scanned += child.map_entries_scanned; self.vector_elements_scanned += child.vector_elements_scanned; self.out_of_region_entries_read += child.out_of_region_entries_read; diff --git a/aver-memory/src/lib.rs b/aver-memory/src/lib.rs index 4ec43c6af..a3d56970f 100644 --- a/aver-memory/src/lib.rs +++ b/aver-memory/src/lib.rs @@ -1491,6 +1491,10 @@ pub struct Arena { /// and [`Arena::absorb_copy_counters`] folds a child's total back into its /// parent when the branch rejoins. map_entries_copied: u64, + /// Total vector elements `Vector.set` duplicated because it was not + /// allowed to write its target in place. Per-arena like + /// `map_entries_copied`. + vector_elements_copied: u64, /// Total map entries the collector has *read* while deciding whether a live /// map needs rewriting. A map whose `all_immediate` flag is set is /// returned unread and adds nothing here; a map holding anything diff --git a/src/checker/shared_update.rs b/src/checker/shared_update.rs index f618ec755..ece77ae29 100644 --- a/src/checker/shared_update.rs +++ b/src/checker/shared_update.rs @@ -717,7 +717,10 @@ pub fn collect_shared_update_warnings( let mut bound = HashMap::new(); collect_bound(&f.body.node, &mut bound); let body = Body { - movable: crate::ir::mir::field_moves::movable_projections(&f.body.node), + movable: crate::ir::mir::field_moves::movable_projections( + &f.body.node, + &program.builtins, + ), carried: carried_params(f), bound, }; diff --git a/src/codegen/rust/from_mir.rs b/src/codegen/rust/from_mir.rs index dd2bef5ba..a49bc63e4 100644 --- a/src/codegen/rust/from_mir.rs +++ b/src/codegen/rust/from_mir.rs @@ -487,9 +487,13 @@ impl MirFnEmitPolicy { /// Record which field reads in `mir_fn`'s body may move their field /// out of the record local they read. The facts are addresses into /// this very body, so the policy must emit `mir_fn.body` itself. - pub(super) fn apply_field_moves(&mut self, mir_fn: &crate::ir::mir::MirFn) { + pub(super) fn apply_field_moves( + &mut self, + mir_fn: &crate::ir::mir::MirFn, + builtins: &[String], + ) { self.movable_projections = - crate::ir::mir::field_moves::movable_projections(&mir_fn.body.node); + crate::ir::mir::field_moves::movable_projections(&mir_fn.body.node, builtins); self.moved_roots = crate::ir::mir::field_moves::moved_roots(&mir_fn.body.node, &self.movable_projections); } @@ -735,7 +739,10 @@ pub(super) fn compute_owned_record_params( program.fn_by_id(*id).map(|mir_fn| { ( *id, - crate::ir::mir::field_moves::movable_projections(&mir_fn.body.node), + crate::ir::mir::field_moves::movable_projections( + &mir_fn.body.node, + &program.builtins, + ), ) }) }) @@ -3102,6 +3109,37 @@ fn emit_mir_match_with( .unwrap_or_default() }; + // `match Vector.set(s.cells, i, x)` whose target may move out of `s` + // (`field_moves::vector_set_match`): evaluate the index and the value, + // check the index, and read the target only in the `Some` arm. The + // `None` arm, which may read `s` whole, then still finds the field there, + // and the `Some` arm hands `set_unchecked` the only reference. + if let Some(set) = crate::ir::mir::field_moves::vector_set_match(m, emit_ctx.mir_builtins) + && super::ownership::projection_root_local(&set.target.node).is_some_and(|root| { + super::ownership::projection_moves(&set.target.node, root, emit_ctx) + }) + { + let index = emit_mir_expr(set.index, emit_ctx)?; + let value = mir_clone_arg( + emit_mir_expr(set.value, emit_ctx)?, + &set.value.node, + emit_ctx, + ); + let target = emit_mir_expr(set.target, emit_ctx)?; + let moved = mir_clone_arg(target.clone(), &set.target.node, emit_ctx); + let updated = match set.updated { + Some((_, name)) if name != "_" => aver_name_to_rust(name), + _ => "_".to_string(), + }; + let index_temp = generated_ident("idx"); + let value_temp = generated_ident("value"); + let some = &arm_bodies[set.some_arm]; + let none = &arm_bodies[set.none_arm]; + return Some(format!( + "{{ let {index_temp} = ({index}).to_usize(); let {value_temp} = {value}; match {index_temp}.filter(|{index_temp}| *{index_temp} < {target}.len()) {{ Some({index_temp}) => {{ let {updated} = {moved}.set_unchecked({index_temp}, {value_temp}); {some} }} None => {{ {none} }} }} }}" + )); + } + // ── 1. Single-arm irrefutable → `let` destructuring. ── // Mirror of `emit_match`'s first branch. if arms.len() == 1 && resolved_pattern_is_irrefutable(&arms[0].pattern) { @@ -3923,7 +3961,13 @@ pub(super) fn emit_mir_fn_body_routed( // `bare_fn_facts`), so body and signature agree on which params / // return are bare. policy.apply_bare_i64(mir_fn.fn_id, ctx); - policy.apply_field_moves(mir_fn); + policy.apply_field_moves( + mir_fn, + ctx.mir_program + .as_ref() + .map(|p| p.builtins.as_slice()) + .unwrap_or(&[]), + ); let emit_ctx = MirEmitCtx::for_fn(ctx, &policy); let body = emit_mir_fn_body(&mir_fn.body, &emit_ctx)?; let Some(prologue) = post_checkpoint_prologue else { @@ -4032,7 +4076,13 @@ pub(super) fn emit_mir_tco_fn( for n in &rc_names { policy.owned_params.remove(n); } - policy.apply_field_moves(mir_fn); + policy.apply_field_moves( + mir_fn, + ctx.mir_program + .as_ref() + .map(|p| p.builtins.as_slice()) + .unwrap_or(&[]), + ); let emit_ctx = MirEmitCtx::for_fn(ctx, &policy); // Render the body in tail position FIRST — bail before emitting any @@ -4452,7 +4502,13 @@ pub(super) fn emit_mir_mutual_tco_block( // Each arm binds its params by value, so a field read may move out // of one exactly as in any other body; the arm's updates then see // which records gave a field up. - policy.apply_field_moves(mir_fn); + policy.apply_field_moves( + mir_fn, + ctx.mir_program + .as_ref() + .map(|p| p.builtins.as_slice()) + .unwrap_or(&[]), + ); let mut arm_ctx = MirEmitCtx::for_fn(ctx, &policy); // Mutual invariants are `rc_wrapped` for owning reads, but unlike // self-TCO's `Arc` representation they are extra `&T` trampoline diff --git a/src/ir/mir/field_moves.rs b/src/ir/mir/field_moves.rs index eca7ccb1d..aa4f531ac 100644 --- a/src/ir/mir/field_moves.rs +++ b/src/ir/mir/field_moves.rs @@ -24,11 +24,120 @@ //! and one of the reads that can run with `q` is the local's last use, so //! nothing reads `s` after them. A read inside an independent product is //! never movable: its branches run on their own threads. +//! +//! ## The target of a matched `Vector.set` +//! +//! In +//! +//! ```text +//! match Vector.set(s.cells, i, x) +//! Option.Some(updated) -> State.update(s, cells = updated) +//! Option.None -> s +//! ``` +//! +//! the `None` arm reads the whole of `s`, so a read of `s.cells` in the +//! subject could never move: the `None` arm still needs the field. But the +//! `None` arm is taken exactly when the index is out of range, and then the +//! set changes nothing. A backend that evaluates the index and the value, +//! checks the index against the length, and only then reads the target, in +//! the `Some` arm, needs the target in the `Some` arm alone +//! ([`vector_set_match`]). The analysis places that read there, so it +//! moves when nothing else in the `Some` arm or after the match needs the +//! field. use std::collections::{HashMap, HashSet}; -use super::expr::{MirExpr, MirRecordUpdate, walk_children}; +use super::expr::{ + MirCallee, MirCtor, MirExpr, MirMatch, MirPattern, MirRecordUpdate, walk_children, +}; use super::program::LocalId; +use crate::ast::Spanned; +use crate::ir::hir::BuiltinCtor; + +/// A `match` on `Vector.set(target, index, value)` with one arm for +/// `Option.Some` and one for `Option.None` (or a wildcard after the `Some` +/// arm), whose target is a field path of a local (`s.cells`, `s.a.cells`). +/// +/// The `None` arm runs exactly when the index is out of range, and then the +/// set has changed nothing: whatever the arm reads of the target's record +/// is what was there before. A backend that evaluates `index` and `value`, +/// checks the index, and reads `target` only once the `Some` arm is chosen +/// never needs the target in the `None` arm, so the target read belongs to +/// the `Some` arm. Generated Rust emits the match that way when the read may +/// move ([`movable_projections`]); the VM takes the field under the same +/// shape. +pub struct VectorSetMatch<'a> { + pub target: &'a Spanned, + pub index: &'a Spanned, + pub value: &'a Spanned, + /// Arm positions in `arms`. + pub some_arm: usize, + pub none_arm: usize, + /// The local the `Some` arm binds the updated Vector to, if any. + pub updated: Option<(LocalId, &'a str)>, + /// Address of the subject call. + pub call: usize, +} + +/// See [`VectorSetMatch`]. `builtins` names the program's builtin ids. +pub fn vector_set_match<'a>(m: &'a MirMatch, builtins: &[String]) -> Option> { + let MirExpr::Call(call) = &m.subject.node else { + return None; + }; + let MirCallee::Builtin(id) = call.node.callee else { + return None; + }; + if builtins.get(id.0 as usize).map(String::as_str) != Some("Vector.set") + || call.node.args.len() != 3 + { + return None; + } + let target = &call.node.args[0]; + if !projection_root(&target.node).is_some_and(|(_, _, path)| !path.is_empty()) { + return None; + } + if m.arms.len() != 2 { + return None; + } + let some = |pattern: &'a MirPattern| match pattern { + MirPattern::Ctor { + ctor: MirCtor::Builtin(BuiltinCtor::OptionSome), + bindings, + binding_names, + } if bindings.len() == 1 => Some(Some(( + bindings[0], + binding_names.first().map(String::as_str).unwrap_or("_"), + ))), + _ => None, + }; + let none = |pattern: &MirPattern| { + matches!( + pattern, + MirPattern::Ctor { + ctor: MirCtor::Builtin(BuiltinCtor::OptionNone), + .. + } + ) + }; + let (some_arm, none_arm, updated) = match (some(&m.arms[0].pattern), some(&m.arms[1].pattern)) { + (Some(updated), None) + if none(&m.arms[1].pattern) || matches!(m.arms[1].pattern, MirPattern::Wildcard) => + { + (0, 1, updated) + } + (None, Some(updated)) if none(&m.arms[0].pattern) => (1, 0, updated), + _ => return None, + }; + Some(VectorSetMatch { + target, + index: &call.node.args[1], + value: &call.node.args[2], + some_arm, + none_arm, + updated, + call: &call.node as *const _ as usize, + }) +} /// The part of a local one read observes. #[derive(Debug, Clone)] @@ -82,10 +191,12 @@ enum Branching { /// is in the set too when it may move what the update keeps: the rest of /// `s.window` then moves into the new record instead of being cloned, and /// the fields it replaces may move out before it. -pub fn movable_projections(body: &MirExpr) -> HashSet { +/// +/// `builtins` names the program's builtin ids, for [`vector_set_match`]. +pub fn movable_projections(body: &MirExpr, builtins: &[String]) -> HashSet { let mut reads: HashMap> = HashMap::new(); let mut trail = Vec::new(); - collect(body, &mut trail, false, &mut reads); + collect(body, &mut trail, false, builtins, &mut reads); let mut out = HashSet::new(); // A chain base is final once nothing outside its own update reads what // it keeps; a base further in may depend on one further out, so this @@ -208,6 +319,7 @@ fn collect( expr: &MirExpr, trail: &mut Vec<(usize, usize, Branching)>, in_product: bool, + builtins: &[String], reads: &mut HashMap>, ) { let addr = expr as *const MirExpr as usize; @@ -264,7 +376,31 @@ fn collect( }); for (index, field) in update.node.updates.iter().enumerate() { trail.push((addr, index + 1, Branching::Other)); - collect(&field.value.node, trail, in_product, reads); + collect(&field.value.node, trail, in_product, builtins, reads); + trail.pop(); + } + return; + } + } + MirExpr::Match(m) => { + if let Some(set) = vector_set_match(&m.node, builtins) { + // The index and the value run in the subject; the target is + // read in the `Some` arm (see `VectorSetMatch`). + trail.push((addr, 0, Branching::Alternatives)); + for (index, arg) in [(1, set.index), (2, set.value)] { + trail.push((set.call, index, Branching::Call)); + collect(&arg.node, trail, in_product, builtins, reads); + trail.pop(); + } + trail.pop(); + trail.push((addr, set.some_arm + 1, Branching::Alternatives)); + trail.push((set.call, 0, Branching::Call)); + collect(&set.target.node, trail, in_product, builtins, reads); + trail.pop(); + trail.pop(); + for (index, arm) in m.node.arms.iter().enumerate() { + trail.push((addr, index + 1, Branching::Alternatives)); + collect(&arm.body.node, trail, in_product, builtins, reads); trail.pop(); } return; @@ -282,7 +418,7 @@ fn collect( let mut index = 0; walk_children(expr, &mut |child| { trail.push((addr, index, branching)); - collect(child, trail, in_product, reads); + collect(child, trail, in_product, builtins, reads); trail.pop(); index += 1; }); @@ -512,7 +648,7 @@ mod tests { project(project(local(0, false), "window"), "created"), project(project(local(0, true), "window"), "spent"), ]); - let movable = movable_projections(&body.node); + let movable = movable_projections(&body.node, &[]); assert!(movable.contains(&addr(&args(&body)[0]))); assert!(movable.contains(&addr(&args(&body)[1]))); } @@ -523,7 +659,7 @@ mod tests { project(project(local(0, false), "window"), "created"), project(local(0, true), "window"), ]); - let movable = movable_projections(&body.node); + let movable = movable_projections(&body.node, &[]); assert!(movable.is_empty()); } @@ -535,7 +671,7 @@ mod tests { value: Box::new(call(vec![project(local(0, false), "window")])), body: Box::new(call(vec![project(local(0, true), "window")])), }))); - let movable = movable_projections(&body.node); + let movable = movable_projections(&body.node, &[]); let MirExpr::Let(chain) = &body.node else { unreachable!() }; @@ -572,7 +708,7 @@ mod tests { }, ], }))); - let movable = movable_projections(&body.node); + let movable = movable_projections(&body.node, &[]); let MirExpr::RecordUpdate(outer) = &body.node else { unreachable!() }; @@ -596,11 +732,11 @@ mod tests { }], }))); let body = call(vec![update, project(local(0, true), "window")]); - assert!(movable_projections(&body.node).len() <= 1); + assert!(movable_projections(&body.node, &[]).len() <= 1); let MirExpr::RecordUpdate(update) = &args(&body)[0].node else { unreachable!() }; - let movable = movable_projections(&body.node); + let movable = movable_projections(&body.node, &[]); assert!(!movable.contains(&addr(&update.node.base))); assert!(!movable.contains(&addr(&args(&update.node.updates[0].value)[0]))); } @@ -634,7 +770,7 @@ mod tests { body: update, }], }))); - let movable = movable_projections(&body.node); + let movable = movable_projections(&body.node, &[]); assert!(movable.contains(&arm_read)); assert!(!movable.contains(&subject_read)); } @@ -653,7 +789,7 @@ mod tests { body: arm, }], }))); - assert!(!movable_projections(&body.node).contains(&arm_read)); + assert!(!movable_projections(&body.node, &[]).contains(&arm_read)); } #[test] @@ -667,6 +803,91 @@ mod tests { value: call(vec![project(project(local(0, true), "window"), "created")]), }], }))); - assert_eq!(movable_projections(&body.node).len(), 1); + assert_eq!(movable_projections(&body.node, &[]).len(), 1); + } + + /// `match Vector.set(s.cells, 0, 1)` with `some_arm` as the `Some` arm and + /// `none_arm` as the `None` arm; returns the body and the target's address. + fn vector_set_match_body( + some_arm: Spanned, + none_arm: Spanned, + ) -> (Spanned, usize) { + use crate::ir::BuiltinId; + use crate::ir::mir::expr::{MirMatch, MirMatchArm}; + let subject = sp(MirExpr::Call(Spanned::bare(MirCall { + callee: MirCallee::Builtin(BuiltinId(0)), + args: vec![ + project(local(0, false), "cells"), + sp(MirExpr::Literal(Spanned::bare(crate::ast::Literal::Int(0)))), + sp(MirExpr::Literal(Spanned::bare(crate::ast::Literal::Int(1)))), + ], + }))); + let target = addr(&args(&subject)[0]); + let body = sp(MirExpr::Match(Spanned::bare(MirMatch { + subject: Box::new(subject), + arms: vec![ + MirMatchArm { + pattern: MirPattern::Ctor { + ctor: MirCtor::Builtin(BuiltinCtor::OptionSome), + bindings: vec![LocalId(1)], + binding_names: vec!["updated".to_string()], + }, + body: some_arm, + }, + MirMatchArm { + pattern: MirPattern::Ctor { + ctor: MirCtor::Builtin(BuiltinCtor::OptionNone), + bindings: vec![], + binding_names: vec![], + }, + body: none_arm, + }, + ], + }))); + (body, target) + } + + fn update_cells(count: Spanned) -> Spanned { + sp(MirExpr::RecordUpdate(Spanned::bare(MirRecordUpdate { + type_id: Some(TypeId(0)), + type_name: "State".to_string(), + base: Box::new(local(0, false)), + updates: vec![ + MirRecordField { + name: "cells".to_string(), + value: local(1, true), + }, + MirRecordField { + name: "count".to_string(), + value: count, + }, + ], + }))) + } + + #[test] + fn a_matched_vector_set_target_moves_past_a_none_arm_that_reads_the_record() { + // match Vector.set(s.cells, 0, 1) + // Option.Some(updated) -> State.update(s, cells = updated, count = s.count) + // Option.None -> s + let (body, target) = vector_set_match_body( + update_cells(project(local(0, true), "count")), + local(0, true), + ); + let builtins = ["Vector.set".to_string()]; + assert!(movable_projections(&body.node, &builtins).contains(&target)); + // Without knowing the call is `Vector.set`, the None arm blocks it. + assert!(!movable_projections(&body.node, &[]).contains(&target)); + } + + #[test] + fn a_matched_vector_set_target_read_again_in_the_some_arm_does_not_move() { + // Option.Some(updated) -> f(s.cells, updated) + let (body, target) = vector_set_match_body( + call(vec![project(local(0, true), "cells"), local(1, true)]), + local(0, true), + ); + let builtins = ["Vector.set".to_string()]; + assert!(!movable_projections(&body.node, &builtins).contains(&target)); } } diff --git a/src/ir/mir/optimize/own_param.rs b/src/ir/mir/optimize/own_param.rs index 60323ee2b..211512da8 100644 --- a/src/ir/mir/optimize/own_param.rs +++ b/src/ir/mir/optimize/own_param.rs @@ -389,7 +389,8 @@ fn own_param_refine_for_model(mut program: MirProgram, model: OwnershipModel) -> // field read counts only for a param updated in place. if model.owned_carriers_are_cow_protected() { for (id, f) in program.iter() { - let movable = crate::ir::mir::field_moves::movable_projections(&f.body.node); + let movable = + crate::ir::mir::field_moves::movable_projections(&f.body.node, &program.builtins); if !movable.is_empty() { let mut slots = HashSet::new(); collect_movable_let_slots(&f.body.node, &movable, &mut slots); diff --git a/src/types/vector.rs b/src/types/vector.rs index 92cebd47f..f207046f6 100644 --- a/src/types/vector.rs +++ b/src/types/vector.rs @@ -166,6 +166,7 @@ fn vec_set_nv(args: &[NanValue], arena: &mut Arena) -> Result= items.len() { return Ok(NanValue::NONE); } + arena.note_vector_elements_copied(items.len()); items[uidx] = args[2]; let new_vec_idx = arena.push_vector(items); Ok(NanValue::new_some_value( diff --git a/src/vm/compiler/classify.rs b/src/vm/compiler/classify.rs index f745edc4b..556f21f9a 100644 --- a/src/vm/compiler/classify.rs +++ b/src/vm/compiler/classify.rs @@ -270,8 +270,8 @@ fn classify_thin_chunk(chunk: &FnChunk) -> Result { STORE_GLOBAL | TAIL_CALL_SELF | TAIL_CALL_KNOWN | CONCAT | LIST_NIL | LIST_CONS | LIST_NEW | RECORD_NEW | RECORD_NEW_INDEXED | WRAP | TUPLE_NEW | CALL_PAR | RECORD_UPDATE | LIST_LEN | LIST_PREPEND | VECTOR_GET | VECTOR_GET_OR | VECTOR_SET - | VECTOR_SET_OR_KEEP | STR_INDEX_BUILD | STR_INDEX_CHAR_AT | STR_INDEX_CODE_AT - | STR_INDEX_SLICE | TAIL_CALL_SELF_THIN => { + | VECTOR_SET_OR_KEEP | VECTOR_SET_FIELD | STR_INDEX_BUILD | STR_INDEX_CHAR_AT + | STR_INDEX_CODE_AT | STR_INDEX_SLICE | TAIL_CALL_SELF_THIN => { return Ok(false); } @@ -306,8 +306,8 @@ fn classify_thin_ignoring_self_tco(chunk: &FnChunk) -> Result { + | VECTOR_SET_OR_KEEP | VECTOR_SET_FIELD | STR_INDEX_BUILD | STR_INDEX_CHAR_AT + | STR_INDEX_CODE_AT | STR_INDEX_SLICE => { return Ok(false); } diff --git a/src/vm/compiler/mir.rs b/src/vm/compiler/mir.rs index ae0ae61e3..24a1d4a23 100644 --- a/src/vm/compiler/mir.rs +++ b/src/vm/compiler/mir.rs @@ -846,7 +846,9 @@ pub(super) fn compile_mir_expr( if try_emit_match_dispatch_const(fc, &m.subject, &m.arms)?.is_some() { return Ok(()); } - compile_mir_expr(fc, &m.subject)?; + if !compile_vector_set_field(fc, m)? { + compile_mir_expr(fc, &m.subject)?; + } // A subject nothing reads after the match (a temporary, or a // local at its last use) is consumed by the arm that matches it: // a tuple or box the arm takes apart, if nothing else holds it, @@ -1092,6 +1094,12 @@ pub(super) fn compile_mir_fn_body( fc: &mut FnCompiler<'_>, mir_fn: &MirFn, ) -> Result<(), MirVmUnsupported> { + fc.movable_projections = fc + .mir_program + .map(|program| { + crate::ir::mir::field_moves::movable_projections(&mir_fn.body.node, &program.builtins) + }) + .unwrap_or_default(); compile_mir_expr(fc, &mir_fn.body)?; fc.emit_op(RETURN); Ok(()) @@ -1737,6 +1745,49 @@ where Ok(vec![outer_fail]) } +/// Compile the subject of `match Vector.set(local.field, index, value)` as one +/// `VECTOR_SET_FIELD` when nothing after the write reads the field again +/// (`field_moves::vector_set_match`, `field_moves::movable_projections`): the +/// record, the index and the value go on the stack, and the runtime takes the +/// Vector out of the record once the index is in range and nothing it cannot +/// account for holds the record or the Vector. Returns `false`, having emitted +/// nothing, for any other subject; a path more than one field below the local +/// is compiled as an ordinary `Vector.set`. +fn compile_vector_set_field( + fc: &mut FnCompiler<'_>, + m: &crate::ir::mir::MirMatch, +) -> Result { + let Some(program) = fc.mir_program else { + return Ok(false); + }; + let Some(set) = crate::ir::mir::field_moves::vector_set_match(m, &program.builtins) else { + return Ok(false); + }; + if !fc + .movable_projections + .contains(&(&set.target.node as *const MirExpr as usize)) + { + return Ok(false); + } + let MirExpr::Project(project) = &set.target.node else { + return Ok(false); + }; + let MirExpr::Local(local) = &project.node.base.node else { + return Ok(false); + }; + // The local's own cell still holds the record unless this read moves it + // onto the stack. + let holders = u8::from(!local.node.last_use); + compile_mir_expr(fc, &project.node.base)?; + compile_mir_expr(fc, set.index)?; + compile_mir_expr(fc, set.value)?; + let field_symbol_id = fc.symbols.intern_name(&project.node.field); + fc.emit_op(VECTOR_SET_FIELD); + fc.emit_u32(field_symbol_id); + fc.emit_u8(holders); + Ok(true) +} + /// Compile `local.f1.….fn`, `n >= 2`, as one `RECORD_TAKE_PATH` when a record /// literal or update being compiled plans to take it, or when the read is the /// local's last use. Returns `false`, having emitted nothing, for any other diff --git a/src/vm/compiler/mod.rs b/src/vm/compiler/mod.rs index ef6584ea5..b45467487 100644 --- a/src/vm/compiler/mod.rs +++ b/src/vm/compiler/mod.rs @@ -1312,6 +1312,10 @@ pub(super) struct FnCompiler<'a> { /// Fields the record literals and updates being compiled may take out of /// a local record, innermost last. See `field_take`. field_takes: Vec, + /// Field reads of the body being compiled that nothing after them reads + /// again (`field_moves::movable_projections`). Consulted only for the + /// target of a matched `Vector.set`; see `field_take`. + movable_projections: std::collections::HashSet, } impl<'a> FnCompiler<'a> { @@ -1354,6 +1358,7 @@ impl<'a> FnCompiler<'a> { last_noted_line: 0, aliased_slots: std::sync::Arc::new(Vec::new()), field_takes: Vec::new(), + movable_projections: std::collections::HashSet::new(), } } diff --git a/src/vm/execute/dispatch.rs b/src/vm/execute/dispatch.rs index a4f1327ed..1787f7253 100644 --- a/src/vm/execute/dispatch.rs +++ b/src/vm/execute/dispatch.rs @@ -2043,6 +2043,7 @@ impl VM { if let Some(i) = idx { let mut items = self.arena.clone_vector_value(vec); if i < items.len() { + self.arena.note_vector_elements_copied(items.len()); items[i] = value; let new_idx = self.arena.push_vector(items); let new_vec = NanValue::new_vector(new_idx); @@ -2084,71 +2085,14 @@ impl VM { let vec_owned = static_grant && self.runtime_confirms_fused_vector_grant(vec, bp + target_slot); if vec_owned && !vec.is_empty_vector_immediate() { - // Owned path: modify vector in-place at the same arena slot. - // No new allocation, no promotion needed. - // - // This is the VM's only true in-place arena write, and - // therefore the only way an arena slot the return - // boundary keeps can come to hold an index into a - // region the boundary drops: the vector may live below - // this frame's marks while `value` was allocated above - // them. Record that so the boundary does not take the - // return path that truncates young with no rewrite. - // - // The region test is the boundary's own predicate - // (`yard_mark`, matching `result_uses_frame_local_heap`) - // rather than the `yard_base` one `STORE_GLOBAL` uses. - // That is the conservative half of the pair — `yard_base - // <= yard_mark`, so `yard_mark` counts fewer slots as - // this frame's own and flags strictly more writes — and - // it is the line the guarded boundary actually draws. - // - // Two things the flag deliberately does NOT ask about. - // It is armed on the TARGET alone, never on where the - // value came from: the frame that wrote and the frame - // whose region holds the value need not be the same one - // (`an_inherited_in_place_write_survives_the_callers_boundary` - // is exactly that shape), so a value-side test would - // stay silent in the frame that must hear about it. And - // it is armed only for a write that actually happened — - // an index past the end stores nothing, and an - // immediate leaves no arena reference behind, so - // neither can leave a slot pointing anywhere. - let target_outside_frame = value.heap_index().is_some() - && self.frames.last().is_some_and(|frame| { - !vec.heap_index().is_some_and(|index| { - self.arena.is_frame_local_index( - index, - frame.arena_mark, - frame.yard_mark, - frame.handoff_mark, - ) - }) - }); - // The one place in the VM where a value enters an arena - // entry without going through `Arena::push`, so it is - // the one place the choke point does not cover: after - // this store the vector holds `value`, and if that is a - // map, the vector is a holder of its slot. - self.arena.note_held_elsewhere(value); - // Through the arena's own write rather than the raw - // element slice: it knows what `value` is, so a loop - // writing integers keeps the collector's escape. - let stored = self - .arena - .vector_store_in_place(vec.arena_index(), i, value); - if stored - && target_outside_frame - && let Some(frame) = self.frames.last_mut() - { - frame.inplace_write_escaped = true; - } + self.store_vector_element_in_place(vec, i, value); // Return the same NanValue — same slot, same space. self.stack.push(vec); } else { let items = self.arena.vector_ref_value(vec); if i < items.len() { let mut updated = items.to_vec(); + self.arena.note_vector_elements_copied(updated.len()); updated[i] = value; let new_idx = self.arena.push_vector(updated); self.stack.push(NanValue::new_vector(new_idx)); @@ -2158,6 +2102,12 @@ impl VM { } } + VECTOR_SET_FIELD => { + let field_symbol_id = read_u32!(code, ip); + let holders = read_u8!(code, ip); + self.vector_set_field(field_symbol_id, holders)?; + } + BUFFER_NEW => { // cap_hint is currently advisory — a `String::with_capacity` hint. // Reuse a freed slot if available to keep the pool from @@ -2912,6 +2862,77 @@ const BYTE_BUILDER_POOL_SLOTS: usize = LIST_BUILDER_POOL_SLOTS; const LIST_BUILDER_CAPACITY_HINT_CAP: usize = 1 << 16; impl VM { + /// Write `value` at `i` of the vector `vec` in its own arena slot, for a + /// write the caller has established nothing else can observe. + /// + /// Shared by `VECTOR_SET_OR_KEEP`'s owned branch and `VECTOR_SET_FIELD`. + pub(super) fn store_vector_element_in_place( + &mut self, + vec: NanValue, + i: usize, + value: NanValue, + ) { + // Owned path: modify vector in-place at the same arena slot. + // No new allocation, no promotion needed. + // + // This is the VM's only true in-place arena write, and + // therefore the only way an arena slot the return + // boundary keeps can come to hold an index into a + // region the boundary drops: the vector may live below + // this frame's marks while `value` was allocated above + // them. Record that so the boundary does not take the + // return path that truncates young with no rewrite. + // + // The region test is the boundary's own predicate + // (`yard_mark`, matching `result_uses_frame_local_heap`) + // rather than the `yard_base` one `STORE_GLOBAL` uses. + // That is the conservative half of the pair — `yard_base + // <= yard_mark`, so `yard_mark` counts fewer slots as + // this frame's own and flags strictly more writes — and + // it is the line the guarded boundary actually draws. + // + // Two things the flag deliberately does NOT ask about. + // It is armed on the TARGET alone, never on where the + // value came from: the frame that wrote and the frame + // whose region holds the value need not be the same one + // (`an_inherited_in_place_write_survives_the_callers_boundary` + // is exactly that shape), so a value-side test would + // stay silent in the frame that must hear about it. And + // it is armed only for a write that actually happened — + // an index past the end stores nothing, and an + // immediate leaves no arena reference behind, so + // neither can leave a slot pointing anywhere. + let target_outside_frame = value.heap_index().is_some() + && self.frames.last().is_some_and(|frame| { + !vec.heap_index().is_some_and(|index| { + self.arena.is_frame_local_index( + index, + frame.arena_mark, + frame.yard_mark, + frame.handoff_mark, + ) + }) + }); + // The one place in the VM where a value enters an arena + // entry without going through `Arena::push`, so it is + // the one place the choke point does not cover: after + // this store the vector holds `value`, and if that is a + // map, the vector is a holder of its slot. + self.arena.note_held_elsewhere(value); + // Through the arena's own write rather than the raw + // element slice: it knows what `value` is, so a loop + // writing integers keeps the collector's escape. + let stored = self + .arena + .vector_store_in_place(vec.arena_index(), i, value); + if stored + && target_outside_frame + && let Some(frame) = self.frames.last_mut() + { + frame.inplace_write_escaped = true; + } + } + /// Append `value` to `builder`, returning the builder that holds it. /// /// Pooled while the elements stay immediate. The first element with diff --git a/src/vm/execute/slots.rs b/src/vm/execute/slots.rs index 6e4ecc693..b5ec40193 100644 --- a/src/vm/execute/slots.rs +++ b/src/vm/execute/slots.rs @@ -934,6 +934,93 @@ impl VM { fn nothing_else_holds_slot(&self, _index: u32, _exempt: Option) -> Option { None } + + /// `VECTOR_SET_FIELD`: `Vector.set(record.field, index, value)` with + /// `[record, index, value]` on top of the stack. + /// + /// The compiler emits it only where nothing after the write reads the + /// field again: the match's `None` arm may read the record whole, which is + /// why nothing is taken before the index is known to be in range. What the + /// compiler cannot see is who else holds the record or the Vector. So the + /// Vector leaves the record only when nothing off the stack holds the + /// record and exactly `holders` cells on it do besides the operand (the + /// local's own cell while a later read still needs it), and it is then + /// written in place only under the fence every in-place vector write + /// passes ([`VM::runtime_confirms_vector_grant`]). Otherwise the Vector is + /// copied, exactly as `VECTOR_SET` does. + pub(super) fn vector_set_field( + &mut self, + field_symbol_id: u32, + holders: u8, + ) -> Result<(), VmError> { + let len = self.stack.len(); + if len < 3 { + return Err(VmError::StackUnderflow); + } + let (record, index, value) = ( + self.stack[len - 3], + self.stack[len - 2], + self.stack[len - 1], + ); + if !record.is_record() { + return Err(VmError::runtime( + "Vector.set on a field of a value that is not a record".to_string(), + )); + } + let (type_id, fields) = self.arena.get_record(record.arena_index()); + let Some(&field_idx) = self + .code + .record_field_slots + .get(&(type_id, field_symbol_id)) + else { + let field_name = self + .code + .symbols + .get(field_symbol_id) + .map(|info| info.name.as_str()) + .unwrap_or(""); + return Err(VmError::runtime(format!( + "record has no field '{}'", + field_name + ))); + }; + let field_idx = field_idx as usize; + let vec = fields[field_idx]; + let len_of_vec = self.arena.vector_slot(vec).map_or(0, |slot| slot.len); + let in_range = self.int_to_index(index).filter(|i| *i < len_of_vec); + let Some(i) = in_range else { + self.stack.truncate(len - 3); + self.stack.push(NanValue::NONE); + return Ok(()); + }; + // Counted with the index and the value still on the stack: a value + // that holds the record is one more cell, or makes the record held + // elsewhere. + let takes = !self.arena.record_is_held_elsewhere(record) + && record.heap_index().is_some_and(|record_index| { + self.stack_holders_excluding(record_index, None) == u32::from(holders) + 1 + }); + if takes { + self.arena.take_record_field(record, field_idx); + } + self.stack.truncate(len - 3); + // Out of the record, the Vector is written in place under the same + // fence as every other in-place vector write: nothing off the stack + // and no stack cell may hold it. + if takes && self.confirm_vector_grant(vec, None) { + self.store_vector_element_in_place(vec, i, value); + let some = NanValue::new_some_value(vec, &mut self.arena); + self.stack.push(some); + return Ok(()); + } + let mut items = self.arena.clone_vector_value(vec); + self.arena.note_vector_elements_copied(items.len()); + items[i] = value; + let copied = NanValue::new_vector(self.arena.push_vector(items)); + let some = NanValue::new_some_value(copied, &mut self.arena); + self.stack.push(some); + Ok(()) + } } /// Grants this thread took whose arena half the mirror could not afford to diff --git a/src/vm/opcode.rs b/src/vm/opcode.rs index b0bb7ddfe..f1e6fb807 100644 --- a/src/vm/opcode.rs +++ b/src/vm/opcode.rs @@ -402,6 +402,19 @@ pub const VECTOR_SET: u8 = 0x84; /// Stack: [vector, index, value] → [vector] pub const VECTOR_SET_OR_KEEP: u8 = 0x85; +/// `Vector.set(record.field, index, value)` as the subject of a match whose +/// `None` arm may read the record whole (`field_moves::vector_set_match`). +/// Stack: [record, index, value] → [option_vector]. +/// +/// An index out of range answers `None` and leaves the record as it was. An +/// index in range takes the Vector out of the record and writes it in place +/// when nothing off the stack holds the record, exactly `holders` cells on +/// the stack hold it besides the operand, and nothing but the record holds +/// the Vector; otherwise the Vector is copied, as `VECTOR_SET` does. The +/// compiler emits it only where no read after the write can see the field +/// (`vm::compiler::field_take`). +pub const VECTOR_SET_FIELD: u8 = 0xB3; // field_symbol_id:u32, holders:u8 + // -- Deforestation buffer (0.15 Traversal) ----------------------------------- // // Mutable byte-buffer scratch backing the synthesizer's `__buf_*` intrinsics. @@ -705,6 +718,7 @@ pub fn opcode_name(op: u8) -> &'static str { VECTOR_GET_OR => "VECTOR_GET_OR", VECTOR_SET => "VECTOR_SET", VECTOR_SET_OR_KEEP => "VECTOR_SET_OR_KEEP", + VECTOR_SET_FIELD => "VECTOR_SET_FIELD", BUFFER_NEW => "BUFFER_NEW", BUFFER_APPEND_STR => "BUFFER_APPEND_STR", BUFFER_APPEND_SEP_UNLESS_FIRST => "BUFFER_APPEND_SEP_UNLESS_FIRST", @@ -850,7 +864,7 @@ pub fn opcode_operand_width(op: u8, code: &[u8], ip: usize) -> usize { RECORD_GET_NAMED | LIST_NEW | TUPLE_NEW => 4, // 5-byte - CALL_BUILTIN | VARIANT_NEW | RECORD_TAKE_NAMED => 5, + CALL_BUILTIN | VARIANT_NEW | RECORD_TAKE_NAMED | VECTOR_SET_FIELD => 5, // u8 + fail_offset:i32 MATCH_UNWRAP | MATCH_TUPLE => 5, diff --git a/tests/fixtures/vector_field_set/main.av b/tests/fixtures/vector_field_set/main.av new file mode 100644 index 000000000..b26947a99 --- /dev/null +++ b/tests/fixtures/vector_field_set/main.av @@ -0,0 +1,49 @@ +module Main + intent = "A record whose Vector field every step sets, the way an answer module's state holds one." + effects [Args.get, Console.print] + +record State + cells: Vector + count: Int + +fn step(s: State, i: Int) -> State + ? "Writes i at i modulo the Vector's size. The None arm hands the state" + "back whole, so the Vector must still be in it when the index misses." + match Vector.set(s.cells, Int.mod(i, 100000), i) + Option.Some(updated) -> State.update(s, cells = updated, count = s.count + 1) + Option.None -> s + +verify step + step(State(cells = Vector.fromList([0, 0]), count = 0), 1).count => 1 + step(State(cells = Vector.fromList([]), count = 0), 1).count => 0 + +fn run(s: State, i: Int, n: Int) -> State + ? "Runs step for i from i up to n." + match i >= n + true -> s + false -> run(step(s, i), i + 1, n) + +verify run + run(State(cells = Vector.fromList([0]), count = 0), 0, 3).count => 3 + +fn total(s: State) -> Int + ? "The last cell plus the number of steps that wrote." + match Vector.get(s.cells, 99999) + Option.Some(v) -> v + s.count + Option.None -> s.count + +verify total + total(State(cells = Vector.fromList([]), count = 2)) => 2 + +fn arg(index: Int, fallback: Int) -> Int + ? "The index-th argument as an Int, or the fallback." + ! [Args.get] + match Vector.get(Vector.fromList(Args.get()), index) + Option.Some(text) -> Result.withDefault(Int.fromString(text), fallback) + Option.None -> fallback + +fn main() -> Unit + ? "Sets a hundred-thousand-cell Vector held by a record, 2000 times by default." + ! [Args.get, Console.print] + ended = run(State(cells = Vector.new(100000, 0), count = 0), 0, arg(0, 2000)) + Console.print("total {total(ended)}") diff --git a/tests/fixtures/vector_field_set_shapes/main.av b/tests/fixtures/vector_field_set_shapes/main.av new file mode 100644 index 000000000..4ecf20a17 --- /dev/null +++ b/tests/fixtures/vector_field_set_shapes/main.av @@ -0,0 +1,179 @@ +module Main + intent = "Shapes around a matched Vector.set of a record field that must keep Aver's value semantics." + effects [Console.print] + +record State + cells: Vector + count: Int + +record Outer + inner: State + turns: Int + +record Pair + left: State + right: Vector + +record Node + kids: Vector> + label: Int + +fn at(v: Vector, i: Int) -> Int + ? "The cell at i, or -1." + Option.withDefault(Vector.get(v, i), 0 - 1) + +verify at + at(Vector.fromList([4]), 0) => 4 + at(Vector.fromList([]), 0) => 0 - 1 + +fn fresh() -> State + ? "Ten zero cells." + State(cells = Vector.new(10, 0), count = 0) + +verify fresh + fresh().count => 0 + +fn step(s: State, i: Int) -> State + ? "Writes i + 1 at i; the None arm hands s back whole." + match Vector.set(s.cells, i, i + 1) + Option.Some(updated) -> State.update(s, cells = updated, count = s.count + 1) + Option.None -> s + +verify step + step(fresh(), 1).count => 1 + step(fresh(), 10).count => 0 + +fn stepNoneFirst(s: State, i: Int) -> State + ? "step with the None arm first." + match Vector.set(s.cells, i, 7) + Option.None -> s + Option.Some(updated) -> State.update(s, cells = updated, count = s.count + 1) + +verify stepNoneFirst + stepNoneFirst(fresh(), 1).count => 1 + +fn loop(s: State, i: Int, n: Int) -> State + ? "Sets each index below n in a loop that hands the state on itself." + match i >= n + true -> s + false -> match Vector.set(s.cells, i, i * 2) + Option.Some(updated) -> loop(State.update(s, cells = updated, count = s.count + 1), i + 1, n) + Option.None -> loop(s, i + 1, n) + +verify loop + loop(fresh(), 0, 12).count => 10 + +fn kept(s: State, i: Int) -> Int + ? "Another local names the record." + other = s + after = step(s, i) + at(other.cells, i) * 100 + at(after.cells, i) + +verify kept + kept(fresh(), 2) => 3 + +fn oldCells(s: State, i: Int) -> Int + ? "A local bound to the Vector before the set." + old = s.cells + after = match Vector.set(s.cells, i, 9) + Option.Some(updated) -> State.update(s, cells = updated, count = s.count + 1) + Option.None -> s + at(old, i) * 100 + at(after.cells, i) + +verify oldCells + oldCells(fresh(), 2) => 9 + +fn readBack(s: State, i: Int) -> Int + ? "The Some arm reads the old Vector." + match Vector.set(s.cells, i, 9) + Option.Some(updated) -> at(s.cells, i) * 100 + at(updated, i) + Option.None -> 0 - 1 + +verify readBack + readBack(fresh(), 2) => 9 + +fn split(s: State, i: Int) -> Pair + ? "The Some arm keeps the record whole beside the new Vector." + match Vector.set(s.cells, i, 9) + Option.Some(updated) -> Pair(left = s, right = updated) + Option.None -> Pair(left = s, right = s.cells) + +verify split + at(split(fresh(), 2).right, 2) => 9 + +fn counted(s: State, i: Int) -> State + ? "The update keeps the base's Vector." + match Vector.set(s.cells, i, 9) + Option.Some(updated) -> State.update(s, count = at(updated, i)) + Option.None -> s + +verify counted + counted(fresh(), 2).count => 9 + +fn nested(o: Outer, i: Int) -> Outer + ? "The Vector two records down." + match Vector.set(o.inner.cells, i, 5) + Option.Some(updated) -> Outer.update(o, inner = State.update(o.inner, cells = updated), turns = o.turns + 1) + Option.None -> o + +verify nested + nested(Outer(inner = fresh(), turns = 0), 1).turns => 1 + +fn nestedLoop(o: Outer, i: Int, n: Int) -> Outer + ? "nested in a loop." + match i >= n + true -> o + false -> nestedLoop(nested(o, i), i + 1, n) + +verify nestedLoop + nestedLoop(Outer(inner = fresh(), turns = 0), 0, 3).turns => 3 + +fn noKid() -> Option + ? "No child." + Option.None + +verify noKid + noKid() => Option.None + +fn adopt(n: Node) -> Node + ? "The value written holds the record the Vector comes from." + match Vector.set(n.kids, 0, Option.Some(n)) + Option.Some(updated) -> Node.update(n, kids = updated, label = n.label + 1) + Option.None -> n + +verify adopt + adopt(Node(kids = Vector.new(1, noKid()), label = 1)).label => 2 + +fn childSize(n: Node) -> Int + ? "The first child's kid count and label." + match Vector.get(n.kids, 0) + Option.Some(Option.Some(child)) -> Vector.len(child.kids) * 100 + child.label + Option.Some(Option.None) -> 0 - 1 + Option.None -> 0 - 2 + +verify childSize + childSize(Node(kids = Vector.new(1, noKid()), label = 1)) => 0 - 1 + +fn main() -> Unit + ? "Prints what every shape answers." + ! [Console.print] + ran = loop(step(stepNoneFirst(step(fresh(), 3), 4), 99), 0, 12) + Console.print("loop {ran.count} {at(ran.cells, 3)} {at(ran.cells, 9)} {Vector.len(ran.cells)}") + before = fresh() + after = step(before, 3) + Console.print("caller {at(before.cells, 3)} {at(after.cells, 3)}") + shared = Vector.new(10, 0) + first = State(cells = shared, count = 0) + second = State(cells = shared, count = 0) + moved = step(first, 6) + Console.print("shared {at(second.cells, 6)} {at(moved.cells, 6)} {at(shared, 6)}") + Console.print("kept {kept(fresh(), 2)} old {oldCells(fresh(), 2)} read {readBack(fresh(), 2)}") + pair = split(fresh(), 4) + Console.print("split {at(pair.left.cells, 4)} {at(pair.right, 4)}") + c = counted(fresh(), 4) + Console.print("counted {c.count} {at(c.cells, 4)}") + holder = Outer(inner = fresh(), turns = 0) + grown = nestedLoop(holder, 0, 12) + Console.print("nested {grown.turns} {at(grown.inner.cells, 5)} {at(holder.inner.cells, 5)}") + adopted = adopt(Node(kids = Vector.new(3, noKid()), label = 5)) + Console.print("adopt {childSize(adopted)} {adopted.label}") diff --git a/tests/rust_work_spec.rs b/tests/rust_work_spec.rs index c2df0db9c..5263b5261 100644 --- a/tests/rust_work_spec.rs +++ b/tests/rust_work_spec.rs @@ -517,6 +517,78 @@ fn a_record_gives_up_its_fields_at_its_last_use() { result.unwrap_or_else(|error| panic!("{error}")); } +/// A matched `Vector.set` of a record field whose `None` arm hands the record +/// back whole moves the Vector out once the index is known to be in range. +/// +/// `step` used to clone `s.cells` into `set_owned`, because the `None` arm +/// still needed the field, so every step copied the whole Vector. The index is +/// now checked first and the field moves only in the `Some` arm. +#[test] +fn a_matched_vector_set_moves_the_field_out_in_the_some_arm() { + let name = "vector_field_set"; + let ws = temp_dir(name); + let project = ws.join("project"); + fs::create_dir_all(&project).expect("create project dir"); + let args = ["3000"]; + let result = (|| { + compile_rust(name, &project, name, &[])?; + let entry = fs::read_to_string(project.join("src/aver_generated/entry/mod.rs")) + .map_err(|error| format!("read the generated entry module: {error}"))?; + for moved in [ + "pub fn step(mut s @ _: State,", + "match __idx.filter(|__idx| *__idx < s.cells.len()) { Some(__idx) => { let updated = s.cells.set_unchecked(__idx, __value); State { cells: updated, ", + "None => { s } }", + ] { + if !entry.contains(moved) { + return Err(format!( + "{name}: the Vector no longer moves out in the Some arm; missing `{moved}` in:\n{entry}" + )); + } + } + let vm = run_vm_with(name, &args)?; + let bin = cargo_build(&project, name)?; + let rust = run_binary_with(&bin, &args)?; + if vm != rust { + return Err(format!( + "{name}: stdout mismatch\n--- VM ---\n{vm}\n--- Rust ---\n{rust}" + )); + } + Ok(()) + })(); + let _ = fs::remove_dir_all(&ws); + result.unwrap_or_else(|error| panic!("{error}")); +} + +/// Shapes around a matched `Vector.set` of a record field: the record or the +/// Vector still held by someone else, the old Vector read after the set, the +/// record kept whole, the Vector two records down, a value that holds the +/// record. Each must build and answer as the VM does. +#[test] +fn matched_vector_set_shapes_build_and_match_the_vm() { + let name = "vector_field_set_shapes"; + let ws = temp_dir(name); + let project = ws.join("project"); + fs::create_dir_all(&project).expect("create project dir"); + let result = (|| { + compile_rust(name, &project, name, &[])?; + let vm = run_vm(name)?; + let expected = "loop 12 6 18 10\ncaller 0 4\nshared 0 7 0\nkept 3 old 9 read 9\nsplit 0 9\ncounted 9 0\nnested 10 5 0\nadopt 305 6"; + if vm.trim() != expected { + return Err(format!("{name}: the VM answered\n{vm}")); + } + let bin = cargo_build(&project, name)?; + let rust = run_binary(&bin)?; + if vm != rust { + return Err(format!( + "{name}: stdout mismatch\n--- VM ---\n{vm}\n--- Rust ---\n{rust}" + )); + } + Ok(()) + })(); + let _ = fs::remove_dir_all(&ws); + result.unwrap_or_else(|error| panic!("{error}")); +} + /// A record handed to a pair of functions that tail-call each other moves /// into the trampoline instead of being cloned by the wrapper. /// diff --git a/tests/shared_update_spec.rs b/tests/shared_update_spec.rs index 8640e79b3..266638c9d 100644 --- a/tests/shared_update_spec.rs +++ b/tests/shared_update_spec.rs @@ -377,3 +377,54 @@ fn step(setting: Setting, left: Int) -> Setting found[0] ); } + +const CELLS: &str = r#"module Main + intent = "shared vector updates" + effects [] + +record State + cells: Vector + count: Int +"#; + +/// `match Vector.set(s.cells, …)` whose `None` arm hands the record back: the +/// Vector moves out in the `Some` arm, so nothing is copied and nothing warns. +#[test] +fn a_matched_vector_set_whose_none_arm_keeps_the_record_does_not_warn() { + let found = warnings(&format!( + r#"{CELLS} +fn run(s: State, i: Int, n: Int) -> State + ? "Sets each index below n." + match i >= n + true -> s + false -> match Vector.set(s.cells, i, i) + Option.Some(updated) -> run(State.update(s, cells = updated, count = s.count + 1), i + 1, n) + Option.None -> run(s, i + 1, n) +"# + )); + assert!(found.is_empty(), "{found:?}"); +} + +/// The `Some` arm reads the old Vector again: it stays in the record, and the +/// set copies it. +#[test] +fn a_matched_vector_set_whose_some_arm_reads_the_old_vector_warns() { + let found = warnings(&format!( + r#"{CELLS} +fn run(s: State, i: Int, n: Int) -> State + ? "Sets each index below n, counting the old length." + match i >= n + true -> s + false -> match Vector.set(s.cells, i, i) + Option.Some(updated) -> run(State.update(s, cells = updated, count = Vector.len(s.cells)), i + 1, n) + Option.None -> run(s, i + 1, n) +"# + )); + assert_eq!(found.len(), 1, "{found:?}"); + assert!( + found[0] + .starts_with("`Vector.set` on `s.cells` updates a Vector that is still held by `s`"), + "{}", + found[0] + ); +} diff --git a/tests/vm_record_field_take.rs b/tests/vm_record_field_take.rs index 1a8d89240..8175babbd 100644 --- a/tests/vm_record_field_take.rs +++ b/tests/vm_record_field_take.rs @@ -837,3 +837,310 @@ fn main() -> Int assert_eq!(answer, 100 + 5); assert!(copied > 0, "the shared Map was written in place"); } + +// ── A Vector field set through a match on `Vector.set` ─────────────────── +// +// `match Vector.set(s.cells, i, x)` whose `None` arm hands `s` back whole: the +// field can only leave the record once the index is known to be in range +// (`VECTOR_SET_FIELD`). The refusals below are every way the record or the +// Vector can still be seen by someone else; each must copy and answer what +// Aver's immutable values mandate. + +const VECTOR_PRELUDE: &str = r#"module Cells + intent = "Vector fields set through a match" + effects [] + +record State + cells: Vector + count: Int + +record Holder + inner: State + +fn fresh() -> State + State(cells = Vector.new(50, 0), count = 0) + +fn at(v: Vector, i: Int) -> Int + Option.withDefault(Vector.get(v, i), 0 - 1) +"#; + +fn cells(body: &str) -> String { + format!("{VECTOR_PRELUDE}\n{body}") +} + +/// What the program answered and how many vector elements it copied. +fn run_cells(src: &str) -> (i64, u64) { + let mut machine = compiled_vm(src); + let result = machine.run().expect("program should run"); + ( + result.as_int(&machine.arena), + machine.arena.vector_elements_copied(), + ) +} + +const STEP: &str = r#" +fn step(s: State, i: Int) -> State + match Vector.set(s.cells, i, i + 1) + Option.Some(updated) -> State.update(s, cells = updated, count = s.count + 1) + Option.None -> s + +fn run(s: State, i: Int, n: Int) -> State + match i >= n + true -> s + false -> run(step(s, i), i + 1, n) +"#; + +/// The measured fixture: two thousand sets of a hundred-thousand-cell Vector +/// copy no element. Before, each set copied all of them. +#[test] +fn the_vector_fixture_sets_its_field_in_place() { + let path = concat!( + env!("CARGO_MANIFEST_DIR"), + "/tests/fixtures/vector_field_set/main.av" + ); + let src = std::fs::read_to_string(path).expect("read the fixture"); + let mut machine = compiled_vm(&src); + machine.run().expect("the fixture should run"); + assert_eq!( + machine.arena.vector_elements_copied(), + 0, + "a step copied the Vector its state holds" + ); +} + +/// A loop over the record copies nothing, and an index past the end leaves +/// the Vector in the record the `None` arm hands back. +#[test] +fn a_vector_field_set_in_a_loop_copies_nothing() { + let src = cells(&format!( + r#"{STEP} +fn main() -> Int + done = step(run(fresh(), 0, 50), 70) + done.count * 10000 + at(done.cells, 49) * 100 + Vector.len(done.cells) +"# + )); + let (answer, copied) = run_cells(&src); + assert_eq!(answer, 50 * 10000 + 50 * 100 + 50); + assert_eq!(copied, 0, "a step copied the Vector it set"); +} + +/// The `None` arm first: the same take. +#[test] +fn a_vector_field_set_with_the_none_arm_first_copies_nothing() { + let src = cells( + r#" +fn step(s: State, i: Int) -> State + match Vector.set(s.cells, i, 7) + Option.None -> s + Option.Some(updated) -> State.update(s, cells = updated, count = s.count + 1) + +fn run(s: State, i: Int, n: Int) -> State + match i >= n + true -> s + false -> run(step(s, i), i + 1, n) + +fn main() -> Int + done = run(fresh(), 0, 60) + done.count * 100 + at(done.cells, 3) +"#, + ); + let (answer, copied) = run_cells(&src); + assert_eq!(answer, 50 * 100 + 7); + assert_eq!(copied, 0, "a step copied the Vector it set"); +} + +/// The caller keeps the record it passed: its Vector must not change. +#[test] +fn a_vector_record_the_caller_still_holds_is_not_taken() { + let src = cells(&format!( + r#"{STEP} +fn main() -> Int + before = fresh() + after = step(before, 3) + at(before.cells, 3) * 100 + at(after.cells, 3) * 10 + Vector.len(before.cells) +"# + )); + let (answer, copied) = run_cells(&src); + assert_eq!(answer, 0 + 4 * 10 + 50); + assert!(copied > 0, "the caller's Vector was written in place"); +} + +/// A second local names the same record inside the function. +#[test] +fn a_vector_record_another_local_holds_is_not_taken() { + let src = cells( + r#" +fn step(s: State, i: Int) -> Int + kept = s + after = match Vector.set(s.cells, i, 9) + Option.Some(updated) -> State.update(s, cells = updated, count = s.count + 1) + Option.None -> s + at(kept.cells, i) * 100 + at(after.cells, i) + +fn main() -> Int + step(fresh(), 4) +"#, + ); + let (answer, copied) = run_cells(&src); + assert_eq!(answer, 9); + assert!( + copied > 0, + "a Vector another local sees was written in place" + ); +} + +/// Another record holds the record. +#[test] +fn a_vector_record_held_by_another_record_is_not_taken() { + let src = cells(&format!( + r#"{STEP} +fn main() -> Int + holder = Holder(inner = fresh()) + after = step(holder.inner, 5) + at(holder.inner.cells, 5) * 100 + at(after.cells, 5) +"# + )); + let (answer, copied) = run_cells(&src); + assert_eq!(answer, 6); + assert!( + copied > 0, + "a Vector another record sees was written in place" + ); +} + +/// One Vector in two records: taking it out of the first leaves the second +/// holding it. +#[test] +fn a_vector_two_records_share_is_not_written_in_place() { + let src = cells(&format!( + r#"{STEP} +fn main() -> Int + shared = Vector.new(50, 0) + first = State(cells = shared, count = 0) + second = State(cells = shared, count = 0) + after = step(first, 6) + at(second.cells, 6) * 100 + at(after.cells, 6) * 10 + at(shared, 6) +"# + )); + let (answer, copied) = run_cells(&src); + assert_eq!(answer, 7 * 10); + assert!(copied > 0, "a shared Vector was written in place"); +} + +/// A local bound to the Vector before the set still sees the old one. +#[test] +fn a_vector_another_local_holds_is_not_written_in_place() { + let src = cells( + r#" +fn step(s: State, i: Int) -> Int + old = s.cells + after = match Vector.set(s.cells, i, 9) + Option.Some(updated) -> State.update(s, cells = updated, count = s.count + 1) + Option.None -> s + at(old, i) * 100 + at(after.cells, i) + +fn main() -> Int + step(fresh(), 4) +"#, + ); + let (answer, copied) = run_cells(&src); + assert_eq!(answer, 9); + assert!( + copied > 0, + "a Vector another local sees was written in place" + ); +} + +/// The `Some` arm reads the old Vector after the set: nothing is taken. +#[test] +fn the_old_vector_read_after_the_set_is_not_taken() { + let src = cells( + r#" +fn step(s: State, i: Int) -> Int + match Vector.set(s.cells, i, 9) + Option.Some(updated) -> at(s.cells, i) * 100 + at(updated, i) + Option.None -> 0 - 1 + +fn main() -> Int + step(fresh(), 4) +"#, + ); + let (answer, _) = run_cells(&src); + assert_eq!(answer, 9); +} + +/// The `Some` arm hands on the record whole beside the new Vector. +#[test] +fn a_record_kept_whole_beside_the_new_vector_is_not_taken() { + let src = cells( + r#" +record Pair + left: State + right: Vector + +fn split(s: State, i: Int) -> Pair + match Vector.set(s.cells, i, 9) + Option.Some(updated) -> Pair(left = s, right = updated) + Option.None -> Pair(left = s, right = s.cells) + +fn main() -> Int + pair = split(fresh(), 4) + at(pair.left.cells, 4) * 100 + at(pair.right, 4) +"#, + ); + let (answer, _) = run_cells(&src); + assert_eq!(answer, 9); +} + +/// The update keeps the base's Vector: it does not write `cells`. +#[test] +fn an_update_that_keeps_the_vector_field_is_not_taken() { + let src = cells( + r#" +fn step(s: State, i: Int) -> State + match Vector.set(s.cells, i, 9) + Option.Some(updated) -> State.update(s, count = at(updated, i)) + Option.None -> s + +fn main() -> Int + after = step(fresh(), 4) + after.count * 100 + at(after.cells, 4) + 1 +"#, + ); + let (answer, _) = run_cells(&src); + assert_eq!(answer, 9 * 100 + 1); +} + +/// The value written holds the record the Vector is taken from. +#[test] +fn a_value_holding_the_record_is_not_written_into_it() { + let src = r#"module Nodes + intent = "a node whose children include itself" + effects [] + +record Node + kids: Vector> + label: Int + +fn adopt(n: Node) -> Node + match Vector.set(n.kids, 0, Option.Some(n)) + Option.Some(updated) -> Node.update(n, kids = updated, label = n.label + 1) + Option.None -> n + +fn size(n: Node) -> Int + match Vector.get(n.kids, 0) + Option.Some(Option.Some(child)) -> Vector.len(child.kids) * 100 + child.label + Option.Some(Option.None) -> 0 - 1 + Option.None -> 0 - 2 + +fn noKid() -> Option + Option.None + +fn main() -> Int + adopted = adopt(Node(kids = Vector.new(3, noKid()), label = 5)) + size(adopted) * 10 + adopted.label +"#; + let (answer, _) = run_cells(src); + // The child is the node before adoption: three kids, label 5. + assert_eq!(answer, (3 * 100 + 5) * 10 + 6); +}